From f0500623e203998dee357adc602922d17fc2d184 Mon Sep 17 00:00:00 2001 From: rakeshgm Date: Wed, 23 Oct 2024 19:53:22 +0530 Subject: [PATCH 1/2] Map PV secrets in parameters when restoring PVs Map PV secrets in parameters when restoring PVs across peer clusters. Signed-off-by: rakeshgm --- internal/controller/vrg_volrep.go | 122 +++++++++++++++++++++++++----- 1 file changed, 101 insertions(+), 21 deletions(-) diff --git a/internal/controller/vrg_volrep.go b/internal/controller/vrg_volrep.go index 5904ae767..51049e445 100644 --- a/internal/controller/vrg_volrep.go +++ b/internal/controller/vrg_volrep.go @@ -32,6 +32,21 @@ const ( defaultVRCAnnotationKey = "replication.storage.openshift.io/is-default-class" ) +//nolint:gosec +const ( + // secretRef keys + controllerPublishSecretName = "csi.storage.k8s.io/controller-publish-secret-name" + controllerPublishSecretNamespace = "csi.storage.k8s.io/controller-publish-secret-namespace" + nodeStageSecretName = "csi.storage.k8s.io/node-stage-secret-name" + nodeStageSecretNamespace = "csi.storage.k8s.io/node-stage-secret-namespace" + nodePublishSecretName = "csi.storage.k8s.io/node-publish-secret-name" + nodePublishSecretNamespace = "csi.storage.k8s.io/node-publish-secret-namespace" + controllerExpandSecretName = "csi.storage.k8s.io/controller-expand-secret-name" + controllerExpandSecretNamespace = "csi.storage.k8s.io/controller-expand-secret-namespace" + nodeExpandSecretName = "csi.storage.k8s.io/node-expand-secret-name" + nodeExpandSecretNamespace = "csi.storage.k8s.io/node-expand-secret-namespace" +) + func logWithPvcName(log logr.Logger, pvc *corev1.PersistentVolumeClaim) logr.Logger { return log.WithValues("pvc", types.NamespacedName{Name: pvc.Name, Namespace: pvc.Namespace}.String()) } @@ -1332,6 +1347,24 @@ func (v *VRGInstance) filterDefaultVRC( defaultVRCAnnotationKey) } +func (v *VRGInstance) getStorageClassFromSCName(scName *string) (*storagev1.StorageClass, error) { + if storageClass, ok := v.storageClassCache[*scName]; ok { + return storageClass, nil + } + + storageClass := &storagev1.StorageClass{} + if err := v.reconciler.Get(v.ctx, types.NamespacedName{Name: *scName}, storageClass); err != nil { + v.log.Info(fmt.Sprintf("Failed to get the storageclass %s", *scName)) + + return nil, fmt.Errorf("failed to get the storageclass with name %s (%w)", + *scName, err) + } + + v.storageClassCache[*scName] = storageClass + + return storageClass, nil +} + // getStorageClass inspects the PVCs being protected by this VRG instance for the passed in namespacedName, and // returns its corresponding StorageClass resource from an instance cache if available, or fetches it from the API // server and stores it in an instance cache before returning the StorageClass @@ -1363,21 +1396,7 @@ func (v *VRGInstance) getStorageClass(namespacedName types.NamespacedName) (*sto return nil, fmt.Errorf("missing StorageClass name for pvc (%s)", namespacedName) } - if storageClass, ok := v.storageClassCache[*scName]; ok { - return storageClass, nil - } - - storageClass := &storagev1.StorageClass{} - if err := v.reconciler.Get(v.ctx, types.NamespacedName{Name: *scName}, storageClass); err != nil { - v.log.Info(fmt.Sprintf("Failed to get the storageclass %s", *scName)) - - return nil, fmt.Errorf("failed to get the storageclass with name %s (%w)", - *scName, err) - } - - v.storageClassCache[*scName] = storageClass - - return storageClass, nil + return v.getStorageClassFromSCName(scName) } // checkVRStatus checks if the VolumeReplication resource has the desired status for the @@ -2025,7 +2044,7 @@ func (v *VRGInstance) restorePVsFromObjectStore(objectStore ObjectStorer, s3Prof return 0, fmt.Errorf("%s: %w", errMsg, err) } - return restoreClusterDataObjects(v, pvList, "PV", cleanupPVForRestore, v.validateExistingPV) + return restoreClusterDataObjects(v, pvList, "PV", v.cleanupPVForRestore, v.validateExistingPV) } func (v *VRGInstance) restorePVCsFromObjectStore(objectStore ObjectStorer, s3ProfileName string) (int, error) { @@ -2092,7 +2111,7 @@ func restoreClusterDataObjects[ ]( v *VRGInstance, objList []ObjectType, objType string, - cleanupForRestore func(*ObjectType), + cleanupForRestore func(*ObjectType) error, validateExistingObject func(*ObjectType) error, ) (int, error) { numRestored := 0 @@ -2102,7 +2121,13 @@ func restoreClusterDataObjects[ objectCopy := &*object obj := ClientObject(objectCopy) - cleanupForRestore(objectCopy) + err := cleanupForRestore(objectCopy) + if err != nil { + v.log.Info("failed to cleanup during restore", "error", err.Error()) + + return numRestored, err + } + addRestoreAnnotation(obj) if err := v.reconciler.Create(v.ctx, obj); err != nil { @@ -2142,7 +2167,11 @@ func (v *VRGInstance) updateExistingPVForSync(pv *corev1.PersistentVolume) error // failover/relocate process. Hence, the restore may not be // required and the annotation for restore can be missing for // the sync mode. - cleanupPVForRestore(pv) + err := v.cleanupPVForRestore(pv) + if err != nil { + return err + } + addRestoreAnnotation(pv) if err := v.reconciler.Update(v.ctx, pv); err != nil { @@ -2300,22 +2329,73 @@ func addRestoreAnnotation(obj client.Object) { obj.GetAnnotations()[RestoreAnnotation] = RestoredByRamen } +func secretsFromSC(params map[string]string, + secretName, secretNamespace string, +) (*corev1.SecretReference, bool) { + secretRef := corev1.SecretReference{ + Name: params[secretName], + Namespace: params[secretNamespace], + } + + exists := secretRef != (corev1.SecretReference{}) + + return &secretRef, exists +} + +func (v *VRGInstance) processPVSecrets(pv *corev1.PersistentVolume) error { + sc, err := v.getStorageClassFromSCName(&pv.Spec.StorageClassName) + if err != nil { + return err + } + + secFromSC, exists := secretsFromSC(sc.Parameters, nodeStageSecretName, nodeStageSecretNamespace) + if exists { + pv.Spec.CSI.NodeStageSecretRef = secFromSC + } + + secFromSC, exists = secretsFromSC(sc.Parameters, nodePublishSecretName, nodePublishSecretNamespace) + if exists { + pv.Spec.CSI.NodePublishSecretRef = secFromSC + } + + secFromSC, exists = secretsFromSC(sc.Parameters, nodeExpandSecretName, nodeExpandSecretNamespace) + if exists { + pv.Spec.CSI.NodeExpandSecretRef = secFromSC + } + + secFromSC, exists = secretsFromSC(sc.Parameters, controllerExpandSecretName, controllerExpandSecretNamespace) + if exists { + pv.Spec.CSI.ControllerExpandSecretRef = secFromSC + } + + secFromSC, exists = secretsFromSC(sc.Parameters, controllerPublishSecretName, controllerPublishSecretNamespace) + if exists { + pv.Spec.CSI.ControllerExpandSecretRef = secFromSC + } + + return nil +} + // cleanupForRestore cleans up required PV or PVC fields, to ensure restore succeeds // to a new cluster, and rebinding the PVC to an existing PV with the same claimRef -func cleanupPVForRestore(pv *corev1.PersistentVolume) { +func (v *VRGInstance) cleanupPVForRestore(pv *corev1.PersistentVolume) error { pv.ResourceVersion = "" if pv.Spec.ClaimRef != nil { pv.Spec.ClaimRef.UID = "" pv.Spec.ClaimRef.ResourceVersion = "" pv.Spec.ClaimRef.APIVersion = "" } + + return v.processPVSecrets(pv) } -func cleanupPVCForRestore(pvc *corev1.PersistentVolumeClaim) { +func cleanupPVCForRestore(pvc *corev1.PersistentVolumeClaim) error { pvc.ObjectMeta.Annotations = PruneAnnotations(pvc.GetAnnotations()) pvc.ObjectMeta.Finalizers = []string{} pvc.ObjectMeta.ResourceVersion = "" pvc.ObjectMeta.OwnerReferences = nil + + return nil } // Follow this logic to update VRG (and also ProtectedPVC) conditions for VolRep From e1e279f487daac26c7493e2a779e69ed1a165ae5 Mon Sep 17 00:00:00 2001 From: rakeshgm Date: Mon, 28 Oct 2024 15:52:01 +0530 Subject: [PATCH 2/2] fix vrg_tests create SC in the sync basic test Signed-off-by: rakeshgm --- internal/controller/vrg_volrep_test.go | 283 +++++++++++++------------ 1 file changed, 151 insertions(+), 132 deletions(-) diff --git a/internal/controller/vrg_volrep_test.go b/internal/controller/vrg_volrep_test.go index 74e9027e9..d428e1f83 100644 --- a/internal/controller/vrg_volrep_test.go +++ b/internal/controller/vrg_volrep_test.go @@ -74,143 +74,158 @@ var _ = Describe("VolumeReplicationGroupVolRepController", func() { return vrgGet().Status.ProtectedPVCs } var dataReadyCondition *metav1.Condition - When("ReplicationState is invalid", func() { - It("should set DataReady status=False reason=Error", func() { - vrg = &ramendrv1alpha1.VolumeReplicationGroup{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - Name: "asdf", - }, - Spec: ramendrv1alpha1.VolumeReplicationGroupSpec{ - PVCSelector: metav1.LabelSelector{}, - ReplicationState: "invalid", - S3Profiles: []string{}, - }, - } - Expect(k8sClient.Create(context.TODO(), vrg)).To(Succeed()) - vrgNamespacedName = types.NamespacedName{Name: vrg.Name, Namespace: vrg.Namespace} - Eventually(func() int { - vrgGet() + Context("Sync Basic Test", func() { + syncBasicTestTemplate := &template{ + ClaimBindInfo: corev1.ClaimBound, + VolumeBindInfo: corev1.VolumeBound, + schedulingInterval: "1h", + storageClassName: "manual", + replicationClassName: "test-replicationclass", + vrcProvisioner: "manual.storage.com", + scProvisioner: "manual.storage.com", + replicationClassLabels: map[string]string{"protection": "ramen"}, + } + It("should initialize test with creating StorageClass and VolumeReplicationClass", func() { + createStorageClass(syncBasicTestTemplate) + }) + When("ReplicationState is invalid", func() { + It("should set DataReady status=False reason=Error", func() { + vrg = &ramendrv1alpha1.VolumeReplicationGroup{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "asdf", + }, + Spec: ramendrv1alpha1.VolumeReplicationGroupSpec{ + PVCSelector: metav1.LabelSelector{}, + ReplicationState: "invalid", + S3Profiles: []string{}, + }, + } + Expect(k8sClient.Create(context.TODO(), vrg)).To(Succeed()) + vrgNamespacedName = types.NamespacedName{Name: vrg.Name, Namespace: vrg.Namespace} + Eventually(func() int { + vrgGet() - return len(vrg.Status.Conditions) - }, timeout, interval).ShouldNot(BeZero()) - dataReadyCondition = vrgConditionStatusReasonExpect("DataReady", metav1.ConditionFalse, "Error") - }) - It("should set DataProtected status=Unknown reason=Initializing", func() { - vrgConditionStatusReasonExpect("DataProtected", metav1.ConditionUnknown, "Initializing") - }) - It("should set ClusterDataReady status=Unknown reason=Initializing", func() { - vrgConditionStatusReasonExpect("ClusterDataReady", metav1.ConditionUnknown, "Initializing") + return len(vrg.Status.Conditions) + }, timeout, interval).ShouldNot(BeZero()) + dataReadyCondition = vrgConditionStatusReasonExpect("DataReady", metav1.ConditionFalse, "Error") + }) + It("should set DataProtected status=Unknown reason=Initializing", func() { + vrgConditionStatusReasonExpect("DataProtected", metav1.ConditionUnknown, "Initializing") + }) + It("should set ClusterDataReady status=Unknown reason=Initializing", func() { + vrgConditionStatusReasonExpect("ClusterDataReady", metav1.ConditionUnknown, "Initializing") + }) + It("should set ClusterDataProtected status=Unknown reason=Initializing", func() { + vrgConditionStatusReasonExpect("ClusterDataProtected", metav1.ConditionUnknown, "Initializing") + }) }) - It("should set ClusterDataProtected status=Unknown reason=Initializing", func() { - vrgConditionStatusReasonExpect("ClusterDataProtected", metav1.ConditionUnknown, "Initializing") + When("ReplicationState is primary, but sync and async are disabled", func() { + It("should change DataReady message", func() { + vrg.Spec.ReplicationState = "primary" + dataReadyConditionMessage := dataReadyCondition.Message + updateVRG(vrg) + Eventually(func() string { + vrgGet() + dataReadyCondition = vrgConditionExpect("DataReady") + + return dataReadyCondition.Message + }, timeout, interval).ShouldNot(Equal(dataReadyConditionMessage)) + vrgConditionStatusReasonExpect("DataReady", metav1.ConditionFalse, "Error") + }) }) - }) - When("ReplicationState is primary, but sync and async are disabled", func() { - It("should change DataReady message", func() { - vrg.Spec.ReplicationState = "primary" - dataReadyConditionMessage := dataReadyCondition.Message - updateVRG(vrg) - Eventually(func() string { - vrgGet() - dataReadyCondition = vrgConditionExpect("DataReady") - - return dataReadyCondition.Message - }, timeout, interval).ShouldNot(Equal(dataReadyConditionMessage)) - vrgConditionStatusReasonExpect("DataReady", metav1.ConditionFalse, "Error") + When("ReplicationState is primary and sync is enabled, but s3 profiles are absent", func() { + It("should set ClusterDataReady status=False reason=Error", func() { + vrg.Spec.Sync = &ramendrv1alpha1.VRGSyncSpec{} + updateVRG(vrg) + var clusterDataReadyCondition *metav1.Condition + Eventually(func() metav1.ConditionStatus { + vrgGet() + clusterDataReadyCondition = vrgConditionExpect("ClusterDataReady") + + return clusterDataReadyCondition.Status + }, timeout, interval).Should(Equal(metav1.ConditionFalse)) + Expect(clusterDataReadyCondition.Reason).To(Equal("Error")) + }) }) - }) - When("ReplicationState is primary and sync is enabled, but s3 profiles are absent", func() { - It("should set ClusterDataReady status=False reason=Error", func() { - vrg.Spec.Sync = &ramendrv1alpha1.VRGSyncSpec{} - updateVRG(vrg) - var clusterDataReadyCondition *metav1.Condition - Eventually(func() metav1.ConditionStatus { - vrgGet() - clusterDataReadyCondition = vrgConditionExpect("ClusterDataReady") - - return clusterDataReadyCondition.Status - }, timeout, interval).Should(Equal(metav1.ConditionFalse)) - Expect(clusterDataReadyCondition.Reason).To(Equal("Error")) + When("VRG is deleted", func() { + BeforeEach(func() { + Expect(k8sClient.Delete(context.TODO(), vrg)).To(Succeed()) + }) + It("should allow the VRG to be deleted", func() { + Eventually(func() error { + return apiReader.Get(context.TODO(), vrgNamespacedName, vrg) + }).Should(MatchError(errors.NewNotFound(schema.GroupResource{ + Group: ramendrv1alpha1.GroupVersion.Group, + Resource: "volumereplicationgroups", + }, vrg.Name))) + }) }) - }) - When("VRG is deleted", func() { - BeforeEach(func() { - Expect(k8sClient.Delete(context.TODO(), vrg)).To(Succeed()) + var pv0 *corev1.PersistentVolume + var pvc0 *corev1.PersistentVolumeClaim + When("PV exists, is bound, and its claim's deletion timestamp is non-zero", func() { + BeforeEach(func() { + pv := pv("pv0", "pvc0", vrg.Namespace, syncBasicTestTemplate.storageClassName) + pvc := pvc(pv.Spec.ClaimRef.Name, pv.Spec.ClaimRef.Namespace, pv.Name, pv.Spec.StorageClassName, nil) + pvc.Finalizers = []string{"ramendr.openshift.io/asdf"} + vrgS3KeyPrefix := vrgS3KeyPrefix(vrgNamespacedName) + populateS3Store(vrgS3KeyPrefix, []corev1.PersistentVolume{*pv}, []corev1.PersistentVolumeClaim{*pvc}) + Expect(k8sClient.Create(context.TODO(), pv)).To(Succeed()) + Expect(k8sClient.Create(context.TODO(), pvc)).To(Succeed()) + Expect(apiReader.Get(context.TODO(), types.NamespacedName{Name: pv.Name}, pv)).To(Succeed()) + pv.Status.Phase = corev1.VolumeBound + Expect(k8sClient.Status().Update(context.TODO(), pv)).To(Succeed()) + Expect(k8sClient.Delete(context.TODO(), pvc)).To(Succeed()) + Expect(apiReader.Get(context.TODO(), types.NamespacedName{Namespace: pvc.Namespace, Name: pvc.Name}, pvc)). + To(Succeed()) + pv0 = pv + pvc0 = pvc + }) + It("should set ClusterDataReady false", func() { + vrg.ResourceVersion = "" + vrg.Spec.S3Profiles = []string{s3Profiles[vrgS3ProfileNumber].S3ProfileName} + Expect(k8sClient.Create(context.TODO(), vrg)).To(Succeed()) + Expect(apiReader.Get(context.TODO(), vrgNamespacedName, vrg)).To(Succeed()) + Eventually(func() *metav1.Condition { + vrgGet() + + return meta.FindStatusCondition(vrg.Status.Conditions, "ClusterDataReady") + }).Should(And( + Not(BeNil()), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", "Error"), + )) + }) }) - It("should allow the VRG to be deleted", func() { - Eventually(func() error { - return apiReader.Get(context.TODO(), vrgNamespacedName, vrg) - }).Should(MatchError(errors.NewNotFound(schema.GroupResource{ - Group: ramendrv1alpha1.GroupVersion.Group, - Resource: "volumereplicationgroups", - }, vrg.Name))) + When("PVC is deleted finally and PV is unbound", func() { + BeforeEach(func() { + pv := pv0 + pvc := pvc0 + Expect(apiReader.Get(context.TODO(), types.NamespacedName{Namespace: pvc.Namespace, Name: pvc.Name}, pvc)). + To(Succeed()) + pvc.Finalizers = []string{} + Expect(k8sClient.Update(context.TODO(), pvc)).To(Succeed()) + Expect(apiReader.Get(context.TODO(), types.NamespacedName{Name: pv.Name}, pv)).To(Succeed()) + pv.Status.Phase = corev1.VolumePending + Expect(k8sClient.Status().Update(context.TODO(), pv)).To(Succeed()) + }) + It("should set ClusterDataReady true", func() { + Eventually(func() *metav1.Condition { + vrgGet() + + return meta.FindStatusCondition(vrg.Status.Conditions, "ClusterDataReady") + }).Should( + HaveField("Status", metav1.ConditionTrue), + ) + }) }) - }) - var pv0 *corev1.PersistentVolume - var pvc0 *corev1.PersistentVolumeClaim - When("PV exists, is bound, and its claim's deletion timestamp is non-zero", func() { - BeforeEach(func() { - pv := pv("pv0", "pvc0", vrg.Namespace, "") - pvc := pvc(pv.Spec.ClaimRef.Name, pv.Spec.ClaimRef.Namespace, pv.Name, pv.Spec.StorageClassName, nil) - pvc.Finalizers = []string{"ramendr.openshift.io/asdf"} - vrgS3KeyPrefix := vrgS3KeyPrefix(vrgNamespacedName) - populateS3Store(vrgS3KeyPrefix, []corev1.PersistentVolume{*pv}, []corev1.PersistentVolumeClaim{*pvc}) - Expect(k8sClient.Create(context.TODO(), pv)).To(Succeed()) - Expect(k8sClient.Create(context.TODO(), pvc)).To(Succeed()) - Expect(apiReader.Get(context.TODO(), types.NamespacedName{Name: pv.Name}, pv)).To(Succeed()) - pv.Status.Phase = corev1.VolumeBound - Expect(k8sClient.Status().Update(context.TODO(), pv)).To(Succeed()) - Expect(k8sClient.Delete(context.TODO(), pvc)).To(Succeed()) - Expect(apiReader.Get(context.TODO(), types.NamespacedName{Namespace: pvc.Namespace, Name: pvc.Name}, pvc)). - To(Succeed()) - pv0 = pv - pvc0 = pvc - }) - It("should set ClusterDataReady false", func() { - vrg.ResourceVersion = "" - vrg.Spec.S3Profiles = []string{s3Profiles[vrgS3ProfileNumber].S3ProfileName} - Expect(k8sClient.Create(context.TODO(), vrg)).To(Succeed()) - Expect(apiReader.Get(context.TODO(), vrgNamespacedName, vrg)).To(Succeed()) - Eventually(func() *metav1.Condition { - vrgGet() - - return meta.FindStatusCondition(vrg.Status.Conditions, "ClusterDataReady") - }).Should(And( - Not(BeNil()), - HaveField("Status", metav1.ConditionFalse), - HaveField("Reason", "Error"), - )) + Specify("PV delete", func() { + Expect(k8sClient.Delete(context.TODO(), pv0)).To(Succeed()) }) - }) - When("PVC is deleted finally and PV is unbound", func() { - BeforeEach(func() { - pv := pv0 - pvc := pvc0 - Expect(apiReader.Get(context.TODO(), types.NamespacedName{Namespace: pvc.Namespace, Name: pvc.Name}, pvc)). - To(Succeed()) - pvc.Finalizers = []string{} - Expect(k8sClient.Update(context.TODO(), pvc)).To(Succeed()) - Expect(apiReader.Get(context.TODO(), types.NamespacedName{Name: pv.Name}, pv)).To(Succeed()) - pv.Status.Phase = corev1.VolumePending - Expect(k8sClient.Status().Update(context.TODO(), pv)).To(Succeed()) - }) - It("should set ClusterDataReady true", func() { - Eventually(func() *metav1.Condition { - vrgGet() - - return meta.FindStatusCondition(vrg.Status.Conditions, "ClusterDataReady") - }).Should( - HaveField("Status", metav1.ConditionTrue), - ) + Specify("VRG delete", func() { + Expect(k8sClient.Delete(context.TODO(), vrg)).To(Succeed()) }) }) - Specify("PV delete", func() { - Expect(k8sClient.Delete(context.TODO(), pv0)).To(Succeed()) - }) - Specify("VRG delete", func() { - Expect(k8sClient.Delete(context.TODO(), vrg)).To(Succeed()) - }) // Test first restore Context("restore test case", func() { @@ -1750,15 +1765,19 @@ func (v *vrgTest) createVRC(testTemplate *template) { } func (v *vrgTest) createSC(testTemplate *template) { - By("creating StorageClass " + v.storageClass) + createStorageClass(testTemplate) +} + +func createStorageClass(testTemplate *template) { + By("creating StorageClass " + testTemplate.storageClassName) - if v.storageClass == "" || testTemplate.scDisabled { + if testTemplate.storageClassName == "" || testTemplate.scDisabled { return } sc := &storagev1.StorageClass{ ObjectMeta: metav1.ObjectMeta{ - Name: v.storageClass, + Name: testTemplate.storageClassName, }, Provisioner: testTemplate.scProvisioner, } @@ -1766,12 +1785,12 @@ func (v *vrgTest) createSC(testTemplate *template) { err := k8sClient.Create(context.TODO(), sc) if err != nil { if errors.IsAlreadyExists(err) { - err = k8sClient.Get(context.TODO(), types.NamespacedName{Name: v.storageClass}, sc) + err = k8sClient.Get(context.TODO(), types.NamespacedName{Name: testTemplate.storageClassName}, sc) } } Expect(err).NotTo(HaveOccurred(), - "failed to create/get StorageClass %s/%s", v.storageClass, v.vrgName) + "failed to create/get StorageClass %s/%s", testTemplate.storageClassName, testTemplate.storageClassName) } func (v *vrgTest) verifyPVCBindingToPV(shouldBeBound bool) {