From d6122032734b4f2bc58cf8871607e216b694c8fd Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Wed, 10 Apr 2024 11:02:41 +0530 Subject: [PATCH] addressing comments and adding testcases --- api/v1/aerospikecluster_mutating_webhook.go | 29 +++++--- api/v1/utils.go | 2 +- controllers/aero_info_calls.go | 6 +- controllers/pod.go | 12 ++-- controllers/rack.go | 46 +++++++----- controllers/reconciler.go | 11 +-- go.mod | 2 +- go.sum | 2 + test/cluster_test.go | 80 ++++++++++++++++++++- test/services_test.go | 4 +- 10 files changed, 151 insertions(+), 43 deletions(-) diff --git a/api/v1/aerospikecluster_mutating_webhook.go b/api/v1/aerospikecluster_mutating_webhook.go index 7089f5d80..7e6203ab9 100644 --- a/api/v1/aerospikecluster_mutating_webhook.go +++ b/api/v1/aerospikecluster_mutating_webhook.go @@ -95,7 +95,7 @@ func (c *AerospikeCluster) setDefaults(asLog logr.Logger) error { // Set common aerospikeConfig defaults // Update configMap - if err := c.setDefaultAerospikeConfigs(asLog, *c.Spec.AerospikeConfig, DefaultRackID); err != nil { + if err := c.setDefaultAerospikeConfigs(asLog, *c.Spec.AerospikeConfig, nil); err != nil { return err } @@ -280,7 +280,7 @@ func (c *AerospikeCluster) updateRacksAerospikeConfigFromGlobal(asLog logr.Logge // Set defaults in updated rack config // Above merge function may have overwritten defaults. - if err := c.setDefaultAerospikeConfigs(asLog, AerospikeConfigSpec{Value: m}, rack.ID); err != nil { + if err := c.setDefaultAerospikeConfigs(asLog, AerospikeConfigSpec{Value: m}, &rack.ID); err != nil { return err } @@ -291,7 +291,7 @@ func (c *AerospikeCluster) updateRacksAerospikeConfigFromGlobal(asLog logr.Logge } func (c *AerospikeCluster) setDefaultAerospikeConfigs(asLog logr.Logger, - configSpec AerospikeConfigSpec, rackID int) error { + configSpec AerospikeConfigSpec, rackID *int) error { config := configSpec.Value // namespace conf @@ -362,7 +362,8 @@ func (n *AerospikeNetworkPolicy) setNetworkNamespace(namespace string) { // Helper // ***************************************************************************** -func setDefaultNsConf(asLog logr.Logger, configSpec AerospikeConfigSpec, rackEnabledNsList []string, rackID int) error { +func setDefaultNsConf(asLog logr.Logger, configSpec AerospikeConfigSpec, + rackEnabledNsList []string, rackID *int) error { config := configSpec.Value // namespace conf nsConf, ok := config["namespaces"] @@ -397,16 +398,28 @@ func setDefaultNsConf(asLog logr.Logger, configSpec AerospikeConfigSpec, rackEna } if nsName, ok := nsMap["name"]; ok { - if _, ok := nsName.(string); ok { - if isNameExist(rackEnabledNsList, nsName.(string)) { - nsMap["rack-id"] = rackID + if name, ok := nsName.(string); ok { + if isNameExist(rackEnabledNsList, name) { + // Add rack-id only for rackEnabled namespaces + if rackID != nil { + // Add rack-id only in rack specific config, not in global config + defaultConfs := map[string]interface{}{"rack-id": *rackID} + if err := setDefaultsInConfigMap( + asLog, nsMap, defaultConfs, + ); err != nil { + return fmt.Errorf( + "failed to set default aerospikeConfig.namespaces rack config: %v", + err, + ) + } + } } else { // User may have added this key or may have patched object with new smaller rackEnabledNamespace list // but left namespace defaults. This key should be removed then only controller will detect // that some namespace is removed from rackEnabledNamespace list and cluster needs rolling restart asLog.Info( "Name aerospikeConfig.namespaces.name not found in rackEnabled namespace list. "+ - "Namespace will not have defaultRackID", + "Namespace will not have any rackID", "nsName", nsName, "rackEnabledNamespaces", rackEnabledNsList, ) diff --git a/api/v1/utils.go b/api/v1/utils.go index 39aa6dfc2..be7a731b3 100644 --- a/api/v1/utils.go +++ b/api/v1/utils.go @@ -63,7 +63,7 @@ const ( AerospikeInitContainerRegistryEnvVar = "AEROSPIKE_KUBERNETES_INIT_REGISTRY" AerospikeInitContainerDefaultRegistry = "docker.io" AerospikeInitContainerDefaultRegistryNamespace = "tanmayj10" - AerospikeInitContainerDefaultRepoAndTag = "aerospike-kubernetes-init:2.1.0-dev1" + AerospikeInitContainerDefaultRepoAndTag = "aerospike-kubernetes-init:2.1.0-dev3" AerospikeAppLabel = "app" AerospikeCustomResourceLabel = "aerospike.com/cr" AerospikeRackIDLabel = "aerospike.com/rack-id" diff --git a/controllers/aero_info_calls.go b/controllers/aero_info_calls.go index a261b5224..df016e31a 100644 --- a/controllers/aero_info_calls.go +++ b/controllers/aero_info_calls.go @@ -346,7 +346,7 @@ func (r *SingleClusterReconciler) setDynamicConfig( return reconcileError(err) } - r.Log.Info("Generated dynamic commands", "commands", fmt.Sprintf("%v", asConfCmds), "pod", podName) + r.Log.Info("Generated dynamic config commands", "commands", fmt.Sprintf("%v", asConfCmds), "pod", podName) if err := deployment.SetConfigCommandsOnHosts(r.Log, r.getClientPolicy(), allHostConns, []*deployment.HostConn{host}, asConfCmds); err != nil { @@ -362,13 +362,13 @@ func (r *SingleClusterReconciler) setDynamicConfig( if patchErr := r.patchPodStatus( context.TODO(), patches, ); patchErr != nil { - return reconcileError(fmt.Errorf("error updating status: %v", patchErr)) + return reconcileError(fmt.Errorf("error updating status: %v, dynamic config command error: %v", patchErr, err)) } return reconcileError(err) } - if err := r.updatePod(podName); err != nil { + if err := r.updateAerospikeConfInPod(podName); err != nil { return reconcileError(err) } } diff --git a/controllers/pod.go b/controllers/pod.go index 698a4dc18..c2e0f9e33 100644 --- a/controllers/pod.go +++ b/controllers/pod.go @@ -112,12 +112,12 @@ func (r *SingleClusterReconciler) getRollingRestartTypeMap(rackState *RackState, return nil, nil, err } - v, err := lib.CompareVersions(version, "7.0.0") + v, err := lib.CompareVersions(version, "6.0.0") if err != nil { return nil, nil, err } - // If version >= 7.0.0, then we can update config dynamically. + // If version >= 6.0.0, then we can update config dynamically. if v >= 0 { // If dynamic commands have failed in previous retry, then we should not try to update config dynamically. if !podStatus.DynamicConfigFailed { @@ -128,7 +128,7 @@ func (r *SingleClusterReconciler) getRollingRestartTypeMap(rackState *RackState, } } } else { - r.Log.Info("Dynamic config change not supported for version < 7.0.0", "currentVersion", version) + r.Log.Info("Dynamic config change not supported for version < 6.0.0", "currentVersion", version) } } @@ -394,14 +394,14 @@ func (r *SingleClusterReconciler) restartPods( return reconcileSuccess() } -func (r *SingleClusterReconciler) updatePod(podName string) error { - r.Log.Info("Updating pod", "pod", podName) +func (r *SingleClusterReconciler) updateAerospikeConfInPod(podName string) error { + r.Log.Info("Updating aerospike config file in pod", "pod", podName) if err := r.restartASDOrUpdateAerospikeConf(podName, noRestartUpdateConf); err != nil { return err } - r.Log.V(1).Info("Pod Updated", "podName", podName) + r.Log.V(1).Info("Updated aerospike config file in pod", "podName", podName) return nil } diff --git a/controllers/rack.go b/controllers/rack.go index 45cd633b0..c442a9c3b 100644 --- a/controllers/rack.go +++ b/controllers/rack.go @@ -397,14 +397,13 @@ func (r *SingleClusterReconciler) upgradeOrRollingRestartRack(found *appsv1.Stat return found, res } } else { - var needRollingRestartRack, needDynamicUpdateRack, restartTypeMap, - dynamicConfDiffPerPod, nErr = r.needRollingRestartRack(rackState, ignorablePodNames) + var rollingRestartInfo, nErr = r.getRollingRestartInfo(rackState, ignorablePodNames) if nErr != nil { return found, reconcileError(nErr) } - if needRollingRestartRack { - found, res = r.rollingRestartRack(found, rackState, ignorablePodNames, restartTypeMap, failedPods) + if rollingRestartInfo.needRestart { + found, res = r.rollingRestartRack(found, rackState, ignorablePodNames, rollingRestartInfo.restartTypeMap, failedPods) if !res.isSuccess { if res.err != nil { r.Log.Error( @@ -424,8 +423,9 @@ func (r *SingleClusterReconciler) upgradeOrRollingRestartRack(found *appsv1.Stat } } - if len(failedPods) == 0 && needDynamicUpdateRack { - res = r.updateDynamicConfig(rackState, ignorablePodNames, restartTypeMap, dynamicConfDiffPerPod) + if len(failedPods) == 0 && rollingRestartInfo.needUpdateConf { + res = r.updateDynamicConfig(rackState, ignorablePodNames, + rollingRestartInfo.restartTypeMap, rollingRestartInfo.dynamicConfDiffPerPod) if !res.isSuccess { if res.err != nil { r.Log.Error( @@ -469,8 +469,8 @@ func (r *SingleClusterReconciler) updateDynamicConfig(rackState *RackState, r.Log.Info("Update dynamic config in Aerospike pods") r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeNormal, "RackDynamicUpdate", - "[rack-%d] Started Dynamic update", rackState.Rack.ID, + r.aeroCluster, corev1.EventTypeNormal, "DynamicConfigUpdate", + "[rack-%d] Started dynamic config update", rackState.Rack.ID, ) var ( @@ -504,8 +504,8 @@ func (r *SingleClusterReconciler) updateDynamicConfig(rackState *RackState, } r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeNormal, "RackDynamicUpdate", - "[rack-%d] Finished Dynamic update", rackState.Rack.ID, + r.aeroCluster, corev1.EventTypeNormal, "DynamicConfigUpdate", + "[rack-%d] Finished Dynamic config update", rackState.Rack.ID, ) return reconcileSuccess() @@ -1215,15 +1215,22 @@ func (r *SingleClusterReconciler) handleK8sNodeBlockListPods(statefulSet *appsv1 return statefulSet, reconcileSuccess() } -func (r *SingleClusterReconciler) needRollingRestartRack(rackState *RackState, ignorablePodNames sets.Set[string]) ( - needRestart, needUpdateConf bool, restartTypeMap map[string]RestartType, - dynamicConfDiffPerPod map[string]asconfig.DynamicConfigMap, err error, +type rollingRestartInfo struct { + restartTypeMap map[string]RestartType + dynamicConfDiffPerPod map[string]asconfig.DynamicConfigMap + needRestart, needUpdateConf bool +} + +func (r *SingleClusterReconciler) getRollingRestartInfo(rackState *RackState, ignorablePodNames sets.Set[string]) ( + info *rollingRestartInfo, err error, ) { - restartTypeMap, dynamicConfDiffPerPod, err = r.getRollingRestartTypeMap(rackState, ignorablePodNames) + restartTypeMap, dynamicConfDiffPerPod, err := r.getRollingRestartTypeMap(rackState, ignorablePodNames) if err != nil { - return needRestart, needUpdateConf, nil, nil, err + return nil, err } + needRestart, needUpdateConf := false, false + for _, restartType := range restartTypeMap { switch restartType { case noRestart: @@ -1235,7 +1242,14 @@ func (r *SingleClusterReconciler) needRollingRestartRack(rackState *RackState, i } } - return needRestart, needUpdateConf, restartTypeMap, dynamicConfDiffPerPod, nil + info = &rollingRestartInfo{ + needRestart: needRestart, + needUpdateConf: needUpdateConf, + restartTypeMap: restartTypeMap, + dynamicConfDiffPerPod: dynamicConfDiffPerPod, + } + + return info, nil } func (r *SingleClusterReconciler) isRackUpgradeNeeded(rackID int, ignorablePodNames sets.Set[string]) ( diff --git a/controllers/reconciler.go b/controllers/reconciler.go index ced438597..0ae486729 100644 --- a/controllers/reconciler.go +++ b/controllers/reconciler.go @@ -463,10 +463,13 @@ func (r *SingleClusterReconciler) updateAccessControlStatus() error { return err } - // AerospikeAccessControl - statusAerospikeAccessControl := lib.DeepCopy( - r.aeroCluster.Spec.AerospikeAccessControl, - ).(*asdbv1.AerospikeAccessControlSpec) + var statusAerospikeAccessControl *asdbv1.AerospikeAccessControlSpec + if r.aeroCluster.Spec.AerospikeAccessControl != nil { + // AerospikeAccessControl + statusAerospikeAccessControl = lib.DeepCopy( + r.aeroCluster.Spec.AerospikeAccessControl, + ).(*asdbv1.AerospikeAccessControlSpec) + } newAeroCluster.Status.AerospikeClusterStatusSpec.AerospikeAccessControl = statusAerospikeAccessControl diff --git a/go.mod b/go.mod index 3b6c6b3ff..df7b4e00c 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.21 toolchain go1.21.8 require ( - github.com/aerospike/aerospike-management-lib v1.2.1-0.20240325134810-f8046fe9872e + github.com/aerospike/aerospike-management-lib v1.3.1-0.20240404063536-2adfbedf9687 github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d github.com/evanphx/json-patch v4.12.0+incompatible github.com/go-logr/logr v1.3.0 diff --git a/go.sum b/go.sum index d5e5f8101..947f1be32 100644 --- a/go.sum +++ b/go.sum @@ -2,6 +2,8 @@ github.com/aerospike/aerospike-client-go/v7 v7.1.0 h1:yvCTKdbpqZxHvv7sWsFHV1j49j github.com/aerospike/aerospike-client-go/v7 v7.1.0/go.mod h1:AkHiKvCbqa1c16gCNGju3c5X/yzwLVvblNczqjxNwNk= github.com/aerospike/aerospike-management-lib v1.2.1-0.20240325134810-f8046fe9872e h1:Q/AfYe++0ouO5csLS8l99kCQqJJvDKlfHwhuWbECpaQ= github.com/aerospike/aerospike-management-lib v1.2.1-0.20240325134810-f8046fe9872e/go.mod h1:E4dk798IikCp9a8fugpYoeQVIXuvdxogHvt6sKhaORQ= +github.com/aerospike/aerospike-management-lib v1.3.1-0.20240404063536-2adfbedf9687 h1:d7oDvHmiKhq4rzcD/w3z9tP3wH0+iaDvxKDk3IYuqeU= +github.com/aerospike/aerospike-management-lib v1.3.1-0.20240404063536-2adfbedf9687/go.mod h1:E4dk798IikCp9a8fugpYoeQVIXuvdxogHvt6sKhaORQ= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs= github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d h1:Byv0BzEl3/e6D5CLfI0j/7hiIEtvGVFPCZ7Ei2oq8iQ= diff --git a/test/cluster_test.go b/test/cluster_test.go index 2181b058a..cdb8efaa0 100644 --- a/test/cluster_test.go +++ b/test/cluster_test.go @@ -74,9 +74,68 @@ var _ = Describe( ScaleDownWithMigrateFillDelay(ctx) }, ) + Context( + "UpdateClusterPre600", func() { + UpdateClusterPre600(ctx) + }, + ) }, ) +func UpdateClusterPre600(ctx goctx.Context) { + Context( + "UpdateClusterPre600", func() { + clusterNamespacedName := getNamespacedName( + "deploy-cluster-pre6", namespace, + ) + + BeforeEach( + func() { + image := fmt.Sprintf( + "aerospike/aerospike-server-enterprise:%s", pre6Version, + ) + aeroCluster, err := getAeroClusterConfig( + clusterNamespacedName, image, + ) + Expect(err).ToNot(HaveOccurred()) + + err = deployCluster(k8sClient, ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + }, + ) + + AfterEach( + func() { + aeroCluster, err := getCluster( + k8sClient, ctx, clusterNamespacedName, + ) + Expect(err).ToNot(HaveOccurred()) + + _ = deleteCluster(k8sClient, ctx, aeroCluster) + }, + ) + + It( + "UpdateReplicationFactor: should fail for updating namespace replication-factor on server"+ + "before 6.0.0. Cannot be updated", func() { + aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + namespaceConfig := + aeroCluster.Spec.AerospikeConfig.Value["namespaces"].([]interface{})[0].(map[string]interface{}) + namespaceConfig["replication-factor"] = 5 + aeroCluster.Spec.AerospikeConfig.Value["namespaces"].([]interface{})[0] = namespaceConfig + + err = k8sClient.Update( + ctx, aeroCluster, + ) + Expect(err).Should(HaveOccurred()) + }, + ) + }, + ) +} + func ScaleDownWithMigrateFillDelay(ctx goctx.Context) { Context( "ScaleDownWithMigrateFillDelay", func() { @@ -920,6 +979,24 @@ func UpdateClusterTest(ctx goctx.Context) { ) Expect(err).ToNot(HaveOccurred()) + By("UpdateReplicationFactor: should update namespace replication-factor on non-SC namespace") + + aeroCluster, err := getCluster( + k8sClient, ctx, + clusterNamespacedName, + ) + Expect(err).ToNot(HaveOccurred()) + nsList := aeroCluster.Spec.AerospikeConfig.Value["namespaces"].([]interface{}) + namespaceConfig := nsList[len(nsList)-1].(map[string]interface{}) + // aeroCluster.Spec.AerospikeConfig.Value["namespaces"].([]interface{})[0].(map[string]interface{}) + namespaceConfig["replication-factor"] = 3 + aeroCluster.Spec.AerospikeConfig.Value["namespaces"].([]interface{})[len(nsList)-1] = namespaceConfig + + err = k8sClient.Update( + ctx, aeroCluster, + ) + Expect(err).ToNot(HaveOccurred()) + By("Scaling up along with modifying Namespace storage Dynamically") err = scaleUpClusterTestWithNSDeviceHandling( @@ -1067,7 +1144,8 @@ func UpdateClusterTest(ctx goctx.Context) { Context( "Namespace", func() { It( - "UpdateReplicationFactor: should fail for updating namespace replication-factor. Cannot be updated", + "UpdateReplicationFactor: should fail for updating namespace"+ + "replication-factor on SC namespace. Cannot be updated", func() { aeroCluster, err := getCluster( k8sClient, ctx, diff --git a/test/services_test.go b/test/services_test.go index c28b0192c..8c80d1329 100644 --- a/test/services_test.go +++ b/test/services_test.go @@ -129,9 +129,7 @@ func createLoadBalancer() *asdbv1.LoadBalancerSpec { ), ) - result := lib.DeepCopy(&lb).(*asdbv1.LoadBalancerSpec) - - return result + return lib.DeepCopy(&lb).(*asdbv1.LoadBalancerSpec) } func loadBalancerName(aeroCluster *asdbv1.AerospikeCluster) types.NamespacedName {