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

Adding webhook 'delete' command to remove non-namespaced objects #791

Merged

Conversation

heyvister1
Copy link
Contributor

@heyvister1 heyvister1 commented Oct 14, 2024

sriov-network-operator dynamically creates validating/mutating/clusterroles/clusterrolebinding cluster objects, for specified SriovOperatorConfig. These objects are not being cleaned once sriov operator is uninstalled (via Helm/Openshift).

The proposed PR is to use a small go binary within Helm pre-delete hook. This binary will delete default SriovOperatorConfig, assuming added SriovOperatorConfig finalizer will make sure sriov controller is deleting all generated cluster level objects (e.g. webhooks), prior to operator deployment object is removed by Helm

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.

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 6877210 to 663ad6b Compare October 15, 2024 06:00
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 663ad6b to 450514e Compare October 15, 2024 06:27
@coveralls
Copy link

coveralls commented Oct 15, 2024

Pull Request Test Coverage Report for Build 11577275447

Details

  • 55 of 83 (66.27%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 45.531%

Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/sriovoperatorconfig_controller.go 19 25 76.0%
cmd/sriov-network-operator-config-cleanup/main.go 0 9 0.0%
cmd/sriov-network-operator-config-cleanup/cleanup.go 36 49 73.47%
Files with Coverage Reduction New Missed Lines %
controllers/drain_controller.go 1 68.06%
Totals Coverage Status
Change from base Build 11576125023: 0.6%
Covered Lines: 6775
Relevant Lines: 14880

💛 - Coveralls

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 3 times, most recently from 52d9671 to 693e48d Compare October 20, 2024 15:21
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 693e48d to bd8a6fc Compare October 20, 2024 15:25
@heyvister1
Copy link
Contributor Author

/test-all

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from 13b3cda to e2440c4 Compare October 22, 2024 07:40
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from e2440c4 to a7f10f2 Compare October 22, 2024 07:42
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from 2ee6dad to 56a8346 Compare October 22, 2024 12:06
@heyvister1 heyvister1 requested a review from zeeke October 22, 2024 12:06
@adrianchiris
Copy link
Collaborator

/test-all

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from ec25c78 to 798f5e7 Compare October 22, 2024 14:14
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 798f5e7 to 797755e Compare October 23, 2024 08:38
@heyvister1 heyvister1 requested a review from zeeke October 23, 2024 08:38

// watching 'default' config deletion with context timeout, in case sriov-operator fails to delete 'default' config
watcher, err := sriovcs.SriovOperatorConfigs(namespace).Watch(ctx, metav1.ListOptions{Watch: true})
defer watcher.Stop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

if err is not nil, is it safe to stop the watcher ? or should the defer be moved after the error check ?

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.

overall LGTM, need CI to be green.

added one last comment from my side , thx for the patience ! :)
before we merge please re-work the commits so they do not reflect the review interations but rather the changes done.

e.g

commit 1 : add finalizer to sriovoperatorconfig
commit 2: cleanup binary related changes

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 78f11fa to 7c3a348 Compare October 28, 2024 13:37
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 7c3a348 to 2886ba3 Compare October 28, 2024 19:55
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch from 2886ba3 to b1bb044 Compare October 29, 2024 15:33
@heyvister1
Copy link
Contributor Author

@zeeke could you please add your approval for this PR?

@zeeke zeeke merged commit 5009e99 into k8snetworkplumbingwg:master Oct 30, 2024
14 checks passed
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.

5 participants