feat: Garbage collector #152
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mateusoliveira43 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 |
| // TODO check UUID as well? | ||
| if !function.CheckVeleroBackupAnnotations(&backup) { | ||
| logger.V(1).Info("Backup does not have required annotations", constant.NameString, backup.Name) | ||
| // TODO delete? |
There was a problem hiding this comment.
object has NAC labels, but no NAC annotations. What should we do?
There was a problem hiding this comment.
This should not happen
but if for some reason it does I think it would be logged as warning or INFO not a DEBUG. Alternative if you want this to be visible in OpenShift’s Events tab, you can emit an Event from your controller. This will help cluster admins and users see what’s happening without digging into logs.There was a problem hiding this comment.
I think there is no warning level in this lib we use
what is the decision here?
There was a problem hiding this comment.
Annotations are only to ensure ownership reference exists between NA* and Velero objects.
If labels are there it means that they are managed and created by the controller.
If annotations are missing it means that for some different update/patch to the Velero object the ownership reference was removed.
On a second thought I believe this is something that is going to be subject for GC removal.
Even if the Velero Syncs back objects from the S3 storage to the openshift-adp namespace, those annotations are preserved.
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
mateusoliveira43
force-pushed
the
feat/garbage-collector
branch
from
05440b7 to
a216e41
Compare
| // TODO duplication in delete logic | ||
| // TODO do deletion in parallel? | ||
|
|
||
| // TODO delete secret as well? |
There was a problem hiding this comment.
should also check for possible leftover secrets?
There was a problem hiding this comment.
Correct, those should be cleaned up by the NABSL controller. On top of that now when I think about that we may want to add OwnerReference between Secret <-> Velero BSL object. This will cause Secret to be removed if the Velero BSL object is gone, so we don't have to implement this part in GC coontroller ?
There was a problem hiding this comment.
that would be good
but secret is created prior to bsl, can be added independent of the creation order?
There was a problem hiding this comment.
You are right. We can not add or patch the ownerReference on the Secret which already exists. What we could do is to add it vice-versa to the Velero BSL and make Velero BSL having ownerReference to the secret and then add blockOwnerDeletion on the Secret object. This should prevent secret to be removed independently from the BSL, but as always it's not 100% guarantee and there are edge cases where this still may happen.
|
/hold |
|
@mateusoliveira43 I am not sure if you will need this part, but implemented in case you will: #162 |
|
@mateusoliveira43 @shubham-pampattiwar After going again through our drawings I found that #162 isn't really what we discussed and implemented the #163 which should now satisfy the deletion workflow and GC. |
Signed-off-by: Mateus Oliveira <[email protected]>
| @@ -82,3 +82,5 @@ require ( | |||
| ) | |||
|
|
|||
| replace github.com/vmware-tanzu/velero => github.com/openshift/velero v0.10.2-0.20241211163542-fa8f2486175b | |||
|
|
|||
| replace github.com/openshift/oadp-operator => github.com/mateusoliveira43/oadp-operator v0.0.0-20250131150824-b010f27c07bb | |||
There was a problem hiding this comment.
TODO remove prior to merge
Why the changes were made
Blocked by #139Fix #145
If for some reason a non admin object is deleted without waiting its finalizer to delete the related Velero object, that object will become a leftover in OADP namespace, which non admin user can not clean up. To avoid these scenarios, garbage collector will run periodically to clean these possible leftover objects.
How to test the changes made
Follow install-from-source testing documentation, pointing this branch and my OADP fork branch
feat/nac-garbage-collector