feat: NonAdminBackup sync controller #138
Conversation
Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>
|
/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
| logger.Error(err, "Failed to get VeleroBackupStorageLocation") | ||
| return ctrl.Result{}, err | ||
| } | ||
| if backupStorageLocation == nil { |
There was a problem hiding this comment.
one of the reasons I think sync controller should NOT be a separate controller. This check would not be necessary if it was part of NonAdminBackupStorageLocation controller
| } | ||
|
|
||
| // // OPTION 1: copy how Velero does | ||
| // // PROBLEM: NAC Pod will not have Velero Pod files |
There was a problem hiding this comment.
We would need to mount secret and plugins to NAC Pod to allow this approach, right?
I do not know how much work would that be
There was a problem hiding this comment.
I think the sync controller should restore the NAB objects based on the Backup objects that are found in the OADP namespace (by default openshift-adp). I am not sure if the sync controller should really go into s3 BSLs itself?
I do not know why there is anything with secrets to be done, or I am missing something.
Once the BSL is in the OADP namespace the secret is also in that namespace and corresponds to the BSL and not the NABSL.
| // ------------------------------------------------------------------------ | ||
|
|
||
| // OPTION 2: compare BSLs specs | ||
| // PROBLEM: if user deletes NABSL, and them recreates it, velero backup will point to a BSL that does not exist |
There was a problem hiding this comment.
I would try this approach, but it is not perfect
We could document that there are limitations, if there no other options :(
| }), | ||
| // ginkgo.Entry("Should check that NonAdminBackupStorageLocation update changes next sync time, then delete NonAdminBackupStorageLocation", nonAdminBackupStorageLocationFullReconcileScenario{}), | ||
| // ginkgo.Entry("Should sync only finished NonAdminBackups, then delete NonAdminBackupStorageLocation", nonAdminBackupStorageLocationFullReconcileScenario{}), | ||
| // ginkgo.Entry("Should sync only related NonAdminBackupStorageLocation NonAdminBackups, then delete NonAdminBackupStorageLocation", nonAdminBackupStorageLocationFullReconcileScenario{}), |
There was a problem hiding this comment.
test some cross cluster scenarios as well
Why the changes were made
Fix #38
Blocked by #115
How to test the changes made
TODO