From b20a4cca56031b12f31f17217dcac31a8309fe4d Mon Sep 17 00:00:00 2001 From: Michal Pryc Date: Mon, 14 Oct 2024 19:49:02 +0200 Subject: [PATCH] Fix for #89 - Use custom UUID for NAB Object 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 --- api/v1alpha1/nonadminbackup_types.go | 4 +- ...nac.oadp.openshift.io_nonadminbackups.yaml | 5 +- docs/design/nab_and_nar_status_update.md | 25 +- internal/common/constant/constant.go | 14 +- internal/common/function/function.go | 125 ++++-- internal/common/function/function_test.go | 222 +++++++++-- .../controller/nonadminbackup_controller.go | 124 +++--- .../nonadminbackup_controller_test.go | 366 ++++++++++-------- 8 files changed, 593 insertions(+), 292 deletions(-) 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{