fix: Update NAB status phase and condition together #86

mateusoliveira43 · 2024-09-24T18:46:30Z

Why the changes were made

As discussed with the team, we want to do less reconcile steps for NAB full reconcile loop (but keeping only making one update call per reconcile). With this PR, NAB reconcile update phases and conditions together in one update call.

Also, simplified Predicate and Handlers logic.

How to test the changes made

Run tests and check logs (make simulation-test) or test in a cluster following development documentation.

openshift-ci · 2024-09-24T18:46:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

internal/controller/nonadminbackup_controller.go

mateusoliveira43 · 2024-09-25T14:56:32Z

internal/common/function/function.go

+	if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) {
+		return false
+	}
+	// TODO what is a valid uuid?


should I have a different validation here? (from what we talked today, this is field will be removed, right? To add one ourselves)

It needs to be updated, not removed, created an issue for this #89

I think we can use go https://github.com/google/uuid to generate UUID ourselves.

yes or another option would be https://github.com/satori/go.uuid
But Google one seems like its more maintained.

@shubham-pampattiwar the satori one was last updated 6 years ago with ~37K uses, so I think it's better stick to something more active. google/uuid was updated 2 weeks ago so it's much more active project with ~320K uses.

yes certainly, google/uuid is the way to go.

internal/controller/nonadminbackup_controller.go

Signed-off-by: Mateus Oliveira <[email protected]>

internal/common/constant/constant.go

mpryc · 2024-09-30T08:59:01Z

internal/common/function/function.go

@@ -125,9 +127,9 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 	veleroBackupName := fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash)

 	// Ensure the name is within the character limit
-	if len(veleroBackupName) > constant.MaxKubernetesNameLength {
+	if len(veleroBackupName) > validation.DNS1123SubdomainMaxLength {


I like this change.

internal/handler/velerobackup_handler.go

internal/predicate/nonadminbackup_predicate.go

internal/predicate/velerobackup_predicate.go

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar · 2024-09-30T23:21:40Z

internal/common/function/function.go

@@ -125,9 +127,9 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
 	veleroBackupName := fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash)

 	// Ensure the name is within the character limit
-	if len(veleroBackupName) > constant.MaxKubernetesNameLength {
+	if len(veleroBackupName) > validation.DNS1123SubdomainMaxLength {


internal/predicate/velerobackup_predicate.go

internal/predicate/nonadminbackup_predicate.go

internal/predicate/velerobackup_predicate.go

internal/controller/nonadminbackup_controller.go

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar · 2024-10-02T00:37:54Z

I tried creating a NAB backup using this PR and found some discrepancy in NAB status, it is showing phase as Created even though the VeleroBackup is Completed. Can you please double check for sanity ?

status:                                                                                                                                             17:33:00 [3/1874]
  conditions:                                                                                                                                                        
  - lastTransitionTime: "2024-10-02T00:31:42Z"                                                                                                                       
    message: backup accepted                                                                                                                                         
    reason: BackupAccepted
    status: "True"
    type: Accepted
  - lastTransitionTime: "2024-10-02T00:31:42Z"
    message: Created Velero Backup object 
    reason: BackupScheduled
    status: "True"
    type: Queued
  phase: Created
  veleroBackupName: nab-nacuser-e68ec47f50dc20
  veleroBackupNamespace: openshift-adp
  veleroBackupStatus:
    completionTimestamp: "2024-10-02T00:31:54Z"
    expiration: "2024-11-01T00:31:42Z"
    formatVersion: 1.1.0
    phase: Completed
    progress:
      itemsBackedUp: 71
      totalItems: 71

mateusoliveira43 · 2024-10-02T11:51:36Z

@shubham-pampattiwar this is expected per design https://github.com/migtools/oadp-non-admin/blob/master/docs/design/nab_status_update.md#phase

we only have 3 phases, created is the last one. We can change that

shubham-pampattiwar · 2024-10-02T13:15:24Z

@mateusoliveira43 yeah correct, On one hand Created seemed a bit out of place to me (maybe just becasue not used to seeing it) but on the other hand we do not want to duplicate data from VeleroBackup. Anyway if we wanted to change, it would be a separate PR.

mpryc · 2024-10-03T06:36:05Z

@mateusoliveira43 yeah correct, On one hand Created seemed a bit out of place to me (maybe just becasue not used to seeing it) but on the other hand we do not want to duplicate data from VeleroBackup. Anyway if we wanted to change, it would be a separate PR.

We have the veleroBackupStatus which reflects the state of the Backup.

On the other hand we could also think about 4th phase in NAB that will be Completed when the veleroBackupStatus also is completed without error.

mpryc

/lgtm

openshift-ci · 2024-10-03T06:36:38Z

[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:

~~OWNERS~~ [mateusoliveira43,mpryc,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Sep 24, 2024

openshift-ci bot added the approved label Sep 24, 2024

mateusoliveira43 commented Sep 24, 2024

View reviewed changes

internal/controller/nonadminbackup_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Sep 25, 2024

View reviewed changes

internal/controller/nonadminbackup_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Sep 25, 2024

View reviewed changes

internal/controller/nonadminbackup_controller.go Outdated Show resolved Hide resolved

fix: Update NAB status phase and condition together

f5e9c6b

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 force-pushed the fix/update-phase-and-condition-together branch from 1d75275 to f5e9c6b Compare September 25, 2024 18:40

mateusoliveira43 marked this pull request as ready for review September 25, 2024 18:43

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 25, 2024

openshift-ci bot requested review from kaovilai and sseago September 25, 2024 18:43

mpryc requested changes Sep 30, 2024

View reviewed changes

fixup! fix: Update NAB status phase and condition together

eb99e1d

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar reviewed Oct 1, 2024

View reviewed changes

mpryc mentioned this pull request Oct 1, 2024

Use custom UUID for NAB Object to Velero Backup Object mapping instead of the Kube UUID #89

Closed

fixup! fix: Update NAB status phase and condition together

348bd2a

Signed-off-by: Mateus Oliveira <[email protected]>

shubham-pampattiwar approved these changes Oct 2, 2024

View reviewed changes

kaovilai requested a review from mpryc October 2, 2024 14:18

mpryc approved these changes Oct 3, 2024

View reviewed changes

openshift-ci bot assigned mpryc Oct 3, 2024

openshift-ci bot added the lgtm label Oct 3, 2024

openshift-merge-bot bot merged commit 0d30f21 into migtools:master Oct 3, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Update NAB status phase and condition together #86

fix: Update NAB status phase and condition together #86

mateusoliveira43 commented Sep 24, 2024

openshift-ci bot commented Sep 24, 2024

mateusoliveira43 Sep 25, 2024

shubham-pampattiwar Sep 25, 2024

mpryc Sep 25, 2024

shubham-pampattiwar Sep 25, 2024

mpryc Oct 1, 2024

shubham-pampattiwar Oct 1, 2024

mpryc Sep 30, 2024

shubham-pampattiwar Sep 30, 2024

shubham-pampattiwar Sep 30, 2024

shubham-pampattiwar commented Oct 2, 2024

mateusoliveira43 commented Oct 2, 2024

shubham-pampattiwar commented Oct 2, 2024

mpryc commented Oct 3, 2024

mpryc left a comment

openshift-ci bot commented Oct 3, 2024

fix: Update NAB status phase and condition together #86

fix: Update NAB status phase and condition together #86

Conversation

mateusoliveira43 commented Sep 24, 2024

Why the changes were made

How to test the changes made

openshift-ci bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham-pampattiwar commented Oct 2, 2024

mateusoliveira43 commented Oct 2, 2024

shubham-pampattiwar commented Oct 2, 2024

mpryc commented Oct 3, 2024

mpryc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2024