ssp-operator
ssp-operator copied to clipboard
kubevirt
fix(instancetypes): Protect instancetype/preference `controllerRevision` objects
What this PR does / why we need it:
It refactors the VAP and VAPB VM delete protection function names to be more descriptive.
Moreover, it enables a protection for instancetype and preference controllerRevision objects, which ensures that they are not deleted before VirtualMachine objects. This is due to the fact that in some scenarios, such as when a VM is deleted protected, if the controllerRevision is deleted, but the VM object is still alive, users will not be able to patch or update the VM object. This leads to virtually deadlocking the VM object.
The VAP ensures that only the Kubernetes garbage collector and kubevirt-controller are able to delete these objects:
- Garbage-collector: It deletes objects when the
ownerReference'd object is removed, e.g., the instancetype "test-it" withownerReference: test-vmis removed when the VM test-vm is removed. Therefore, by only allowing it to drop these objects, we ensure the deletion order. - Kubevirt-controller: It is required to no block this SA because it is the main owner of these
controllerRevision, it needs to delete them in upgrade scenarios, live migration, etc.
Which issue(s) this PR fixes:
Fixes # CNV-69511
Special notes for your reviewer:
Release note:
Add protection to preference and instancetype `controllerRevision` objects.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign ksimon1 for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Quality Gate failed
Failed conditions
23.4% Duplication on New Code (required ≤ 3%)
See analysis details on SonarQube Cloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Reflecting on this I'm not convinced by the approach as I'm worried it's too brittle and will break backup/restore tooling.
Could you explain how it will break backup/restore? Won't these operations be executed by kubevirt-controller?
What about having SSP itself reconcile all VMs with the delete protection label and then applying or removing it from any additional resource we also need to protect? Possibly using the objectgraph subresource API?
Yes, we could apply the same concept. However, what other objects should we protect? AFAIK, we just have controllerRevision and VMs. One question, won't the controllerRevision be reconciled if we set the protection label?
This would keep the VAPs simple, don't delete anything with the delete protection label while ensuring we programmatically include resources the VM depends on.
The requirements of this feature are just to protect VMs, in the past we explored what would happen if we overprotect objects like, launcher pods, VMIs, etc. and the result for instance, vms won't be able to stop, migrate, etc.
Reflecting on this I'm not convinced by the approach as I'm worried it's too brittle and will break backup/restore tooling.
Could you explain how it will break backup/restore? Won't these operations be executed by kubevirt-controller?
No, not always AFAIK.
What about having SSP itself reconcile all VMs with the delete protection label and then applying or removing it from any additional resource we also need to protect? Possibly using the objectgraph subresource API?
Yes, we could apply the same concept. However, what other objects should we protect? AFAIK, we just have
controllerRevisionand VMs.
We could use the ObjectGraph to understand if there are any other objects we would also need to protect.
One question, won't the
controllerRevisionbe reconciled if we set the protection label?
Reconciled by what? SSP would only watch VMs, adding the labels when a VM is updated with the label and removing them when the label is dropped from the VM.
This would keep the VAPs simple, don't delete anything with the delete protection label while ensuring we programmatically include resources the VM depends on.
The requirements of this feature are just to protect VMs, in the past we explored what would happen if we overprotect objects like, launcher pods, VMIs, etc. and the result for instance, vms won't be able to stop, migrate, etc.
Yeah understood, we wouldn't need to protect everything listed by the objectgraph but I still think it should be used as a potential source of truth for these objects instead of hardcoding VAPs for all objects in the cluster.
Reflecting on this I'm not convinced by the approach as I'm worried it's too brittle and will break backup/restore tooling.
Could you explain how it will break backup/restore? Won't these operations be executed by kubevirt-controller?
No, not always AFAIK. Understood.
What about having SSP itself reconcile all VMs with the delete protection label and then applying or removing it from any additional resource we also need to protect? Possibly using the objectgraph subresource API?
Yes, we could apply the same concept. However, what other objects should we protect? AFAIK, we just have
controllerRevisionand VMs.We could use the ObjectGraph to understand if there are any other objects we would also need to protect.
One question, won't the
controllerRevisionbe reconciled if we set the protection label?Reconciled by what? SSP would only watch VMs, adding the labels when a VM is updated with the label and removing them when the label is dropped from the VM.
IIUC, the idea is: If a VM is updated with the protection label, we update the associated controllerRevisions setting the label, the question is: will the virt-controller remove the label we just set?
This would keep the VAPs simple, don't delete anything with the delete protection label while ensuring we programmatically include resources the VM depends on.
The requirements of this feature are just to protect VMs, in the past we explored what would happen if we overprotect objects like, launcher pods, VMIs, etc. and the result for instance, vms won't be able to stop, migrate, etc.
Yeah understood, we wouldn't need to protect everything listed by the objectgraph but I still think it should be used as a potential source of truth for these objects instead of hardcoding VAPs for all objects in the cluster.
Fair point. As you suggested, we can programmatically include just some of them in the VAP and reconcile the dependants to include the protection label. However, IMHO, we should be safe and only include controllerRevisions and VirtualMachines for now.
@lyarwood isn't it a generic problem, independently from the deletion protection, that a controller revision of a instancetype/preference can be deleted before the related VM is deleted? As long as there is a controller revision per VM, might there be a generic solution like having a finalizer on the controller revision, and during VM deletion the finalizer on the controller revision is removed?
IIUC, the idea is: If a VM is updated with the protection label, we update the associated
controllerRevisions setting the label, the question is: will the virt-controller remove the label we just set?
No it will not.
Fair point. As you suggested, we can programmatically include just some of them in the VAP and reconcile the dependants to include the protection label. However, IMHO, we should be safe and only include
controllerRevisionsandVirtualMachinesfor now.
Yup that's fine.
@lyarwood isn't it a generic problem, independently from the deletion protection, that a controller revision of a instancetype/preference can be deleted before the related VM is deleted? As long as there is a controller revision per VM, might there be a generic solution like having a finalizer on the controller revision, and during VM deletion the finalizer on the controller revision is removed?
Yes it can also be framed as a generic issue but there's lots of hidden complexity and cost to adding finalizers at this stage that I'd like to avoid if possible. If folks are against a solution for this in SSP I can take a deeper look but it's not going to be as simple as adding and removing a finalizer on create and delete respectively.
@0xFelix, any thoughts on this?
I don't see benefit in contrast to the added complexity. We should not be protecting CRs at all.
Why is it not enough to clear either spec.instancetype.revisionName or spec.preference.revisionName before or while deleting the deletion protection label? Please also note that in later versions of KubeVirt (>= 1.5.0) the revisionName fields are no longer filled in automatically but only at the user's request.
Are there no further admission validations for VMs that could cause the same behavior after deleting a resource on which a VM depends?
isn't it a generic problem, independently from the deletion protection, that a controller revision of a instancetype/preference can be deleted before the related VM is deleted?
This is a valid concern, but it only prevents the deletion of a VM in combination with this external feature.
Please also note that in later versions of KubeVirt (>= 1.5.0) the revisionName fields are no longer filled in automatically but only at the user's request.
We will still see a failure attempting to mutate the labels after this as it's the lookup of the referenced CR that's failing. The workaround would remain the same.
What about going the dependency tree upwards instead of downwards similar to the namespace deletion protection in https://openkruise.io/docs/user-manuals/deletionprotection / https://github.com/openkruise/kruise/blob/master/pkg/webhook/util/deletionprotection/deletion_protection.go#L76 ? Would it make sense to block the namespace deletion, as long as it hosts a protected VM? (Even I am unsure if it is implementable without race conditions).
What about going the dependency tree upwards instead of downwards similar to the namespace deletion protection in https://openkruise.io/docs/user-manuals/deletionprotection / https://github.com/openkruise/kruise/blob/master/pkg/webhook/util/deletionprotection/deletion_protection.go#L76 ? Would it make sense to block the namespace deletion, as long as it hosts a protected VM? (Even I am unsure if it is implementable without race conditions).
The only limitation I can see is; it won't be implementable with just a VAP. A webhook will be required. VAPs can't look up of other objects rather than the one you are currently modifying.
The only limitation I can see is; it won't be implementable with just a VAP. A webhook will be required. VAPs can't look up of other objects rather than the one you are currently modifying.
For a good reason, Webhooks should not depend on foreign data because there is no strong consistency in kubernets.
To use finalizes on the controllerRevision there's lots of hidden complexity and cost.
Might it solve the initial issue if there are new controller revisions created when to old ones are deleted, e.g. in a recocile loop?
The only limitation I can see is; it won't be implementable with just a VAP. A webhook will be required. VAPs can't look up of other objects rather than the one you are currently modifying.
For a good reason, Webhooks should not depend on foreign data because there is no strong consistency in kubernets.
To use finalizes on the controllerRevision there's lots of hidden complexity and cost.
Might it solve the initial issue if there are new controller revisions created when to old ones are deleted, e.g. in a recocile loop?
Maybe this is the way to go, IMHO, if the user deletes the controller revision while the VM is still running, it should be recreated. WDYT @lyarwood @0xFelix?
We should avoid simply recreating the CR, as instancetypes or preferences may have changed since they were captured previously. Doing so would defeat the purpose of using a CR to begin with.