feat: NonAdminBackup sync controller #138
Conversation
|
/hold |
|
[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 |
| @@ -89,6 +89,8 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| _, syncBackup := nab.Labels["openshift.io/oadp-nab-synced-from-nacuuid"] | |||
There was a problem hiding this comment.
this was my idea to differ NABs created by user and by sync controller: adding a label to object
what you think about this approach?
There was a problem hiding this comment.
That sounds like a proper design for that. Would use something from https://github.com/openshift/oadp-operator/blob/master/api/v1alpha1/oadp_types.go#L35 and we are using it in few places already in the constant.go
Possibly adding yet one label, but that would mean the object is managed by entirely and I think that may be not true, as once it's synced it's no longer managed by sync controller it's the user responsibility to do something with it like deletion:
app.kubernetes.io/managed-by: self-service-sync-controller
mateusoliveira43
force-pushed
the
feat/non-admin-backup-sync-controller
branch
from
8d585cb to
db30aeb
Compare
internal/controller/nonadminbackupstoragelocation_controller.go
Outdated
Show resolved
Hide resolved
cmd/main.go
Outdated
| @@ -169,6 +170,8 @@ func main() { | |||
| Client: mgr.GetClient(), | |||
| Scheme: mgr.GetScheme(), | |||
| OADPNamespace: oadpNamespace, | |||
| // TODO user input | |||
| SyncPeriod: 5 * time.Minute, | |||
There was a problem hiding this comment.
yeah correct, this would be a user input but we would need a default value.
Have you given any thoughts on how to accept this from the user ? I can think of a new possibilities:
- From
DPA.spec.nonAdminstruct. - Create a struct under
DPA.spec.nonAdmin.Argsand accept the value there. - Introduce a configmap for nacusers/admin users for specifying such things.
- All of the above are extensible enough for similar user inputs in the future NAC world if needed.
There was a problem hiding this comment.
The minimum should be no less than velero's sync controller IMO.
There was a problem hiding this comment.
it will be DPA field under nonAdmin
I would not validate if the value is greater or equal to velero's value, do not know if is simple to be done
There was a problem hiding this comment.
Velero value isn't customizable AFAICT so if you want to, it can be a hardcoded copy.
There was a problem hiding this comment.
There was a problem hiding this comment.
looks like that will eventually set here
and there are code to sync backup to cluster and code to delete orphaned backups as part of this velero upstream sync. So I think this is the same case.
Velero default is 1 minute So our 5 minutes works. Could be as low as 2 min imo.
internal/controller/nonadminbackupstoragelocation_controller.go
Outdated
Show resolved
Hide resolved
| @@ -117,6 +117,14 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| r.removeNabFinalizerUponVeleroBackupDeletion, | |||
| } | |||
|
|
|||
| case function.CheckLabelAnnotationValueIsValid(nab.Labels, constant.NabSyncLabel): | |||
There was a problem hiding this comment.
write test for this scenario
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
mateusoliveira43
force-pushed
the
feat/non-admin-backup-sync-controller
branch
from
3887cb2 to
48273eb
Compare
| @@ -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-20250205190228-fcc5ea4a6c32 | |||
There was a problem hiding this comment.
TODO remove prior to merge
Why the changes were made
Fix #38
Blocked by #139Blocked by #116Blocked by #152Blocked by openshift/oadp-operator#1624
Once NAC Pod is created, NonAdminBackup Synchronizer controller will start, and will run periodically. Its frequency run is defined by admin user in DPA. By default 2 minutes.
NonAdminBackup Synchronizer controller will:
How to test the changes made
Follow install-from-source testing documentation, pointing this branch and my OADP fork branch
feat/nac-nab-sync-controllerCreate NonAdminBackups, using NABSL and default BSL from OADP namespace. Delete them from cluster (but not from object storage). They should be recreated after sync period is reached.