From 4277c006ee224269b4600b12f8dc204e736d0002 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Tue, 7 Jan 2025 06:18:44 -0500 Subject: [PATCH 1/6] vrg: always get complete RecipeElements if recipe exists Co-Authored-by: Annaraya Narasagond Signed-off-by: Raghavendra Talur (cherry picked from commit 23911a5ea93a72fc4478e9eac39e115303e8716f) --- internal/controller/vrg_kubeobjects.go | 25 +++++++++++++++++++++++++ internal/controller/vrg_recipe.go | 19 +++---------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/internal/controller/vrg_kubeobjects.go b/internal/controller/vrg_kubeobjects.go index 43722bd57..da0a52996 100644 --- a/internal/controller/vrg_kubeobjects.go +++ b/internal/controller/vrg_kubeobjects.go @@ -820,6 +820,12 @@ func getCaptureGroups(recipe Recipe.Recipe) ([]kubeobjects.CaptureSpec, error) { for resourceType, resourceName := range resource { captureInstance, err := getResourceAndConvertToCaptureGroup(recipe, resourceType, resourceName) if err != nil { + if errors.Is(err, ErrVolumeCaptureNotSupported) { + // we only use the volumes group for determining the label selector + // ignore it in the capture sequence + continue + } + return resources, err } @@ -852,6 +858,12 @@ func getRecoverGroups(recipe Recipe.Recipe) ([]kubeobjects.RecoverSpec, error) { for resourceType, resourceName := range resource { captureInstance, err := getResourceAndConvertToRecoverGroup(recipe, resourceType, resourceName) if err != nil { + if errors.Is(err, ErrVolumeRecoverNotSupported) { + // we only use the volumes group for determining the label selector + // ignore it in the capture sequence + continue + } + return resources, err } @@ -862,6 +874,11 @@ func getRecoverGroups(recipe Recipe.Recipe) ([]kubeobjects.RecoverSpec, error) { return resources, nil } +var ( + ErrVolumeCaptureNotSupported = errors.New("volume capture not supported") + ErrVolumeRecoverNotSupported = errors.New("volume recover not supported") +) + func getResourceAndConvertToCaptureGroup( recipe Recipe.Recipe, resourceType, name string) (*kubeobjects.CaptureSpec, error, ) { @@ -873,6 +890,10 @@ func getResourceAndConvertToCaptureGroup( } } + if name == recipe.Spec.Volumes.Name { + return nil, ErrVolumeCaptureNotSupported + } + return nil, k8serrors.NewNotFound(schema.GroupResource{Resource: "Recipe.Spec.Group.Name"}, name) } @@ -904,6 +925,10 @@ func getResourceAndConvertToRecoverGroup( } } + if name == recipe.Spec.Volumes.Name { + return nil, ErrVolumeRecoverNotSupported + } + return nil, k8serrors.NewNotFound(schema.GroupResource{Resource: "Recipe.Spec.Group.Name"}, name) } diff --git a/internal/controller/vrg_recipe.go b/internal/controller/vrg_recipe.go index 5c46a50fb..bd787588b 100644 --- a/internal/controller/vrg_recipe.go +++ b/internal/controller/vrg_recipe.go @@ -81,25 +81,12 @@ func GetPVCSelector(ctx context.Context, reader client.Reader, vrg ramen.VolumeR ) (PvcSelector, error) { var recipeElements RecipeElements - return recipeElements.PvcSelector, recipeVolumesAndOptionallyWorkflowsGet( - ctx, reader, vrg, ramenConfig, log, &recipeElements, - func(recipe.Recipe, *RecipeElements, ramen.VolumeReplicationGroup, ramen.RamenConfig) error { - return nil - }, - ) + return recipeElements.PvcSelector, RecipeElementsGet( + ctx, reader, vrg, ramenConfig, log, &recipeElements) } func RecipeElementsGet(ctx context.Context, reader client.Reader, vrg ramen.VolumeReplicationGroup, ramenConfig ramen.RamenConfig, log logr.Logger, recipeElements *RecipeElements, -) error { - return recipeVolumesAndOptionallyWorkflowsGet(ctx, reader, vrg, ramenConfig, log, recipeElements, - recipeWorkflowsGet, - ) -} - -func recipeVolumesAndOptionallyWorkflowsGet(ctx context.Context, reader client.Reader, vrg ramen.VolumeReplicationGroup, - ramenConfig ramen.RamenConfig, log logr.Logger, recipeElements *RecipeElements, - workflowsGet func(recipe.Recipe, *RecipeElements, ramen.VolumeReplicationGroup, ramen.RamenConfig) error, ) error { if vrg.Spec.KubeObjectProtection == nil { *recipeElements = RecipeElements{ @@ -145,7 +132,7 @@ func recipeVolumesAndOptionallyWorkflowsGet(ctx context.Context, reader client.R PvcSelector: selector, } - if err := workflowsGet(recipe, recipeElements, vrg, ramenConfig); err != nil { + if err := recipeWorkflowsGet(recipe, recipeElements, vrg, ramenConfig); err != nil { return err } From 5b37d24a065665239230f180b3f0297661e24553 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Tue, 7 Jan 2025 06:27:03 -0500 Subject: [PATCH 2/6] vrg: RecipeElementsGet should return RecipeElements Co-Authored-by: Annaraya Narasagond Signed-off-by: Raghavendra Talur (cherry picked from commit d2590381e337dc163cd570b060aeda0401c86d9b) --- .../volumereplicationgroup_controller.go | 7 ++-- internal/controller/vrg_recipe.go | 38 +++++++++++-------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/internal/controller/volumereplicationgroup_controller.go b/internal/controller/volumereplicationgroup_controller.go index 226523428..d4ecbb752 100644 --- a/internal/controller/volumereplicationgroup_controller.go +++ b/internal/controller/volumereplicationgroup_controller.go @@ -547,9 +547,10 @@ func (v *VRGInstance) processVRG() ctrl.Result { } } - if err := RecipeElementsGet( - v.ctx, v.reconciler.Client, *v.instance, *v.ramenConfig, v.log, &v.recipeElements, - ); err != nil { + var err error + + v.recipeElements, err = RecipeElementsGet(v.ctx, v.reconciler.Client, *v.instance, *v.ramenConfig, v.log) + if err != nil { return v.invalid(err, "Failed to get recipe", false) } diff --git a/internal/controller/vrg_recipe.go b/internal/controller/vrg_recipe.go index bd787588b..452f55ddf 100644 --- a/internal/controller/vrg_recipe.go +++ b/internal/controller/vrg_recipe.go @@ -79,31 +79,35 @@ func GetPVCSelector(ctx context.Context, reader client.Reader, vrg ramen.VolumeR ramenConfig ramen.RamenConfig, log logr.Logger, ) (PvcSelector, error) { - var recipeElements RecipeElements + recipeElements, err := RecipeElementsGet(ctx, reader, vrg, ramenConfig, log) + if err != nil { + return recipeElements.PvcSelector, err + } - return recipeElements.PvcSelector, RecipeElementsGet( - ctx, reader, vrg, ramenConfig, log, &recipeElements) + return recipeElements.PvcSelector, nil } func RecipeElementsGet(ctx context.Context, reader client.Reader, vrg ramen.VolumeReplicationGroup, - ramenConfig ramen.RamenConfig, log logr.Logger, recipeElements *RecipeElements, -) error { + ramenConfig ramen.RamenConfig, log logr.Logger, +) (RecipeElements, error) { + var recipeElements RecipeElements + if vrg.Spec.KubeObjectProtection == nil { - *recipeElements = RecipeElements{ + recipeElements = RecipeElements{ PvcSelector: getPVCSelector(vrg, ramenConfig, nil, nil), } - return nil + return recipeElements, nil } if vrg.Spec.KubeObjectProtection.RecipeRef == nil { - *recipeElements = RecipeElements{ + recipeElements = RecipeElements{ PvcSelector: getPVCSelector(vrg, ramenConfig, nil, nil), CaptureWorkflow: captureWorkflowDefault(vrg, ramenConfig), RecoverWorkflow: recoverWorkflowDefault(vrg, ramenConfig), } - return nil + return recipeElements, nil } recipeNamespacedName := types.NamespacedName{ @@ -113,11 +117,11 @@ func RecipeElementsGet(ctx context.Context, reader client.Reader, vrg ramen.Volu recipe := recipe.Recipe{} if err := reader.Get(ctx, recipeNamespacedName, &recipe); err != nil { - return fmt.Errorf("recipe %v get error: %w", recipeNamespacedName.String(), err) + return recipeElements, fmt.Errorf("recipe %v get error: %w", recipeNamespacedName.String(), err) } if err := RecipeParametersExpand(&recipe, vrg.Spec.KubeObjectProtection.RecipeParameters, log); err != nil { - return err + return recipeElements, fmt.Errorf("recipe %v parameters expansion error: %w", recipeNamespacedName.String(), err) } var selector PvcSelector @@ -128,15 +132,19 @@ func RecipeElementsGet(ctx context.Context, reader client.Reader, vrg ramen.Volu recipe.Spec.Volumes.LabelSelector) } - *recipeElements = RecipeElements{ + recipeElements = RecipeElements{ PvcSelector: selector, } - if err := recipeWorkflowsGet(recipe, recipeElements, vrg, ramenConfig); err != nil { - return err + if err := recipeWorkflowsGet(recipe, &recipeElements, vrg, ramenConfig); err != nil { + return recipeElements, fmt.Errorf("recipe %v workflows get error: %w", recipeNamespacedName.String(), err) + } + + if err := recipeNamespacesValidate(recipeElements, vrg, ramenConfig); err != nil { + return recipeElements, fmt.Errorf("recipe %v namespaces validation error: %w", recipeNamespacedName.String(), err) } - return recipeNamespacesValidate(*recipeElements, vrg, ramenConfig) + return recipeElements, nil } func RecipeParametersExpand(recipe *recipe.Recipe, parameters map[string][]string, From b6eca82b5c6f2d2d86ba2a85f2f9ed86fedc5101 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Wed, 8 Jan 2025 13:19:51 -0500 Subject: [PATCH 3/6] tests: fix the tests Co-Authored-by: Annaraya Narasagond Signed-off-by: Raghavendra Talur (cherry picked from commit c4d7b33fbb650cf3b875eb9ff1d992572b60e690) --- internal/controller/vrg_pvc_selector_test.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/controller/vrg_pvc_selector_test.go b/internal/controller/vrg_pvc_selector_test.go index 318f8f9fa..9dd2d279b 100644 --- a/internal/controller/vrg_pvc_selector_test.go +++ b/internal/controller/vrg_pvc_selector_test.go @@ -41,10 +41,12 @@ var _ = Describe("VolumeReplicationGroupPVCSelector", func() { testCtx, cancel = context.WithCancel(context.TODO()) Expect(k8sClient).NotTo(BeNil()) vrgTestNamespace = createUniqueNamespace(testCtx) + ramenConfig.RamenOpsNamespace = vrgTestNamespace }) AfterEach(func() { Expect(k8sClient.Delete(testCtx, testNamespace)).To(Succeed()) + ramenConfig.RamenOpsNamespace = "" cancel() }) @@ -164,8 +166,9 @@ func getBaseVRG(namespace string) *ramen.VolumeReplicationGroup { Async: &ramen.VRGAsyncSpec{ SchedulingInterval: "5m", }, - ReplicationState: ramen.Primary, - S3Profiles: []string{"dummy-s3-profile"}, + ReplicationState: ramen.Primary, + S3Profiles: []string{"dummy-s3-profile"}, + ProtectedNamespaces: &[]string{namespace}, }, } } @@ -200,12 +203,13 @@ func getVRGDefinitionWithKubeObjectProtection(hasPVCSelectorLabels bool, namespa return vrg } -func getTestHook() *Recipe.Hook { +func getTestHook(testNamespace string) *Recipe.Hook { duration := 30 return &Recipe.Hook{ - Name: "hook-single", - Type: "exec", + Name: "hook-single", + Type: "exec", + Namespace: testNamespace, LabelSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "myapp": "testapp", @@ -267,7 +271,7 @@ func getRecipeDefinition(namespace string) *Recipe.Recipe { Spec: Recipe.RecipeSpec{ Groups: []*Recipe.Group{getTestGroup()}, Volumes: getTestVolumeGroup(), - Hooks: []*Recipe.Hook{getTestHook()}, + Hooks: []*Recipe.Hook{getTestHook(namespace)}, Workflows: []*Recipe.Workflow{ { Name: "backup", @@ -279,7 +283,7 @@ func getRecipeDefinition(namespace string) *Recipe.Recipe { "group": "test-group", }, { - "hook": "test-hook", + "hook": "hook-single/checkpoint", }, }, }, From e87b31744f5324648d743f5e7cc1de9e263e7c28 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Tue, 7 Jan 2025 05:08:08 -0500 Subject: [PATCH 4/6] vrg: cleanup rd when vrg is secondary In processForDeletion, we cleanup the resources that are related to volsync. It relies on v.volSyncPVCs but the list is empty when the vrg is secondary because it is populated by looking at the vrg.status.ProtectedPVCs. For managed applications, the RD,RS and Snapshots were being as a consequence of the deletion of VRG as the VRG is the owner for them. In the case of discovered apps, the vrg is not the owner and therefore the RD was being left behind. Co-Authored-by: Annaraya Narasagond Signed-off-by: Raghavendra Talur (cherry picked from commit 4a6edf3f58e8c63a50678de3c71d8001fe0a8154) --- internal/controller/vrg_volsync.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/internal/controller/vrg_volsync.go b/internal/controller/vrg_volsync.go index 500abee86..c23d127ad 100644 --- a/internal/controller/vrg_volsync.go +++ b/internal/controller/vrg_volsync.go @@ -554,23 +554,39 @@ func (v *VRGInstance) disownPVCs() error { return nil } -// cleanupResources this function deleted all PS, PD and VolumeSnapshots from its owner (VRG) +// cleanupResources this function deleted all RS, RD and VolumeSnapshots from its owner (VRG) func (v *VRGInstance) cleanupResources() error { for idx := range v.volSyncPVCs { pvc := &v.volSyncPVCs[idx] - if err := v.volSyncHandler.DeleteRS(pvc.Name, pvc.Namespace); err != nil { + if err := v.doCleanupResources(pvc.Name, pvc.Namespace); err != nil { return err } + } - if err := v.volSyncHandler.DeleteRD(pvc.Name, pvc.Namespace); err != nil { - return err - } + for idx := range v.instance.Spec.VolSync.RDSpec { + protectedPVC := v.instance.Spec.VolSync.RDSpec[idx].ProtectedPVC - if err := v.volSyncHandler.DeleteSnapshots(pvc.Namespace); err != nil { + if err := v.doCleanupResources(protectedPVC.Name, protectedPVC.Namespace); err != nil { return err } } return nil } + +func (v *VRGInstance) doCleanupResources(name, namespace string) error { + if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { + return err + } + + if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { + return err + } + + if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil { + return err + } + + return nil +} From bd34794417f0ee814e285e06c47e1820edad7883 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Wed, 8 Jan 2025 21:10:11 -0500 Subject: [PATCH 5/6] golangci-lint: remove if-return from the linters if-return has been removed from the default list of revive for a valid reason. See the argument in https://github.com/mgechev/revive/pull/843. Comparing two snippets 1. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil { return err } return nil ``` 2. ``` if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil { return err } if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil { return err } return v.volSyncHandler.DeleteSnapshots(namespace) ``` 1 is a lot easier to read compared to 2 but if-return throws an error for it. Signed-off-by: Raghavendra Talur (cherry picked from commit 5a49bea375dacb5d6f2f5378a8f3147fb52418b9) --- .golangci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.golangci.yaml b/.golangci.yaml index 9abb9af87..71863f699 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -47,7 +47,6 @@ linters-settings: - name: error-strings - name: error-naming - name: exported - - name: if-return - name: increment-decrement - name: var-naming - name: var-declaration From c453c89ab5cfd8d3b111e99a9584f238972a53e9 Mon Sep 17 00:00:00 2001 From: Raghavendra Talur Date: Fri, 10 Jan 2025 10:15:55 -0500 Subject: [PATCH 6/6] vrg: return nil on error Signed-off-by: Raghavendra Talur (cherry picked from commit fad02f27d0a18bc921f4b1d0b205ba945beb21af) --- internal/controller/vrg_recipe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/vrg_recipe.go b/internal/controller/vrg_recipe.go index 452f55ddf..0049629d1 100644 --- a/internal/controller/vrg_recipe.go +++ b/internal/controller/vrg_recipe.go @@ -81,7 +81,7 @@ func GetPVCSelector(ctx context.Context, reader client.Reader, vrg ramen.VolumeR ) (PvcSelector, error) { recipeElements, err := RecipeElementsGet(ctx, reader, vrg, ramenConfig, log) if err != nil { - return recipeElements.PvcSelector, err + return PvcSelector{}, err } return recipeElements.PvcSelector, nil