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.

Sign up for GitHub

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

Jump to bottom

Validate .Spec.DisableDrain #464

Merged
merged 2 commits into from
Jul 4, 2023
Merged

Validate .Spec.DisableDrain #464

merged 2 commits into from
Jul 4, 2023

Conversation

zeeke
Copy link
Member

@zeeke zeeke commented Jun 28, 2023

This PR introduces a webhook validation for the SriovOperatorConfig to avoid disabling the drain while
the config-daemon is updating a node. Disabling the drain at this stage would prevent the operator to uncordon a node at the end of the update operation, keeping nodes un-schedulable until manual intervention.

Introduce a small refactor of validateSriovOperatorConfig(...)

this PR needs

cc @mlguerrero12

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs 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 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 zeeke changed the title Validate disable drain Validate .Spec.DisableDrain Jun 28, 2023
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs 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 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.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs 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 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.

@coveralls
Copy link

coveralls commented Jun 28, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5452582694

  • 25 of 35 (71.43%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 26.105%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/webhook/validate.go 25 35 71.43%
Files with Coverage Reduction New Missed Lines %
controllers/sriovnetwork_controller.go 2 65.71%
api/v1/helper.go 3 41.92%
Totals Coverage Status
Change from base Build 5443063037: 0.3%
Covered Lines: 1990
Relevant Lines: 7623
💛 - Coveralls

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs 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 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.

@mlguerrero12 mlguerrero12 mentioned this pull request Jul 3, 2023
Merged
adrianchiris
adrianchiris reviewed Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will technically not solve the issue, but narrow down the when it might happen.

minor nit otherwise lgtm.

pkg/webhook/validate.go Outdated Show resolved Hide resolved
zeeke added 2 commits July 4, 2023 10:35
@zeeke
Refactor validateSriovOperatorConfig
1a6040f 
Early exit from the function to make the validation
logic more evident and allow further improvements.

Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
@zeeke
webhook: Avoid setting DisableDrain while updating
91ea65f 
Disable the drain while a node is updating its configuration
prevents the config-daemon to uncordon the node, leading
to a node that needs to be manually uncordoned.

Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Thanks for your PR,
To run vendors CIs 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 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.

@adrianchiris adrianchiris merged commit 4ddfd02 into k8snetworkplumbingwg:master Jul 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianchiris adrianchiris adrianchiris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zeeke @coveralls @adrianchiris