Adjust the Status of NonAdminBackup #13

mpryc · 2024-03-06T12:56:39Z

Moves Status outside of Spec and adjusts this to reflect
Velero Backup status as well additional Status when the
Spec within NonAdminBackup is not defined.

To test:
Scenario 1:

Create NonAdminBackup without BackupSpec, for example:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example
  namespace: nacproject
spec: {}

Check that the Status contains the desired fields:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-03-06T12:53:27Z'
  generation: 1
  managedFields:
    - apiVersion: nac.oadp.openshift.io/v1alpha1
      fieldsType: FieldsV1
      fieldsV1:
        'f:spec': {}
      manager: Mozilla
      operation: Update
      time: '2024-03-06T12:53:27Z'
    - apiVersion: nac.oadp.openshift.io/v1alpha1
      fieldsType: FieldsV1
      fieldsV1:
        'f:status':
          .: {}
          'f:failureReason': {}
          'f:phase': {}
      manager: main
      operation: Update
      subresource: status
      time: '2024-03-06T12:53:44Z'
  name: example
  namespace: nacproject
  resourceVersion: '37684386'
  uid: bd21d62c-3e86-49de-a2e4-23d42db94ddd
spec: {}
status:
  failureReason: NonAdminBackup CR does not contain valid VeleroBackupSpec
  phase: FailedValidation

Scenario 2:

Make sure to include the backupSpec:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example
  namespace: nacproject
spec:
  backupSpec: {}

Save the object and check if the Status was populated correctly based on the Velero Backup Status. It can be Failed, but should reflect what original Velero Backup object contains:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-03-06T12:55:55Z'
  generation: 2
  managedFields:
    - apiVersion: nac.oadp.openshift.io/v1alpha1
      fieldsType: FieldsV1
      fieldsV1:
        'f:spec':
          .: {}
          'f:backupSpec': {}
      manager: Mozilla
      operation: Update
      time: '2024-03-06T12:55:55Z'
    - apiVersion: nac.oadp.openshift.io/v1alpha1
      fieldsType: FieldsV1
      fieldsV1:
        'f:status':
          .: {}
          'f:expiration': {}
          'f:failureReason': {}
          'f:formatVersion': {}
          'f:phase': {}
          'f:startTimestamp': {}
          'f:version': {}
      manager: main
      operation: Update
      subresource: status
      time: '2024-03-06T12:55:55Z'
    - apiVersion: nac.oadp.openshift.io/v1alpha1
      fieldsType: FieldsV1
      fieldsV1:
        'f:spec':
          'f:backupSpec':
            'f:itemOperationTimeout': {}
            'f:metadata': {}
            'f:volumeSnapshotLocations': {}
            'f:snapshotMoveData': {}
            'f:defaultVolumesToFsBackup': {}
            'f:ttl': {}
            'f:csiSnapshotTimeout': {}
            'f:storageLocation': {}
            'f:hooks': {}
      manager: manager
      operation: Update
      time: '2024-03-06T12:55:55Z'
  name: example
  namespace: nacproject
  resourceVersion: '37685280'
  uid: 20b1d693-98e0-4adb-bc76-48a531daa421
spec:
  backupSpec:
    volumeSnapshotLocations:
      - velero-sample-1
    defaultVolumesToFsBackup: false
    csiSnapshotTimeout: 10m0s
    ttl: 720h0m0s
    itemOperationTimeout: 4h0m0s
    metadata: {}
    storageLocation: velero-sample-1
    hooks: {}
    snapshotMoveData: false
status:
  expiration: '2024-04-05T12:55:55Z'
  failureReason: >-
    unable to get credentials: unable to get key for secret: Secret
    "cloud-credentials" not found
  formatVersion: 1.1.0
  phase: Failed
  startTimestamp: '2024-03-06T12:55:55Z'
  version: 1

api/v1alpha1/nonadminbackup_types.go

mateusoliveira43 · 2024-03-06T13:01:35Z

This one has a lot of duplication with #12

lets merge that one, then linters one, then this one

mpryc · 2024-03-06T13:02:19Z

This one has a lot of duplication with #12

lets merge that one, then linters one, then this one

Yes it does as the #12 was not merged I based this PR on the previous branch to reduce conflicts.

shubham-pampattiwar · 2024-03-06T16:14:47Z

api/v1alpha1/nonadminbackup_types.go

@@ -32,14 +32,12 @@ type NonAdminBackupSpec struct {

 	// BackupSpec defines the specification for a Velero backup.
 	BackupSpec *velerov1api.BackupSpec `json:"backupSpec,omitempty"`
-
-	// BackupStatus captures the current status of a Velero backup.
-	BackupStatus *velerov1api.BackupStatus `json:"backupStatus,omitempty"`
 }

 // NonAdminBackupStatus defines the observed state of NonAdminBackup
 type NonAdminBackupStatus struct {


Non-Admin backup status can have more details than velero backup status, eg: validation status or reconciliation status of the non-admin backup CR itself.

Modified. We now have (two examples from this PR):

spec: {} status: failureReason: NonAdminBackup CR does not contain valid VeleroBackupSpec phase: FailedValidation

spec: backupSpec: volumeSnapshotLocations: - velero-sample-1 defaultVolumesToFsBackup: false csiSnapshotTimeout: 10m0s ttl: 720h0m0s itemOperationTimeout: 4h0m0s metadata: {} storageLocation: velero-sample-1 hooks: {} snapshotMoveData: false status: backupStatus: expiration: '2024-04-05T16:46:15Z' failureReason: >- unable to get credentials: unable to get key for secret: Secret "cloud-credentials" not found formatVersion: 1.1.0 phase: Failed startTimestamp: '2024-03-06T16:46:15Z' version: 1

mpryc · 2024-03-06T16:55:02Z

/hold need to move update Velero Backup Status within it's own section.

mpryc · 2024-03-06T19:39:45Z

/unhold

shubham-pampattiwar · 2024-03-06T19:49:40Z

api/v1alpha1/nonadminbackup_types.go

+	// BackupStatus captures the current status of a Velero backup.
+
+	// +optional
+	Phase velerov1api.BackupPhase `json:"phase,omitempty"`


This phase should be Non-Admin Backup CR Reconciliation phase, right ?

or better option would be to use conditions just like we do for DPA CR status ?

yeah this seems duplicated information. What is your goal here?

Yes, however for this implementation I've used same as https://github.com/vmware-tanzu/velero/blob/4d548612d45d5eb0c61b2473286127dfa6e3c39d/pkg/apis/velero/v1/backup_types.go#L290-L348

It's because I think at the moment it's enough for us within velerov1api.BackupPhase. If we decide to have more states we can always create our own Non Admin Backup Phase.

@mateusoliveira43 the goal was to create Phase in the Non Admin Backup separate from BackupStatus Phase, I've used the velerov1api.BackupPhase, but we can change it as condidions or create our own NonAdminPhase type for the Non Admin Backup.

@shubham-pampattiwar I 100% agree with that, but isn't current list of conditions from velerov1api.BackupPhase enough? The idea was to use one of those as an overall status, but we can have own:

BackupPhaseNew BackupPhase = "New" BackupPhaseFailedValidation BackupPhase = "FailedValidation" BackupPhaseInProgress BackupPhase = "InProgress" BackupPhaseWaitingForPluginOperations BackupPhase = "WaitingForPluginOperations" BackupPhaseWaitingForPluginOperationsPartiallyFailed BackupPhase = "WaitingForPluginOperationsPartiallyFailed" BackupPhaseFinalizing BackupPhase = "Finalizing" BackupPhaseFinalizingPartiallyFailed BackupPhase = "FinalizingPartiallyFailed" BackupPhaseCompleted BackupPhase = "Completed" BackupPhasePartiallyFailed BackupPhase = "PartiallyFailed" BackupPhaseFailed BackupPhase = "Failed" BackupPhaseDeleting BackupPhase = "Deleting"

Anyway, I will change :)

@mpryc Let elaborate more and give you an example why we need independent conditions for non-admin backup CR:
The Velero BackupStatus provides us information on the reconciliation status of Velero Backup CR but how would we know bout the reconciliation of the non-admin backup CR ? This is when the independent conditions on non-admin backup CR come into picture. For example if the non-admin backup controller failed to created the Velero backup and got an error, how would the user know that the request failed, in such scenario the non-admin backup controller should add a error condition on non-admin backup CR and specify the error on the non-admin CR status conditions so that the user gains transparency on what is actually happening with non-admin backup CR. This is in general how controllers work, spec is what the user wants and status provides information on how the CR is doing, whether its performing the intended tasks and whether the actions intended via spec are done.

Added Conditions as well.

shubham-pampattiwar · 2024-03-08T17:04:04Z

api/v1alpha1/nonadminbackup_types.go

 }

 // NonAdminBackupStatus defines the observed state of NonAdminBackup
 type NonAdminBackupStatus struct {
 	Conditions []metav1.Condition `json:"conditions,omitempty"`
+
+	// +optional
+	Phase NonAdminBackupPhase `json:"phase,omitempty"`


The Phase and FailureReason might be redundant here, becasue conditions consists of similar fields like Conditions.Type , Conditions.Reason and Conditions.Message
Reference: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition

mpryc · 2024-04-15T14:10:04Z

Rebased work. Now it's following design from 2nd scenario which is described in:
#23

mateusoliveira43 · 2024-04-15T18:26:35Z

Is this PR blocker by #23 ?

mateusoliveira43 · 2024-04-15T18:36:37Z

api/v1alpha1/nonadminbackup_types.go

+
+// NonAdminCondition are used for more detailed information supporing NonAdminBackupPhase state.
+// +kubebuilder:validation:Enum=BackupAccepted;BackupQueued
+type NonAdminCondition string


I do not like declaring things on a file that are not used on that file. Would move to file that uses it

kubebuilder annotation here does not do anything, right?

I have no idea what is the comment here about, could you give me more information?

check CRD yaml, it does not mention NonAdminCondition, right? then // +kubebuilder:validation:Enum=BackupAccepted;BackupQueued does not do anything

This type is declared here, but never used here, only by other files. If it is used by more then one file, would move to constants file, otherwise, just to the file that uses this type

Ah right, I moved that from spec to status and now it's not used by kubebuilder

this is still never used in the file it was declared, right ❓

It is used in this file, few lines below as a NonAdminCondition:

const ( NonAdminConditionAccepted NonAdminCondition = "Accepted" NonAdminConditionQueued NonAdminCondition = "Queued" )

the NonAdminCondition type is not used in this file (it is just created and its allowed values declared), it should be moved to another file

I think this is pretty much standard to keep the type with const that is expecting that type, see even in velero:

https://github.com/vmware-tanzu/velero/blob/main/pkg/apis/velero/v1/backupstoragelocation_types.go#L168

Where would you move it?

it is used by functions and nab controller, so common folder

we can add it to constants directly, or create a types file in their first

here you go

api/v1alpha1/nonadminbackup_types.go

internal/controller/common_nab.go

shubham-pampattiwar · 2024-04-23T20:21:42Z

internal/common/function/function.go

 		// No change, no need to update
-		logger.V(1).Info("NonAdminBackup status and spec is already up to date")
-		return nil
+		logger.V(1).Info("NonAdminBackup Phase is already up to date")


NIT: Should not be an Info log, can make it debug

shubham-pampattiwar · 2024-04-23T20:26:34Z

internal/common/function/function.go

+	currentCondition := apimeta.FindStatusCondition(nab.Status.Conditions, string(condition))
+	if currentCondition != nil && currentCondition.Status == conditionStatus {
+		// Condition is already set to the desired status, no need to update
+		logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition is already set to: %s", condition))


NIT: Unsure if this should be Info, might get logs flooded in reconcile cycles

shubham-pampattiwar · 2024-04-23T20:27:15Z

internal/common/function/function.go

+		},
+	)
+
+	logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition to: %s", condition))


NIT: Again should not be info level I think

shubham-pampattiwar · 2024-04-23T20:32:39Z

internal/common/function/function.go

+	}
+
+	// Check if BackupSpec needs to be updated
+	if !reflect.DeepEqual(nab.Spec.BackupSpec, &veleroBackup.Spec) {


I am not sure about the spec update from velero backup spec to nab backup spec, seems opposite to what we want to do in our NAC feature.
NAC spec should be cascaded to Velero backup spec, not the other way round.

shubham-pampattiwar · 2024-04-23T20:33:21Z