Fix for #89 - Use custom UUID for NAB Object #101
Conversation
|
@shubham-pampattiwar FYI please review |
mpryc
force-pushed
the
velero_nab_ownership
branch
from
ac7eba1
to
b20a4cc
Compare
With this change a new UUID is generated to reference parent/child relationship between objects in the Non Admin Controller use cases. The first consumer of this UUID is a Velero Backup, created when the NonAdminBackup object is reconciled. The NonAdminBackup object generates the NAC UUID and stores it in its Status. This prevents users from modifying it. The UUID is later used to create the Velero Backup during reconciliation. While the NAC UUID is currently used as the Velero Backup name, this is not required, as the UUID is also stored as a Velero Backup label, which is used during the reconcile loop. Usage of NAC UUID as Velero Backup name is to easy it's creation. This PR also includes small changes to fix linting issues of the code, as well reworks the tests to properly take advantage of gingko BeforeEach function. Signed-off-by: Michal Pryc <[email protected]>
mpryc
force-pushed
the
velero_nab_ownership
branch
from
b20a4cc
to
2cbd085
Compare
Improved function to properly validate NAC Object Labels Dropped length of Annotation Value validation, as it's not limited to 63 or 256 chars. Signed-off-by: Michal Pryc <[email protected]>
Changes to address code review. Signed-off-by: Michal Pryc <[email protected]>
|
@shubham-pampattiwar @mateusoliveira43 Added commit which I hope answers all comments. |
|
@mpryc ++ |
Renamed function which lists objects by label in a specific namespace. Signed-off-by: Michal Pryc <[email protected]>
mpryc
force-pushed
the
velero_nab_ownership
branch
2 times, most recently
from
72fe8c4
to
90893cb
Compare
Change to include: - Reworked diagram to include one reconcile entry and multiple states - Labels and annotations uses common function to check for the name and namespace value length - Additional test to cover above scenario. Signed-off-by: Michal Pryc <[email protected]>
| gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.ExpectedStatus, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) | ||
|
|
||
| gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackupAfterReconcile, scenario.nonAdminBackupExpectedStatus, oadpNamespace)).To(gomega.Succeed()) | ||
| if scenario.uuidCreatedByReconcile { |
There was a problem hiding this comment.
this check needs to be moved inside checkTestNonAdminBackupStatus function
first expect in this if here is not valid, no? if namespace name is big, it wont be present in nameUUID
There was a problem hiding this comment.
I don't know how to nicely do that without changing the arguments of the checkTestNonAdminBackupStatus function, which I do not want to do at this point. Added TODO and comment in the code.
The problem is that the structure in the nonAdminBackupSingleReconcileScenario can not take advantage of variables in an easy way, so we would need to pass some extra args to the checkTestNonAdminBackupStatus instead of using what's in the nonAdminBackupSingleReconcileScenario, bacause we are using here the nonAdminObjectNamespace whcih is var nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-%v", counter) assigned on every BeforeEach
There was a problem hiding this comment.
note to self: would have done things a little different, setting UUID (only 36 length) as a NAB and Velero Backup label (less secure to use as label instead of status in NAB, but easier to iterate over). After creating UUID, would do get call in NAB and OADP namespace, to confirm UUID is unique in the cluster (#102), but that would not check things in storage provider.
There was a problem hiding this comment.
@mateusoliveira43 I think I responded to all of your comments. Please point me the ones which must be fixed before this will get merged, which we want to fix as follow up and which are just nice to have. Also I am not sure why it's approved by you with so many comments added.
Signed-off-by: Michal Pryc <[email protected]>
mpryc
dismissed stale reviews from mateusoliveira43 and shubham-pampattiwar
via
ba72f89
|
@mateusoliveira43 I've made changes and submitted last commit. There is still one unresolved comment in my opinion, which I've added TODO as I am not sure how to nicely change the current implementation to satisfy your request. Let me know if we can merge it with that or you want to make additional changes. |
|
@shubham-pampattiwar @mateusoliveira43 can we merge this or something else is stopping e.g. I am expected to propose corresponding PR to oadp operator? I am asking because based on this PR I have another one for NAB deletion pending, which I would like to submit for review. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mateusoliveira43, mpryc, shubham-pampattiwar 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 |
|
/lgtm |
With this change a new UUID is generated to reference parent/child relationship between objects in the Non Admin Controller use cases.
The first consumer of this UUID is a Velero Backup, created when the NonAdminBackup object is reconciled.
The NonAdminBackup object generates the NAC UUID and stores it in its Status. This prevents users from modifying it. The UUID is later used to create the Velero Backup during reconciliation.
While the NAC UUID is currently used as the Velero Backup name, this is not required, as the UUID is also stored as a Velero Backup label, which is used during the reconcile loop. Usage of NAC UUID as Velero Backup name is to easy it's creation.
This PR also includes small changes to fix linting issues of the code, as well reworks the tests to properly take advantage of gingko BeforeEach function.
Why the changes were made
Fixes #89
Fixes #90