Remove immutable fields when patching VM Service VMs after creation

[dilyar85]

/kind feature

Describe the solution you'd like
Currently, the following fields are immutable after a VM Service VM resource is created (related code in vmop repo):

• ImageName
• ClassName
• StorageClass
• ResourcePolicyName

Therefore, patching these fields after a VM Service VM is created could potentially be denied, as they might be updated during the VM creation by a mutation webhook.

Anything else you would like to add:
For example, in the latest VMOP version, vm.spec.imageName can be mutated from a friendly image name to a "vmi-" name if there is a single VM image that can be resolved. If CAPV continues patching such VM with the previous friendly image name, the request would be denied as the imageName is an immutable field, leading to reconciliation errors for TKG VMs.

Environment:
N/A.

[chrischdi]

Question: does VM Operator only mutate the vm.spec.imageName for Creation?

If it also mutates the same on Update there should not be a patching issue.

[dilyar85]

Question: does VM Operator only mutate the vm.spec.imageName for Creation?

Yes. Once a VM is created, its imageName field cannot be changed which is enforced by the VM validation webhook.

[akutz]

To be clear, the validation webhook to prevent mutating the spec.imageName field always existed. It's just that before it didn't result in an error since CAPV would patch the same value back in, thus resulting in a no-op, not triggering the validation error.

If it also mutates the same on Update there should not be a patching issue.

Also, the reason we do not do this is because the vmi-xxx unique ID should be resolved exactly once, as early as possible. If CAPV later changes the value to a friendly name that must be resovled again, there's a chance (small, but one that does exist) that it could resolve to a different image.

[sbueringer]

Sounds like we should make the corresponding fields on the VSphereMachine immutable in CAPV (currently we don't have any webhooks on this type)

[chrischdi]

Sounds like we should make the corresponding fields on the VSphereMachine immutable in CAPV (currently we don't have any webhooks on this type)

Ack, but as addition: don't try to patch it on an existing object / ignore if it is already set to match the above scenario.

[sbueringer]

It's just pretty confusing to have a field which is then just not applied without any sort of error

[chrischdi]

I think the best we can do on CAPV side is:

• Add a comment on the relevant fields that they only get used when creating a VM
• Adjust the code to not try to update the fields after creation of the vm (to match the above then documented behaviour)
• Add a webhook to make these fields immutable on CAPV side too.

[sbueringer]

We could also return errors in some condition theoretically if the desired state cannot be applied

[sbueringer]

Okay I think now I got the ask

• CAPV set the image name on creation of a vmoprv1.VirtualMachine
• vmoperator resolves the image name and sets the spec field to something else
• CAPV continues to try to write the original image name (which then fails because the imageName is immutable)

So the ideal way to address this is probably:

• Ensure we don't update the imageName after creation of vmoprv1.VirtualMachine
• Also make the field immutable on the vmware.VSphereMachine

[fabriziopandini]

I think we should fix, but this vm-operator behavior of mutating a spec value provided by something else is sort of unusual and unexpected.
IMO, long term, If the VM operator should resolve the value provided to internal names or IDs it should store the resolved value on different fields.

[chrischdi]

Cherry-picks:

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

We will publish releases for 1.7 and 1.8 after merging.

[chrischdi]

/reopen

The fix did not include ResourcePolicyName but MinHardwareRevision instead (which is ok according to https://github.com/vmware-tanzu/vm-operator/blob/main/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go#L143-L149).

We should file another PR which also adds the same behaviour for the ResourcePolicyName.

[k8s-ci-robot]

@chrischdi: Reopened this issue.

In response to this:

/reopen

The fix did not include ResourcePolicyName but MinHardwareRevision instead (which is ok according to https://github.com/vmware-tanzu/vm-operator/blob/main/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go#L143-L149).

We should file another PR which also adds the same behaviour for the ResourcePolicyName.

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-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sbueringer]

@chrischdi Do you still remember where we are with this issue? I think we did it all?

[chrischdi]

I think we can close this.

resourcePolicyName was also implemented:

• 🌱 Prevent updating supervisors VSphereMachine.Spec.ResourcePolicyName #2597
• 🌱 [release-1.7] Prevent updating supervisors VSphereMachine.Spec.ResourcePolicyName #2602
• 🌱 [release-1.8] Prevent updating supervisors VSphereMachine.Spec.ResourcePolicyName #2603

[chrischdi]

/close

[k8s-ci-robot]

@chrischdi: Closing this issue.

In response to this:

/close

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.