Incorporate review comments

aerospike · Aug 5, 2024 · fc668b0 · fc668b0
1 parent 2169d11
commit fc668b0
Show file tree

Hide file tree

Showing 14 changed files with 356 additions and 258 deletions.
diff --git a/Makefile b/Makefile
@@ -142,29 +142,25 @@ go-lint: golanci-lint ## Run golangci-lint against code.
 
 .PHONY: all-test
 all-test: manifests generate fmt vet envtest ## Run tests.
-	# KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -r --keep-going -coverprofile cover.out -show-node-events -v -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -coverprofile cover.out -r --keep-going -show-node-events -v -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
 
 .PHONY: cluster-test
 cluster-test: manifests generate fmt vet envtest ## Run tests.
-	# KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -v . ./cluster -coverprofile cover.out -show-node-events  -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -coverprofile cover.out --keep-separate-coverprofiles -v . ./cluster -show-node-events -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
 
 
 .PHONY: backup-service-test
 backup-service-test: manifests generate fmt vet envtest ## Run tests.
-	# KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -v . ./backup_service -coverprofile cover.out -show-node-events  -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -coverprofile cover.out --keep-separate-coverprofiles -v . ./backup_service -show-node-events -timeout=1h0m0s --junit-report="junit.xml"  -- ${ARGS}
 
 .PHONY: backup-test
 backup-test: manifests generate fmt vet envtest ## Run tests.
-	# KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -v . ./backup -coverprofile cover.out -show-node-events  -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -coverprofile cover.out --keep-separate-coverprofiles -v . ./backup -show-node-events -timeout=1h0m0s --junit-report="junit.xml"  -- ${ARGS}
 
 .PHONY: restore-test
 restore-test: manifests generate fmt vet envtest ## Run tests.
 	# KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -v . ./restore -coverprofile cover.out -show-node-events  -timeout=12h0m0s --junit-report="junit.xml"  -- ${ARGS}
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" cd $(shell pwd)/test; go run github.com/onsi/ginkgo/v2/ginkgo -coverprofile cover.out --keep-separate-coverprofiles -v . ./restore -show-node-events -timeout=1h0m0s --junit-report="junit.xml"  -- ${ARGS}
 
 ##@ Build
 

diff --git a/api/v1beta1/aerospikebackup_types.go b/api/v1beta1/aerospikebackup_types.go
@@ -17,6 +17,8 @@ limitations under the License.
 package v1beta1
 
 import (
+	"fmt"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
@@ -37,7 +39,7 @@ type AerospikeBackupSpec struct {
 	// OnDemandBackups is the configuration for on-demand backups.
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="On Demand Backups"
 	// +kubebuilder:validation:MaxItems:=1
-	OnDemandBackups []OnDemandBackupSpec `json:"OnDemandBackups,omitempty"`
+	OnDemandBackups []OnDemandBackupSpec `json:"onDemandBackups,omitempty"`
 }
 
 type BackupService struct {
@@ -48,6 +50,10 @@ type BackupService struct {
 	Namespace string `json:"namespace"`
 }
 
+func (b *BackupService) String() string {
+	return fmt.Sprintf("%s/%s", b.Namespace, b.Name)
+}
+
 type OnDemandBackupSpec struct {
 	// ID is the unique identifier for the on-demand backup.
 	// +kubebuilder:validation:MinLength=1
@@ -70,7 +76,7 @@ type AerospikeBackupStatus struct {
 	Config runtime.RawExtension `json:"config"`
 
 	// OnDemandBackups is the configuration for on-demand backups.
-	OnDemandBackups []OnDemandBackupSpec `json:"OnDemandBackups,omitempty"`
+	OnDemandBackups []OnDemandBackupSpec `json:"onDemandBackups,omitempty"`
 
 	// TODO: finalize the status and phase
 }

diff --git a/api/v1beta1/aerospikebackup_webhook.go b/api/v1beta1/aerospikebackup_webhook.go
@@ -64,7 +64,7 @@ func (r *AerospikeBackup) ValidateCreate() (admission.Warnings, error) {
 	aerospikeBackupLog.Info("validate create", "name", r.Name)
 
 	if len(r.Spec.OnDemandBackups) != 0 && r.Spec.Config.Raw != nil {
-		return nil, fmt.Errorf("onDemand and backup config cannot be specified together while creating backup")
+		return nil, fmt.Errorf("onDemand backups config cannot be specified while creating backup")
 	}
 
 	if err := r.validateBackupConfig(); err != nil {
@@ -108,25 +108,6 @@ func (r *AerospikeBackup) ValidateDelete() (admission.Warnings, error) {
 }
 
 func (r *AerospikeBackup) validateBackupConfig() error {
-	var backupSvc AerospikeBackupService
-
-	cl, gErr := getK8sClient()
-	if gErr != nil {
-		return gErr
-	}
-
-	if err := cl.Get(context.TODO(),
-		types.NamespacedName{Name: r.Spec.BackupService.Name, Namespace: r.Spec.BackupService.Namespace},
-		&backupSvc); err != nil {
-		return err
-	}
-
-	var backupSvcConfig model.Config
-
-	if err := yaml.Unmarshal(backupSvc.Spec.Config.Raw, &backupSvcConfig); err != nil {
-		return err
-	}
-
 	backupConfigMap := make(map[string]interface{})
 
 	if err := yaml.Unmarshal(r.Spec.Config.Raw, &backupConfigMap); err != nil {
@@ -149,6 +130,25 @@ func (r *AerospikeBackup) validateBackupConfig() error {
 		return fmt.Errorf("secret-agent field cannot be specified in backup config")
 	}
 
+	var backupSvc AerospikeBackupService
+
+	cl, gErr := getK8sClient()
+	if gErr != nil {
+		return gErr
+	}
+
+	if err := cl.Get(context.TODO(),
+		types.NamespacedName{Name: r.Spec.BackupService.Name, Namespace: r.Spec.BackupService.Namespace},
+		&backupSvc); err != nil {
+		return err
+	}
+
+	var backupSvcConfig model.Config
+
+	if err := yaml.Unmarshal(backupSvc.Spec.Config.Raw, &backupSvcConfig); err != nil {
+		return err
+	}
+
 	aeroClusters, err := r.validateAerospikeCluster(&backupSvcConfig, backupConfigMap)
 	if err != nil {
 		return err
@@ -207,13 +207,13 @@ func getK8sClient() (client.Client, error) {
 }
 
 func (r *AerospikeBackup) validateAerospikeCluster(backupSvcConfig *model.Config,
-	backupSvcConfigMap map[string]interface{},
+	backupConfigMap map[string]interface{},
 ) (map[string]*model.AerospikeCluster, error) {
-	if _, ok := backupSvcConfigMap[common.AerospikeClusterKey]; !ok {
-		return nil, fmt.Errorf("aerospike-cluster field is required in backupSvcConfig")
+	if _, ok := backupConfigMap[common.AerospikeClusterKey]; !ok {
+		return nil, fmt.Errorf("aerospike-cluster field is required field in backup config")
 	}
 
-	cluster, ok := backupSvcConfigMap[common.AerospikeClusterKey].(map[string]interface{})
+	cluster, ok := backupConfigMap[common.AerospikeClusterKey].(map[string]interface{})
 	if !ok {
 		return nil, fmt.Errorf("aerospike-cluster field is not in the right format")
 	}
@@ -238,7 +238,7 @@ func (r *AerospikeBackup) validateAerospikeCluster(backupSvcConfig *model.Config
 	}
 
 	for clusterName, aeroCluster := range aeroClusters {
-		if err := validateName(r.namePrefix(), clusterName); err != nil {
+		if err := validateName(r.NamePrefix(), clusterName); err != nil {
 			return nil, fmt.Errorf("invalid cluster name %s, %s", clusterName, err.Error())
 		}
 
@@ -249,6 +249,7 @@ func (r *AerospikeBackup) validateAerospikeCluster(backupSvcConfig *model.Config
 }
 
 func (r *AerospikeBackup) validateOnDemandBackupsUpdate(oldObj *AerospikeBackup) error {
+	// Check if backup config is updated along with onDemand backup add/update
 	if !reflect.DeepEqual(r.Spec.OnDemandBackups, r.Status.OnDemandBackups) &&
 		!reflect.DeepEqual(r.Spec.Config.Raw, r.Status.Config.Raw) {
 		return fmt.Errorf("can not add/update onDemand backup along with backup config change")
@@ -300,7 +301,7 @@ func (r *AerospikeBackup) validateBackupRoutines(backupSvcConfig *model.Config,
 	aeroClusters map[string]*model.AerospikeCluster,
 ) error {
 	if _, ok := backupSvcConfigMap[common.BackupRoutinesKey]; !ok {
-		return fmt.Errorf("backup-routines field is required in backup onfig")
+		return fmt.Errorf("backup-routines field is required in backup config")
 	}
 
 	routines, ok := backupSvcConfigMap[common.BackupRoutinesKey].(map[string]interface{})
@@ -327,7 +328,7 @@ func (r *AerospikeBackup) validateBackupRoutines(backupSvcConfig *model.Config,
 	// 1. if the correct format name is given
 	// 2. if only correct aerospike cluster (the one referred in Backup CR) is used in backup routines
 	for routineName, routine := range backupRoutines {
-		if err := validateName(r.namePrefix(), routineName); err != nil {
+		if err := validateName(r.NamePrefix(), routineName); err != nil {
 			return fmt.Errorf("invalid backup routine name %s, %s", routineName, err.Error())
 		}
 
@@ -347,7 +348,7 @@ func (r *AerospikeBackup) validateBackupRoutines(backupSvcConfig *model.Config,
 	return nil
 }
 
-func (r *AerospikeBackup) namePrefix() string {
+func (r *AerospikeBackup) NamePrefix() string {
 	return r.Namespace + "-" + r.Name
 }
 

diff --git a/api/v1beta1/aerospikerestore_webhook.go b/api/v1beta1/aerospikerestore_webhook.go
@@ -106,11 +106,11 @@ func (r *AerospikeRestore) validateRestoreConfig() error {
 		var restoreRequest model.RestoreRequest
 
 		if _, ok := restoreConfigInMap[common.RoutineKey]; ok {
-			return fmt.Errorf("routine key is not allowed in restore config for restore type %s", r.Spec.Type)
+			return fmt.Errorf("routine field is not allowed in restore config for restore type %s", r.Spec.Type)
 		}
 
 		if _, ok := restoreConfigInMap[common.TimeKey]; ok {
-			return fmt.Errorf("time key is not allowed in restore config for restore type %s", r.Spec.Type)
+			return fmt.Errorf("time field is not allowed in restore config for restore type %s", r.Spec.Type)
 		}
 
 		if err := yaml.Unmarshal(r.Spec.Config.Raw, &restoreRequest); err != nil {
@@ -123,7 +123,7 @@ func (r *AerospikeRestore) validateRestoreConfig() error {
 		var restoreRequest model.RestoreTimestampRequest
 
 		if _, ok := restoreConfigInMap[common.SourceKey]; ok {
-			return fmt.Errorf("source key is not allowed in restore config for restore type %s", r.Spec.Type)
+			return fmt.Errorf("source field is not allowed in restore config for restore type %s", r.Spec.Type)
 		}
 
 		if err := yaml.Unmarshal(r.Spec.Config.Raw, &restoreRequest); err != nil {

diff --git a/config/crd/bases/asdb.aerospike.com_aerospikebackups.yaml b/config/crd/bases/asdb.aerospike.com_aerospikebackups.yaml
@@ -45,28 +45,6 @@ spec:
             description: AerospikeBackupSpec defines the desired state of AerospikeBackup
               for a given AerospikeCluster
             properties:
-              OnDemandBackups:
-                description: OnDemandBackups is the configuration for on-demand backups.
-                items:
-                  properties:
-                    delay:
-                      description: Delay is the interval before starting the on-demand
-                        backup.
-                      type: string
-                    id:
-                      description: ID is the unique identifier for the on-demand backup.
-                      minLength: 1
-                      type: string
-                    routineName:
-                      description: RoutineName is the routine name used to trigger
-                        on-demand backup.
-                      type: string
-                  required:
-                  - id
-                  - routineName
-                  type: object
-                maxItems: 1
-                type: array
               backupService:
                 description: BackupService is the backup service reference i.e. name
                   and namespace. It is used to communicate to the backup service to
@@ -88,14 +66,7 @@ spec:
                   aerospike-cluster, backup-routines.'
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
-            required:
-            - backupService
-            - config
-            type: object
-          status:
-            description: AerospikeBackupStatus defines the observed state of AerospikeBackup
-            properties:
-              OnDemandBackups:
+              onDemandBackups:
                 description: OnDemandBackups is the configuration for on-demand backups.
                 items:
                   properties:
@@ -115,7 +86,15 @@ spec:
                   - id
                   - routineName
                   type: object
+                maxItems: 1
                 type: array
+            required:
+            - backupService
+            - config
+            type: object
+          status:
+            description: AerospikeBackupStatus defines the observed state of AerospikeBackup
+            properties:
               backupService:
                 description: BackupService is the backup service reference i.e. name
                   and namespace.
@@ -136,6 +115,27 @@ spec:
                   backup-routines.'
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
+              onDemandBackups:
+                description: OnDemandBackups is the configuration for on-demand backups.
+                items:
+                  properties:
+                    delay:
+                      description: Delay is the interval before starting the on-demand
+                        backup.
+                      type: string
+                    id:
+                      description: ID is the unique identifier for the on-demand backup.
+                      minLength: 1
+                      type: string
+                    routineName:
+                      description: RoutineName is the routine name used to trigger
+                        on-demand backup.
+                      type: string
+                  required:
+                  - id
+                  - routineName
+                  type: object
+                type: array
             required:
             - backupService
             - config

diff --git a/config/manifests/bases/aerospike-kubernetes-operator.clusterserviceversion.yaml b/config/manifests/bases/aerospike-kubernetes-operator.clusterserviceversion.yaml
@@ -25,9 +25,6 @@ spec:
       kind: AerospikeBackup
       name: aerospikebackups.asdb.aerospike.com
       specDescriptors:
-      - description: OnDemandBackups is the configuration for on-demand backups.
-        displayName: On Demand Backups
-        path: OnDemandBackups
       - description: BackupService is the backup service reference i.e. name and namespace.
           It is used to communicate to the backup service to trigger backups. This
           field is immutable
@@ -38,6 +35,9 @@ spec:
           backup-routines.'
         displayName: Backup Config
         path: config
+      - description: OnDemandBackups is the configuration for on-demand backups.
+        displayName: On Demand Backups
+        path: onDemandBackups
       version: v1beta1
     - description: AerospikeBackupService is the Schema for the aerospikebackupservices
         API

diff --git a/config/samples/aerospikebackup.yaml b/config/samples/aerospikebackup.yaml
@@ -15,7 +15,7 @@ spec:
     namespace: aerospike
 #  onDemandBackups:
 #    - id: first-ad-hoc-backup
-#      routineName: test-routine
+#      routineName: aerospike-aerospikebackup-test-routine
   config:
     aerospike-cluster:
       aerospike-aerospikebackup-test-cluster: