diff --git a/api/v1alpha1/nonadminbackup_types.go b/api/v1alpha1/nonadminbackup_types.go index 4e29310..3b25f9c 100644 --- a/api/v1alpha1/nonadminbackup_types.go +++ b/api/v1alpha1/nonadminbackup_types.go @@ -51,9 +51,9 @@ type VeleroBackup struct { // +optional Status *velerov1.BackupStatus `json:"status,omitempty"` - // name references the Velero backup by it's name. + // nabnacuuid references the Non Admin Backup object by it's nacUUID. // +optional - Name string `json:"name,omitempty"` + NabNacUUID string `json:"nabnacuuid,omitempty"` // namespace references the Namespace in which Velero backup exists. // +optional diff --git a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml index 495d8a3..5b3fccd 100644 --- a/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml +++ b/config/crd/bases/nac.oadp.openshift.io_nonadminbackups.yaml @@ -612,8 +612,9 @@ spec: description: VeleroBackup contains information of the related Velero backup object. properties: - name: - description: name references the Velero backup by it's name. + nabnacuuid: + description: nabnacuuid references the Non Admin Backup object + by it's nacUUID. type: string namespace: description: namespace references the Namespace in which Velero diff --git a/docs/design/nab_and_nar_status_update.md b/docs/design/nab_and_nar_status_update.md index 4ea0927..4af9cf9 100644 --- a/docs/design/nab_and_nar_status_update.md +++ b/docs/design/nab_and_nar_status_update.md @@ -56,8 +56,8 @@ Those are are the possible values for `NonAdminCondition`: NonAdminBackup/NonAdminRestore `status` contains reference to the related Velero Backup/Restore. -NonAdminBackup `status.veleroBackup` contains `name`, `namespace` and `status`. -- `status.veleroBackup.name` represents the name of the `VeleroBackup` object. +NonAdminBackup `status.veleroBackup` contains `nabnacuuid`, `namespace` and `status`. +- `status.veleroBackup.nabnacuuid` field stores generated unique UUID of the `VeleroBackup` object. The same UUID is also stored as the label value `openshift.io/oadp-nab-origin-uuid` within the created `VeleroBackup` object. - `status.veleroBackup.namespace` represents the namespace in which the `VeleroBackup` object was created. - `status.veleroBackup.status` field is a copy of the `VeleroBackup` object status. @@ -76,10 +76,10 @@ status: velero backup describe -n openshift-adp nab-nacproject-c3499c2729730a ``` -Similarly, NonAdminRestore `status.veleroRestore` contains `name`, `namespace` and `status`. -- `status.veleroRestore.name` represents the name of the `veleroRestore` object. +Similarly, NonAdminRestore `status.veleroRestore` contains `nabnacuuid`, `namespace` and `status`. +- `status.veleroRestore.nabnacuuid` field stores generated unique UUID of the `VeleroRestore` object. The same UUID is also stored as the label value `openshift.io/oadp-nar-origin-uuid` within the created `VeleroRestore` object. - `status.veleroRestore.namespace` represents the namespace in which the `veleroRestore` object was created. -- `status.veleroRestore.status` field is a copy of the `VeleroBackup` object status. +- `status.veleroRestore.status` field is a copy of the `VeleroRestore` object status. ## Example @@ -91,7 +91,7 @@ Object passed validation and Velero `Backup` object was created, but there was a ```yaml status: veleroBackup: - name: nab-nacproject-83fc04a2fd253d + nabnacuuid: nonadmin-test-86b8d92b-66b2-11e4-8a2d-42010af06f3f namespace: openshift-adp status: expiration: '2024-05-16T08:12:11Z' @@ -135,16 +135,25 @@ reconcileExit1[\Requeue: true, nil/] reconcileExit1 ==> question(is NonAdminBackup Spec valid?) == yes ==> reconcileStart2 question == no ==> reconcileStartInvalid1 + + reconcileStart2[/Reconcile start\] ==> conditionsAcceptedTrue["`status.conditions[Accepted] to **True**`"] -. Requeue: false, err .- reconcileStart2 + conditionsAcceptedTrue ==> reconcileExit2[\Requeue: true, nil/] ==> reconcileStart3[/Reconcile start\] ==> -createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart3 +setVeleroBackupUUID([Set status.veleroBackup.nabNacUUID]) -. Requeue: false, err .- reconcileStart3 + +setVeleroBackupUUID ==> + +reconcileStart4[/Reconcile start\] ==> + +createVeleroBackup([Create Velero Backup]) -. Requeue: false, err .- reconcileStart4 createVeleroBackup ==> phaseCreated["` @@ -152,7 +161,7 @@ status.phase: **Created** status.conditions[Queued] to **True** status.conditions.veleroBackup.name status.conditions.veleroBackup.namespace -`"] -. Requeue: false, err .- reconcileStart3 +`"] -. Requeue: false, err .- reconcileStart4 phaseCreated ==> reconcileExit4[\Requeue: false, nil/] diff --git a/internal/common/constant/constant.go b/internal/common/constant/constant.go index c7a41a8..0aa1a0f 100644 --- a/internal/common/constant/constant.go +++ b/internal/common/constant/constant.go @@ -17,6 +17,8 @@ limitations under the License. // Package constant contains all common constants used in the project package constant +import "k8s.io/apimachinery/pkg/util/validation" + // Common labels for objects manipulated by the Non Admin Controller // Labels should be used to identify the NAC object // Annotations on the other hand should be used to define ownership @@ -28,7 +30,8 @@ const ( ManagedByLabelValue = "oadp-nac-controller" // TODO why not use same project name as in PROJECT file? NabOriginNameAnnotation = "openshift.io/oadp-nab-origin-name" NabOriginNamespaceAnnotation = "openshift.io/oadp-nab-origin-namespace" - NabOriginUUIDAnnotation = "openshift.io/oadp-nab-origin-uuid" + NabOriginUUIDLabel = "openshift.io/oadp-nab-origin-uuid" + NarOriginUUIDLabel = "openshift.io/oadp-nar-origin-uuid" ) // Common environment variables for the Non Admin Controller @@ -39,9 +42,12 @@ const ( // EmptyString defines a constant for the empty string const EmptyString = "" +// NameDelimiter defines character that is used to separate name parts +const NameDelimiter = "-" + // TrueString defines a constant for the True string const TrueString = "True" -// VeleroBackupNamePrefix represents the prefix for the object name generated -// by the NonAdminController -const VeleroBackupNamePrefix = "nab" +// MaximumNacObjectNameLength represents Generated Non Admin Object Name and +// must be below 63 characters, because it's used within object Label Value +const MaximumNacObjectNameLength = validation.DNS1123LabelMaxLength diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 66a34c9..e5e0deb 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -19,12 +19,13 @@ package function import ( "context" - "crypto/sha256" - "encoding/hex" "fmt" "github.com/go-logr/logr" + "github.com/google/uuid" + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/client" @@ -47,7 +48,6 @@ func GetNonAdminBackupAnnotations(objectMeta metav1.ObjectMeta) map[string]strin return map[string]string{ constant.NabOriginNamespaceAnnotation: objectMeta.Namespace, constant.NabOriginNameAnnotation: objectMeta.Name, - constant.NabOriginUUIDAnnotation: string(objectMeta.UID), } } @@ -77,66 +77,125 @@ func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup) error { return nil } -// GenerateVeleroBackupName generates a Velero backup name based on the provided namespace and NonAdminBackup name. -// It calculates a hash of the NonAdminBackup name and combines it with the namespace and a prefix to create the Velero backup name. -// If the resulting name exceeds the maximum Kubernetes name length, it truncates the namespace to fit within the limit. -func GenerateVeleroBackupName(namespace, nabName string) string { - // Calculate a hash of the name - hasher := sha256.New() - _, err := hasher.Write([]byte(nabName)) - if err != nil { - return "" +// GenerateNacObjectNameWithUUID generates a unique name based on the provided namespace and object origin name. +// It includes a UUID suffix. If the name exceeds the maximum length, it truncates nacName first, then namespace. +func GenerateNacObjectNameWithUUID(namespace, nacName string) string { + // Generate UUID suffix + uuidSuffix := uuid.New().String() + + // Build the initial name based on the presence of namespace and nacName + nacObjectName := uuidSuffix + if len(nacName) > 0 { + nacObjectName = nacName + constant.NameDelimiter + nacObjectName + } + if len(namespace) > 0 { + nacObjectName = namespace + constant.NameDelimiter + nacObjectName + } + + if len(nacObjectName) > constant.MaximumNacObjectNameLength { + // Calculate remaining length after UUID + remainingLength := constant.MaximumNacObjectNameLength - len(uuidSuffix) + + delimeterLength := len(constant.NameDelimiter) + + // Subtract two delimiter lengths to avoid a corner case where the namespace + // and delimiters leave no space for any part of nabName + if len(namespace) > remainingLength-delimeterLength-delimeterLength { + namespace = namespace[:remainingLength-delimeterLength-delimeterLength] + nacObjectName = namespace + constant.NameDelimiter + uuidSuffix + } else { + remainingLength = remainingLength - len(namespace) - delimeterLength - delimeterLength + nacName = nacName[:remainingLength] + nacObjectName = uuidSuffix + if len(nacName) > 0 { + nacObjectName = nacName + constant.NameDelimiter + nacObjectName + } + if len(namespace) > 0 { + nacObjectName = namespace + constant.NameDelimiter + nacObjectName + } + } } - const hashLength = 14 - nameHash := hex.EncodeToString(hasher.Sum(nil))[:hashLength] // Take first 14 chars + return nacObjectName +} - usedLength := hashLength + len(constant.VeleroBackupNamePrefix) + len("--") - maxNamespaceLength := validation.DNS1123SubdomainMaxLength - usedLength - // Ensure the name is within the character limit - if len(namespace) > maxNamespaceLength { - // Truncate the namespace if necessary - return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace[:maxNamespaceLength], nameHash) +// GetObjectByLabel retrieves a list of Kubernetes objects of a specified type based on a label within a given namespace. +// It returns a slice of the specified object type and an error. +func GetObjectByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelKey string, labelValue string, objectList client.ObjectList) error { + // Validate input parameters + if namespace == constant.EmptyString || labelKey == constant.EmptyString || labelValue == constant.EmptyString { + return fmt.Errorf("invalid input: namespace, labelKey, and labelValue must not be empty") } - return fmt.Sprintf("%s-%s-%s", constant.VeleroBackupNamePrefix, namespace, nameHash) + labelSelector := labels.SelectorFromSet(labels.Set{labelKey: labelValue}) + + // Attempt to list objects with the specified label + if err := clientInstance.List(ctx, objectList, &client.ListOptions{ + LabelSelector: labelSelector, + Namespace: namespace, + }); err != nil { + return fmt.Errorf("failed to list objects in namespace '%s': %w", namespace, err) + } + + return nil +} + +// GetVeleroBackupByLabel retrieves a VeleroBackup object based on a specified label within a given namespace. +// It returns the VeleroBackup only when exactly one object is found, throws an error if multiple backups are found, +// or returns nil if no matches are found. +func GetVeleroBackupByLabel(ctx context.Context, clientInstance client.Client, namespace string, labelValue string) (*velerov1.Backup, error) { + veleroBackupList := &velerov1.BackupList{} + + // Call the generic GetObjectByLabel function + if err := GetObjectByLabel(ctx, clientInstance, namespace, constant.NabOriginUUIDLabel, labelValue, veleroBackupList); err != nil { + return nil, err + } + + switch len(veleroBackupList.Items) { + case 0: + return nil, nil // No matching VeleroBackup found + case 1: + return &veleroBackupList.Items[0], nil // Found 1 matching VeleroBackup + default: + return nil, fmt.Errorf("multiple VeleroBackup objects found with label %s=%s in namespace '%s'", constant.NabOriginUUIDLabel, labelValue, namespace) + } } // CheckVeleroBackupMetadata return true if Velero Backup object has required Non Admin labels and annotations, false otherwise func CheckVeleroBackupMetadata(obj client.Object) bool { - labels := obj.GetLabels() - if !checkLabelValue(labels, constant.OadpLabel, constant.OadpLabelValue) { + objLabels := obj.GetLabels() + if !checkLabelValue(objLabels, constant.OadpLabel, constant.OadpLabelValue) { return false } - if !checkLabelValue(labels, constant.ManagedByLabel, constant.ManagedByLabelValue) { + if !checkLabelValue(objLabels, constant.ManagedByLabel, constant.ManagedByLabelValue) { return false } - annotations := obj.GetAnnotations() - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { + if !checkAnnotationOrLabelsValueIsValid(objLabels, constant.NabOriginUUIDLabel) { return false } - if !checkAnnotationValueIsValid(annotations, constant.NabOriginNameAnnotation) { + + annotations := obj.GetAnnotations() + if !checkAnnotationOrLabelsValueIsValid(annotations, constant.NabOriginNamespaceAnnotation) { return false } - // TODO what is a valid uuid? - if !checkAnnotationValueIsValid(annotations, constant.NabOriginUUIDAnnotation) { + if !checkAnnotationOrLabelsValueIsValid(annotations, constant.NabOriginNameAnnotation) { return false } return true } -func checkLabelValue(labels map[string]string, key string, value string) bool { - got, exists := labels[key] +func checkLabelValue(objLabels map[string]string, key string, value string) bool { + got, exists := objLabels[key] if !exists { return false } return got == value } -func checkAnnotationValueIsValid(annotations map[string]string, key string) bool { - value, exists := annotations[key] +func checkAnnotationOrLabelsValueIsValid(annotationsOrLabels map[string]string, key string) bool { + value, exists := annotationsOrLabels[key] if !exists { return false } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 034b963..5f6209b 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -17,16 +17,23 @@ limitations under the License. package function import ( - "fmt" + "context" + "errors" "strings" "testing" + "github.com/google/uuid" "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/log/zap" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/migtools/oadp-non-admin/internal/common/constant" @@ -38,6 +45,7 @@ const ( testNonAdminBackupNamespace = "non-admin-backup-namespace" testNonAdminBackupName = "non-admin-backup-name" testNonAdminBackupUUID = "12345678-1234-1234-1234-123456789abc" + defaultStr = "default" ) func TestGetNonAdminLabels(t *testing.T) { @@ -62,7 +70,6 @@ func TestGetNonAdminBackupAnnotations(t *testing.T) { expected := map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, } result := GetNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta) @@ -115,36 +122,184 @@ func TestValidateBackupSpec(t *testing.T) { } } -func TestGenerateVeleroBackupName(t *testing.T) { - // Max Kubernetes name length := 253 - // hashLength := 14 - // deduct -3: constant.VeleroBackupNamePrefix = "nab" - // deduct -2: two "-" in the name - // 253 - 14 - 3 - 2 = 234 - const truncatedStringSize = 234 - - testCases := []struct { - namespace string +func TestGenerateNacObjectNameWithUUID(t *testing.T) { + tests := []struct { name string - expected string + namespace string + nabName string + }{ + { + name: "Valid names without truncation", + namespace: defaultStr, + nabName: "my-backup", + }, + { + name: "Truncate nabName due to length", + namespace: "some", + nabName: strings.Repeat("q", constant.MaximumNacObjectNameLength+10), // too long for DNS limit + }, + { + name: "Truncate very long namespace and very long name", + namespace: strings.Repeat("w", constant.MaximumNacObjectNameLength+10), + nabName: strings.Repeat("e", constant.MaximumNacObjectNameLength+10), + }, + { + name: "nabName empty", + namespace: "example", + nabName: constant.EmptyString, + }, + { + name: "namespace empty", + namespace: constant.EmptyString, + nabName: "my-backup", + }, + { + name: "very long name and namespace empty", + namespace: constant.EmptyString, + nabName: strings.Repeat("r", constant.MaximumNacObjectNameLength+10), + }, + { + name: "very long namespace and name empty", + namespace: strings.Repeat("t", constant.MaximumNacObjectNameLength+10), + nabName: constant.EmptyString, + }, + { + name: "empty namespace and empty name", + namespace: constant.EmptyString, + nabName: constant.EmptyString, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := GenerateNacObjectNameWithUUID(tt.namespace, tt.nabName) + + // Check length, don't use constant.MaximumNacObjectNameLength here + // so if constant is changed test needs to be changed as well + if len(result) > validation.DNS1123LabelMaxLength { + t.Errorf("Generated name is too long: %s", result) + } + + // Extract the last 36 characters, which should be the UUID + if len(result) < 36 { + t.Errorf("Generated name is too short to contain a valid UUID: %s", result) + } + uuidPart := result[len(result)-36:] // The UUID is always the last 36 characters + + // Attempt to parse the UUID part + if _, err := uuid.Parse(uuidPart); err != nil { + t.Errorf("Last part is not a valid UUID: %s", uuidPart) + } + + // Check if no double hyphens are present + if strings.Contains(result, "--") { + t.Errorf("Generated name contains double hyphens: %s", result) + } + }) + } +} + +func TestGetVeleroBackupByLabel(t *testing.T) { + log := zap.New(zap.UseDevMode(true)) + ctx := context.Background() + ctx = ctrl.LoggerInto(ctx, log) + scheme := runtime.NewScheme() + const testAppStr = "test-app" + + // Register VeleroBackup type with the scheme + if err := velerov1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to register VeleroBackup type: %v", err) + } + + tests := []struct { + name string + namespace string + labelValue string + expected *velerov1.Backup + expectedError error + mockBackups []velerov1.Backup }{ { - namespace: "example-namespace", - name: "example-name", - expected: "nab-example-namespace-1cdadb729eaac1", + name: "Single VeleroBackup found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup1", + Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + }, + }, + }, + expected: &velerov1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: defaultStr, Name: "backup1", Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}}}, + expectedError: nil, + }, + { + name: "No VeleroBackups found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{}, + expected: nil, + expectedError: nil, + }, + { + name: "Multiple VeleroBackups found", + namespace: defaultStr, + labelValue: testAppStr, + mockBackups: []velerov1.Backup{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup2", + Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultStr, + Name: "backup3", + Labels: map[string]string{constant.NabOriginUUIDLabel: testAppStr}, + }, + }, + }, + expected: nil, + expectedError: errors.New("multiple VeleroBackup objects found with label openshift.io/oadp-nab-origin-uuid=test-app in namespace 'default'"), }, { - namespace: strings.Repeat("m", validation.DNS1123SubdomainMaxLength), - name: "example-name", - expected: fmt.Sprintf("nab-%s-1cdadb729eaac1", strings.Repeat("m", truncatedStringSize)), + name: "Invalid input - empty namespace", + namespace: "", + labelValue: testAppStr, + mockBackups: []velerov1.Backup{}, + expected: nil, + expectedError: errors.New("invalid input: namespace, labelKey, and labelValue must not be empty"), }, } - for _, tc := range testCases { - actual := GenerateVeleroBackupName(tc.namespace, tc.name) - if actual != tc.expected { - t.Errorf("Expected: %s, but got: %s", tc.expected, actual) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var objects []client.Object + for _, backup := range tt.mockBackups { + backupCopy := backup // Create a copy to avoid memory aliasing + objects = append(objects, &backupCopy) + } + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + result, err := GetVeleroBackupByLabel(ctx, client, tt.namespace, tt.labelValue) + + if tt.expectedError != nil { + assert.EqualError(t, err, tt.expectedError.Error()) + } else { + assert.NoError(t, err) + if tt.expected != nil && result != nil { + assert.Equal(t, tt.expected.Name, result.Name, "VeleroBackup Name should match") + assert.Equal(t, tt.expected.Namespace, result.Namespace, "VeleroBackup Namespace should match") + assert.Equal(t, tt.expected.Labels, result.Labels, "VeleroBackup Labels should match") + } else { + assert.Nil(t, result, "Expected result should be nil") + } + } + }) } } @@ -190,7 +345,6 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -201,13 +355,13 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: constant.EmptyString, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -218,13 +372,13 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: strings.Repeat("nn", validation.DNS1123SubdomainMaxLength), - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, @@ -235,13 +389,13 @@ func TestCheckVeleroBackupMetadata(t *testing.T) { backup: &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - constant.OadpLabel: constant.OadpLabelValue, - constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginUUIDLabel: testNonAdminBackupUUID, }, Annotations: map[string]string{ constant.NabOriginNamespaceAnnotation: testNonAdminBackupNamespace, constant.NabOriginNameAnnotation: testNonAdminBackupName, - constant.NabOriginUUIDAnnotation: testNonAdminBackupUUID, }, }, }, diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index e1ca726..b4b23c2 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -51,9 +51,11 @@ type NonAdminBackupReconciler struct { type reconcileStepFunction func(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) const ( - phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" - conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" - statusUpdateError = "Failed to update NonAdminBackup Status" + phaseUpdateRequeue = "NonAdminBackup - Requeue after Phase Update" + conditionUpdateRequeue = "NonAdminBackup - Requeue after Condition Update" + veleroReferenceUpdateRequeue = "NonAdminBackup - Requeue after Status Update with UUID reference" + statusUpdateExit = "NonAdminBackup - Exit after Status Update" + statusUpdateError = "Failed to update NonAdminBackup Status" ) // +kubebuilder:rbac:groups=nac.oadp.openshift.io,resources=nonadminbackups,verbs=get;list;watch;create;update;patch;delete @@ -84,7 +86,8 @@ func (r *NonAdminBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconcileSteps := []reconcileStepFunction{ r.init, r.validateSpec, - r.syncVeleroBackupWithNonAdminBackup, + r.setBackupUUIDInStatus, + r.createVeleroBackupAndSyncWithNonAdminBackup, } for _, step := range reconcileSteps { requeue, err := step(ctx, logger, nab) @@ -185,7 +188,34 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr return false, nil } -// syncVeleroBackupWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource +// setBackupUUIDInStatus generates a UUID for VeleroBackup and stores it in the NonAdminBackup status. +// +// Parameters: +// +// ctx: Context for the request. +// logger: Logger instance for logging messages. +// nab: Pointer to the NonAdminBackup object. +// +// This function generates a UUID and stores it in the VeleroBackup status field of NonAdminBackup. +func (r *NonAdminBackupReconciler) setBackupUUIDInStatus(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NabNacUUID == constant.EmptyString { + nabNacUUID := function.GenerateNacObjectNameWithUUID(nab.Namespace, nab.Name) + nab.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ + NabNacUUID: nabNacUUID, + Namespace: r.OADPNamespace, + } + if err := r.Status().Update(ctx, nab); err != nil { + logger.Error(err, statusUpdateError) + return false, err + } + logger.V(1).Info(veleroReferenceUpdateRequeue) + return true, nil + } + logger.V(1).Info("NonAdminBackup already contains VeleroBackup UUID reference") + return false, nil +} + +// createVeleroBackupAndSyncWithNonAdminBackup ensures the VeleroBackup associated with the given NonAdminBackup resource // is created, if it does not exist. // The function also updates the status and conditions of the NonAdminBackup resource to reflect the state // of the VeleroBackup. @@ -195,29 +225,30 @@ func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr // ctx: Context for the request. // logger: Logger instance for logging messages. // nab: Pointer to the NonAdminBackup object. -func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { - veleroBackupName := function.GenerateVeleroBackupName(nab.Namespace, nab.Name) - if veleroBackupName == constant.EmptyString { - return false, errors.New("unable to generate Velero Backup name") +func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { + if nab.Status.VeleroBackup == nil || nab.Status.VeleroBackup.NabNacUUID == constant.EmptyString { + return false, errors.New("unable to get Velero Backup UUID from NonAdminBackup Status") } - veleroBackup := velerov1.Backup{} - veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupName, Namespace: r.OADPNamespace}) - err := r.Get(ctx, client.ObjectKey{Namespace: r.OADPNamespace, Name: veleroBackupName}, &veleroBackup) + veleroBackupUUID := nab.Status.VeleroBackup.NabNacUUID + + veleroBackup, err := function.GetVeleroBackupByLabel(ctx, r.Client, r.OADPNamespace, veleroBackupUUID) + if err != nil { - if !apierrors.IsNotFound(err) { - veleroBackupLogger.Error(err, "Unable to fetch VeleroBackup") - return false, err - } - // Create VeleroBackup - veleroBackupLogger.Info("VeleroBackup not found") + // Case in which more then one VeleroBackup is found with the same label UUID + logger.Error(err, "Failed to find single VeleroBackup object", "UUID", veleroBackupUUID) + return false, err + } + + if veleroBackup == nil { + logger.Info("VeleroBackup with label not found, creating one", "UUID", veleroBackupUUID) backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace} - veleroBackup = velerov1.Backup{ + veleroBackup := velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: veleroBackupName, + Name: veleroBackupUUID, Namespace: r.OADPNamespace, Labels: function.GetNonAdminLabels(), Annotations: function.GetNonAdminBackupAnnotations(nab.ObjectMeta), @@ -225,15 +256,26 @@ func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx contex Spec: *backupSpec, } + // Add NonAdminBackup's veleroBackupUUID as the label to the VeleroBackup object + // We don't add this as an argument of GetNonAdminLabels(), because there may be + // situations where NAC object do not require NabOriginUUIDLabel + veleroBackup.Labels[constant.NabOriginUUIDLabel] = veleroBackupUUID + err = r.Create(ctx, &veleroBackup) + if err != nil { - veleroBackupLogger.Error(err, "Failed to create VeleroBackup") + // We do not retry here as the veleroBackupUUID + // should be guaranteed to be unique + logger.Error(err, "Failed to create VeleroBackup") return false, err } - veleroBackupLogger.Info("VeleroBackup successfully created") + logger.Info("VeleroBackup successfully created") } + veleroBackupLogger := logger.WithValues("VeleroBackup", types.NamespacedName{Name: veleroBackupUUID, Namespace: r.OADPNamespace}) + updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseCreated) + updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, metav1.Condition{ Type: string(nacv1alpha1.NonAdminConditionQueued), @@ -242,22 +284,20 @@ func (r *NonAdminBackupReconciler) syncVeleroBackupWithNonAdminBackup(ctx contex Message: "Created Velero Backup object", }, ) - updatedReference := updateNonAdminBackupVeleroBackupReference(&nab.Status, &veleroBackup) - if updatedPhase || updatedCondition || updatedReference { + + if updatedPhase || updatedCondition { if err := r.Status().Update(ctx, nab); err != nil { logger.Error(err, statusUpdateError) return false, err } - - logger.V(1).Info("NonAdminBackup - Exit after Status Update") + logger.V(1).Info(statusUpdateExit) return false, nil } // Ensure that the NonAdminBackup's NonAdminBackupStatus is in sync // with the VeleroBackup. Any required updates to the NonAdminBackup // Status will be applied based on the current state of the VeleroBackup. - veleroBackupLogger.Info("VeleroBackup already exists, verifying if NonAdminBackup Status requires update") - updated := updateNonAdminBackupVeleroBackupStatus(&nab.Status, &veleroBackup) + updated := updateNonAdminBackupVeleroBackupStatus(&nab.Status, veleroBackup) if updated { if err := r.Status().Update(ctx, nab); err != nil { veleroBackupLogger.Error(err, "Failed to update NonAdminBackup Status after VeleroBackup reconciliation") @@ -301,32 +341,14 @@ func updateNonAdminPhase(phase *nacv1alpha1.NonAdminBackupPhase, newPhase nacv1a return true } -// updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup reference fields in NonAdminBackup object status and returns true -// if the VeleroBackup fields are changed by this call. -func updateNonAdminBackupVeleroBackupReference(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { - if status.VeleroBackup == nil { - status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - Name: veleroBackup.Name, - Namespace: veleroBackup.Namespace, - } - return true - } else if status.VeleroBackup.Name != veleroBackup.Name || status.VeleroBackup.Namespace != veleroBackup.Namespace { - status.VeleroBackup.Name = veleroBackup.Name - status.VeleroBackup.Namespace = veleroBackup.Namespace - return true - } - return false -} - // updateNonAdminBackupVeleroBackupStatus sets the VeleroBackup status field in NonAdminBackup object status and returns true // if the VeleroBackup fields are changed by this call. func updateNonAdminBackupVeleroBackupStatus(status *nacv1alpha1.NonAdminBackupStatus, veleroBackup *velerov1.Backup) bool { - if status.VeleroBackup == nil { - status.VeleroBackup = &nacv1alpha1.VeleroBackup{ - Status: veleroBackup.Status.DeepCopy(), - } - return true - } else if !reflect.DeepEqual(status.VeleroBackup.Status, &veleroBackup.Status) { + if status == nil || veleroBackup == nil { + return false + } + + if status.VeleroBackup.Status == nil || !reflect.DeepEqual(status.VeleroBackup.Status, &veleroBackup.Status) { status.VeleroBackup.Status = veleroBackup.Status.DeepCopy() return true } diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index d72ece9..6e16198 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -29,25 +29,24 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/migtools/oadp-non-admin/internal/common/constant" "github.com/migtools/oadp-non-admin/internal/common/function" ) -const ( - testNonAdminBackupName = "test-non-admin-backup" - placeholder = "PLACEHOLDER" -) - type nonAdminBackupSingleReconcileScenario struct { - resultError error - priorStatus *nacv1alpha1.NonAdminBackupStatus - spec nacv1alpha1.NonAdminBackupSpec - ExpectedStatus nacv1alpha1.NonAdminBackupStatus - result reconcile.Result - createVeleroBackup bool + resultError error + nonAdminBackupPriorStatus *nacv1alpha1.NonAdminBackupStatus + nonAdminBackupSpec nacv1alpha1.NonAdminBackupSpec + nonAdminBackupExpectedStatus nacv1alpha1.NonAdminBackupStatus + result reconcile.Result + createVeleroBackup bool + uuidCreatedByReconcile bool + uuidFromTestCase bool } type nonAdminBackupFullReconcileScenario struct { @@ -55,37 +54,40 @@ type nonAdminBackupFullReconcileScenario struct { status nacv1alpha1.NonAdminBackupStatus } -func buildTestNonAdminBackup(namespace string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { +func buildTestNonAdminBackup(nonAdminNamespace string, nonAdminName string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { return &nacv1alpha1.NonAdminBackup{ ObjectMeta: metav1.ObjectMeta{ - Name: testNonAdminBackupName, - Namespace: namespace, + Name: nonAdminName, + Namespace: nonAdminNamespace, }, Spec: spec, } } -func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, expectedStatus nacv1alpha1.NonAdminBackupStatus, nonAdminNamespaceName string, oadpNamespaceName string) error { +func checkTestNonAdminBackupStatus(nonAdminBackup *nacv1alpha1.NonAdminBackup, expectedStatus nacv1alpha1.NonAdminBackupStatus, oadpNamespaceName string) error { if nonAdminBackup.Status.Phase != expectedStatus.Phase { return fmt.Errorf("NonAdminBackup Status Phase %v is not equal to expected %v", nonAdminBackup.Status.Phase, expectedStatus.Phase) } - if expectedStatus.VeleroBackup != nil { - veleroBackupName := expectedStatus.VeleroBackup.Name - if expectedStatus.VeleroBackup.Name == placeholder { - veleroBackupName = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) - } - if nonAdminBackup.Status.VeleroBackup.Name != veleroBackupName { - return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Name, veleroBackupName) - } - veleroBackupNamespace := expectedStatus.VeleroBackup.Namespace - if expectedStatus.VeleroBackup.Namespace == placeholder { - veleroBackupNamespace = oadpNamespaceName - } - if nonAdminBackup.Status.VeleroBackup.Namespace != veleroBackupNamespace { - return fmt.Errorf("NonAdminBackup Status VeleroBackupNamespace %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Namespace, veleroBackupNamespace) + + if nonAdminBackup.Status.VeleroBackup != nil { + if nonAdminBackup.Status.VeleroBackup.NabNacUUID == "" { + return fmt.Errorf("NonAdminBackup Status VeleroBackupName %v is 0 length string", nonAdminBackup.Status.VeleroBackup.NabNacUUID) } - if !reflect.DeepEqual(nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) { - return fmt.Errorf("NonAdminBackup Status VeleroBackupStatus %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) + + if expectedStatus.VeleroBackup != nil { + // When there is no VeleroBackup expected Namespace provided, use one that should be result of reconcile loop + veleroBackupNamespace := expectedStatus.VeleroBackup.Namespace + if veleroBackupNamespace == "" { + veleroBackupNamespace = oadpNamespaceName + } + if nonAdminBackup.Status.VeleroBackup.Namespace != veleroBackupNamespace { + return fmt.Errorf("NonAdminBackup Status VeleroBackupNamespace %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Namespace, veleroBackupNamespace) + } + if expectedStatus.VeleroBackup.Status != nil { + if !reflect.DeepEqual(nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) { + return fmt.Errorf("NonAdminBackup Status VeleroBackupStatus %v is not equal to expected %v", nonAdminBackup.Status.VeleroBackup.Status, expectedStatus.VeleroBackup.Status) + } + } } } @@ -119,7 +121,6 @@ func createTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad if err != nil { return err } - oadpNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: oadpNamespaceName, @@ -127,7 +128,6 @@ func createTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad } return k8sClient.Create(ctx, oadpNamespace) } - func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oadpNamespaceName string) error { oadpNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -138,7 +138,6 @@ func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad if err != nil { return err } - nonAdminNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: nonAdminNamespaceName, @@ -149,50 +148,47 @@ func deleteTestNamespaces(ctx context.Context, nonAdminNamespaceName string, oad var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile function", func() { var ( - ctx = context.Background() - nonAdminNamespaceName = "" - oadpNamespaceName = "" - counter = 0 - updateTestScenario = func() { - counter++ - nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-%v", counter) - oadpNamespaceName = nonAdminNamespaceName + "-oadp" - } + ctx = context.Background() + nonAdminObjectName string + nonAdminObjectNamespace string + oadpNamespace string + veleroBackupUUID string + counter = 0 ) - + ginkgo.BeforeEach(func() { + counter++ + nonAdminObjectName = fmt.Sprintf("nab-object-%v", counter) + nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-%v", counter) + oadpNamespace = nonAdminObjectNamespace + "-oadp" + veleroBackupUUID = function.GenerateNacObjectNameWithUUID(nonAdminObjectNamespace, nonAdminObjectName) + gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) + }) ginkgo.AfterEach(func() { nonAdminBackup := &nacv1alpha1.NonAdminBackup{} if k8sClient.Get( ctx, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, ) == nil { gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) } - - gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(deleteTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) }) - ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Delete event", func(scenario nonAdminBackupSingleReconcileScenario) { - updateTestScenario() - - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - result, err := (&NonAdminBackupReconciler{ Client: k8sClient, Scheme: testEnv.Scheme, }).Reconcile( context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{ - Namespace: nonAdminNamespaceName, - Name: testNonAdminBackupName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }}, ) - gomega.Expect(result).To(gomega.Equal(scenario.result)) gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) }, @@ -200,56 +196,60 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func result: reconcile.Result{}, }), ) - ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create/Update events and by Requeue", func(scenario nonAdminBackupSingleReconcileScenario) { - updateTestScenario() - - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - - nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) - gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + nonAdminBackup := buildTestNonAdminBackup(nonAdminObjectNamespace, nonAdminObjectName, scenario.nonAdminBackupSpec) + gomega.Expect(k8sClient.Create(ctx, nonAdminBackup.DeepCopy())).To(gomega.Succeed()) + nonAdminBackupAfterCreate := &nacv1alpha1.NonAdminBackup{} + gomega.Expect(k8sClient.Get( + ctx, + types.NamespacedName{ + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, + }, + nonAdminBackupAfterCreate, + )).To(gomega.Succeed()) + if scenario.nonAdminBackupPriorStatus != nil { + nonAdminBackupAfterCreate.Status = *scenario.nonAdminBackupPriorStatus + if scenario.uuidFromTestCase { + nonAdminBackupAfterCreate.Status.VeleroBackup = &nacv1alpha1.VeleroBackup{ + NabNacUUID: veleroBackupUUID, + Namespace: oadpNamespace, + } + } + gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackupAfterCreate)).To(gomega.Succeed()) + } if scenario.createVeleroBackup { veleroBackup := &velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ - Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), - Namespace: oadpNamespaceName, - Labels: function.GetNonAdminLabels(), + Name: nonAdminBackupAfterCreate.Status.VeleroBackup.NabNacUUID, + Namespace: oadpNamespace, + Labels: map[string]string{ + constant.OadpLabel: constant.OadpLabelValue, + constant.ManagedByLabel: constant.ManagedByLabelValue, + constant.NabOriginUUIDLabel: nonAdminBackupAfterCreate.Status.VeleroBackup.NabNacUUID, + }, Annotations: function.GetNonAdminBackupAnnotations(nonAdminBackup.ObjectMeta), }, Spec: velerov1.BackupSpec{ - IncludedNamespaces: []string{nonAdminNamespaceName}, + IncludedNamespaces: []string{nonAdminObjectNamespace}, }, } gomega.Expect(k8sClient.Create(ctx, veleroBackup)).To(gomega.Succeed()) } - - if scenario.priorStatus != nil { - nonAdminBackup.Status = *scenario.priorStatus - if nonAdminBackup.Status.VeleroBackup != nil { - if nonAdminBackup.Status.VeleroBackup.Name == placeholder { - nonAdminBackup.Status.VeleroBackup.Name = function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName) - } - if nonAdminBackup.Status.VeleroBackup.Namespace == placeholder { - nonAdminBackup.Status.VeleroBackup.Namespace = oadpNamespaceName - } - } - gomega.Expect(k8sClient.Status().Update(ctx, nonAdminBackup)).To(gomega.Succeed()) - } // easy hack to test that only one update call happens per reconcile // priorResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) - result, err := (&NonAdminBackupReconciler{ Client: k8sClient, Scheme: testEnv.Scheme, - OADPNamespace: oadpNamespaceName, + OADPNamespace: oadpNamespace, }).Reconcile( context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{ - Namespace: nonAdminNamespaceName, - Name: testNonAdminBackupName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }}, ) gomega.Expect(result).To(gomega.Equal(scenario.result)) @@ -259,37 +259,39 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func gomega.Expect(err).To(gomega.HaveOccurred()) gomega.Expect(err.Error()).To(gomega.ContainSubstring(scenario.resultError.Error())) } - + nonAdminBackupAfterReconcile := &nacv1alpha1.NonAdminBackup{} gomega.Expect(k8sClient.Get( ctx, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, - nonAdminBackup, + nonAdminBackupAfterReconcile, )).To(gomega.Succeed()) - - gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.ExpectedStatus, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackupAfterReconcile, scenario.nonAdminBackupExpectedStatus, oadpNamespace)).To(gomega.Succeed()) + if scenario.uuidCreatedByReconcile { + gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.NabNacUUID).To(gomega.ContainSubstring(nonAdminObjectNamespace)) + gomega.Expect(nonAdminBackupAfterReconcile.Status.VeleroBackup.Namespace).To(gomega.Equal(oadpNamespace)) + } // easy hack to test that only one update call happens per reconcile // currentResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) // gomega.Expect(currentResourceVersion - priorResourceVersion).To(gomega.Equal(1)) }, ginkgo.Entry("When triggered by NonAdminBackup Create event, should update NonAdminBackup phase to new and Requeue", nonAdminBackupSingleReconcileScenario{ - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, result: reconcile.Result{Requeue: true}, }), ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new), should update NonAdminBackup Condition to Accepted True and Requeue", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, Conditions: []metav1.Condition{ { @@ -302,11 +304,41 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, result: reconcile.Result{Requeue: true}, }), - ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup phase to created and Condition to Queued True and VeleroBackup reference and Exit", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True), should update NonAdminBackup Status generated UUID for VeleroBackup and Requeue", nonAdminBackupSingleReconcileScenario{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &velerov1.BackupSpec{}, + }, + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + }, + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseNew, + Conditions: []metav1.Condition{ + { + Type: string(nacv1alpha1.NonAdminConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "BackupAccepted", + Message: "backup accepted", + }, + }, + }, + uuidCreatedByReconcile: true, + result: reconcile.Result{Requeue: true}, + }), + ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new; Conditions Accepted True; NonAdminBackup Status NabNacUUID set), should update NonAdminBackup phase to created and Condition to Queued True and Exit", nonAdminBackupSingleReconcileScenario{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, Conditions: []metav1.Condition{ { @@ -318,12 +350,8 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - }, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -339,19 +367,17 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{}, + uuidFromTestCase: true, + result: reconcile.Result{}, }), ginkgo.Entry("When triggered by VeleroBackup Update event, should update NonAdminBackup VeleroBackupStatus and Exit", nonAdminBackupSingleReconcileScenario{ createVeleroBackup: true, - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{}, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ - Phase: nacv1alpha1.NonAdminBackupPhaseCreated, - VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - }, + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ + Phase: nacv1alpha1.NonAdminBackupPhaseCreated, + VeleroBackup: &nacv1alpha1.VeleroBackup{}, Conditions: []metav1.Condition{ { Type: "Accepted", @@ -369,12 +395,10 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, - Status: &velerov1.BackupStatus{}, + Status: &velerov1.BackupStatus{}, }, Conditions: []metav1.Condition{ { @@ -391,18 +415,19 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, }, - result: reconcile.Result{}, + uuidFromTestCase: true, + result: reconcile.Result{}, }), ginkgo.Entry("When triggered by Requeue(NonAdminBackup phase new) [invalid spec], should update NonAdminBackup phase to BackingOff and Condition to Accepted False and Exit with terminal error", nonAdminBackupSingleReconcileScenario{ - spec: nacv1alpha1.NonAdminBackupSpec{ + nonAdminBackupSpec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ IncludedNamespaces: []string{"not-valid"}, }, }, - priorStatus: &nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupPriorStatus: &nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseNew, }, - ExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ + nonAdminBackupExpectedStatus: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseBackingOff, Conditions: []metav1.Condition{ { @@ -414,27 +439,28 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func }, }, resultError: reconcile.TerminalError(fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: ")), - }), - ) + })) }) var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", func() { var ( - ctx context.Context - cancel context.CancelFunc - nonAdminNamespaceName = "" - oadpNamespaceName = "" - counter = 0 - updateTestScenario = func() { - ctx, cancel = context.WithCancel(context.Background()) - counter++ - nonAdminNamespaceName = fmt.Sprintf("test-nonadminbackup-reconcile-full-%v", counter) - oadpNamespaceName = nonAdminNamespaceName + "-oadp" - } + ctx context.Context + cancel context.CancelFunc + nonAdminObjectName = "" + nonAdminObjectNamespace = "" + oadpNamespace = "" + counter = 0 ) + ginkgo.BeforeEach(func() { + counter++ + nonAdminObjectName = fmt.Sprintf("nab-object-%v", counter) + nonAdminObjectNamespace = fmt.Sprintf("test-nab-reconcile-full-%v", counter) + oadpNamespace = nonAdminObjectNamespace + "-oadp" + }) + ginkgo.AfterEach(func() { - gomega.Expect(deleteTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(deleteTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) cancel() // wait cancel @@ -443,9 +469,9 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", ginkgo.DescribeTable("Reconcile triggered by NonAdminBackup Create event", func(scenario nonAdminBackupFullReconcileScenario) { - updateTestScenario() + ctx, cancel = context.WithCancel(context.Background()) - gomega.Expect(createTestNamespaces(ctx, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) + gomega.Expect(createTestNamespaces(ctx, nonAdminObjectNamespace, oadpNamespace)).To(gomega.Succeed()) k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: k8sClient.Scheme(), @@ -455,7 +481,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", err = (&NonAdminBackupReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), - OADPNamespace: oadpNamespaceName, + OADPNamespace: oadpNamespace, }).SetupWithManager(k8sManager) gomega.Expect(err).ToNot(gomega.HaveOccurred()) @@ -465,28 +491,44 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", gomega.Expect(err).ToNot(gomega.HaveOccurred(), "failed to run manager") }() // wait manager start - time.Sleep(1 * time.Second) + managerStartTimeout := 10 * time.Second + pollInterval := 100 * time.Millisecond + ctxTimeout, cancel := context.WithTimeout(ctx, managerStartTimeout) + defer cancel() + + err = wait.PollUntilContextTimeout(ctxTimeout, pollInterval, managerStartTimeout, true, func(ctx context.Context) (done bool, err error) { + select { + case <-ctx.Done(): + return false, ctx.Err() + default: + // Check if the manager has started by verifying if the client is initialized + return k8sManager.GetClient() != nil, nil + } + }) + // Check if the context timeout or another error occurred + gomega.Expect(err).ToNot(gomega.HaveOccurred(), "Manager failed to start within the timeout period") ginkgo.By("Waiting Reconcile of create event") - nonAdminBackup := buildTestNonAdminBackup(nonAdminNamespaceName, scenario.spec) - gomega.Expect(k8sClient.Create(ctx, nonAdminBackup)).To(gomega.Succeed()) + nonAdminBackup := buildTestNonAdminBackup(nonAdminObjectNamespace, nonAdminObjectName, scenario.spec) + gomega.Expect(k8sClient.Create(ctxTimeout, nonAdminBackup)).To(gomega.Succeed()) // wait NAB reconcile - time.Sleep(1 * time.Second) + time.Sleep(2 * time.Second) ginkgo.By("Fetching NonAdminBackup after Reconcile") gomega.Expect(k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, )).To(gomega.Succeed()) ginkgo.By("Validating NonAdminBackup Status") - gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, nonAdminNamespaceName, oadpNamespaceName)).To(gomega.Succeed()) - if scenario.status.VeleroBackup != nil && len(scenario.status.VeleroBackup.Name) > 0 { + gomega.Expect(checkTestNonAdminBackupStatus(nonAdminBackup, scenario.status, oadpNamespace)).To(gomega.Succeed()) + + if scenario.status.VeleroBackup != nil && len(nonAdminBackup.Status.VeleroBackup.NabNacUUID) > 0 { ginkgo.By("Checking if NonAdminBackup Spec was not changed") gomega.Expect(reflect.DeepEqual( nonAdminBackup.Spec, @@ -494,32 +536,41 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", )).To(gomega.BeTrue()) ginkgo.By("Simulating VeleroBackup update to finished state") + veleroBackup := &velerov1.Backup{} gomega.Expect(k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: function.GenerateVeleroBackupName(nonAdminNamespaceName, testNonAdminBackupName), - Namespace: oadpNamespaceName, + Name: nonAdminBackup.Status.VeleroBackup.NabNacUUID, + Namespace: oadpNamespace, }, veleroBackup, )).To(gomega.Succeed()) - veleroBackup.Status.Phase = velerov1.BackupPhaseCompleted - // TODO can not call .Status().Update() for veleroBackup object: backups.velero.io "name..." not found error - gomega.Expect(k8sClient.Update(ctx, veleroBackup)).To(gomega.Succeed()) + + veleroBackup.Status = velerov1.BackupStatus{ + Phase: velerov1.BackupPhaseCompleted, + } + + gomega.Expect(k8sClient.Update(ctxTimeout, veleroBackup)).To(gomega.Succeed()) + + time.Sleep(1 * time.Second) + ginkgo.By("VeleroBackup updated") + + // wait NAB reconcile gomega.Eventually(func() (bool, error) { err := k8sClient.Get( - ctx, + ctxTimeout, types.NamespacedName{ - Name: testNonAdminBackupName, - Namespace: nonAdminNamespaceName, + Name: nonAdminObjectName, + Namespace: nonAdminObjectNamespace, }, nonAdminBackup, ) if err != nil { return false, err } - if nonAdminBackup.Status.VeleroBackup.Status == nil { + if nonAdminBackup == nil || nonAdminBackup.Status.VeleroBackup == nil || nonAdminBackup.Status.VeleroBackup.Status == nil { return false, nil } return nonAdminBackup.Status.VeleroBackup.Status.Phase == velerov1.BackupPhaseCompleted, nil @@ -527,7 +578,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", } ginkgo.By("Waiting Reconcile of delete event") - gomega.Expect(k8sClient.Delete(ctx, nonAdminBackup)).To(gomega.Succeed()) + gomega.Expect(k8sClient.Delete(ctxTimeout, nonAdminBackup)).To(gomega.Succeed()) time.Sleep(1 * time.Second) }, ginkgo.Entry("Should update NonAdminBackup until VeleroBackup completes and then delete it", nonAdminBackupFullReconcileScenario{ @@ -537,8 +588,7 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", status: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminBackupPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ - Name: placeholder, - Namespace: placeholder, + Namespace: oadpNamespace, Status: nil, }, Conditions: []metav1.Condition{