🐛 Skip updating VMOp immutable fields #2566

srm09 · 2023-12-22T02:34:37Z

What this PR does / why we need it:
VMOp mutation webhook updates the VMOp Spec.ImageName to a user-friendly name prefixed vmi- or cvmi-. This causes the reconciliation of the VSphereMachine to fail since the controllers copy the all spec fields to the VMOp VirtualMachine on every reconcile. Since imageName is an immutable field we see the below error:

Name:         guestcluster1-control-plane-bz7k9-c9whq
Namespace:    gctest
Labels:       cluster.x-k8s.io/cluster-name=guestcluster1
              cluster.x-k8s.io/control-plane=
              cluster.x-k8s.io/control-plane-name=guestcluster1-control-plane
              topology.cluster.x-k8s.io/owned=
Annotations:  cluster.x-k8s.io/cloned-from-groupkind: VSphereMachineTemplate.vmware.infrastructure.cluster.x-k8s.io
              cluster.x-k8s.io/cloned-from-name: guestcluster1-control-plane-bz7k9
              run.tanzu.vmware.com/resolve-os-image: os-name=photon
API Version:  vmware.infrastructure.cluster.x-k8s.io/v1beta1
Kind:         VSphereMachine
Metadata:
  Creation Timestamp:  2023-12-14T20:01:42Z
  Finalizers:
    vspheremachine.infrastructure.cluster.x-k8s.io
  Generation:  2
  Owner References:
    API Version:           cluster.x-k8s.io/v1beta1
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  Machine
    Name:                  guestcluster1-control-plane-wr5w2
    UID:                   fd9da4eb-8b09-4484-9e1a-90a284861f4c
  Resource Version:        415153
  UID:                     1ba496ef-ccc4-4a6a-9e29-4a8009ec7957
Spec:
  Class Name:      best-effort-xsmall
  Failure Domain:  vmware-system-legacy
  Image Name:      tkgs-ova-photon-3-v1.23.8---vmware.3-tkg.1
  Storage Class:   wcpglobalstorageprofile
Status:
  Conditions:
    Last Transition Time:  2023-12-14T20:01:46Z
    Message:               failed to create or update VirtualMachine: admission webhook "default.validating.virtualmachine.v1alpha1.vmoperator.vmware.com" denied the request: spec.imageName: Invalid value: "tkgs-ova-photon-3-v1.23.8---vmware.3-tkg.1": field is immutable
    Reason:                VMCreationFailed
    Severity:              Warning
    Status:                False
    Type:                  Ready
    Last Transition Time:  2023-12-14T20:01:46Z
    Message:               failed to create or update VirtualMachine: admission webhook "default.validating.virtualmachine.v1alpha1.vmoperator.vmware.com" denied the request: spec.imageName: Invalid value: "tkgs-ova-photon-3-v1.23.8---vmware.3-tkg.1": field is immutable
    Reason:                VMCreationFailed
    Severity:              Warning
    Status:                False
    Type:                  VMProvisioned
  Vmstatus:                pending

This patch ensures the immutable fields on the VMop VM Spec do not get set if these fields on the existing VM object are not empty.

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 #2430

codecov · 2023-12-22T02:39:02Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d8bb2e3) 64.67% compared to head (99f247e) 64.32%.
Report is 12 commits behind head on main.

Files	Patch %	Lines
pkg/services/vmoperator/vmopmachine.go	86.36%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
- Coverage   64.67%   64.32%   -0.36%     
==========================================
  Files         118      118              
  Lines        8539     8585      +46     
==========================================
- Hits         5523     5522       -1     
- Misses       2595     2630      +35     
- Partials      421      433      +12

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

srm09 · 2023-12-22T05:08:39Z

/override pull-cluster-api-provider-vsphere-apidiff-main

k8s-ci-robot · 2023-12-22T05:08:42Z

@srm09: Overrode contexts on behalf of srm09: pull-cluster-api-provider-vsphere-apidiff-main

In response to this:

/override pull-cluster-api-provider-vsphere-apidiff-main

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/test-infra repository.

chrischdi

First of all thanks for the PR!

This PR also contains some optimizations we might not want to cherry-pick.

Should we reduce this PR to the bare minimum required to solve the related issue and do the other changes on a separate PR?

That would come down to the changes in:
https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/2566/files#diff-c2e693327f8831614b68401c025d9322280d92938f4a064bcd949656feb52770R179-R337

if I did not miss a point here.

This way it would also be more easily cherry-pickable.

pkg/services/vimmachine.go

chrischdi · 2023-12-22T09:28:09Z

pkg/services/vimmachine.go

@@ -47,8 +47,31 @@ type VimMachineService struct {
 	Client client.Client
 }

+// GetMachinesInCluster returns a list of VSphereMachine objects belonging to the cluster.
+func (v *VimMachineService) GetMachinesInCluster(


Don't know if we should make this part of the VimMachineService. We don't even use v.Client :think:

Makes sense, since the client is a part of the VimMachineService and VMopMachineService, no use passing them in the function call. Missed that since most of it was copying over the function as is from the util package.

pkg/services/vmoperator/constants.go

pkg/services/vmoperator/vmopmachine_test.go

fabriziopandini

just a couple of nits from my side

pkg/services/vmoperator/vmoperator_suite_test.go

pkg/services/vmoperator/vmopmachine_test.go

chrischdi

Some more nits + please move the predicate relevant changes to a separate PR :-)

pkg/services/vmoperator/vmopmachine.go

pkg/util/testutil.go

controllers/vspheremachine_controller.go

Signed-off-by: Sagar Muchhal <[email protected]>

srm09 · 2023-12-27T18:59:03Z

Reverted all the irrelavant changes so that this one just handles skipping immutable fields

chrischdi

/lgtm
/approve

k8s-ci-robot · 2023-12-28T07:10:52Z

LGTM label has been added.

Git tree hash: ce01bd84254186927aea547b9ac17a39d3909431

k8s-ci-robot · 2023-12-28T07:10:53Z

[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

chrischdi · 2023-12-28T07:11:01Z

/cherry-pick release-1.8

k8s-infra-cherrypick-robot · 2023-12-28T07:11:03Z

@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.8 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.8

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/test-infra repository.

chrischdi · 2023-12-28T07:11:11Z

/cherry-pick release-1.7

k8s-infra-cherrypick-robot · 2023-12-28T07:11:13Z

@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.7

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/test-infra repository.

k8s-infra-cherrypick-robot · 2023-12-28T07:16:06Z

@chrischdi: #2566 failed to apply on top of branch "release-1.8":

Applying: Skip updating immutable VMOp fields
Using index info to reconstruct a base tree...
M	pkg/services/vmoperator/vmopmachine.go
M	pkg/services/vmoperator/vmopmachine_test.go
M	pkg/util/testutil.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/testutil.go
Auto-merging pkg/services/vmoperator/vmopmachine_test.go
CONFLICT (content): Merge conflict in pkg/services/vmoperator/vmopmachine_test.go
Auto-merging pkg/services/vmoperator/vmopmachine.go
CONFLICT (content): Merge conflict in pkg/services/vmoperator/vmopmachine.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Skip updating immutable VMOp fields
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.8

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/test-infra repository.

k8s-infra-cherrypick-robot · 2023-12-28T07:16:39Z

@chrischdi: #2566 failed to apply on top of branch "release-1.7":

Applying: Skip updating immutable VMOp fields
Using index info to reconstruct a base tree...
M	pkg/services/vmoperator/vmopmachine.go
M	pkg/services/vmoperator/vmopmachine_test.go
M	pkg/util/testutil.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/testutil.go
Auto-merging pkg/services/vmoperator/vmopmachine_test.go
CONFLICT (content): Merge conflict in pkg/services/vmoperator/vmopmachine_test.go
Auto-merging pkg/services/vmoperator/vmopmachine.go
CONFLICT (content): Merge conflict in pkg/services/vmoperator/vmopmachine.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Skip updating immutable VMOp fields
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.7

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/test-infra repository.

sbueringer · 2024-01-02T14:22:16Z

pkg/services/vmoperator/vmopmachine.go

 		// NOTE: Set field-by-field in order to preserve changes made directly
 		//  to the VirtualMachine spec by other sources (e.g. the cloud provider)


On the issue we wrote that the following fields are immutable:

ImageName

ClassName

StorageClass

ResourcePolicyName

Here we stop mutating / reconciling:

ImageName

ClassName

StorageClass

MinHardwareVersion

(MinHardwareVersion instead of ResourcePolicyName)

Q: Is this intended?

Additionally, @srm09 @chrischdi @fabriziopandini What do we think about making those 4 fields immutable on the VSphereMachine? (and also updating the godoc comments accordingly)

I think it's confusing if folks can update the fields and then they are just silently not reconciled.

According vm-operator code it is als MinHardwareVersion:
https://github.com/vmware-tanzu/vm-operator/blob/main/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go#L143-L149

but great catch, we should also have the same behaviour for ResourcePolicyName!

Also +1 for making these immutable on CAPV side. We will have to introduce webhooks for vm-operator mode capv first!

created #2595 to track this validating webhook part.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 22, 2023

k8s-ci-robot requested review from fabriziopandini and vrabbi December 22, 2023 02:34

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 22, 2023

srm09 force-pushed the vmop/ignore-immutable-fields branch 2 times, most recently from b6bd18c to 178bf0c Compare December 22, 2023 03:42

srm09 force-pushed the vmop/ignore-immutable-fields branch from 178bf0c to b7a724c Compare December 22, 2023 05:44

chrischdi reviewed Dec 22, 2023

View reviewed changes

fabriziopandini reviewed Dec 22, 2023

View reviewed changes

pkg/services/vmoperator/vmoperator_suite_test.go Outdated Show resolved Hide resolved

pkg/services/vmoperator/vmopmachine_test.go Outdated Show resolved Hide resolved

pkg/services/vmoperator/vmopmachine_test.go Outdated Show resolved Hide resolved

srm09 force-pushed the vmop/ignore-immutable-fields branch 2 times, most recently from 76f70ab to 4daa7e7 Compare December 26, 2023 22:44

chrischdi reviewed Dec 27, 2023

View reviewed changes

Skip updating immutable VMOp fields

99f247e

Signed-off-by: Sagar Muchhal <[email protected]>

srm09 force-pushed the vmop/ignore-immutable-fields branch from 4daa7e7 to 99f247e Compare December 27, 2023 18:53

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2023

srm09 mentioned this pull request Dec 27, 2023

🌱 Refactor: Get VSphereMachine functions #2577

Merged

chrischdi reviewed Dec 28, 2023

View reviewed changes

k8s-ci-robot assigned chrischdi Dec 28, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 28, 2023

k8s-ci-robot merged commit c60d8ba into kubernetes-sigs:main Dec 28, 2023
19 checks passed

k8s-ci-robot added this to the v1.9 milestone Dec 28, 2023

This was referenced Dec 28, 2023

🐛 [release-1.8] Skip updating immutable VMOp fields #2579

Merged

🐛 [release-1.7] Skip updating immutable VMOp fields #2580

Merged

sbueringer reviewed Jan 2, 2024

View reviewed changes

This was referenced Jan 8, 2024

Implement validating webhook for vm-operator VSphereMachine #2595

Closed

🌱 Prevent updating supervisors VSphereMachine.Spec.ResourcePolicyName #2597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Skip updating VMOp immutable fields #2566

🐛 Skip updating VMOp immutable fields #2566

srm09 commented Dec 22, 2023 •

edited

Loading

codecov bot commented Dec 22, 2023 •

edited

Loading

srm09 commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

chrischdi left a comment •

edited

Loading

chrischdi Dec 22, 2023

srm09 Dec 22, 2023

fabriziopandini left a comment

chrischdi left a comment

srm09 commented Dec 27, 2023

chrischdi left a comment

k8s-ci-robot commented Dec 28, 2023

k8s-ci-robot commented Dec 28, 2023

chrischdi commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

chrischdi commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

sbueringer Jan 2, 2024

chrischdi Jan 8, 2024 •

edited

Loading

chrischdi Jan 8, 2024

sbueringer Jan 8, 2024

		// NOTE: Set field-by-field in order to preserve changes made directly
		// to the VirtualMachine spec by other sources (e.g. the cloud provider)

🐛 Skip updating VMOp immutable fields #2566

🐛 Skip updating VMOp immutable fields #2566

Conversation

srm09 commented Dec 22, 2023 • edited Loading

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

srm09 commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

chrischdi left a comment • edited Loading

Choose a reason for hiding this comment

chrischdi Dec 22, 2023

Choose a reason for hiding this comment

srm09 Dec 22, 2023

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

srm09 commented Dec 27, 2023

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 28, 2023

k8s-ci-robot commented Dec 28, 2023

chrischdi commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

chrischdi commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

k8s-infra-cherrypick-robot commented Dec 28, 2023

sbueringer Jan 2, 2024

Choose a reason for hiding this comment

chrischdi Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

chrischdi Jan 8, 2024

Choose a reason for hiding this comment

sbueringer Jan 8, 2024

Choose a reason for hiding this comment

srm09 commented Dec 22, 2023 •

edited

Loading

codecov bot commented Dec 22, 2023 •

edited

Loading

chrischdi left a comment •

edited

Loading

chrischdi Jan 8, 2024 •

edited

Loading