Report mistakes in HW plugin configuration to the PtpConfig status by gtannous-spec · Pull Request #152 · k8snetworkplumbingwg/linuxptp-daemon

gtannous-spec · 2026-02-16T20:54:35Z

Overview

This PR adds CRD status condition reporting for hardware plugin misconfiguration in PtpConfig. When the daemon detects invalid plugin names (typos) or plugin configuration errors during profile application, it now writes a HardwarePluginReady condition to the PtpConfig status, making the failure visible to operators instead of silently continuing. On successful configuration, the condition is set to True.

addressing CNF-16423

Impact

This is a bug fix / enhancement addressing a painful debugging scenario where typos in the hardware plugin configuration section of PtpConfig caused the daemon to silently fail hardware setup and continue running. The change introduces a new status condition on the PtpConfig CRD, which is a non-breaking API addition (additive Conditions field on PtpConfigStatus). Requires a corresponding CRD update in ptp-operator to include the Conditions field in the schema.

Files Changed

File Changed	Explanation
`pkg/daemon/ptpconfig_status.go`	New file. Contains `FindPtpConfigByProfileName` (resolves owning PtpConfig by profile name) and `UpdatePtpConfigCondition` (sets/updates a `metav1.Condition` on the PtpConfig status).
`pkg/daemon/daemon.go`	Adds `ptpClient` field to `Daemon` struct and `New()` signature. Adds plugin name validation in `applyNodePtpProfile` before plugin execution. Captures errors from `OnPTPConfigChange` and reports them to PtpConfig CRD status via conditions. Clears condition on success.
`pkg/daemon/plugin.go`	Changes `registerPlugins` to return `(PluginManager, []string)` -- the second value is a list of unrecognized plugin names.
`pkg/plugin/plugin.go`	Changes `OnPTPConfigChange` to return `[]error` instead of silently discarding plugin errors. Logs a warning per failed plugin.
`cmd/main.go`	Passes `ptpClient` to `daemon.New()`.
`pkg/daemon/daemon_internal_test.go`	Updates two `New()` call sites and two `registerPlugins()` call sites to match the new signatures.
`vendor/.../ptpconfig_types.go`	Adds `Conditions []metav1.Condition` field to `PtpConfigStatus` (vendored copy of ptp-operator CRD type).

Technical Implementation

Error Detection

Validates plugin names in nodeProfile.Plugins against dn.pluginManager.Plugins (registered plugins) before invoking any plugin logic. Unknown names are flagged with descriptive errors.
PluginManager.OnPTPConfigChange now propagates errors returned by individual plugins (e.g., unmarshal failures, missing required fields).

CRD Status Reporting

FindPtpConfigByProfileName queries the API server to dynamically resolve which PtpConfig CR owns a given profile name. This works in all daemon modes (full controller, hybrid, and legacy) without requiring metadata injection from the controller.
UpdatePtpConfigCondition performs a read-modify-write on the PtpConfig status using meta.SetStatusCondition for proper upsert semantics.
On error: sets HardwarePluginReady=False with reason HardwarePluginConfigError and a message listing all errors.
On success: sets HardwarePluginReady=True with reason HardwarePluginConfigured.

Dependency Wiring

ptpClient (typed PTP clientset) is threaded from cmd/main.go through daemon.New() into the Daemon struct, enabling direct API access to PtpConfig resources.

github-actions · 2026-02-16T20:54:44Z

Thanks for your PR,
Best regards.

gemini-code-assist · 2026-02-16T20:54:58Z

Summary of Changes

Hello @gtannous-spec, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the observability and debugging experience for PtpConfig hardware plugin configurations. It addresses a critical issue where misconfigurations, such as typos in plugin names or internal plugin errors, would silently fail without providing feedback to operators. By introducing explicit status conditions on the PtpConfig CRD, the system now clearly communicates the health and configuration status of hardware plugins, making it easier to identify and resolve issues.

Highlights

Enhanced Error Reporting: The system now reports hardware plugin misconfigurations and errors directly to the PtpConfig status, preventing silent failures and providing clear feedback to operators.
Plugin Validation: Introduced validation for plugin names specified in PtpConfig profiles, identifying unknown or mistyped plugins before execution.
Status Conditions: Added a new HardwarePluginReady condition to the PtpConfig status, which is set to False on configuration errors and True upon successful configuration.
API Integration: Integrated a PTP client into the daemon, enabling direct querying and updating of PtpConfig resources for status reporting.
Error Propagation: Modified plugin interfaces to propagate errors from individual plugins, ensuring all issues encountered during plugin execution are captured and reported.

Changelog

cmd/main.go
- Passed the new ptpClient to the daemon.New() function call.
go.mod
- Updated the ptp-operator dependency version to a newer commit.
go.sum
- Updated checksums for the ptp-operator dependency to reflect the version change.
pkg/daemon/daemon.go
- Imported ptpclient and metav1 packages.
- Added a ptpClient field to the Daemon struct.
- Updated the New function signature to accept ptpClient.
- Implemented plugin name validation within applyNodePtpProfile.
- Added logic to collect plugin errors and report them to the PtpConfig status via conditions.
- Included logic to clear the HardwarePluginReady condition on successful configuration.
pkg/daemon/daemon_internal_test.go
- Updated calls to daemon.New() to include the new ptpClient parameter (passed as nil for tests).
- Adjusted calls to registerPlugins() to handle its new return signature.
pkg/daemon/plugin.go
- Modified the registerPlugins function to return a slice of unknownPlugins in addition to the PluginManager.
pkg/daemon/ptpconfig_status.go
- Added a new file containing FindPtpConfigByProfileName to locate PtpConfig by profile name.
- Added UpdatePtpConfigCondition to set or update metav1.Condition on PtpConfig status.
pkg/plugin/plugin.go
- Changed the OnPTPConfigChange method in PluginManager to return a slice of errors, allowing plugins to report failures.
- Added logging for plugin failures during OnPTPConfigChange.
vendor/github.com/k8snetworkplumbingwg/ptp-operator/api/v1/ptpconfig_types.go
- Added a Conditions field of type []metav1.Condition to the PtpConfigStatus struct.
vendor/github.com/k8snetworkplumbingwg/ptp-operator/api/v1/zz_generated.deepcopy.go
- Updated the DeepCopyInto method for PtpConfigStatus to correctly handle the new Conditions field.
vendor/modules.txt
- Updated the vendored module entry for ptp-operator to reflect the new version.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request enhances PtpConfig status reporting by introducing CRD status condition reporting for hardware plugin misconfigurations, which will significantly improve the debugging experience for operators. However, a critical security vulnerability has been identified: the implementation lacks proper validation for the Name field in the PtpProfile struct. Since this field is a pointer and comes from an external source (the PtpConfig CRD), dereferencing it without a nil check can lead to daemon panics and Denial of Service. Additionally, there are suggestions to improve maintainability and adhere to best practices, mainly around reducing code duplication and context propagation.

pkg/plugin/plugin.go

pkg/daemon/daemon.go

pkg/daemon/ptpconfig_status.go

nocturnalastro · 2026-02-17T15:35:51Z

pkg/daemon/ptpconfig_status.go

+	}
+	for _, cfg := range ptpConfigs.Items {
+		for _, p := range cfg.Spec.Profile {
+			if p.Name != nil && *p.Name == profileName {


Are we guaranteed that a profile name is unique?

In practice the daemon likely expects profile name to be unique. But there is nothing that enforces it on the CRD level it seems.

Thought that was supported by the webhook

@gtannous-spec Can you double check the webhook. If there is link it here. If not we probably want to add that it if that what we're using to find it.

Are we referring to "unique profiles" per one ptpconfig or same profile name across mutliple Ptpconfigs ?

I think we can force that condition under the namespace openshift-ptp

[core@master-0 ~]$ oc get packagemanifest ptp-operator -n openshift-marketplace -o jsonpath='{.status.channels[0].currentCSVDesc.installModes}'
[{"supported":true,"type":"OwnNamespace"},{"supported":true,"type":"SingleNamespace"},{"supported":false,"type":"MultiNamespace"},{"supported":false,"type":"AllNamespaces"}

Single namespace, own namespace

The PTP Operator controller watches PtpConfig CRs in the openshift-ptp namespace. Hardcoded

Just wanted to make sure it was by definition and not an assumption :)

nocturnalastro · 2026-03-17T12:57:08Z

addons/intel/testdata/e825-tgm.yaml

    devices:
      - enp108s0f0
  e825:
-    enableDefaultConfig: false


Whats going on here?

I guess this was auto-added when was fixing Merge conflicts, should fix it.

I see why now it was added..
The unit tests couldn't pass with it

addons/intel/validate.go

pkg/daemon/daemon.go

nocturnalastro · 2026-03-24T11:05:41Z

pkg/daemon/daemon.go


+	if len(dn.unknownPlugins) > 0 {
+		pluginErrors = append(pluginErrors,
+			fmt.Errorf("unknown plugins specified (possible typo): %v", dn.unknownPlugins))


If the plugin is never used why throw an error?

do you mean if the plugin is incorrect from the first place we should return error?
Because in this lines we're catching plugins that couldn't be registered at all in the daemon-level start.
not per profile tho

Its reported on the profile though. If we don't use that plugin that couldn't be registered then it doesn't matter right?

Ok good point.
Fixed it, such that if no profile is using that plugin then it doesn't get reported globally.

nocturnalastro · 2026-03-24T12:50:47Z

addons/intel/validate.go

+// sysfs pins or DPLL board labels. This mirrors each plugin's runtime behavior:
+// - E810: passes hasSysfsSMAPins (sysfs when SMA1 exists, DPLL otherwise)
+// - E825/E830: passes alwaysSysfs (always sysfs, matching pinConfig.applyPinSet)
+func validatePinNames(devicePins map[string]pinSet, pluginName string, useSysfs func(string) bool) []string {


Why not simplify this with a book that defines if you call back to dpll or not?

In fact you can do that just off the plugin name you don't even need to pass anything in.

You mean the decision is static per plugin?

nocturnalastro · 2026-03-24T12:53:25Z

addons/intel/validate.go

+			for _, p := range sysfsPins {
+				validSet[p] = true
+			}
+			for pinName := range pins {


As I said before returning all of them when you can just loop through the ones you want and check they exist is much simpler. Y

You're having too 3 loops for each device when it could just be one for each.

nocturnalastro · 2026-03-24T13:16:03Z

addons/intel/validate.go

+
+// validatePinValues checks that pin values have the expected "<direction> <channel>" format
+// where direction is 0-2 and channel is a non-negative integer
+func validatePinValues(devicePins map[string]pinSet, pluginName string) []string {


Having all these checks in there own functions just means your looping over the same data over and over again.

nocturnalastro · 2026-03-24T13:20:24Z

addons/intel/validate.go

+func validateInterconnections(inputs []PhaseInputs) []string {
+	var errs []string
+	validParts := validPartNames()
+	validConns := validConnectorNames()


I don't think this is correct. I don't think its caching the correct data structure you'd need. As I said before I'm pretty sure what a valid connector name is depends on the part.

You should get the part name from the input.
Look up that part from the hardware getting the valid connections for that.
Then from there get the valid connections for that part (cache here).
Finally verify the connections are valid.

I updated that.

addons/intel/validate.go

Strip qualified prefix from profile names in logs and status conditions Add generic detection of HW plugin misconfiguration

…ks SMA pins On newer kernels (4.22+) SMA pins are not exposed via sysfs but are still configurable through DPLL. validatePinNames now accepts a strategy function (hasSysfsSMAPins) to decide per-device whether to check sysfs pins or DPLL board labels, mirroring the runtime pin-configuration path used by each plugin. Made-with: Cursor

…eportPluginStatus Remove indirection through mockable function variables (discoverSysfsPinsFunc, hasDPLLPinLabelFunc, hasSysfsSMAPinsFunc) in favor of direct filesystem and DPLL label lookups. Merge validatePinNames and validatePinValues into a single validateDevicePins pass. Narrow connector validation to be per-part instead of across all hardware specs. Extract inline plugin status reporting from daemon.go into reportPluginStatus() in ptpconfig_status.go, and stop misclassifying hardware config errors as plugin errors.

gtannous-spec added the ok-to-test ok to test label Feb 16, 2026

gemini-code-assist bot reviewed Feb 16, 2026

View reviewed changes

pkg/plugin/plugin.go Show resolved Hide resolved

pkg/daemon/daemon.go Outdated Show resolved Hide resolved

pkg/daemon/ptpconfig_status.go Outdated Show resolved Hide resolved

gtannous-spec force-pushed the hwconfigs branch from c2db9fa to 324fe27 Compare February 16, 2026 21:19

github-actions bot removed the ok-to-test ok to test label Feb 16, 2026

gtannous-spec force-pushed the hwconfigs branch from 11d3748 to 4e28fe0 Compare February 16, 2026 21:51

gtannous-spec mentioned this pull request Feb 17, 2026

Revert "Merge pull request #170 from gtannous-spec/nodeptpdevice-feat" k8snetworkplumbingwg/ptp-operator#175

Merged

gtannous-spec force-pushed the hwconfigs branch from 4e28fe0 to 648b5ca Compare February 17, 2026 10:32

gtannous-spec added the ok-to-test ok to test label Feb 17, 2026

gtannous-spec force-pushed the hwconfigs branch from 648b5ca to 2736dc2 Compare February 17, 2026 12:58

github-actions bot removed the ok-to-test ok to test label Feb 17, 2026

gtannous-spec added the ok-to-test ok to test label Feb 17, 2026

gtannous-spec force-pushed the hwconfigs branch from 2736dc2 to 13fb797 Compare February 17, 2026 15:03

github-actions bot removed the ok-to-test ok to test label Feb 17, 2026

gtannous-spec added the ok-to-test ok to test label Feb 17, 2026

gtannous-spec mentioned this pull request Feb 17, 2026

Report hardware config errors to PtpConfig status conditions #130

Closed

gtannous-spec marked this pull request as ready for review February 17, 2026 15:28

nocturnalastro reviewed Feb 17, 2026

View reviewed changes

gtannous-spec force-pushed the hwconfigs branch from 13fb797 to 3f653fc Compare March 2, 2026 23:52

github-actions bot removed the ok-to-test ok to test label Mar 2, 2026

gtannous-spec added the ok-to-test ok to test label Mar 2, 2026

gtannous-spec force-pushed the hwconfigs branch from 3f653fc to 224eb76 Compare March 3, 2026 00:46

github-actions bot removed the ok-to-test ok to test label Mar 3, 2026

gtannous-spec added the ok-to-test ok to test label Mar 3, 2026

gtannous-spec force-pushed the hwconfigs branch from 224eb76 to 67a5777 Compare March 3, 2026 12:32

github-actions bot removed the ok-to-test ok to test label Mar 3, 2026

gtannous-spec added ok-to-test ok to test labels Mar 3, 2026

gtannous-spec force-pushed the hwconfigs branch from 67a5777 to d88b871 Compare March 3, 2026 17:37

nocturnalastro reviewed Mar 17, 2026

View reviewed changes

addons/intel/validate.go Show resolved Hide resolved

gtannous-spec force-pushed the hwconfigs branch from ec3f53b to dab224c Compare March 17, 2026 13:17

github-actions bot removed the ok-to-test ok to test label Mar 17, 2026

gtannous-spec added the ok-to-test ok to test label Mar 17, 2026

gtannous-spec force-pushed the hwconfigs branch from dab224c to 0e5bedc Compare March 17, 2026 13:43

github-actions bot removed the ok-to-test ok to test label Mar 17, 2026

gtannous-spec added ok-to-test ok to test and removed ok-to-test ok to test labels Mar 17, 2026

gtannous-spec force-pushed the hwconfigs branch from 0e5bedc to 0f06bb3 Compare March 18, 2026 11:47

github-actions bot removed the ok-to-test ok to test label Mar 18, 2026

gtannous-spec added the ok-to-test ok to test label Mar 18, 2026

gtannous-spec force-pushed the hwconfigs branch from 0f06bb3 to ac49a7f Compare March 23, 2026 18:31

github-actions bot removed the ok-to-test ok to test label Mar 23, 2026

gtannous-spec force-pushed the hwconfigs branch 2 times, most recently from 0ef3ab0 to 7185394 Compare March 23, 2026 18:41

gtannous-spec added the ok-to-test ok to test label Mar 23, 2026

gtannous-spec requested a review from nocturnalastro March 23, 2026 18:59

nocturnalastro added ok-to-test ok to test and removed ok-to-test ok to test labels Mar 24, 2026

nocturnalastro requested changes Mar 24, 2026

View reviewed changes

gtannous-spec force-pushed the hwconfigs branch from 7185394 to f27f8aa Compare March 29, 2026 09:42

github-actions bot removed the ok-to-test ok to test label Mar 29, 2026

gtannous-spec added 4 commits April 3, 2026 18:04

Report HW plugin misconfigurations to the PtpConfig status

a39f2dc

Strip qualified prefix from profile names in logs and status conditions Add generic detection of HW plugin misconfiguration

Remove redundant global unknownPlugins error check

107d2fe

Conversation

gtannous-spec commented Feb 16, 2026

Overview

Impact

Files Changed

Technical Implementation

Error Detection

CRD Status Reporting

Dependency Wiring

Uh oh!

github-actions bot commented Feb 16, 2026

Uh oh!

gemini-code-assist bot commented Feb 16, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtannous-spec Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

gtannous-spec Mar 17, 2026 •

edited

Loading