Revert "nfd-master: use only unbuffered chans in the nfd api-controller" #2097
Conversation
Fixes a bug where updates to NodeFeatureGroup objects were not happening after the initial sync. We need to use buffered channels for the "update all" channels as there is the default: case in select that is sending the update all signal (to the channel). An alternative would be to remove the default: case from the select block but it is more efficient this way - just skip if the reader side hasn't yet got the "update all" signal. This reverts commit bf6ffad.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
approved
|
/assign @ArangoGutierrez |
k8s-ci-robot
added
the
size/M
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/retest |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug where updates to NodeFeatureGroup objects did not occur after the initial sync by switching the "update all" channels back to buffered channels. It also introduces additional end-to-end tests for NodeFeatureGroup, including related test data changes.
- Updated pkg/nfd-master/nfd-api-controller.go to use buffered channels for updateAllNodesChan and updateAllNodeFeatureGroupsChan.
- Extended e2e tests in test/e2e/node_feature_discovery_test.go and added new test data files nodefeaturegroup-2.yaml and nodefeature-1.yaml to validate the update behavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/nfd-master/nfd-api-controller.go | Reverted channels to buffered, supporting sync update behavior. |
| test/e2e/node_feature_discovery_test.go | Added new test steps to verify NodeFeatureGroup update behavior. |
| test/e2e/data/nodefeaturegroup-2.yaml | New test data for NodeFeatureGroup with templating and backreference. |
| test/e2e/data/nodefeature-1.yaml | Updated NodeFeature test data with an instance name for clarity. |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 786e0922509ac9adce78fcfb4b5d04af75fbc631
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, marquiz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes a bug where updates to NodeFeatureGroup objects were not happening after the initial sync.
We need to use buffered channels for the "update all" channels as there is the default: case in select that is sending the update all signal (to the channel). An alternative would be to remove the default: case from the select block but it is more efficient this way - just skip if the reader side hasn't yet got the "update all" signal.
This PR also adds new e2e-tests for NodeFeatureGroup to cover this case (plus test the recently added templating and backrefs).