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

Validate .Spec.DisableDrain #464

Merged

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
@zeeke zeeke force-pushed the validate-disable-drain branch from 3715423 to 9096080 Compare June 28, 2023 14:40
@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 force-pushed the validate-disable-drain branch from 9096080 to a40aa44 Compare June 28, 2023 15:06
@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

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

@zeeke zeeke force-pushed the validate-disable-drain branch from a40aa44 to c262772 Compare June 28, 2023 15:21
@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.

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
Early exit from the function to make the validation
logic more evident and allow further improvements.

Signed-off-by: Andrea Panattoni <[email protected]>
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 <[email protected]>
@zeeke zeeke force-pushed the validate-disable-drain branch from c262772 to 91ea65f Compare July 4, 2023 08:35
@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
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