Change logic to prevent device-plugin daemonset spec change

k8snetworkplumbingwg / sriov-network-operator

Operator for provisioning and configuring SR-IOV CNI plugin and device plugin

Apache License 2.0

85 stars 114 forks source link

Change logic to prevent device-plugin daemonset spec change #740

Closed AMacedoP closed 3 months ago

AMacedoP commented 4 months ago

Instead of adding a NodeSelectorRequirement per NodePolicy we add all the NodeSelectors to a single NodeSelectorRequirent so they can all be sorted. This will prevent that the daemonset's node selector changes when there are multiple NodePolicies applied.

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs, Maintainers can use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:
/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor. Best regards.

zeeke commented 4 months ago

hi @AMacedoP , the problem you're describing has probably already been solved by:

https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/721 can you check that?

That said, I'm not against the proposed changes, as they make the code clearer and more resilient.

AMacedoP commented 4 months ago

@zeeke yes, that PR also solves my problem. How do you want to proceed with this PR? If you want I can resolve the failed test cases and merge it.

zeeke commented 4 months ago

Please, fix the unit test and wait for the other community member's feedback.

@adrianchiris , @SchSeba WDYT?

SchSeba commented 4 months ago

Hi @AMacedoP,

Thanks for the PR! I am working on a different solution that I think will be better here. My idea is to leave the device plugin with one node selector something like sriov-device-plugin-deployed and the daemon will remove and add the label when it needs to reset the device plugin

I think that is cleaner let me know what you think

AMacedoP commented 4 months ago

@SchSeba that looks like a good solution. In that case what should we do with this PR?

SchSeba commented 3 months ago

If possible I would like to close this one in favor of https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/747

@AMacedoP let me know what do you think

Thanks!

AMacedoP commented 3 months ago

@SchSeba will close this PR. Do you have an expected release date for a new version with the changes in #721?

Thanks!