-
Notifications
You must be signed in to change notification settings - Fork 258
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
GC only if allowed to update DV owner finalizers #2416
GC only if allowed to update DV owner finalizers #2416
Conversation
b29d5fe
to
9dd985a
Compare
for _, pvcOwnerRef := range pvc.OwnerReferences { | ||
if dvOwnerRef.UID == pvcOwnerRef.UID { | ||
refExists = true | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe extratc this as
findRef(pvc.OwnerReferences, dvOwnerRef.UID)
or
existsRef(dvOwnerRef.UID, pvc.OwnerReferences)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already a compact function, so extracting this loop to another func seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it will be obvious what it does without reading two levels of for loop and break statement;
for _, dvOwnerRef := range dv.OwnerReferences {
if !refExists(dvOwnerRef.UID, pvc.OwnerReferences) {
dvOwnerRef.BlockOwnerDeletion = nil
refs = append(refs, dvOwnerRef)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw: this is square complexity, but I suppose we should never see more that a few references so it is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, in the OpenShift case it's actually 1x1 😂
Anyway, I'll extract it as I really want to make you happy today, bro.
You say "Eliminate errors like persistentvolumeclaims SOME_PVC is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers". But I do not understand what does it mean. Why do we have such error? |
Ok, so we are trying to copy ownerReferences from DV to PVC, before deleting DV. And we cannot copy So why it was possible to have |
We have this error in OpenShift where VM and DV are created. However, the PVC is created by CDI which is not allowed to set finalizers of the VM, triggered when PVC is updated with its DV ownerRef to the VM with |
See answer to your other comment. Adding it to the PR description. |
} | ||
} | ||
if !refExists { | ||
dvOwnerRef.BlockOwnerDeletion = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment above this with the admission plugin name and short explanation that we're taking ownerrefs that can be anything in the world?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in the PR description. Looks ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the PR description is awesome I just wanted to make sure we have something in the code that screams it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, adding.
} | ||
} | ||
if !refExists { | ||
dvOwnerRef.BlockOwnerDeletion = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/:
Kubernetes automatically sets this field to true if a [controller](https://kubernetes.io/docs/concepts/architecture/controller/) (for example, the Deployment controller) sets the value of the metadata.ownerReferences field.
I think we may be better with an explicit false value here, but maybe I got this wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably try to test this somehow that I am not lying :D
Maybe push the cdi-controller image to quay and we'll try to swap to it in a running cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it already on @kgoldbla cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
Nice description, thanks. Thinking more about the second part of my question: Don't we want to have I see that it is impossible. Does it mean we loose part of functionality - namely deletion protection - by doing DV GC? |
After DV is GC'ed, PVC is still protected from deletion:
|
So the documentation says this about
For our purposes we use the ownerRefs for cascading deletion of the dependents and we don't really care about the blockOwnerDeletion. So right now if you have a VM with DVTemplates that created the DVs, and then the DVs created the PVCs, if you delete the VM, it also cascade deletes the DVs and PVCs. Now if we do a foreground delete of the VM, I guess it makes sure that the DVs and PVCs are deleted before the VM is deleted. I don't think we care about the order in which the objects are deleted in that scenario. |
I also think the deletion order is not critical here, so |
What is the minimal rbac that would be required to get this to work (keep block owner deletion) in the KubeVirt universe? My preference would be go that route. Then if we encounter a DV that is owned by some other resource (prob very rare in practice) we don't garbage collect it. |
its
Wouldn't we GC it? I thought we would, but would just lose the order mechanism with blockOwnerDeletion set to false |
Not sure if I like this tightly coupled KubeVirt-CDI behavior and different/no-GC for other envs. Why keeping |
/test pull-containerized-data-importer-e2e-ceph |
Just so I understand the problem properly. When KubeVirt created a VM with some DVTemplates, it creates the VM with an ownerRef of the VM, and it has There is no issue because when KubeVirt creates the DV it has the permissions to put a finalizer on the VM. But when CDI tries to set the same ownerRef it can't because it has no permissions to put a finalizer on the VM. Is there a possibility for us to push the responsibility of setting |
b7aaf62
to
c89fad0
Compare
// https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement | ||
func isGarbageCollectionAllowed(dv *cdiv1.DataVolume, log logr.Logger) bool { | ||
for _, dvOwnerRef := range dv.OwnerReferences { | ||
if dvOwnerRef.Kind != "VirtualMachine" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using the authorization APIs to check if you are able to update the finalizer for the parent object rather than checking for a specific type. If you do this, then someone can make garbage collection work for any parent type so long as the provide the required rbac rules in the deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provide the required rbac rules in the deployment
As in, they would provide the RBAC to the cdi controller service account? through their projects?
authorization APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aglitke, now using authorization API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea @aglitke!
406530b
to
1f90644
Compare
1c2edab
to
4ab4594
Compare
4ab4594
to
572bcf6
Compare
PVC gets its DV ownerRefs, which can be any CR. If BlockOwnerDeletion is true, we allow GC only if CDI has RBAC to update the owner finalizers (we explicitly added it for VirtualMachines). BlockOwnerDeletions gets validated with this admission plugin which is enabled in OpenShift, but disabled in our kubevirtci clusters: https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
572bcf6
to
1f80b98
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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:
Approvers can indicate their approval by writing |
/test pull-cdi-generate-verify |
Signed-off-by: Arnon Gilboa agilboa@redhat.com
What this PR does / why we need it:
Eliminate errors like
persistentvolumeclaims SOME_PVC is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers
.We have this error in OpenShift where VM and DV are created. However, the PVC is created by CDI which is not allowed to update finalizers of the VM, triggered when PVC is updated with its DV ownerRef to the VM with
blockOwnerDeletion: true
.PVC gets its DV ownerRefs, which can be any CR. If BlockOwnerDeletion is true, we allow GC only if CDI has RBAC to update the owner finalizers (we explicitly added it for VirtualMachines). BlockOwnerDeletions gets validated with this admission plugin which is enabled in OpenShift, but disabled in our kubevirtci clusters.
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 #
Special notes for your reviewer:
Release note: