Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

AMacedoP
Copy link

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.

Copy link

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
Copy link
Member

zeeke commented Jul 18, 2024

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

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

@AMacedoP
Copy link
Author

@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
Copy link
Member

zeeke commented Jul 19, 2024

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

@adrianchiris , @SchSeba WDYT?

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.

Signed-off-by: Alejandro Macedo <alex.macedopereira@gmail.com>
@AMacedoP AMacedoP force-pushed the fix-device-plugin-daemonset-spec-change branch from 05702e7 to 79ff9d2 Compare July 19, 2024 15:43
@SchSeba
Copy link
Collaborator

SchSeba commented Jul 22, 2024

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
Copy link
Author

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

@SchSeba
Copy link
Collaborator

SchSeba commented Jul 30, 2024

If possible I would like to close this one in favor of #747

@AMacedoP let me know what do you think

Thanks!

@AMacedoP
Copy link
Author

AMacedoP commented Jul 30, 2024

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

Thanks!

@AMacedoP AMacedoP closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants