✨ Add validating and mutating webhook for supervisor mode #2651

zhanggbj · 2024-01-23T09:17:39Z

What this PR does / why we need it:

Add validating and mutating webhook for supervisor mode:

- Refactor supervisor config folder and generate webhook config and manifest.
- Enable update validating for vm-operator VSphereMachine to block in-place update of following fields.
	- ImageName
	- ClassName
	- StorageClass
	- MinHardwareVersion

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2595

codecov · 2024-01-23T09:21:33Z

Codecov Report

Attention: Patch coverage is 72.72727% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 64.07%. Comparing base (b89c791) to head (4ec4228).
Report is 2 commits behind head on main.

❗ Current head 4ec4228 differs from pull request most recent head e910494. Consider uploading reports for the commit e910494 to get more accurate results

Files	Patch %	Lines
internal/webhooks/vmoperator/vspheremachine.go	72.09%	10 Missing and 2 partials ⚠️
internal/test/helpers/envtest.go	25.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2651      +/-   ##
==========================================
- Coverage   65.48%   64.07%   -1.42%     
==========================================
  Files         161      161              
  Lines        7525     9391    +1866     
==========================================
+ Hits         4928     6017    +1089     
- Misses       2134     2914     +780     
+ Partials      463      460       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhanggbj · 2024-01-24T05:46:31Z

It's ready for a review. And I'll try to add some tests if possible to make codecov happy.

chrischdi · 2024-01-24T12:03:09Z

It's ready for a review. And I'll try to add some tests if possible to make codecov happy.

Thanks, please ignore codecov, its not failing because of your PR, its the usual codecov flakyness when uploading the report 😭

chrischdi

All in all great code :-)

Some nits.

internal/webhooks/vmoperator/vspheremachine.go

zhanggbj · 2024-01-25T08:37:32Z

All comments addressed, it's ready for another review :-)

zhanggbj · 2024-01-25T08:58:47Z

/test pull-cluster-api-provider-vsphere-test-main

internal/webhooks/vmoperator/vspheremachine.go

internal/webhooks/vmoperator/vspheremachine_test.go

sbueringer · 2024-01-25T17:22:40Z

Once we have the manifests working. Let's please do one manual test with tilt/kind or something (with a real Kubernetes cluster not just unit tests) to verify the webhooks are really active. It's very easy to make a mistake somewhere (e.g. incorrect / missing webhook manifests, incorrect resource match in the webhook config, ...)

zhanggbj · 2024-01-29T07:25:50Z

Once we have the manifests working. Let's please do one manual test with tilt/kind or something (with a real Kubernetes cluster not just unit tests) to verify the webhooks are really active. It's very easy to make a mistake somewhere (e.g. incorrect / missing webhook manifests, incorrect resource match in the webhook config, ...)

Make sense to me, will run tilt to verify it and update result later.

zhanggbj · 2024-02-04T08:13:16Z

/test pull-cluster-api-provider-vsphere-test-integration-main

sbueringer · 2024-05-16T09:35:25Z

@zhanggbj: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-vsphere-test-integration-main 4ec4228 link true /test pull-cluster-api-provider-vsphere-test-integration-main
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Interesting that Prow shows us a job run from February :) https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-vsphere/2651/pull-cluster-api-provider-vsphere-test-integration-main/1759866415801700352

(we deleted the job since then, so let's ignore)

sbueringer · 2024-05-16T09:36:05Z

/lgtm

k8s-ci-robot · 2024-05-16T09:36:10Z

LGTM label has been added.

Git tree hash: 0cbe2e1a85f3451505df183f9a362f7067af0325

sbueringer · 2024-05-16T09:36:22Z

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-supervisor-main

sbueringer · 2024-05-16T10:11:58Z

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

chrischdi · 2024-05-16T10:35:08Z

This is nice :-)

/approve

k8s-ci-robot · 2024-05-16T10:35:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [chrischdi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2024-05-16T11:03:36Z

/hold

for some manual testing

sbueringer · 2024-05-16T11:04:09Z

The govmomi test now failed 2 times in a row. I'm starting to think we broke something

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

k8s-ci-robot · 2024-05-16T11:50:36Z

@zhanggbj: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-test-integration-main	`4ec4228`	link	true	`/test pull-cluster-api-provider-vsphere-test-integration-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

sbueringer · 2024-05-16T15:04:55Z

Okay 3 times in a row seems too often

zhanggbj · 2024-05-17T07:13:00Z

Checked the test artifacts, the failed cluster is quick-start-3x2kmb and it's blocked at WaitForOneKubeadmControlPlaneMachineToExist, which seems blocked due to no ready node.
From the kubelet.log, the latest error is CNI not ready as below, I cannot think of any relationship with this PR change🧐

May 16 11:17:54.628315 quick-start-3x2kmb-7q8wn kubelet[1552]: E0516 11:17:54.628259    1552 kubelet.go:2900] "Container runtime network not ready" networkReady="NetworkReady=false reason:NetworkPluginNotReady message:Network plugin returns error: cni plugin not initialized"

sbueringer · 2024-05-17T07:16:24Z

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

sbueringer · 2024-05-17T07:18:14Z

Same, but the job is 100% green on main. Let's see if it fails again. We might have to figure out why exactly the job is failing

chrischdi · 2024-05-17T07:35:25Z

The one at pull-cluster-api-provider-vsphere-e2e-govmomi-main was in fact a flake. We also have that on release branches.

xref triage

zhanggbj · 2024-05-17T09:22:50Z

Interesting... the flakes all failed at /home/prow/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/controlplane_helpers.go:153

The one at pull-cluster-api-provider-vsphere-e2e-govmomi-main was in fact a flake. We also have that on release branches.

xref triage

sbueringer · 2024-05-17T11:36:08Z

Kk. Maybe just bad luck :)

sbueringer · 2024-05-17T11:36:14Z

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

sbueringer · 2024-05-17T11:36:42Z

Let's run it 2-3 more times overall and if it looks better, then let's merge it (assuming the manual tests are also completed)

sbueringer · 2024-05-17T13:21:42Z

Okay looking better, great :)

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

chrischdi · 2024-05-17T14:14:22Z

/test pull-cluster-api-provider-vsphere-e2e-govmomi-main

sbueringer · 2024-05-22T09:09:12Z

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 23, 2024

k8s-ci-robot requested review from chrischdi and rvanderp3 January 23, 2024 09:17

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2024

zhanggbj force-pushed the vmop_vm_webhook branch from 6749156 to f7b286e Compare January 23, 2024 09:53

zhanggbj changed the title ~~[WIP] ✨ Add validating webhook for vm-operator VSphereMachine~~ ✨ Add validating webhook for vm-operator VSphereMachine Jan 24, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2024

chrischdi reviewed Jan 24, 2024

View reviewed changes

zhanggbj requested a review from chrischdi January 25, 2024 08:37

zhanggbj force-pushed the vmop_vm_webhook branch from f7b286e to 9be9e10 Compare January 25, 2024 08:38

sbueringer reviewed Jan 25, 2024

View reviewed changes

zhanggbj force-pushed the vmop_vm_webhook branch 2 times, most recently from b512e4c to 3ac090a Compare January 31, 2024 06:54

zhanggbj force-pushed the vmop_vm_webhook branch from 3ac090a to 29f365c Compare February 4, 2024 08:19

zhanggbj changed the title ~~✨ Add validating webhook for vm-operator VSphereMachine~~ [WIP] ✨ Add validating webhook for vm-operator VSphereMachine Feb 4, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2024

zhanggbj force-pushed the vmop_vm_webhook branch 4 times, most recently from 3456b2c to 4506f71 Compare February 6, 2024 09:43

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2024

zhanggbj force-pushed the vmop_vm_webhook branch from 29eb9d1 to e910494 Compare May 16, 2024 08:32

k8s-ci-robot assigned sbueringer May 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2024

k8s-ci-robot merged commit 4e8f26a into kubernetes-sigs:main May 22, 2024
18 checks passed

k8s-ci-robot added this to the v1.11 milestone May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add validating and mutating webhook for supervisor mode #2651

✨ Add validating and mutating webhook for supervisor mode #2651

zhanggbj commented Jan 23, 2024 •

edited

codecov bot commented Jan 23, 2024 •

edited

zhanggbj commented Jan 24, 2024

chrischdi commented Jan 24, 2024

chrischdi left a comment

zhanggbj commented Jan 25, 2024 •

edited

zhanggbj commented Jan 25, 2024

sbueringer commented Jan 25, 2024 •

edited

zhanggbj commented Jan 29, 2024

zhanggbj commented Feb 4, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

k8s-ci-robot commented May 16, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

chrischdi commented May 16, 2024

k8s-ci-robot commented May 16, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

k8s-ci-robot commented May 16, 2024 •

edited

sbueringer commented May 16, 2024

zhanggbj commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024

chrischdi commented May 17, 2024 •

edited

zhanggbj commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024 •

edited

sbueringer commented May 17, 2024 •

edited

chrischdi commented May 17, 2024

sbueringer commented May 22, 2024

✨ Add validating and mutating webhook for supervisor mode #2651

✨ Add validating and mutating webhook for supervisor mode #2651

Conversation

zhanggbj commented Jan 23, 2024 • edited

codecov bot commented Jan 23, 2024 • edited

Codecov Report

zhanggbj commented Jan 24, 2024

chrischdi commented Jan 24, 2024

chrischdi left a comment

Choose a reason for hiding this comment

zhanggbj commented Jan 25, 2024 • edited

zhanggbj commented Jan 25, 2024

sbueringer commented Jan 25, 2024 • edited

zhanggbj commented Jan 29, 2024

zhanggbj commented Feb 4, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

k8s-ci-robot commented May 16, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

chrischdi commented May 16, 2024

k8s-ci-robot commented May 16, 2024

sbueringer commented May 16, 2024

sbueringer commented May 16, 2024

k8s-ci-robot commented May 16, 2024 • edited

sbueringer commented May 16, 2024

zhanggbj commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024

chrischdi commented May 17, 2024 • edited

zhanggbj commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024

sbueringer commented May 17, 2024 • edited

sbueringer commented May 17, 2024 • edited

chrischdi commented May 17, 2024

sbueringer commented May 22, 2024

zhanggbj commented Jan 23, 2024 •

edited

codecov bot commented Jan 23, 2024 •

edited

zhanggbj commented Jan 25, 2024 •

edited

sbueringer commented Jan 25, 2024 •

edited

k8s-ci-robot commented May 16, 2024 •

edited

chrischdi commented May 17, 2024 •

edited

sbueringer commented May 17, 2024 •

edited

sbueringer commented May 17, 2024 •

edited