From ba54c087ba352850d24fd08d3900ec06afb79126 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Wed, 14 May 2025 17:18:55 +0530 Subject: [PATCH 1/9] feat: adding support for dynamic rackid assignment after pod schedule [KO-385] --- api/v1/aerospikecluster_types.go | 23 +++ api/v1/utils.go | 6 +- api/v1/zz_generated.deepcopy.go | 30 ++++ .../asdb.aerospike.com_aerospikeclusters.yaml | 158 ++++++++++++++++++ config/manager/manager.yaml | 4 +- ..._aerospikeclusters.asdb.aerospike.com.yaml | 158 ++++++++++++++++++ .../aerospike-kubernetes-operator/values.yaml | 4 +- internal/controller/cluster/pod.go | 13 ++ internal/controller/cluster/statefulset.go | 5 +- .../v1/aerospikecluster_mutating_webhook.go | 20 ++- .../v1/aerospikecluster_validating_webhook.go | 48 +++++- internal/webhook/v1/storage.go | 4 + 12 files changed, 453 insertions(+), 20 deletions(-) diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index 6d01b1af..db97ed69 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -483,6 +483,11 @@ type RackConfig struct { //nolint:govet // for readability // This makes sure that later on, all pods are properly counted when evaluating the cluster stability. // +optional MaxIgnorablePods *intstr.IntOrString `json:"maxIgnorablePods,omitempty"` + + // RackIDSource specifies the source from which to read the rack ID. + // If not specified, the rack ID is read from the CR. + // +optional + RackIDSource *RackIDSource `json:"rackIDSource,omitempty"` } // Rack specifies single rack config @@ -766,6 +771,12 @@ type VolumeSource struct { // +optional PersistentVolume *PersistentVolumeSpec `json:"persistentVolume,omitempty"` + + // HostPath represents a directory on the host provisioned by an administrator. + // This is useful for exposing host paths to pods in a controlled, read-only manner. + // More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + // +optional + HostPath *corev1.HostPathVolumeSource `json:"hostPath,omitempty"` } type VolumeSpec struct { @@ -1100,6 +1111,14 @@ type AerospikeNetworkPolicy struct { CustomTLSFabricNetworkNames []string `json:"customTLSFabricNetworkNames,omitempty"` } +// RackIDSource specifies the source from which to read the rack ID. +type RackIDSource struct { + // FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + // The file should contain a single integer value. + // +optional + FilePath string `json:"filePath,omitempty"` +} + // AerospikeInstanceSummary defines the observed state of a pod's Aerospike Server Instance. // +k8s:openapi-gen=true type AerospikeInstanceSummary struct { //nolint:govet // for readability @@ -1191,6 +1210,10 @@ type AerospikePodStatus struct { //nolint:govet // for readability // Empty "" status means successful update. // +optional DynamicConfigUpdateStatus DynamicConfigUpdateStatus `json:"dynamicConfigUpdateStatus,omitempty"` + + // RackIDSource is the source from which the rack ID is read. + // +optional + RackIDSource *RackIDSource `json:"rackIDSource,omitempty"` } // +kubebuilder:object:root=true diff --git a/api/v1/utils.go b/api/v1/utils.go index 358dcd7c..a651066a 100644 --- a/api/v1/utils.go +++ b/api/v1/utils.go @@ -69,8 +69,8 @@ const ( AerospikeInitContainerRegistryNamespaceEnvVar = "AEROSPIKE_KUBERNETES_INIT_REGISTRY_NAMESPACE" AerospikeInitContainerNameTagEnvVar = "AEROSPIKE_KUBERNETES_INIT_NAME_TAG" AerospikeInitContainerDefaultRegistry = "docker.io" - AerospikeInitContainerDefaultRegistryNamespace = "aerospike" - AerospikeInitContainerDefaultNameAndTag = "aerospike-kubernetes-init:2.3.0-dev2" + AerospikeInitContainerDefaultRegistryNamespace = "tanmay10" + AerospikeInitContainerDefaultNameAndTag = "aerospike-kubernetes-init:2.3.0-dev3" AerospikeAppLabel = "app" AerospikeAppLabelValue = "aerospike-cluster" AerospikeCustomResourceLabel = "aerospike.com/cr" @@ -633,7 +633,7 @@ func GetVolumeForAerospikePath(storage *AerospikeStorageSpec, path string) *Volu return matchedVolume } -// IsPathParentOrSame indicates if dir1 is a parent or same as dir2. +// IsPathParentOrSame indicates if dir1 is a parent or the same as dir2. func IsPathParentOrSame(dir1, dir2 string) bool { if relPath, err := filepath.Rel(dir1, dir2); err == nil { // If dir1 is not a parent directory then relative path will have to climb up directory hierarchy of dir1. diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 1d768905..3b06f497 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -651,6 +651,11 @@ func (in *AerospikePodStatus) DeepCopyInto(out *AerospikePodStatus) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.RackIDSource != nil { + in, out := &in.RackIDSource, &out.RackIDSource + *out = new(RackIDSource) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AerospikePodStatus. @@ -960,6 +965,11 @@ func (in *RackConfig) DeepCopyInto(out *RackConfig) { *out = new(intstr.IntOrString) **out = **in } + if in.RackIDSource != nil { + in, out := &in.RackIDSource, &out.RackIDSource + *out = new(RackIDSource) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RackConfig. @@ -972,6 +982,21 @@ func (in *RackConfig) DeepCopy() *RackConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RackIDSource) DeepCopyInto(out *RackIDSource) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RackIDSource. +func (in *RackIDSource) DeepCopy() *RackIDSource { + if in == nil { + return nil + } + out := new(RackIDSource) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RackPodSpec) DeepCopyInto(out *RackPodSpec) { *out = *in @@ -1112,6 +1137,11 @@ func (in *VolumeSource) DeepCopyInto(out *VolumeSource) { *out = new(PersistentVolumeSpec) (*in).DeepCopyInto(*out) } + if in.HostPath != nil { + in, out := &in.HostPath, &out.HostPath + *out = new(corev1.HostPathVolumeSource) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VolumeSource. diff --git a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml index 98ea6c8b..1e27d810 100644 --- a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml +++ b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml @@ -5150,6 +5150,17 @@ spec: items: type: string type: array + rackIDSource: + description: |- + RackIDSource specifies the source from which to read the rack ID. + If not specified, the rack ID is read from the CR. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object racks: description: Racks is the list of all racks items: @@ -6564,6 +6575,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -8165,6 +8197,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -8838,6 +8891,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach to Aerospike pods. @@ -14246,6 +14320,16 @@ spec: description: PodSpecHash is ripemd160 hash of PodSpec used by this pod type: string + rackIDSource: + description: RackIDSource is the source from which the rack + ID is read. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object servicePort: description: ServicePort is the port Aerospike clients outside K8s can connect to. @@ -14291,6 +14375,17 @@ spec: items: type: string type: array + rackIDSource: + description: |- + RackIDSource specifies the source from which to read the rack ID. + If not specified, the rack ID is read from the CR. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object racks: description: Racks is the list of all racks items: @@ -15705,6 +15800,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -17306,6 +17422,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -18045,6 +18182,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach to Aerospike pods. diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index ccd5cdff..1b6ecc5a 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -65,10 +65,10 @@ spec: value: docker.io - name: AEROSPIKE_KUBERNETES_INIT_REGISTRY_NAMESPACE # this is the namespace in registry used to pull aerospike-init image - value: aerospike + value: tanmayj10 - name: AEROSPIKE_KUBERNETES_INIT_NAME_TAG # this is the name and tag of aerospike-init image - value: aerospike-kubernetes-init:2.3.0-dev2 + value: aerospike-kubernetes-init:2.3.0-dev3 volumes: [] serviceAccountName: controller-manager diff --git a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml index 98ea6c8b..1e27d810 100644 --- a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml +++ b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml @@ -5150,6 +5150,17 @@ spec: items: type: string type: array + rackIDSource: + description: |- + RackIDSource specifies the source from which to read the rack ID. + If not specified, the rack ID is read from the CR. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object racks: description: Racks is the list of all racks items: @@ -6564,6 +6575,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -8165,6 +8197,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -8838,6 +8891,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach to Aerospike pods. @@ -14246,6 +14320,16 @@ spec: description: PodSpecHash is ripemd160 hash of PodSpec used by this pod type: string + rackIDSource: + description: RackIDSource is the source from which the rack + ID is read. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object servicePort: description: ServicePort is the port Aerospike clients outside K8s can connect to. @@ -14291,6 +14375,17 @@ spec: items: type: string type: array + rackIDSource: + description: |- + RackIDSource specifies the source from which to read the rack ID. + If not specified, the rack ID is read from the CR. + properties: + filePath: + description: |- + FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + The file should contain a single integer value. + type: string + type: object racks: description: Racks is the list of all racks items: @@ -15705,6 +15800,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -17306,6 +17422,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach @@ -18045,6 +18182,27 @@ spec: pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true type: object + hostPath: + description: |- + HostPath represents a directory on the host provisioned by an administrator. + This is useful for exposing host paths to pods in a controlled, read-only manner. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + properties: + path: + description: |- + path of the directory on the host. + If the path is a symlink, it will follow the link to the real path. + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + type: + description: |- + type for HostPath Volume + Defaults to "" + More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + type: string + required: + - path + type: object persistentVolume: description: PersistentVolumeSpec describes a persistent volume to claim and attach to Aerospike pods. diff --git a/helm-charts/aerospike-kubernetes-operator/values.yaml b/helm-charts/aerospike-kubernetes-operator/values.yaml index 03c4a98c..4438ad53 100644 --- a/helm-charts/aerospike-kubernetes-operator/values.yaml +++ b/helm-charts/aerospike-kubernetes-operator/values.yaml @@ -38,10 +38,10 @@ watchNamespaces: "default,aerospike" aerospikeKubernetesInitRegistry: "docker.io" # Namespace in registry used to pull aerospike-init image -aerospikeKubernetesInitRegistryNamespace: "aerospike" +aerospikeKubernetesInitRegistryNamespace: "tanmay10" # Name and tag of aerospike-init image -aerospikeKubernetesInitNameTag: "aerospike-kubernetes-init:2.3.0-dev2" +aerospikeKubernetesInitNameTag: "aerospike-kubernetes-init:2.3.0-dev3" ## Resources - limits / requests resources: diff --git a/internal/controller/cluster/pod.go b/internal/controller/cluster/pod.go index 6f3abc03..3273e4a1 100644 --- a/internal/controller/cluster/pod.go +++ b/internal/controller/cluster/pod.go @@ -137,6 +137,10 @@ func (r *SingleClusterReconciler) getRollingRestartTypeMap(rackState *RackState, if podStatus.DynamicConfigUpdateStatus == asdbv1.PartiallyFailed { restartTypeMap[pods[idx].Name] = mergeRestartType(restartTypeMap[pods[idx].Name], quickRestart) } + + if !reflect.DeepEqual(podStatus.RackIDSource, r.aeroCluster.Spec.RackConfig.RackIDSource) { + restartTypeMap[pods[idx].Name] = mergeRestartType(restartTypeMap[pods[idx].Name], quickRestart) + } } return restartTypeMap, dynamicConfDiffPerPod, nil @@ -178,6 +182,7 @@ func (r *SingleClusterReconciler) getRollingRestartTypePod( "AerospikeConfig changed. Need rolling restart or update config dynamically", "requiredHash", requiredConfHash, "currentHash", podStatus.AerospikeConfigHash, + "podRestartType", podRestartType, ) } @@ -1391,6 +1396,14 @@ func (r *SingleClusterReconciler) handleDynamicConfigChange(rackState *RackState } if len(specToStatusDiffs) > 0 { + // rackd-id change is being handled as rack add/remove + // Ignoring it to eliminate rolling restart in case of dynamic rack id allocation. + for key := range specToStatusDiffs { + if asconfig.BaseKey(key) == "rack-id" { + delete(specToStatusDiffs, key) + } + } + isDynamic, err := asconfig.IsAllDynamicConfig(r.Log, specToStatusDiffs, version) if err != nil { r.Log.Info("Failed to check if all config is dynamic, fallback to rolling restart", "error", err.Error()) diff --git a/internal/controller/cluster/statefulset.go b/internal/controller/cluster/statefulset.go index e703ce41..660155f8 100644 --- a/internal/controller/cluster/statefulset.go +++ b/internal/controller/cluster/statefulset.go @@ -1431,12 +1431,13 @@ func createPVCForVolumeAttachment( func createVolumeForVolumeAttachment(volume *asdbv1.VolumeSpec) corev1.Volume { return corev1.Volume{ Name: volume.Name, - // Add all type of source, - // we have already validated in webhook that only one of the source is present, rest are nil. + // Add all types of source; + // we have already validated in webhook that only one of the sources is present, the rest are nil. VolumeSource: corev1.VolumeSource{ ConfigMap: volume.Source.ConfigMap, Secret: volume.Source.Secret, EmptyDir: volume.Source.EmptyDir, + HostPath: volume.Source.HostPath, }, } } diff --git a/internal/webhook/v1/aerospikecluster_mutating_webhook.go b/internal/webhook/v1/aerospikecluster_mutating_webhook.go index 3875d4af..2f96e666 100644 --- a/internal/webhook/v1/aerospikecluster_mutating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_mutating_webhook.go @@ -299,7 +299,7 @@ func setDefaultAerospikeConfigs(asLog logr.Logger, config := configSpec.Value // namespace conf - if err := setDefaultNsConf(asLog, configSpec, cluster.Spec.RackConfig.Namespaces, rackID); err != nil { + if err := setDefaultNsConf(asLog, configSpec, &cluster.Spec.RackConfig, rackID); err != nil { return err } @@ -366,8 +366,8 @@ func setNetworkNamespace(namespace string, networkPolicy *asdbv1.AerospikeNetwor // Helper // ***************************************************************************** -func setDefaultNsConf(asLog logr.Logger, configSpec asdbv1.AerospikeConfigSpec, - rackEnabledNsList []string, rackID *int) error { +func setDefaultNsConf(asLog logr.Logger, configSpec asdbv1.AerospikeConfigSpec, rackConfig *asdbv1.RackConfig, + rackID *int) error { config := configSpec.Value // namespace conf nsConf, ok := config["namespaces"] @@ -403,14 +403,20 @@ func setDefaultNsConf(asLog logr.Logger, configSpec asdbv1.AerospikeConfigSpec, if nsName, ok := nsMap["name"]; ok { if name, ok := nsName.(string); ok { - if isNameExist(rackEnabledNsList, name) { + if isNameExist(rackConfig.Namespaces, name) { // Add rack-id only for rackEnabled namespaces if rackID != nil { - // Add rack-id only in rack specific config, not in global config + // Add rack-id only in rack-specific config, not in global config defaultConfs := map[string]interface{}{"rack-id": *rackID} + if rackConfig.RackIDSource != nil { + // For dynamic rack ID, set a placeholder rack-id of 0 + // This will be replaced with the actual rack ID at runtime + defaultConfs = map[string]interface{}{"rack-id": 0} + } + // rack-id was historically set to 0 for all namespaces, but since the AKO 3.3.0, it reflects actual values. - // During the AKO 3.3.0 upgrade rack-id for namespaces in rack specific config is set to 0. + // During the AKO 3.3.0 upgrade rack-id for namespaces in rack-specific config is set to 0. // Hence, deleting this 0 rack-id so that correct rack-id will be added. if id, ok := nsMap["rack-id"]; ok && id == float64(0) && *rackID != 0 { delete(nsMap, "rack-id") @@ -437,7 +443,7 @@ func setDefaultNsConf(asLog logr.Logger, configSpec asdbv1.AerospikeConfigSpec, "Name aerospikeConfig.namespaces.name not found in rackEnabled namespace list. "+ "Namespace will not have any rackID", "nsName", nsName, "rackEnabledNamespaces", - rackEnabledNsList, + rackConfig.Namespaces, ) delete(nsMap, "rack-id") diff --git a/internal/webhook/v1/aerospikecluster_validating_webhook.go b/internal/webhook/v1/aerospikecluster_validating_webhook.go index 41a03435..cb2ea52b 100644 --- a/internal/webhook/v1/aerospikecluster_validating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_validating_webhook.go @@ -591,6 +591,46 @@ func validateRackConfig(_ logr.Logger, cluster *asdbv1.AerospikeCluster) error { } } + // Validate dynamic rack ID configuration + rackIDSource := cluster.Spec.RackConfig.RackIDSource + if rackIDSource != nil { + if rackIDSource.FilePath != "" { + if !filepath.IsAbs(rackIDSource.FilePath) { + return fmt.Errorf("rackIDSource file path must be absolute") + } + + // Verify the volume exists in storage spec + volume := asdbv1.GetVolumeForAerospikePath(&cluster.Spec.RackConfig.Racks[0].Storage, rackIDSource.FilePath) + if volume == nil { + return fmt.Errorf("volume not found in storage spec for mount path %s", rackIDSource.FilePath) + } + + if volume.Source.HostPath == nil { + return fmt.Errorf("HostPathVolumeName %s must be a hostpath volume", volume.Name) + } + } else { + return fmt.Errorf("filePath cannot be empty when rackIDSource is specified") + } + + // Cannot specify static racks when rackIDSource is provided + if len(cluster.Spec.RackConfig.Racks) > 1 { + return fmt.Errorf("cannot specify more than 1 rack when rackIDSource is provided") + } + + // Cannot specify batch operations or maxignorable pods when rackIDSource is provided + if cluster.Spec.RackConfig.RollingUpdateBatchSize != nil { + return fmt.Errorf("rollingUpdateBatchSize cannot be specified when rackIDSource is provided") + } + + if cluster.Spec.RackConfig.ScaleDownBatchSize != nil { + return fmt.Errorf("scaleDownBatchSize cannot be specified when rackIDSource is provided") + } + + if cluster.Spec.RackConfig.MaxIgnorablePods != nil { + return fmt.Errorf("maxIgnorablePods cannot be specified when rackIDSource is provided") + } + } + rackMap := map[int]bool{} migrateFillDelaySet := sets.Set[int]{} @@ -674,11 +714,11 @@ type nsConf struct { scEnabled bool } -func getNsConfForNamespaces(rackConfig asdbv1.RackConfig) map[string]nsConf { +func getNsConfForNamespaces(racks []asdbv1.Rack) map[string]nsConf { nsConfs := map[string]nsConf{} - for idx := range rackConfig.Racks { - rack := &rackConfig.Racks[idx] + for idx := range racks { + rack := &racks[idx] nsList := rack.AerospikeConfig.Value["namespaces"].([]interface{}) for _, nsInterface := range nsList { @@ -1414,7 +1454,7 @@ func validateBatchSize(batchSize *intstr.IntOrString, rollingUpdateBatch bool, c return fmt.Errorf("can not use %s when number of racks is less than two", fieldPath) } - nsConfsNamespaces := getNsConfForNamespaces(rackConfig) + nsConfsNamespaces := getNsConfForNamespaces(rackConfig.Racks) for ns, nsConf := range nsConfsNamespaces { if !isNameExist(rackConfig.Namespaces, ns) { return fmt.Errorf( diff --git a/internal/webhook/v1/storage.go b/internal/webhook/v1/storage.go index ec513162..609f9841 100644 --- a/internal/webhook/v1/storage.go +++ b/internal/webhook/v1/storage.go @@ -428,6 +428,10 @@ func validateStorageVolumeSource(volume *asdbv1.VolumeSpec) error { sourceCount++ } + if source.HostPath != nil { + sourceCount++ + } + if sourceCount == 0 { return fmt.Errorf("no volume source found") } From f66cae06c001bf252641db653e845e445e9cf335 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Fri, 16 May 2025 15:06:49 +0530 Subject: [PATCH 2/9] adding tests --- internal/controller/cluster/rack.go | 12 +---------- internal/controller/cluster/statefulset.go | 14 +++++++++++++ test/cluster/cluster_helper.go | 23 +++++++++++++++++++++- test/test.sh | 4 ++-- 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/internal/controller/cluster/rack.go b/internal/controller/cluster/rack.go index 514ec592..f36a058e 100644 --- a/internal/controller/cluster/rack.go +++ b/internal/controller/cluster/rack.go @@ -1344,17 +1344,7 @@ func (r *SingleClusterReconciler) isRackStorageUpdatedInAeroCluster( } // Check for Added/Updated volumeAttachments - var containerAttachments []asdbv1.VolumeAttachment - containerAttachments = append(containerAttachments, volume.Sidecars...) - - if volume.Aerospike != nil { - containerAttachments = append( - containerAttachments, asdbv1.VolumeAttachment{ - ContainerName: asdbv1.AerospikeServerContainerName, - Path: volume.Aerospike.Path, - }, - ) - } + _, containerAttachments := getFinalVolumeAttachmentsForVolume(volume) if r.isVolumeAttachmentAddedOrUpdated( volume.Name, containerAttachments, pod.Spec.Containers, diff --git a/internal/controller/cluster/statefulset.go b/internal/controller/cluster/statefulset.go index 660155f8..dd04a28e 100644 --- a/internal/controller/cluster/statefulset.go +++ b/internal/controller/cluster/statefulset.go @@ -1454,10 +1454,19 @@ func getFinalVolumeAttachmentsForVolume(volume *asdbv1.VolumeSpec) ( initContainerAttachments = append( initContainerAttachments, volume.InitContainers..., ) + + readOnlyVolume := false + if volume.Source.HostPath != nil { + readOnlyVolume = true + } + initContainerAttachments = append( initContainerAttachments, asdbv1.VolumeAttachment{ ContainerName: asdbv1.AerospikeInitContainerName, Path: initVolumePath, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ReadOnly: readOnlyVolume}, + }, }, ) @@ -1469,6 +1478,9 @@ func getFinalVolumeAttachmentsForVolume(volume *asdbv1.VolumeSpec) ( containerAttachments, asdbv1.VolumeAttachment{ ContainerName: asdbv1.AerospikeServerContainerName, Path: volume.Aerospike.Path, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ReadOnly: readOnlyVolume}, + }, }, ) } @@ -1491,11 +1503,13 @@ func addVolumeMountInContainer( volumeMount = corev1.VolumeMount{ Name: volumeName, MountPath: pathPrefix + volumeAttachment.Path, + ReadOnly: volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, } } else { volumeMount = corev1.VolumeMount{ Name: volumeName, MountPath: volumeAttachment.Path, + ReadOnly: volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, } } diff --git a/test/cluster/cluster_helper.go b/test/cluster/cluster_helper.go index 4a342647..abb778ca 100644 --- a/test/cluster/cluster_helper.go +++ b/test/cluster/cluster_helper.go @@ -984,7 +984,6 @@ func createDummyRackAwareWithStorageAerospikeCluster( return aeroCluster } -//nolint:unparam // generic function func createDummyRackAwareAerospikeCluster( clusterNamespacedName types.NamespacedName, size int32, ) *asdbv1.AerospikeCluster { @@ -1081,6 +1080,28 @@ func createNonSCDummyAerospikeCluster( return aerospikeCluster } +//nolint:unparam // generic function +func createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName types.NamespacedName, size int32, + aerospikePath, hostPath string) *asdbv1.AerospikeCluster { + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, size) + + hostPathVolume := asdbv1.VolumeSpec{ + Name: "hostpath", + Source: asdbv1.VolumeSource{ + HostPath: &corev1.HostPathVolumeSource{ + Path: hostPath, + }, + }, + Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ + Path: aerospikePath, + }, + } + + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, hostPathVolume) + + return aeroCluster +} + func createDummyAerospikeCluster( clusterNamespacedName types.NamespacedName, size int32, ) *asdbv1.AerospikeCluster { diff --git a/test/test.sh b/test/test.sh index 86aba083..1d59619d 100755 --- a/test/test.sh +++ b/test/test.sh @@ -30,8 +30,8 @@ done # Defaults CRED_PATH=${CRED_PATH:-$HOME/.docker/config.json} REGISTRY=${REGISTRY:-568976754000.dkr.ecr.ap-south-1.amazonaws.com} -REGISTRY_NAMESPACE=${REGISTRY_NAMESPACE:-aerospike} -INIT_IMAGE_NAME_TAG=${INIT_IMAGE_NAME_TAG:-aerospike-kubernetes-init:2.3.0-dev2} +REGISTRY_NAMESPACE=${REGISTRY_NAMESPACE:-tanmayj10} +INIT_IMAGE_NAME_TAG=${INIT_IMAGE_NAME_TAG:-aerospike-kubernetes-init:2.3.0-dev3} DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" From 9e87e8219c6e6e95e960844e6eea846f67861776 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Mon, 19 May 2025 13:25:24 +0530 Subject: [PATCH 3/9] adding tests --- test/cluster/dynamic_rackid_test.go | 357 ++++++++++++++++++++++++++++ 1 file changed, 357 insertions(+) create mode 100644 test/cluster/dynamic_rackid_test.go diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go new file mode 100644 index 00000000..c0fff3bd --- /dev/null +++ b/test/cluster/dynamic_rackid_test.go @@ -0,0 +1,357 @@ +package cluster + +import ( + goctx "context" + "fmt" + "strings" + + asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" + "github.com/aerospike/aerospike-kubernetes-operator/v4/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +var _ = FDescribe( + "DynamicRack", func() { + ctx := goctx.TODO() + clusterName := fmt.Sprintf("dynamic-rack-%d", GinkgoParallelProcess()) + clusterNamespacedName := test.GetNamespacedName( + clusterName, namespace, + ) + FContext( + "When doing valid operations", Ordered, func() { + AfterEach( + func() { + aeroCluster := &asdbv1.AerospikeCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + } + + Expect(DeleteCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + Expect(CleanupPVC(k8sClient, aeroCluster.Namespace, aeroCluster.Name)).ToNot(HaveOccurred()) + }, + ) + + BeforeAll( + func() { + dir := v1.HostPathDirectory + ds := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hostpath-writer", + Namespace: namespace, + Labels: map[string]string{ + "app": "hostpath-writer", + }, + }, + Spec: appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "hostpath-writer"}, + }, + Template: v1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "hostpath-writer"}, // must match selector + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "writer", + Image: "busybox", + Command: []string{"sh", "-c", `echo "2" > /tmp/rackid && sleep 3600`}, + VolumeMounts: []v1.VolumeMount{ + { + Name: "tmp-mount", + MountPath: "/tmp", + }, + }, + Lifecycle: &v1.Lifecycle{ + PreStop: &v1.LifecycleHandler{ + Exec: &v1.ExecAction{ + Command: []string{ + "/bin/sh", "-c", "rm /tmp/rackid; sleep 5", + }, + }, + }, + }, + }, + }, + Volumes: []v1.Volume{ + { + Name: "tmp-mount", + VolumeSource: v1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{ + Path: "/tmp", + Type: &dir, + }, + }, + }, + }, + }, + }, + }, + } + + err := k8sClient.Create(ctx, ds) + Expect(err).NotTo(HaveOccurred()) + }, + ) + + AfterAll( + func() { + ds := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hostpath-writer", + Namespace: namespace, + }, + } + + err := k8sClient.Delete(ctx, ds) + Expect(err).NotTo(HaveOccurred()) + }) + + Context( + "When deploying cluster", func() { + It( + "Should deploy with dynamic rack", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, "/opt/hostpath", "/tmp") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.Namespaces = []string{"test"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + + aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + + info, err := requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + Expect(err).ToNot(HaveOccurred()) + + confs := strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + Expect(keyValue[1]).To(Equal("2")) + } + } + }, + ) + }, + ) + + Context( + "When updating existing cluster", func() { + It( + "Should deploy with dynamic rack", func() { + By("Deploying cluster with single rack") + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) + aeroCluster.Spec.RackConfig.Namespaces = []string{"test"} + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + + oldPodIDs, err := getPodIDs(ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + + By("Updating cluster with dynamic rack and add hostpath volume") + + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + hostPathVolume := asdbv1.VolumeSpec{ + Name: "hostpath", + Source: asdbv1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{ + Path: "/tmp", + }, + }, + Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ + Path: "/opt/hostpath", + }, + } + + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, hostPathVolume) + + Expect(updateCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + + operationTypeMap := map[string]asdbv1.OperationKind{ + aeroCluster.Name + "-1-0": asdbv1.OperationPodRestart, + aeroCluster.Name + "-1-1": asdbv1.OperationPodRestart, + } + + aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) + Expect(err).ToNot(HaveOccurred()) + + pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + + info, err := requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + Expect(err).ToNot(HaveOccurred()) + + confs := strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + Expect(keyValue[1]).To(Equal("2")) + } + } + + By("Updating cluster by disabling dynamic rack") + oldPodIDs, err = getPodIDs(ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + + aeroCluster.Spec.RackConfig.RackIDSource = nil + Expect(updateCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + + operationTypeMap = map[string]asdbv1.OperationKind{ + aeroCluster.Name + "-1-0": asdbv1.OperationWarmRestart, + aeroCluster.Name + "-1-1": asdbv1.OperationWarmRestart, + } + + aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) + Expect(err).ToNot(HaveOccurred()) + + pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + + info, err = requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + Expect(err).ToNot(HaveOccurred()) + + confs = strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + Expect(keyValue[1]).To(Equal("1")) + } + } + + By("Updating cluster by enabling dynamic rack") + oldPodIDs, err = getPodIDs(ctx, aeroCluster) + Expect(err).ToNot(HaveOccurred()) + + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + Expect(updateCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) + + operationTypeMap = map[string]asdbv1.OperationKind{ + aeroCluster.Name + "-1-0": asdbv1.OperationWarmRestart, + aeroCluster.Name + "-1-1": asdbv1.OperationWarmRestart, + } + + aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) + Expect(err).ToNot(HaveOccurred()) + + pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + + info, err = requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + Expect(err).ToNot(HaveOccurred()) + + confs = strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + Expect(keyValue[1]).To(Equal("2")) + } + } + }, + ) + }, + ) + }, + ) + Context( + "When doing invalid operations", func() { + + It( + "Should fail if multiple racks are given along with dynamic rack", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, + "/opt/hostpath", "/dev/null") + aeroCluster.Spec.RackConfig.Racks = append(aeroCluster.Spec.RackConfig.Racks, asdbv1.Rack{ID: 2}) + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if RollingUpdateBatchSize is given along with dynamic rack", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, + "/opt/hostpath", "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.RollingUpdateBatchSize = &intstr.IntOrString{IntVal: 2} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if ScaleDownBatchSize is given along with dynamic rack", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, + "/opt/hostpath", "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.ScaleDownBatchSize = &intstr.IntOrString{IntVal: 2} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if MaxIgnorablePods is given along with dynamic rack", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, + "/opt/hostpath", "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.MaxIgnorablePods = &intstr.IntOrString{IntVal: 2} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if dynamic rackID source file path is not absolute", func() { + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, + "/opt/hostpath", "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "rackid"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if dynamic rackID source volume is not mounted", func() { + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + + It( + "Should fail if dynamic rackID source volume is not hostpath type", func() { + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) + emptydirVolume := asdbv1.VolumeSpec{ + Name: "empty", + Source: asdbv1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ + Path: "/opt/empty", + }, + } + + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, emptydirVolume) + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/empty/rackid"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) + }, + ) + }, +) From 28054a19f07974edf9b038a4327bd170fe321793 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Tue, 20 May 2025 15:02:59 +0530 Subject: [PATCH 4/9] removing some checks, as these are already covered. --- .../v1/aerospikecluster_validating_webhook.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/internal/webhook/v1/aerospikecluster_validating_webhook.go b/internal/webhook/v1/aerospikecluster_validating_webhook.go index cb2ea52b..1c10e6da 100644 --- a/internal/webhook/v1/aerospikecluster_validating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_validating_webhook.go @@ -616,19 +616,6 @@ func validateRackConfig(_ logr.Logger, cluster *asdbv1.AerospikeCluster) error { if len(cluster.Spec.RackConfig.Racks) > 1 { return fmt.Errorf("cannot specify more than 1 rack when rackIDSource is provided") } - - // Cannot specify batch operations or maxignorable pods when rackIDSource is provided - if cluster.Spec.RackConfig.RollingUpdateBatchSize != nil { - return fmt.Errorf("rollingUpdateBatchSize cannot be specified when rackIDSource is provided") - } - - if cluster.Spec.RackConfig.ScaleDownBatchSize != nil { - return fmt.Errorf("scaleDownBatchSize cannot be specified when rackIDSource is provided") - } - - if cluster.Spec.RackConfig.MaxIgnorablePods != nil { - return fmt.Errorf("maxIgnorablePods cannot be specified when rackIDSource is provided") - } } rackMap := map[int]bool{} From 3625bfa91ed835fcfdae17e01c8703f45072edac Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Thu, 22 May 2025 14:02:08 +0530 Subject: [PATCH 5/9] Adding readonly for volume mounts --- api/v1/aerospikecluster_types.go | 4 +- api/v1/zz_generated.deepcopy.go | 5 + internal/controller/cluster/rack.go | 3 +- internal/controller/cluster/statefulset.go | 22 ++-- .../v1/aerospikecluster_mutating_webhook.go | 18 +++- internal/webhook/v1/storage.go | 56 ++++++++-- test/cluster/dynamic_rackid_test.go | 101 ++++++++++++++---- 7 files changed, 166 insertions(+), 43 deletions(-) diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index db97ed69..f8d1c10c 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -703,11 +703,11 @@ type AttachmentOptions struct { MountOptions `json:"mountOptions,omitempty"` } -type MountOptions struct { //nolint:govet // for readability +type MountOptions struct { // Mounted read-only if true, read-write otherwise (false or unspecified). // Defaults to false. // +optional - ReadOnly bool `json:"readOnly,omitempty"` + ReadOnly *bool `json:"readOnly,omitempty"` // Path within the volume from which the container's volume should be mounted. // Defaults to "" (volume's root). diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 3b06f497..1210159a 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -839,6 +839,11 @@ func (in *LoadBalancerSpec) DeepCopy() *LoadBalancerSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MountOptions) DeepCopyInto(out *MountOptions) { *out = *in + if in.ReadOnly != nil { + in, out := &in.ReadOnly, &out.ReadOnly + *out = new(bool) + **out = **in + } if in.MountPropagation != nil { in, out := &in.MountPropagation, &out.MountPropagation *out = new(corev1.MountPropagationMode) diff --git a/internal/controller/cluster/rack.go b/internal/controller/cluster/rack.go index f36a058e..f810ec64 100644 --- a/internal/controller/cluster/rack.go +++ b/internal/controller/cluster/rack.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" @@ -1497,7 +1498,7 @@ func (r *SingleClusterReconciler) isVolumeAttachmentAddedOrUpdated( if volumeMount != nil { // Found, check for updated if getOriginalPath(volumeMount.MountPath) != attachment.Path || - volumeMount.ReadOnly != attachment.ReadOnly || + volumeMount.ReadOnly != ptr.Deref(attachment.ReadOnly, false) || volumeMount.SubPath != attachment.SubPath || volumeMount.SubPathExpr != attachment.SubPathExpr || !reflect.DeepEqual( diff --git a/internal/controller/cluster/statefulset.go b/internal/controller/cluster/statefulset.go index dd04a28e..7b932c68 100644 --- a/internal/controller/cluster/statefulset.go +++ b/internal/controller/cluster/statefulset.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/retry" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -1465,7 +1466,7 @@ func getFinalVolumeAttachmentsForVolume(volume *asdbv1.VolumeSpec) ( ContainerName: asdbv1.AerospikeInitContainerName, Path: initVolumePath, AttachmentOptions: asdbv1.AttachmentOptions{ - MountOptions: asdbv1.MountOptions{ReadOnly: readOnlyVolume}, + MountOptions: asdbv1.MountOptions{ReadOnly: ptr.To(readOnlyVolume)}, }, }, ) @@ -1476,11 +1477,9 @@ func getFinalVolumeAttachmentsForVolume(volume *asdbv1.VolumeSpec) ( if volume.Aerospike != nil { containerAttachments = append( containerAttachments, asdbv1.VolumeAttachment{ - ContainerName: asdbv1.AerospikeServerContainerName, - Path: volume.Aerospike.Path, - AttachmentOptions: asdbv1.AttachmentOptions{ - MountOptions: asdbv1.MountOptions{ReadOnly: readOnlyVolume}, - }, + ContainerName: asdbv1.AerospikeServerContainerName, + Path: volume.Aerospike.Path, + AttachmentOptions: volume.Aerospike.AttachmentOptions, }, ) } @@ -1503,13 +1502,16 @@ func addVolumeMountInContainer( volumeMount = corev1.VolumeMount{ Name: volumeName, MountPath: pathPrefix + volumeAttachment.Path, - ReadOnly: volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, + ReadOnly: ptr.Deref(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, false), } } else { volumeMount = corev1.VolumeMount{ - Name: volumeName, - MountPath: volumeAttachment.Path, - ReadOnly: volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, + Name: volumeName, + MountPath: volumeAttachment.Path, + ReadOnly: ptr.Deref(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, false), + SubPath: volumeAttachment.AttachmentOptions.MountOptions.SubPath, + SubPathExpr: volumeAttachment.AttachmentOptions.MountOptions.SubPathExpr, + MountPropagation: volumeAttachment.AttachmentOptions.MountOptions.MountPropagation, } } diff --git a/internal/webhook/v1/aerospikecluster_mutating_webhook.go b/internal/webhook/v1/aerospikecluster_mutating_webhook.go index 2f96e666..bdcc80b0 100644 --- a/internal/webhook/v1/aerospikecluster_mutating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_mutating_webhook.go @@ -89,7 +89,9 @@ func (acd *AerospikeClusterCustomDefaulter) setDefaults(asLog logr.Logger, clust setNetworkPolicyDefaults(&cluster.Spec.AerospikeNetworkPolicy, cluster.Namespace) // Set common storage defaults. - setStorageDefaults(&cluster.Spec.Storage) + if err := setStorageDefaults(&cluster.Spec.Storage); err != nil { + return err + } // Add default rackConfig if not already given. Disallow use of defaultRackID by user. // Need to set before setting defaults in aerospikeConfig. @@ -201,7 +203,9 @@ func setDefaultRackConf(asLog logr.Logger, rackConfig *asdbv1.RackConfig) error } func updateRacks(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { - updateRacksStorageFromGlobal(asLog, cluster) + if err := updateRacksStorageFromGlobal(asLog, cluster); err != nil { + return fmt.Errorf("error updating rack storage: %v", err) + } if err := updateRacksAerospikeConfigFromGlobal(asLog, cluster); err != nil { return fmt.Errorf("error updating rack aerospike config: %v", err) @@ -212,7 +216,7 @@ func updateRacks(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { return nil } -func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) { +func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { for idx := range cluster.Spec.RackConfig.Racks { rack := &cluster.Spec.RackConfig.Racks[idx] @@ -227,9 +231,13 @@ func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCl rack.Storage = *rack.InputStorage } - // Set storage defaults if rack has storage section - setStorageDefaults(&rack.Storage) + // Set storage defaults if rack has a storage section + if err := setStorageDefaults(&rack.Storage); err != nil { + return err + } } + + return nil } func updateRacksPodSpecFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) { diff --git a/internal/webhook/v1/storage.go b/internal/webhook/v1/storage.go index 609f9841..556b9f0e 100644 --- a/internal/webhook/v1/storage.go +++ b/internal/webhook/v1/storage.go @@ -8,6 +8,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" ) @@ -93,12 +94,26 @@ func validateAddedOrRemovedVolumes(oldStorage, newStorage *asdbv1.AerospikeStora } // setStorageDefaults sets default values for storage spec fields. -func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) { +func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) error { + if storage.CleanupThreads == 0 { + storage.CleanupThreads = asdbv1.AerospikeVolumeSingleCleanupThread + } + + if err := setHostPathVolumeMountDefaults(storage.Volumes); err != nil { + return fmt.Errorf("failed to set host path volume mount defaults: %v", err) + } + + setStoragePolicyDefaults(storage) + + return nil +} + +// setStoragePolicyDefaults sets default values for storage policy fields. +func setStoragePolicyDefaults(storage *asdbv1.AerospikeStorageSpec) { defaultFilesystemInitMethod := asdbv1.AerospikeVolumeMethodNone defaultFilesystemWipeMethod := asdbv1.AerospikeVolumeMethodDeleteFiles defaultBlockInitMethod := asdbv1.AerospikeVolumeMethodNone defaultBlockWipeMethod := asdbv1.AerospikeVolumeMethodDD - defaultCleanupThreads := asdbv1.AerospikeVolumeSingleCleanupThread // Set storage level defaults. setAerospikePersistentVolumePolicyDefaults( &storage.FileSystemVolumePolicy, @@ -114,10 +129,6 @@ func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) { }, ) - if storage.CleanupThreads == 0 { - storage.CleanupThreads = defaultCleanupThreads - } - for idx := range storage.Volumes { switch { case storage.Volumes[idx].Source.PersistentVolume == nil: @@ -134,6 +145,39 @@ func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) { } } +func setHostPathVolumeMountDefaults(volumes []asdbv1.VolumeSpec) error { + for idx := range volumes { + volume := &volumes[idx] + if volume.Source.HostPath != nil { + for idx := range volume.Sidecars { + if volume.Sidecars[idx].ReadOnly != nil && !*volume.Sidecars[idx].ReadOnly { + return fmt.Errorf("hostpath volumes can only be mounted as read only file system") + } + + volume.Sidecars[idx].ReadOnly = ptr.To(true) + } + + for idx := range volume.InitContainers { + if volume.InitContainers[idx].ReadOnly != nil && !*volume.InitContainers[idx].ReadOnly { + return fmt.Errorf("hostpath volumes can only be mounted as read only file system") + } + + volume.InitContainers[idx].ReadOnly = ptr.To(true) + } + + if volume.Aerospike != nil { + if volume.Aerospike.ReadOnly != nil && !*volume.Aerospike.ReadOnly { + return fmt.Errorf("hostpath volumes can only be mounted as read only file system") + } + + volume.Aerospike.ReadOnly = ptr.To(true) + } + } + } + + return nil +} + // setAerospikePersistentVolumePolicyDefaults applies default values to unset fields of the policy using corresponding // fields from defaultPolicy func setAerospikePersistentVolumePolicyDefaults(pvPolicy *asdbv1.AerospikePersistentVolumePolicySpec, diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go index c0fff3bd..f39ec100 100644 --- a/test/cluster/dynamic_rackid_test.go +++ b/test/cluster/dynamic_rackid_test.go @@ -4,25 +4,32 @@ import ( goctx "context" "fmt" "strings" + "time" - asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" - "github.com/aerospike/aerospike-kubernetes-operator/v4/test" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + + asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" + "github.com/aerospike/aerospike-kubernetes-operator/v4/test" ) -var _ = FDescribe( +const aerospikePath = "/opt/hostpath" + +var _ = Describe( "DynamicRack", func() { ctx := goctx.TODO() clusterName := fmt.Sprintf("dynamic-rack-%d", GinkgoParallelProcess()) clusterNamespacedName := test.GetNamespacedName( clusterName, namespace, ) - FContext( + + Context( "When doing valid operations", Ordered, func() { AfterEach( func() { @@ -118,8 +125,9 @@ var _ = FDescribe( "When deploying cluster", func() { It( "Should deploy with dynamic rack", func() { - aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, "/opt/hostpath", "/tmp") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, aerospikePath, "/tmp") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.Namespaces = []string{"test"} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) @@ -132,6 +140,20 @@ var _ = FDescribe( info, err := requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) Expect(err).ToNot(HaveOccurred()) + podObject := &v1.Pod{} + Eventually( + func() bool { + err = k8sClient.Get( + ctx, types.NamespacedName{ + Name: aeroCluster.Name + "-1-0", + Namespace: clusterNamespacedName.Namespace, + }, podObject, + ) + + return isHostPathReadOnly(podObject.Status.ContainerStatuses, aerospikePath) + }, time.Minute, time.Second, + ).Should(BeTrue()) + confs := strings.Split(info["namespace/test"], ";") for _, conf := range confs { if strings.Contains(conf, "rack-id") { @@ -158,7 +180,7 @@ var _ = FDescribe( By("Updating cluster with dynamic rack and add hostpath volume") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} hostPathVolume := asdbv1.VolumeSpec{ Name: "hostpath", Source: asdbv1.VolumeSource{ @@ -167,7 +189,7 @@ var _ = FDescribe( }, }, Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ - Path: "/opt/hostpath", + Path: aerospikePath, }, } @@ -234,7 +256,7 @@ var _ = FDescribe( oldPodIDs, err = getPodIDs(ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} Expect(updateCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) operationTypeMap = map[string]asdbv1.OperationKind{ @@ -272,9 +294,9 @@ var _ = FDescribe( It( "Should fail if multiple racks are given along with dynamic rack", func() { aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - "/opt/hostpath", "/dev/null") + aerospikePath, "/dev/null") aeroCluster.Spec.RackConfig.Racks = append(aeroCluster.Spec.RackConfig.Racks, asdbv1.Rack{ID: 2}) - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) }, @@ -283,8 +305,8 @@ var _ = FDescribe( It( "Should fail if RollingUpdateBatchSize is given along with dynamic rack", func() { aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - "/opt/hostpath", "/dev/null") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aerospikePath, "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.RollingUpdateBatchSize = &intstr.IntOrString{IntVal: 2} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) @@ -294,8 +316,8 @@ var _ = FDescribe( It( "Should fail if ScaleDownBatchSize is given along with dynamic rack", func() { aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - "/opt/hostpath", "/dev/null") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aerospikePath, "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.ScaleDownBatchSize = &intstr.IntOrString{IntVal: 2} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) @@ -305,8 +327,8 @@ var _ = FDescribe( It( "Should fail if MaxIgnorablePods is given along with dynamic rack", func() { aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - "/opt/hostpath", "/dev/null") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aerospikePath, "/dev/null") + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.MaxIgnorablePods = &intstr.IntOrString{IntVal: 2} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) @@ -316,7 +338,7 @@ var _ = FDescribe( It( "Should fail if dynamic rackID source file path is not absolute", func() { aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - "/opt/hostpath", "/dev/null") + aerospikePath, "/dev/null") aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "rackid"} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) @@ -326,7 +348,7 @@ var _ = FDescribe( It( "Should fail if dynamic rackID source volume is not mounted", func() { aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/hostpath/rackid"} + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) }, @@ -351,7 +373,48 @@ var _ = FDescribe( Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) }, ) + + It( + "Should fail if dynamic rackID source volume is mounted with readOnly false", func() { + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) + hostpathVolume := asdbv1.VolumeSpec{ + Name: "hostpath", + Source: asdbv1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{ + Path: "/dev/null", + }, + }, + Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ + Path: aerospikePath, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ + ReadOnly: ptr.To(false), + }, + }, + }, + } + + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, hostpathVolume) + aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} + + Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + }, + ) }, ) }, ) + +func isHostPathReadOnly(containerStatuses []v1.ContainerStatus, mountPath string) bool { + if len(containerStatuses) == 0 { + return false + } + + for idx := range containerStatuses[0].VolumeMounts { + if containerStatuses[0].VolumeMounts[idx].MountPath == mountPath { + return containerStatuses[0].VolumeMounts[idx].ReadOnly + } + } + + return false +} From 360ada98a95f1eb424cc434a3b0981a65ef6db77 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Fri, 23 May 2025 00:12:21 +0530 Subject: [PATCH 6/9] Addressing review comments and trying hash comparison for rackIDSource --- api/v1/aerospikecluster_types.go | 7 +- api/v1/zz_generated.deepcopy.go | 5 - .../asdb.aerospike.com_aerospikeclusters.yaml | 18 +-- ..._aerospikeclusters.asdb.aerospike.com.yaml | 18 +-- internal/controller/cluster/configmap.go | 19 +++ internal/controller/cluster/pod.go | 19 ++- .../v1/aerospikecluster_validating_webhook.go | 4 +- internal/webhook/v1/storage.go | 16 +-- test/cluster/dynamic_rackid_test.go | 130 ++++++++++++------ 9 files changed, 144 insertions(+), 92 deletions(-) diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index f8d1c10c..d0b41797 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -1113,9 +1113,8 @@ type AerospikeNetworkPolicy struct { // RackIDSource specifies the source from which to read the rack ID. type RackIDSource struct { - // FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + // FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. // The file should contain a single integer value. - // +optional FilePath string `json:"filePath,omitempty"` } @@ -1211,9 +1210,9 @@ type AerospikePodStatus struct { //nolint:govet // for readability // +optional DynamicConfigUpdateStatus DynamicConfigUpdateStatus `json:"dynamicConfigUpdateStatus,omitempty"` - // RackIDSource is the source from which the rack ID is read. + // RackIDSourceHash is ripemd160 hash of RackIDSource used by this pod // +optional - RackIDSource *RackIDSource `json:"rackIDSource,omitempty"` + RackIDSourceHash string `json:"rackIDSourceHash"` } // +kubebuilder:object:root=true diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 1210159a..cb078c58 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -651,11 +651,6 @@ func (in *AerospikePodStatus) DeepCopyInto(out *AerospikePodStatus) { *out = make([]string, len(*in)) copy(*out, *in) } - if in.RackIDSource != nil { - in, out := &in.RackIDSource, &out.RackIDSource - *out = new(RackIDSource) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AerospikePodStatus. diff --git a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml index 1e27d810..99e66e0c 100644 --- a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml +++ b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml @@ -5157,7 +5157,7 @@ spec: properties: filePath: description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string type: object @@ -14320,16 +14320,10 @@ spec: description: PodSpecHash is ripemd160 hash of PodSpec used by this pod type: string - rackIDSource: - description: RackIDSource is the source from which the rack - ID is read. - properties: - filePath: - description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. - The file should contain a single integer value. - type: string - type: object + rackIDSourceHash: + description: RackIDSourceHash is ripemd160 hash of RackIDSource + used by this pod + type: string servicePort: description: ServicePort is the port Aerospike clients outside K8s can connect to. @@ -14382,7 +14376,7 @@ spec: properties: filePath: description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string type: object diff --git a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml index 1e27d810..99e66e0c 100644 --- a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml +++ b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml @@ -5157,7 +5157,7 @@ spec: properties: filePath: description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string type: object @@ -14320,16 +14320,10 @@ spec: description: PodSpecHash is ripemd160 hash of PodSpec used by this pod type: string - rackIDSource: - description: RackIDSource is the source from which the rack - ID is read. - properties: - filePath: - description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. - The file should contain a single integer value. - type: string - type: object + rackIDSourceHash: + description: RackIDSourceHash is ripemd160 hash of RackIDSource + used by this pod + type: string servicePort: description: ServicePort is the port Aerospike clients outside K8s can connect to. @@ -14382,7 +14376,7 @@ spec: properties: filePath: description: |- - FilePath specifies the absolute path to a file containing the rack ID mounted in aerospike server. + FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string type: object diff --git a/internal/controller/cluster/configmap.go b/internal/controller/cluster/configmap.go index 93ff8e98..1a527bd5 100644 --- a/internal/controller/cluster/configmap.go +++ b/internal/controller/cluster/configmap.go @@ -39,6 +39,9 @@ const ( // aerospikeConfHashFileName stores the Aerospike config hash aerospikeConfHashFileName = "aerospikeConfHash" + + // rackIDSourceHashFileName stores the rack ID source hash + rackIDSourceHashFileName = "rackIDSourceHash" ) type initializeTemplateInput struct { @@ -158,6 +161,22 @@ func (r *SingleClusterReconciler) createConfigMapData(rack *asdbv1.Rack) ( confData[networkPolicyHashFileName] = policyHash + // Add rackIDSource hash + rackIDSource := r.aeroCluster.Spec.RackConfig.RackIDSource + if rackIDSource != nil { + rackIDSourceStr, rackErr := json.Marshal(rackIDSource) + if rackErr != nil { + return nil, rackErr + } + + rackIDSourceHash, rackErr := utils.GetHash(string(rackIDSourceStr)) + if rackErr != nil { + return nil, rackErr + } + + confData[rackIDSourceHashFileName] = rackIDSourceHash + } + // Add podSpec hash podSpec := createPodSpecForRack(r.aeroCluster, rack) diff --git a/internal/controller/cluster/pod.go b/internal/controller/cluster/pod.go index 3273e4a1..e46818ed 100644 --- a/internal/controller/cluster/pod.go +++ b/internal/controller/cluster/pod.go @@ -137,10 +137,6 @@ func (r *SingleClusterReconciler) getRollingRestartTypeMap(rackState *RackState, if podStatus.DynamicConfigUpdateStatus == asdbv1.PartiallyFailed { restartTypeMap[pods[idx].Name] = mergeRestartType(restartTypeMap[pods[idx].Name], quickRestart) } - - if !reflect.DeepEqual(podStatus.RackIDSource, r.aeroCluster.Spec.RackConfig.RackIDSource) { - restartTypeMap[pods[idx].Name] = mergeRestartType(restartTypeMap[pods[idx].Name], quickRestart) - } } return restartTypeMap, dynamicConfDiffPerPod, nil @@ -161,6 +157,7 @@ func (r *SingleClusterReconciler) getRollingRestartTypePod( requiredConfHash := confMap.Data[aerospikeConfHashFileName] requiredNetworkPolicyHash := confMap.Data[networkPolicyHashFileName] requiredPodSpecHash := confMap.Data[podSpecHashFileName] + requiredRackIDSourceHash := confMap.Data[rackIDSourceHashFileName] podStatus := r.aeroCluster.Status.Pods[pod.Name] @@ -197,6 +194,17 @@ func (r *SingleClusterReconciler) getRollingRestartTypePod( ) } + // Check if rackIDSource is updated + if podStatus.RackIDSourceHash != requiredRackIDSourceHash { + restartType = mergeRestartType(restartType, quickRestart) + + r.Log.Info( + "Aerospike RackIDSource changed. Need rolling restart", + "requiredHash", requiredRackIDSourceHash, + "currentHash", podStatus.RackIDSourceHash, + ) + } + // Check if podSpec is updated if podStatus.PodSpecHash != requiredPodSpecHash { restartType = mergeRestartType(restartType, podRestart) @@ -1397,7 +1405,8 @@ func (r *SingleClusterReconciler) handleDynamicConfigChange(rackState *RackState if len(specToStatusDiffs) > 0 { // rackd-id change is being handled as rack add/remove - // Ignoring it to eliminate rolling restart in case of dynamic rack id allocation. + // Ignoring it to eliminate rolling restart, because webhook sets rack-id to 0 in case of dynamic rack id allocation. + // Due to which rack-id will always be different. for key := range specToStatusDiffs { if asconfig.BaseKey(key) == "rack-id" { delete(specToStatusDiffs, key) diff --git a/internal/webhook/v1/aerospikecluster_validating_webhook.go b/internal/webhook/v1/aerospikecluster_validating_webhook.go index 1c10e6da..6e2bcdb1 100644 --- a/internal/webhook/v1/aerospikecluster_validating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_validating_webhook.go @@ -596,7 +596,7 @@ func validateRackConfig(_ logr.Logger, cluster *asdbv1.AerospikeCluster) error { if rackIDSource != nil { if rackIDSource.FilePath != "" { if !filepath.IsAbs(rackIDSource.FilePath) { - return fmt.Errorf("rackIDSource file path must be absolute") + return fmt.Errorf("rackIDSource file path %s must be absolute", rackIDSource.FilePath) } // Verify the volume exists in storage spec @@ -606,7 +606,7 @@ func validateRackConfig(_ logr.Logger, cluster *asdbv1.AerospikeCluster) error { } if volume.Source.HostPath == nil { - return fmt.Errorf("HostPathVolumeName %s must be a hostpath volume", volume.Name) + return fmt.Errorf("volume %s for file path %s must be a hostpath volume", volume.Name, rackIDSource.FilePath) } } else { return fmt.Errorf("filePath cannot be empty when rackIDSource is specified") diff --git a/internal/webhook/v1/storage.go b/internal/webhook/v1/storage.go index 556b9f0e..070c9551 100644 --- a/internal/webhook/v1/storage.go +++ b/internal/webhook/v1/storage.go @@ -149,20 +149,16 @@ func setHostPathVolumeMountDefaults(volumes []asdbv1.VolumeSpec) error { for idx := range volumes { volume := &volumes[idx] if volume.Source.HostPath != nil { - for idx := range volume.Sidecars { - if volume.Sidecars[idx].ReadOnly != nil && !*volume.Sidecars[idx].ReadOnly { - return fmt.Errorf("hostpath volumes can only be mounted as read only file system") - } - - volume.Sidecars[idx].ReadOnly = ptr.To(true) - } + var attachments []asdbv1.VolumeAttachment + attachments = append(attachments, volume.Sidecars...) + attachments = append(attachments, volume.InitContainers...) - for idx := range volume.InitContainers { - if volume.InitContainers[idx].ReadOnly != nil && !*volume.InitContainers[idx].ReadOnly { + for idx := range attachments { + if attachments[idx].ReadOnly != nil && !*attachments[idx].ReadOnly { return fmt.Errorf("hostpath volumes can only be mounted as read only file system") } - volume.InitContainers[idx].ReadOnly = ptr.To(true) + attachments[idx].ReadOnly = ptr.To(true) } if volume.Aerospike != nil { diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go index f39ec100..c2fbcde4 100644 --- a/test/cluster/dynamic_rackid_test.go +++ b/test/cluster/dynamic_rackid_test.go @@ -6,6 +6,12 @@ import ( "strings" "time" + "github.com/aerospike/aerospike-kubernetes-operator/v4/pkg/utils" + "golang.org/x/net/context" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -105,6 +111,12 @@ var _ = Describe( err := k8sClient.Create(ctx, ds) Expect(err).NotTo(HaveOccurred()) + + err = waitForDaemonSetPodsRunning( + k8sClient, ctx, namespace, "hostpath-writer", retryInterval, + time.Minute*2, + ) + Expect(err).NotTo(HaveOccurred()) }, ) @@ -135,11 +147,6 @@ var _ = Describe( aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) Expect(err).ToNot(HaveOccurred()) - pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] - - info, err := requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) - Expect(err).ToNot(HaveOccurred()) - podObject := &v1.Pod{} Eventually( func() bool { @@ -154,13 +161,10 @@ var _ = Describe( }, time.Minute, time.Second, ).Should(BeTrue()) - confs := strings.Split(info["namespace/test"], ";") - for _, conf := range confs { - if strings.Contains(conf, "rack-id") { - keyValue := strings.Split(conf, "=") - Expect(keyValue[1]).To(Equal("2")) - } - } + pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + + err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") + Expect(err).ToNot(HaveOccurred()) }, ) }, @@ -210,17 +214,9 @@ var _ = Describe( pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] - info, err := requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") Expect(err).ToNot(HaveOccurred()) - confs := strings.Split(info["namespace/test"], ";") - for _, conf := range confs { - if strings.Contains(conf, "rack-id") { - keyValue := strings.Split(conf, "=") - Expect(keyValue[1]).To(Equal("2")) - } - } - By("Updating cluster by disabling dynamic rack") oldPodIDs, err = getPodIDs(ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) @@ -241,17 +237,9 @@ var _ = Describe( pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] - info, err = requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "1") Expect(err).ToNot(HaveOccurred()) - confs = strings.Split(info["namespace/test"], ";") - for _, conf := range confs { - if strings.Contains(conf, "rack-id") { - keyValue := strings.Split(conf, "=") - Expect(keyValue[1]).To(Equal("1")) - } - } - By("Updating cluster by enabling dynamic rack") oldPodIDs, err = getPodIDs(ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) @@ -272,16 +260,8 @@ var _ = Describe( pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] - info, err = requestInfoFromNode(logger, k8sClient, ctx, clusterNamespacedName, "namespace/test", &pod) + err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") Expect(err).ToNot(HaveOccurred()) - - confs = strings.Split(info["namespace/test"], ";") - for _, conf := range confs { - if strings.Contains(conf, "rack-id") { - keyValue := strings.Split(conf, "=") - Expect(keyValue[1]).To(Equal("2")) - } - } }, ) }, @@ -357,7 +337,7 @@ var _ = Describe( It( "Should fail if dynamic rackID source volume is not hostpath type", func() { aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) - emptydirVolume := asdbv1.VolumeSpec{ + emptyDirVolume := asdbv1.VolumeSpec{ Name: "empty", Source: asdbv1.VolumeSource{ EmptyDir: &v1.EmptyDirVolumeSource{}, @@ -367,7 +347,7 @@ var _ = Describe( }, } - aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, emptydirVolume) + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, emptyDirVolume) aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/empty/rackid"} Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) @@ -375,7 +355,7 @@ var _ = Describe( ) It( - "Should fail if dynamic rackID source volume is mounted with readOnly false", func() { + "Should fail if dynamic rackID source volume is mounted with readOnly false", func() { aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) hostpathVolume := asdbv1.VolumeSpec{ Name: "hostpath", @@ -418,3 +398,69 @@ func isHostPathReadOnly(containerStatuses []v1.ContainerStatus, mountPath string return false } + +func waitForDaemonSetPodsRunning(k8sClient client.Client, ctx goctx.Context, namespace, dsName string, + retryInterval, timeout time.Duration) error { + return wait.PollUntilContextTimeout(ctx, + retryInterval, timeout, true, func(ctx goctx.Context) (done bool, err error) { + ds := &appsv1.DaemonSet{} + if err := k8sClient.Get( + ctx, types.NamespacedName{ + Name: dsName, Namespace: namespace, + }, ds, + ); err != nil { + return false, err + } + + // DaemonSet selector + selector := labels.Set(ds.Spec.Selector.MatchLabels).AsSelector() + + podList := &v1.PodList{} + listOps := &client.ListOptions{ + Namespace: namespace, LabelSelector: selector, + } + + if err := k8sClient.List(goctx.TODO(), podList, listOps); err != nil { + return false, err + } + + var runningAndReady int32 + + for idx := range podList.Items { + if utils.IsPodRunningAndReady(&podList.Items[idx]) { + runningAndReady++ + } + } + + if runningAndReady == ds.Status.DesiredNumberScheduled { + return true, nil + } + + return false, nil + }, + ) +} + +func validateDynamicRackID( + ctx context.Context, k8sClient client.Client, pod *asdbv1.AerospikePodStatus, namespacedName types.NamespacedName, + expectedValue string, +) error { + info, err := requestInfoFromNode(logger, k8sClient, ctx, namespacedName, "namespace/test", pod) + if err != nil { + return err + } + + confs := strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + if keyValue[1] != expectedValue { + return fmt.Errorf("expected rack-id %s, but got %s", expectedValue, keyValue[1]) + } + + return nil + } + } + + return nil +} From a8f330b6cff6733ddc8c1d127e1794b9fef829e0 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Tue, 27 May 2025 22:05:30 +0530 Subject: [PATCH 7/9] Addressing comments --- internal/controller/cluster/configmap.go | 20 ++--- internal/controller/cluster/rack.go | 3 +- internal/controller/cluster/statefulset.go | 12 +-- .../v1/aerospikecluster_mutating_webhook.go | 18 ++--- .../v1/aerospikecluster_validating_webhook.go | 5 ++ internal/webhook/v1/storage.go | 61 ++++++--------- test/cluster/batch_scaledown_pods_test.go | 24 ++++++ test/cluster/cluster_helper.go | 27 +++++++ test/cluster/dynamic_rackid_test.go | 77 ++++++++++--------- test/cluster/storage_test.go | 34 ++++++++ 10 files changed, 177 insertions(+), 104 deletions(-) diff --git a/internal/controller/cluster/configmap.go b/internal/controller/cluster/configmap.go index 1a527bd5..1a358642 100644 --- a/internal/controller/cluster/configmap.go +++ b/internal/controller/cluster/configmap.go @@ -162,21 +162,23 @@ func (r *SingleClusterReconciler) createConfigMapData(rack *asdbv1.Rack) ( confData[networkPolicyHashFileName] = policyHash // Add rackIDSource hash + rackIDSourceStr := []byte{} + rackIDSource := r.aeroCluster.Spec.RackConfig.RackIDSource if rackIDSource != nil { - rackIDSourceStr, rackErr := json.Marshal(rackIDSource) - if rackErr != nil { - return nil, rackErr - } - - rackIDSourceHash, rackErr := utils.GetHash(string(rackIDSourceStr)) - if rackErr != nil { - return nil, rackErr + rackIDSourceStr, err = json.Marshal(*rackIDSource) + if err != nil { + return nil, err } + } - confData[rackIDSourceHashFileName] = rackIDSourceHash + rackIDSourceHash, err := utils.GetHash(string(rackIDSourceStr)) + if err != nil { + return nil, err } + confData[rackIDSourceHashFileName] = rackIDSourceHash + // Add podSpec hash podSpec := createPodSpecForRack(r.aeroCluster, rack) diff --git a/internal/controller/cluster/rack.go b/internal/controller/cluster/rack.go index f810ec64..43c10634 100644 --- a/internal/controller/cluster/rack.go +++ b/internal/controller/cluster/rack.go @@ -13,7 +13,6 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" @@ -1498,7 +1497,7 @@ func (r *SingleClusterReconciler) isVolumeAttachmentAddedOrUpdated( if volumeMount != nil { // Found, check for updated if getOriginalPath(volumeMount.MountPath) != attachment.Path || - volumeMount.ReadOnly != ptr.Deref(attachment.ReadOnly, false) || + volumeMount.ReadOnly != asdbv1.GetBool(attachment.ReadOnly) || volumeMount.SubPath != attachment.SubPath || volumeMount.SubPathExpr != attachment.SubPathExpr || !reflect.DeepEqual( diff --git a/internal/controller/cluster/statefulset.go b/internal/controller/cluster/statefulset.go index 7b932c68..721ab9f2 100644 --- a/internal/controller/cluster/statefulset.go +++ b/internal/controller/cluster/statefulset.go @@ -1477,9 +1477,11 @@ func getFinalVolumeAttachmentsForVolume(volume *asdbv1.VolumeSpec) ( if volume.Aerospike != nil { containerAttachments = append( containerAttachments, asdbv1.VolumeAttachment{ - ContainerName: asdbv1.AerospikeServerContainerName, - Path: volume.Aerospike.Path, - AttachmentOptions: volume.Aerospike.AttachmentOptions, + ContainerName: asdbv1.AerospikeServerContainerName, + Path: volume.Aerospike.Path, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ReadOnly: ptr.To(readOnlyVolume)}, + }, }, ) } @@ -1502,13 +1504,13 @@ func addVolumeMountInContainer( volumeMount = corev1.VolumeMount{ Name: volumeName, MountPath: pathPrefix + volumeAttachment.Path, - ReadOnly: ptr.Deref(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, false), + ReadOnly: asdbv1.GetBool(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly), } } else { volumeMount = corev1.VolumeMount{ Name: volumeName, MountPath: volumeAttachment.Path, - ReadOnly: ptr.Deref(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly, false), + ReadOnly: asdbv1.GetBool(volumeAttachment.AttachmentOptions.MountOptions.ReadOnly), SubPath: volumeAttachment.AttachmentOptions.MountOptions.SubPath, SubPathExpr: volumeAttachment.AttachmentOptions.MountOptions.SubPathExpr, MountPropagation: volumeAttachment.AttachmentOptions.MountOptions.MountPropagation, diff --git a/internal/webhook/v1/aerospikecluster_mutating_webhook.go b/internal/webhook/v1/aerospikecluster_mutating_webhook.go index bdcc80b0..2f96e666 100644 --- a/internal/webhook/v1/aerospikecluster_mutating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_mutating_webhook.go @@ -89,9 +89,7 @@ func (acd *AerospikeClusterCustomDefaulter) setDefaults(asLog logr.Logger, clust setNetworkPolicyDefaults(&cluster.Spec.AerospikeNetworkPolicy, cluster.Namespace) // Set common storage defaults. - if err := setStorageDefaults(&cluster.Spec.Storage); err != nil { - return err - } + setStorageDefaults(&cluster.Spec.Storage) // Add default rackConfig if not already given. Disallow use of defaultRackID by user. // Need to set before setting defaults in aerospikeConfig. @@ -203,9 +201,7 @@ func setDefaultRackConf(asLog logr.Logger, rackConfig *asdbv1.RackConfig) error } func updateRacks(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { - if err := updateRacksStorageFromGlobal(asLog, cluster); err != nil { - return fmt.Errorf("error updating rack storage: %v", err) - } + updateRacksStorageFromGlobal(asLog, cluster) if err := updateRacksAerospikeConfigFromGlobal(asLog, cluster); err != nil { return fmt.Errorf("error updating rack aerospike config: %v", err) @@ -216,7 +212,7 @@ func updateRacks(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { return nil } -func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) error { +func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) { for idx := range cluster.Spec.RackConfig.Racks { rack := &cluster.Spec.RackConfig.Racks[idx] @@ -231,13 +227,9 @@ func updateRacksStorageFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCl rack.Storage = *rack.InputStorage } - // Set storage defaults if rack has a storage section - if err := setStorageDefaults(&rack.Storage); err != nil { - return err - } + // Set storage defaults if rack has storage section + setStorageDefaults(&rack.Storage) } - - return nil } func updateRacksPodSpecFromGlobal(asLog logr.Logger, cluster *asdbv1.AerospikeCluster) { diff --git a/internal/webhook/v1/aerospikecluster_validating_webhook.go b/internal/webhook/v1/aerospikecluster_validating_webhook.go index 6e2bcdb1..4d89f181 100644 --- a/internal/webhook/v1/aerospikecluster_validating_webhook.go +++ b/internal/webhook/v1/aerospikecluster_validating_webhook.go @@ -616,6 +616,11 @@ func validateRackConfig(_ logr.Logger, cluster *asdbv1.AerospikeCluster) error { if len(cluster.Spec.RackConfig.Racks) > 1 { return fmt.Errorf("cannot specify more than 1 rack when rackIDSource is provided") } + + // Cannot specify MaxIgnorablePods when rackIDSource is provided + if cluster.Spec.RackConfig.MaxIgnorablePods != nil { + return fmt.Errorf("cannot specify MaxIgnorablePods when rackIDSource is provided") + } } rackMap := map[int]bool{} diff --git a/internal/webhook/v1/storage.go b/internal/webhook/v1/storage.go index 070c9551..27adbc88 100644 --- a/internal/webhook/v1/storage.go +++ b/internal/webhook/v1/storage.go @@ -5,12 +5,10 @@ import ( "path/filepath" "reflect" + asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" - "k8s.io/utils/ptr" - - asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" ) // validateStorageSpecChange indicates if a change to storage spec is safe to apply. @@ -94,26 +92,12 @@ func validateAddedOrRemovedVolumes(oldStorage, newStorage *asdbv1.AerospikeStora } // setStorageDefaults sets default values for storage spec fields. -func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) error { - if storage.CleanupThreads == 0 { - storage.CleanupThreads = asdbv1.AerospikeVolumeSingleCleanupThread - } - - if err := setHostPathVolumeMountDefaults(storage.Volumes); err != nil { - return fmt.Errorf("failed to set host path volume mount defaults: %v", err) - } - - setStoragePolicyDefaults(storage) - - return nil -} - -// setStoragePolicyDefaults sets default values for storage policy fields. -func setStoragePolicyDefaults(storage *asdbv1.AerospikeStorageSpec) { +func setStorageDefaults(storage *asdbv1.AerospikeStorageSpec) { defaultFilesystemInitMethod := asdbv1.AerospikeVolumeMethodNone defaultFilesystemWipeMethod := asdbv1.AerospikeVolumeMethodDeleteFiles defaultBlockInitMethod := asdbv1.AerospikeVolumeMethodNone defaultBlockWipeMethod := asdbv1.AerospikeVolumeMethodDD + defaultCleanupThreads := asdbv1.AerospikeVolumeSingleCleanupThread // Set storage level defaults. setAerospikePersistentVolumePolicyDefaults( &storage.FileSystemVolumePolicy, @@ -129,6 +113,10 @@ func setStoragePolicyDefaults(storage *asdbv1.AerospikeStorageSpec) { }, ) + if storage.CleanupThreads == 0 { + storage.CleanupThreads = defaultCleanupThreads + } + for idx := range storage.Volumes { switch { case storage.Volumes[idx].Source.PersistentVolume == nil: @@ -145,28 +133,21 @@ func setStoragePolicyDefaults(storage *asdbv1.AerospikeStorageSpec) { } } -func setHostPathVolumeMountDefaults(volumes []asdbv1.VolumeSpec) error { - for idx := range volumes { - volume := &volumes[idx] - if volume.Source.HostPath != nil { - var attachments []asdbv1.VolumeAttachment - attachments = append(attachments, volume.Sidecars...) - attachments = append(attachments, volume.InitContainers...) - - for idx := range attachments { - if attachments[idx].ReadOnly != nil && !*attachments[idx].ReadOnly { - return fmt.Errorf("hostpath volumes can only be mounted as read only file system") - } +func validateHostPathVolumeReadOnly(volume *asdbv1.VolumeSpec) error { + if volume.Source.HostPath != nil { + var attachments []asdbv1.VolumeAttachment + attachments = append(attachments, volume.Sidecars...) + attachments = append(attachments, volume.InitContainers...) - attachments[idx].ReadOnly = ptr.To(true) + for idx := range attachments { + if attachments[idx].ReadOnly == nil || !*attachments[idx].ReadOnly { + return fmt.Errorf("hostpath volumes can only be mounted as read only file system") } + } - if volume.Aerospike != nil { - if volume.Aerospike.ReadOnly != nil && !*volume.Aerospike.ReadOnly { - return fmt.Errorf("hostpath volumes can only be mounted as read only file system") - } - - volume.Aerospike.ReadOnly = ptr.To(true) + if volume.Aerospike != nil { + if volume.Aerospike.ReadOnly == nil || !*volume.Aerospike.ReadOnly { + return fmt.Errorf("hostpath volumes can only be mounted as read only file system") } } } @@ -345,6 +326,10 @@ func validateStorage( ); err != nil { return err } + + if err := validateHostPathVolumeReadOnly(volume); err != nil { + return err + } } return nil diff --git a/test/cluster/batch_scaledown_pods_test.go b/test/cluster/batch_scaledown_pods_test.go index 5b6ace3a..cd2691ed 100644 --- a/test/cluster/batch_scaledown_pods_test.go +++ b/test/cluster/batch_scaledown_pods_test.go @@ -112,6 +112,30 @@ var _ = Describe("BatchScaleDown", func() { err := batchScaleDownTest(k8sClient, ctx, clusterNamespacedName, count(3), 2) Expect(err).Should(HaveOccurred()) }) + + It("Should fail if number of racks is less than 2 and ScaleDownBatchSize "+ + "PCT or ScaleDownBatchSize Count is given", func() { + // During deployment + // During update. User should not be allowed to remove rack if above condition is met. + By("Using ScaleDownBatchSize PCT") + aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + racks := getDummyRackConf(1) + aeroCluster.Spec.RackConfig.Racks = racks + aeroCluster.Spec.RackConfig.ScaleDownBatchSize = percent("100%") + err = updateCluster(k8sClient, ctx, aeroCluster) + Expect(err).To(HaveOccurred()) + + By("Using ScaleDownBatchSize Count") + aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) + Expect(err).ToNot(HaveOccurred()) + + aeroCluster.Spec.RackConfig.Racks = racks + aeroCluster.Spec.RackConfig.ScaleDownBatchSize = count(1) + err = updateCluster(k8sClient, ctx, aeroCluster) + Expect(err).To(HaveOccurred()) + }) }) }) diff --git a/test/cluster/cluster_helper.go b/test/cluster/cluster_helper.go index abb778ca..c9b49e9a 100644 --- a/test/cluster/cluster_helper.go +++ b/test/cluster/cluster_helper.go @@ -1094,6 +1094,11 @@ func createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName types.N }, Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ Path: aerospikePath, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ + ReadOnly: ptr.To(true), + }, + }, }, } @@ -1521,6 +1526,28 @@ func getStorageVolumeForSecret() asdbv1.VolumeSpec { } } +func getStorageVolumeForSidecar(volumeName, path, containerName string, readOnly bool) asdbv1.VolumeSpec { + return asdbv1.VolumeSpec{ + Name: volumeName, + Source: asdbv1.VolumeSource{ + HostPath: &corev1.HostPathVolumeSource{ + Path: "/dev/null", + }, + }, + Sidecars: []asdbv1.VolumeAttachment{ + { + Path: path, + ContainerName: containerName, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ + ReadOnly: ptr.To(readOnly), + }, + }, + }, + }, + } +} + func getSCNamespaceConfig(name, path string) map[string]interface{} { return map[string]interface{}{ "name": name, diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go index c2fbcde4..c203a58c 100644 --- a/test/cluster/dynamic_rackid_test.go +++ b/test/cluster/dynamic_rackid_test.go @@ -6,28 +6,27 @@ import ( "strings" "time" - "github.com/aerospike/aerospike-kubernetes-operator/v4/pkg/utils" - "golang.org/x/net/context" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/wait" - "sigs.k8s.io/controller-runtime/pkg/client" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "golang.org/x/net/context" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" + "github.com/aerospike/aerospike-kubernetes-operator/v4/pkg/utils" "github.com/aerospike/aerospike-kubernetes-operator/v4/test" ) const aerospikePath = "/opt/hostpath" -var _ = Describe( +var _ = FDescribe( "DynamicRack", func() { ctx := goctx.TODO() clusterName := fmt.Sprintf("dynamic-rack-%d", GinkgoParallelProcess()) @@ -75,7 +74,7 @@ var _ = Describe( { Name: "writer", Image: "busybox", - Command: []string{"sh", "-c", `echo "2" > /tmp/rackid && sleep 3600`}, + Command: []string{"sh", "-c", `echo "2" > /tmp/rackid`}, VolumeMounts: []v1.VolumeMount{ { Name: "tmp-mount", @@ -86,7 +85,7 @@ var _ = Describe( PreStop: &v1.LifecycleHandler{ Exec: &v1.ExecAction{ Command: []string{ - "/bin/sh", "-c", "rm /tmp/rackid; sleep 5", + "/bin/sh", "-c", "rm -r /tmp/rackid; sleep 5", }, }, }, @@ -194,6 +193,11 @@ var _ = Describe( }, Aerospike: &asdbv1.AerospikeServerVolumeAttachment{ Path: aerospikePath, + AttachmentOptions: asdbv1.AttachmentOptions{ + MountOptions: asdbv1.MountOptions{ + ReadOnly: ptr.To(true), + }, + }, }, } @@ -278,29 +282,10 @@ var _ = Describe( aeroCluster.Spec.RackConfig.Racks = append(aeroCluster.Spec.RackConfig.Racks, asdbv1.Rack{ID: 2}) aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) - }, - ) - - It( - "Should fail if RollingUpdateBatchSize is given along with dynamic rack", func() { - aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - aerospikePath, "/dev/null") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} - aeroCluster.Spec.RackConfig.RollingUpdateBatchSize = &intstr.IntOrString{IntVal: 2} - - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) - }, - ) - - It( - "Should fail if ScaleDownBatchSize is given along with dynamic rack", func() { - aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, - aerospikePath, "/dev/null") - aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} - aeroCluster.Spec.RackConfig.ScaleDownBatchSize = &intstr.IntOrString{IntVal: 2} - - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To( + ContainSubstring("cannot specify more than 1 rack when rackIDSource is provided")) }, ) @@ -311,7 +296,10 @@ var _ = Describe( aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.MaxIgnorablePods = &intstr.IntOrString{IntVal: 2} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To( + ContainSubstring("cannot specify MaxIgnorablePods when rackIDSource is provided")) }, ) @@ -321,7 +309,11 @@ var _ = Describe( aerospikePath, "/dev/null") aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "rackid"} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + + Expect(err.Error()).To( + ContainSubstring("must be absolute")) }, ) @@ -330,7 +322,11 @@ var _ = Describe( aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + + Expect(err.Error()).To( + ContainSubstring("volume not found in storage spec")) }, ) @@ -350,7 +346,11 @@ var _ = Describe( aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, emptyDirVolume) aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: "/opt/empty/rackid"} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + + Expect(err.Error()).To( + ContainSubstring("must be a hostpath volume")) }, ) @@ -377,7 +377,10 @@ var _ = Describe( aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, hostpathVolume) aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} - Expect(DeployCluster(k8sClient, ctx, aeroCluster)).Should(HaveOccurred()) + err := DeployCluster(k8sClient, ctx, aeroCluster) + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To( + ContainSubstring("hostpath volumes can only be mounted as read only file system")) }, ) }, diff --git a/test/cluster/storage_test.go b/test/cluster/storage_test.go index 43255c2c..ec523492 100644 --- a/test/cluster/storage_test.go +++ b/test/cluster/storage_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "golang.org/x/net/context" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -573,9 +574,19 @@ var _ = Describe( aeroCluster.Spec.Storage.Volumes[0].Sidecars, va, ) + + aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, + getStorageVolumeForSidecar("sidecar-volume", + "/para/tomcat1", containerName, true)) + err = updateCluster(k8sClient, ctx, aeroCluster) Expect(err).ToNot(HaveOccurred()) + readOnly, err := isVolumeMountReadOnly(test.GetNamespacedName(aeroCluster.Name+"-0-1", aeroCluster.Namespace), + containerName, "sidecar-volume") + Expect(err).ToNot(HaveOccurred()) + Expect(readOnly).To(BeTrue()) + // Update aeroCluster.Spec.Storage.Volumes[0].Sidecars[0].Path = "/newpath2" @@ -644,3 +655,26 @@ func createDummyAerospikeClusterWithNonPVWorkdir( return aeroCluster } + +// isVolumeMountReadOnly checks if the given volume name is mounted as read-only in the specified container. +func isVolumeMountReadOnly(podNamespacedName types.NamespacedName, containerName, volumeName string) (bool, error) { + updatedPod := &v1.Pod{} + + if err := k8sClient.Get(context.TODO(), podNamespacedName, updatedPod); err != nil { + return false, err + } + + for idx := range updatedPod.Spec.Containers { + if updatedPod.Spec.Containers[idx].Name == containerName { + for _, vm := range updatedPod.Spec.Containers[idx].VolumeMounts { + if vm.Name == volumeName { + return vm.ReadOnly, nil + } + } + + return false, fmt.Errorf("volume %s not found in container mounts", volumeName) + } + } + + return false, fmt.Errorf("container not found %s", containerName) +} From 0708dc2a7110a67b10d1bbd0c4629ac5e5039d58 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Fri, 30 May 2025 14:10:01 +0530 Subject: [PATCH 8/9] Addressing comments --- api/v1/aerospikecluster_types.go | 2 +- .../asdb.aerospike.com_aerospikeclusters.yaml | 4 + ..._aerospikeclusters.asdb.aerospike.com.yaml | 4 + internal/webhook/v1/storage.go | 7 +- test/cluster/batch_scaledown_pods_test.go | 24 -- test/cluster/cluster_helper.go | 2 +- test/cluster/dynamic_rackid_test.go | 246 +++++++++++------- test/cluster/network_policy_test.go | 61 +---- test/cluster/storage_test.go | 2 +- test/cluster/utils.go | 47 ++++ 10 files changed, 226 insertions(+), 173 deletions(-) diff --git a/api/v1/aerospikecluster_types.go b/api/v1/aerospikecluster_types.go index d0b41797..3cff707a 100644 --- a/api/v1/aerospikecluster_types.go +++ b/api/v1/aerospikecluster_types.go @@ -1115,7 +1115,7 @@ type AerospikeNetworkPolicy struct { type RackIDSource struct { // FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. // The file should contain a single integer value. - FilePath string `json:"filePath,omitempty"` + FilePath string `json:"filePath"` } // AerospikeInstanceSummary defines the observed state of a pod's Aerospike Server Instance. diff --git a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml index 99e66e0c..c1df615b 100644 --- a/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml +++ b/config/crd/bases/asdb.aerospike.com_aerospikeclusters.yaml @@ -5160,6 +5160,8 @@ spec: FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string + required: + - filePath type: object racks: description: Racks is the list of all racks @@ -14379,6 +14381,8 @@ spec: FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string + required: + - filePath type: object racks: description: Racks is the list of all racks diff --git a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml index 99e66e0c..c1df615b 100644 --- a/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml +++ b/helm-charts/aerospike-kubernetes-operator/crds/customresourcedefinition_aerospikeclusters.asdb.aerospike.com.yaml @@ -5160,6 +5160,8 @@ spec: FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string + required: + - filePath type: object racks: description: Racks is the list of all racks @@ -14379,6 +14381,8 @@ spec: FilePath specifies an absolute path to a file containing the rack ID mounted in the aerospike server container. The file should contain a single integer value. type: string + required: + - filePath type: object racks: description: Racks is the list of all racks diff --git a/internal/webhook/v1/storage.go b/internal/webhook/v1/storage.go index 27adbc88..9139bfea 100644 --- a/internal/webhook/v1/storage.go +++ b/internal/webhook/v1/storage.go @@ -5,10 +5,11 @@ import ( "path/filepath" "reflect" - asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" + + asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" ) // validateStorageSpecChange indicates if a change to storage spec is safe to apply. @@ -141,13 +142,13 @@ func validateHostPathVolumeReadOnly(volume *asdbv1.VolumeSpec) error { for idx := range attachments { if attachments[idx].ReadOnly == nil || !*attachments[idx].ReadOnly { - return fmt.Errorf("hostpath volumes can only be mounted as read only file system") + return fmt.Errorf("hostpath volumes can only be mounted as read-only filesystem") } } if volume.Aerospike != nil { if volume.Aerospike.ReadOnly == nil || !*volume.Aerospike.ReadOnly { - return fmt.Errorf("hostpath volumes can only be mounted as read only file system") + return fmt.Errorf("hostpath volumes can only be mounted as read-only filesystem") } } } diff --git a/test/cluster/batch_scaledown_pods_test.go b/test/cluster/batch_scaledown_pods_test.go index cd2691ed..5b6ace3a 100644 --- a/test/cluster/batch_scaledown_pods_test.go +++ b/test/cluster/batch_scaledown_pods_test.go @@ -112,30 +112,6 @@ var _ = Describe("BatchScaleDown", func() { err := batchScaleDownTest(k8sClient, ctx, clusterNamespacedName, count(3), 2) Expect(err).Should(HaveOccurred()) }) - - It("Should fail if number of racks is less than 2 and ScaleDownBatchSize "+ - "PCT or ScaleDownBatchSize Count is given", func() { - // During deployment - // During update. User should not be allowed to remove rack if above condition is met. - By("Using ScaleDownBatchSize PCT") - aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) - Expect(err).ToNot(HaveOccurred()) - - racks := getDummyRackConf(1) - aeroCluster.Spec.RackConfig.Racks = racks - aeroCluster.Spec.RackConfig.ScaleDownBatchSize = percent("100%") - err = updateCluster(k8sClient, ctx, aeroCluster) - Expect(err).To(HaveOccurred()) - - By("Using ScaleDownBatchSize Count") - aeroCluster, err = getCluster(k8sClient, ctx, clusterNamespacedName) - Expect(err).ToNot(HaveOccurred()) - - aeroCluster.Spec.RackConfig.Racks = racks - aeroCluster.Spec.RackConfig.ScaleDownBatchSize = count(1) - err = updateCluster(k8sClient, ctx, aeroCluster) - Expect(err).To(HaveOccurred()) - }) }) }) diff --git a/test/cluster/cluster_helper.go b/test/cluster/cluster_helper.go index c9b49e9a..1cc63efe 100644 --- a/test/cluster/cluster_helper.go +++ b/test/cluster/cluster_helper.go @@ -1526,7 +1526,7 @@ func getStorageVolumeForSecret() asdbv1.VolumeSpec { } } -func getStorageVolumeForSidecar(volumeName, path, containerName string, readOnly bool) asdbv1.VolumeSpec { +func getHostPathStorageVolumeForSidecar(volumeName, path, containerName string, readOnly bool) asdbv1.VolumeSpec { return asdbv1.VolumeSpec{ Name: volumeName, Source: asdbv1.VolumeSource{ diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go index c203a58c..3ab3cd2a 100644 --- a/test/cluster/dynamic_rackid_test.go +++ b/test/cluster/dynamic_rackid_test.go @@ -3,6 +3,7 @@ package cluster import ( goctx "context" "fmt" + "reflect" "strings" "time" @@ -36,6 +37,22 @@ var _ = FDescribe( Context( "When doing valid operations", Ordered, func() { + + var ( + firstHalfNodeList *v1.NodeList + secondHalfNodeList *v1.NodeList + nodeList *v1.NodeList + err error + ) + + const ( + rackLabelKey = "rack" + rack1LabelValue = "1" + rack2LabelValue = "2" + ds1Name = "hostpath-writer-rack1" + ds2Name = "hostpath-writer-rack2" + ) + AfterEach( func() { aeroCluster := &asdbv1.AerospikeCluster{ @@ -53,66 +70,106 @@ var _ = FDescribe( BeforeAll( func() { dir := v1.HostPathDirectory - ds := &appsv1.DaemonSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hostpath-writer", - Namespace: namespace, - Labels: map[string]string{ - "app": "hostpath-writer", - }, - }, - Spec: appsv1.DaemonSetSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"app": "hostpath-writer"}, + // Define a helper function to create a DaemonSet with custom rackid and optional node selector + createDaemonSet := func(name, labelKey, labelValue string) *appsv1.DaemonSet { + ds := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{ + "app": name, + }, }, - Template: v1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"app": "hostpath-writer"}, // must match selector + Spec: appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": name}, }, - Spec: v1.PodSpec{ - Containers: []v1.Container{ - { - Name: "writer", - Image: "busybox", - Command: []string{"sh", "-c", `echo "2" > /tmp/rackid`}, - VolumeMounts: []v1.VolumeMount{ - { - Name: "tmp-mount", - MountPath: "/tmp", + Template: v1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": name}, + }, + Spec: v1.PodSpec{ + NodeSelector: map[string]string{labelKey: labelValue}, + Containers: []v1.Container{ + { + Name: "writer", + Image: "busybox", + Command: []string{"sh", "-c", + fmt.Sprintf(`rm -rf /tmp/rackid && echo "%q" > /tmp/rackid; sleep 3600`, labelValue)}, + VolumeMounts: []v1.VolumeMount{ + { + Name: "tmp-mount", + MountPath: "/tmp", + }, }, - }, - Lifecycle: &v1.Lifecycle{ - PreStop: &v1.LifecycleHandler{ - Exec: &v1.ExecAction{ - Command: []string{ - "/bin/sh", "-c", "rm -r /tmp/rackid; sleep 5", + Lifecycle: &v1.Lifecycle{ + PreStop: &v1.LifecycleHandler{ + Exec: &v1.ExecAction{ + Command: []string{ + "/bin/sh", "-c", "rm -rf /tmp/rackid; sleep 5", + }, }, }, }, }, }, - }, - Volumes: []v1.Volume{ - { - Name: "tmp-mount", - VolumeSource: v1.VolumeSource{ - HostPath: &v1.HostPathVolumeSource{ - Path: "/tmp", - Type: &dir, + Volumes: []v1.Volume{ + { + Name: "tmp-mount", + VolumeSource: v1.VolumeSource{ + HostPath: &v1.HostPathVolumeSource{ + Path: "/tmp", + Type: &dir, + }, }, }, }, }, }, }, - }, + } + + return ds } - err := k8sClient.Create(ctx, ds) + nodeList, err = getNodeList(ctx, k8sClient) Expect(err).NotTo(HaveOccurred()) + Expect(nodeList.Items).NotTo(BeEmpty(), "No nodes found in the cluster") + + n := len(nodeList.Items) + + // Calculate the split point + half := n / 2 + if n%2 != 0 { + half++ // Put the extra node in the first half if odd + } + + firstHalfNodeList = &v1.NodeList{ + Items: append([]v1.Node{}, nodeList.Items[:half]...), + } + secondHalfNodeList = &v1.NodeList{ + Items: append([]v1.Node{}, nodeList.Items[half:]...), + } + + err = setNodeLabels(ctx, k8sClient, firstHalfNodeList, map[string]string{rackLabelKey: rack1LabelValue}) + Expect(err).NotTo(HaveOccurred()) + err = setNodeLabels(ctx, k8sClient, secondHalfNodeList, map[string]string{rackLabelKey: rack2LabelValue}) + Expect(err).NotTo(HaveOccurred()) + + ds1 := createDaemonSet(ds1Name, rackLabelKey, rack1LabelValue) + Expect(k8sClient.Create(ctx, ds1)).To(Succeed()) + + ds2 := createDaemonSet(ds2Name, rackLabelKey, rack2LabelValue) + Expect(k8sClient.Create(ctx, ds2)).To(Succeed()) err = waitForDaemonSetPodsRunning( - k8sClient, ctx, namespace, "hostpath-writer", retryInterval, + k8sClient, ctx, namespace, ds1Name, retryInterval, + time.Minute*2, + ) + Expect(err).NotTo(HaveOccurred()) + + err = waitForDaemonSetPodsRunning( + k8sClient, ctx, namespace, ds2Name, retryInterval, time.Minute*2, ) Expect(err).NotTo(HaveOccurred()) @@ -123,13 +180,20 @@ var _ = FDescribe( func() { ds := &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ - Name: "hostpath-writer", + Name: ds1Name, Namespace: namespace, }, } err := k8sClient.Delete(ctx, ds) Expect(err).NotTo(HaveOccurred()) + + ds.Name = ds2Name + err = k8sClient.Delete(ctx, ds) + Expect(err).NotTo(HaveOccurred()) + + err = deleteLabelsAllNodes(ctx, []string{rackLabelKey}) + Expect(err).NotTo(HaveOccurred()) }) Context( @@ -137,32 +201,22 @@ var _ = FDescribe( It( "Should deploy with dynamic rack", func() { - aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, 2, aerospikePath, "/tmp") + aeroCluster := createDummyAerospikeClusterWithHostPathVolume(clusterNamespacedName, int32(len(nodeList.Items)), + aerospikePath, "/tmp") aeroCluster.Spec.RackConfig.RackIDSource = &asdbv1.RackIDSource{FilePath: aerospikePath + "/rackid"} aeroCluster.Spec.RackConfig.Namespaces = []string{"test"} - + aeroCluster.Spec.PodSpec.MultiPodPerHost = ptr.To(false) Expect(DeployCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) aeroCluster, err := getCluster(k8sClient, ctx, clusterNamespacedName) Expect(err).ToNot(HaveOccurred()) - podObject := &v1.Pod{} - Eventually( - func() bool { - err = k8sClient.Get( - ctx, types.NamespacedName{ - Name: aeroCluster.Name + "-1-0", - Namespace: clusterNamespacedName.Namespace, - }, podObject, - ) - - return isHostPathReadOnly(podObject.Status.ContainerStatuses, aerospikePath) - }, time.Minute, time.Second, - ).Should(BeTrue()) - - pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + expectedRackIDs := map[string]int{ + rack1LabelValue: len(firstHalfNodeList.Items), + rack2LabelValue: len(secondHalfNodeList.Items), + } - err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") + err = validateDynamicRackID(ctx, k8sClient, aeroCluster, expectedRackIDs) Expect(err).ToNot(HaveOccurred()) }, ) @@ -174,8 +228,9 @@ var _ = FDescribe( It( "Should deploy with dynamic rack", func() { By("Deploying cluster with single rack") - aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, 2) + aeroCluster := createDummyRackAwareAerospikeCluster(clusterNamespacedName, int32(len(nodeList.Items))) aeroCluster.Spec.RackConfig.Namespaces = []string{"test"} + aeroCluster.Spec.PodSpec.MultiPodPerHost = ptr.To(false) Expect(DeployCluster(k8sClient, ctx, aeroCluster)).ToNot(HaveOccurred()) oldPodIDs, err := getPodIDs(ctx, aeroCluster) @@ -216,9 +271,12 @@ var _ = FDescribe( err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) Expect(err).ToNot(HaveOccurred()) - pod := aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + expectedRackIDs := map[string]int{ + rack1LabelValue: len(firstHalfNodeList.Items), + rack2LabelValue: len(secondHalfNodeList.Items), + } - err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") + err = validateDynamicRackID(ctx, k8sClient, aeroCluster, expectedRackIDs) Expect(err).ToNot(HaveOccurred()) By("Updating cluster by disabling dynamic rack") @@ -239,9 +297,11 @@ var _ = FDescribe( err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) Expect(err).ToNot(HaveOccurred()) - pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + expectedRackIDs = map[string]int{ + rack1LabelValue: len(nodeList.Items), + } - err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "1") + err = validateDynamicRackID(ctx, k8sClient, aeroCluster, expectedRackIDs) Expect(err).ToNot(HaveOccurred()) By("Updating cluster by enabling dynamic rack") @@ -262,9 +322,12 @@ var _ = FDescribe( err = validateOperationTypes(ctx, aeroCluster, oldPodIDs, operationTypeMap) Expect(err).ToNot(HaveOccurred()) - pod = aeroCluster.Status.Pods[aeroCluster.Name+"-1-0"] + expectedRackIDs = map[string]int{ + rack1LabelValue: len(firstHalfNodeList.Items), + rack2LabelValue: len(secondHalfNodeList.Items), + } - err = validateDynamicRackID(ctx, k8sClient, &pod, clusterNamespacedName, "2") + err = validateDynamicRackID(ctx, k8sClient, aeroCluster, expectedRackIDs) Expect(err).ToNot(HaveOccurred()) }, ) @@ -380,7 +443,7 @@ var _ = FDescribe( err := DeployCluster(k8sClient, ctx, aeroCluster) Expect(err).Should(HaveOccurred()) Expect(err.Error()).To( - ContainSubstring("hostpath volumes can only be mounted as read only file system")) + ContainSubstring("hostpath volumes can only be mounted as read-only filesystem")) }, ) }, @@ -388,20 +451,6 @@ var _ = FDescribe( }, ) -func isHostPathReadOnly(containerStatuses []v1.ContainerStatus, mountPath string) bool { - if len(containerStatuses) == 0 { - return false - } - - for idx := range containerStatuses[0].VolumeMounts { - if containerStatuses[0].VolumeMounts[idx].MountPath == mountPath { - return containerStatuses[0].VolumeMounts[idx].ReadOnly - } - } - - return false -} - func waitForDaemonSetPodsRunning(k8sClient client.Client, ctx goctx.Context, namespace, dsName string, retryInterval, timeout time.Duration) error { return wait.PollUntilContextTimeout(ctx, @@ -445,24 +494,35 @@ func waitForDaemonSetPodsRunning(k8sClient client.Client, ctx goctx.Context, nam } func validateDynamicRackID( - ctx context.Context, k8sClient client.Client, pod *asdbv1.AerospikePodStatus, namespacedName types.NamespacedName, - expectedValue string, + ctx context.Context, k8sClient client.Client, aeroCluster *asdbv1.AerospikeCluster, expectedRackIDs map[string]int, ) error { - info, err := requestInfoFromNode(logger, k8sClient, ctx, namespacedName, "namespace/test", pod) + podList, err := getPodList(aeroCluster, k8sClient) if err != nil { return err } - confs := strings.Split(info["namespace/test"], ";") - for _, conf := range confs { - if strings.Contains(conf, "rack-id") { - keyValue := strings.Split(conf, "=") - if keyValue[1] != expectedValue { - return fmt.Errorf("expected rack-id %s, but got %s", expectedValue, keyValue[1]) - } + actualRackIDs := make(map[string]int) + + for idx := range podList.Items { + podStatus := aeroCluster.Status.Pods[podList.Items[idx].Name] - return nil + info, err := requestInfoFromNode(logger, k8sClient, ctx, utils.GetNamespacedName(aeroCluster), + "namespace/test", &podStatus) + if err != nil { + return err } + + confs := strings.Split(info["namespace/test"], ";") + for _, conf := range confs { + if strings.Contains(conf, "rack-id") { + keyValue := strings.Split(conf, "=") + actualRackIDs[keyValue[1]]++ + } + } + } + + if !reflect.DeepEqual(expectedRackIDs, actualRackIDs) { + return fmt.Errorf("rackIDs are not getting assigned dynamically in expected manner") } return nil diff --git a/test/cluster/network_policy_test.go b/test/cluster/network_policy_test.go index e7d778bf..f592dc22 100644 --- a/test/cluster/network_policy_test.go +++ b/test/cluster/network_policy_test.go @@ -18,7 +18,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/retry" "k8s.io/utils/ptr" asdbv1 "github.com/aerospike/aerospike-kubernetes-operator/v4/api/v1" @@ -268,7 +267,7 @@ func negativeDeployNetworkPolicyTest(ctx goctx.Context, multiPodPerHost, enableT BeforeEach( func() { - err := deleteNodeLabels(ctx, []string{labelAccessAddress, labelAlternateAccessAddress}) + err := deleteLabelsAllNodes(ctx, []string{labelAccessAddress, labelAlternateAccessAddress}) Expect(err).ToNot(HaveOccurred()) }, ) @@ -289,7 +288,7 @@ func negativeDeployNetworkPolicyTest(ctx goctx.Context, multiPodPerHost, enableT It( "setting configured access-address without right label", Serial, func() { - err := setNodeLabels( + err := setLabelsAllNodes( ctx, map[string]string{labelAlternateAccessAddress: valueAlternateAccessAddress}, ) @@ -310,7 +309,7 @@ func negativeDeployNetworkPolicyTest(ctx goctx.Context, multiPodPerHost, enableT ) It( "setting configured alternate-access-address without right label", Serial, func() { - err := setNodeLabels(ctx, map[string]string{labelAccessAddress: valueAccessAddress}) + err := setLabelsAllNodes(ctx, map[string]string{labelAccessAddress: valueAccessAddress}) Expect(err).ToNot(HaveOccurred()) networkPolicy := &asdbv1.AerospikeNetworkPolicy{ @@ -815,14 +814,14 @@ func doTestNetworkPolicy( BeforeEach( func() { - err := deleteNodeLabels(ctx, []string{labelAccessAddress, labelAlternateAccessAddress}) + err := deleteLabelsAllNodes(ctx, []string{labelAccessAddress, labelAlternateAccessAddress}) Expect(err).ToNot(HaveOccurred()) }, ) It( "setting configured access-address", Serial, func() { - err := setNodeLabels(ctx, map[string]string{labelAccessAddress: valueAccessAddress}) + err := setLabelsAllNodes(ctx, map[string]string{labelAccessAddress: valueAccessAddress}) Expect(err).ToNot(HaveOccurred()) networkPolicy := &asdbv1.AerospikeNetworkPolicy{ @@ -848,7 +847,7 @@ func doTestNetworkPolicy( It( "setting configured alternate-access-address", Serial, func() { - err := setNodeLabels( + err := setLabelsAllNodes( ctx, map[string]string{ labelAlternateAccessAddress: valueAlternateAccessAddress, }, @@ -880,7 +879,7 @@ func doTestNetworkPolicy( clusterName := fmt.Sprintf("np-configured-ip-%d", GinkgoParallelProcess()) clusterNamespacedName := test.GetNamespacedName(clusterName, test.MultiClusterNs1) - err := setNodeLabels( + err := setLabelsAllNodes( ctx, map[string]string{ labelAccessAddress: valueAccessAddress, labelAlternateAccessAddress: valueAlternateAccessAddress, @@ -1452,58 +1451,20 @@ func getAerospikeClusterSpecWithNetworkPolicy( } } -func setNodeLabels(ctx goctx.Context, labels map[string]string) error { +func setLabelsAllNodes(ctx goctx.Context, labels map[string]string) error { nodeList, err := getNodeList(ctx, k8sClient) if err != nil { return err } - for idx := range nodeList.Items { - if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - node := &nodeList.Items[idx] - - if err := k8sClient.Get( - ctx, types.NamespacedName{Name: node.Name}, node); err != nil { - return err - } - - for key, val := range labels { - node.Labels[key] = val - } - - return k8sClient.Update(ctx, node) - }); err != nil { - return err - } - } - - return nil + return setNodeLabels(ctx, k8sClient, nodeList, labels) } -func deleteNodeLabels(ctx goctx.Context, keys []string) error { +func deleteLabelsAllNodes(ctx goctx.Context, keys []string) error { nodeList, err := getNodeList(ctx, k8sClient) if err != nil { return err } - for idx := range nodeList.Items { - if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - node := &nodeList.Items[idx] - - if err := k8sClient.Get( - ctx, types.NamespacedName{Name: node.Name}, node); err != nil { - return err - } - - for _, key := range keys { - delete(node.Labels, key) - } - - return k8sClient.Update(ctx, node) - }); err != nil { - return err - } - } - - return nil + return deleteNodeLabels(ctx, k8sClient, nodeList, keys) } diff --git a/test/cluster/storage_test.go b/test/cluster/storage_test.go index ec523492..bf60544e 100644 --- a/test/cluster/storage_test.go +++ b/test/cluster/storage_test.go @@ -576,7 +576,7 @@ var _ = Describe( ) aeroCluster.Spec.Storage.Volumes = append(aeroCluster.Spec.Storage.Volumes, - getStorageVolumeForSidecar("sidecar-volume", + getHostPathStorageVolumeForSidecar("sidecar-volume", "/para/tomcat1", containerName, true)) err = updateCluster(k8sClient, ctx, aeroCluster) diff --git a/test/cluster/utils.go b/test/cluster/utils.go index c546b08b..7146e7a5 100644 --- a/test/cluster/utils.go +++ b/test/cluster/utils.go @@ -22,6 +22,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" as "github.com/aerospike/aerospike-client-go/v8" @@ -861,6 +862,52 @@ func getNodeList(ctx goctx.Context, k8sClient client.Client) ( return nodeList, nil } +func setNodeLabels(ctx goctx.Context, k8sClient client.Client, nodeList *corev1.NodeList, l map[string]string) error { + for idx := range nodeList.Items { + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + node := &nodeList.Items[idx] + + if err := k8sClient.Get( + ctx, types.NamespacedName{Name: node.Name}, node); err != nil { + return err + } + + for key, val := range l { + node.Labels[key] = val + } + + return k8sClient.Update(ctx, node) + }); err != nil { + return err + } + } + + return nil +} + +func deleteNodeLabels(ctx goctx.Context, k8sClient client.Client, nodeList *corev1.NodeList, keys []string) error { + for idx := range nodeList.Items { + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + node := &nodeList.Items[idx] + + if err := k8sClient.Get( + ctx, types.NamespacedName{Name: node.Name}, node); err != nil { + return err + } + + for _, key := range keys { + delete(node.Labels, key) + } + + return k8sClient.Update(ctx, node) + }); err != nil { + return err + } + } + + return nil +} + func getRegion(ctx goctx.Context, k8sClient client.Client) (string, error) { nodes, err := getNodeList(ctx, k8sClient) if err != nil { From 4e8c73c17bf5dc46ada37e40149cf97de1ce6e88 Mon Sep 17 00:00:00 2001 From: Tanmay Jain Date: Fri, 30 May 2025 14:11:10 +0530 Subject: [PATCH 9/9] Addressing comments --- test/cluster/dynamic_rackid_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cluster/dynamic_rackid_test.go b/test/cluster/dynamic_rackid_test.go index 3ab3cd2a..47e60ff6 100644 --- a/test/cluster/dynamic_rackid_test.go +++ b/test/cluster/dynamic_rackid_test.go @@ -27,7 +27,7 @@ import ( const aerospikePath = "/opt/hostpath" -var _ = FDescribe( +var _ = Describe( "DynamicRack", func() { ctx := goctx.TODO() clusterName := fmt.Sprintf("dynamic-rack-%d", GinkgoParallelProcess())