feat: preventing zombie filestore backups and patching status instead…

… of update

.github/workflows/pr-checks.yml

@@ -9,6 +9,7 @@ jobs:
     strategy:
       matrix:
         ipRangeAutomaticCidrAllocation: [ "false", "true" ]
+        gcpNfsVolumeAutomaticLocationAllocation: [ "false", "true" ]
     runs-on: ubuntu-latest
     steps:
       - name: Checkout repository
@@ -22,6 +23,7 @@ jobs:
       - name: Build and test
         env:
           FF_IP_RANGE_AUTOMATIC_CIDR_ALLOCATION: ${{ matrix.ipRangeAutomaticCidrAllocation }}
+          FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION: ${{ matrix.gcpNfsVolumeAutomaticLocationAllocation }}
         run: |
           ./config/sync.sh
           go mod tidy

api/cloud-resources/v1beta1/gcpnfsbackupschedule_types.go

@@ -20,6 +20,7 @@ import (
 	featuretypes "github.com/kyma-project/cloud-manager/pkg/feature/types"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
@@ -280,3 +281,17 @@ type GcpNfsBackupScheduleList struct {
 func init() {
 	SchemeBuilder.Register(&GcpNfsBackupSchedule{}, &GcpNfsBackupScheduleList{})
 }
+
+func (sc *GcpNfsBackupSchedule) CloneForPatchStatus() client.Object {
+	return &GcpNfsBackupSchedule{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "GcpNfsBackupSchedule",
+			APIVersion: GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: sc.Namespace,
+			Name:      sc.Name,
+		},
+		Status: sc.Status,
+	}
+}

api/cloud-resources/v1beta1/gcpnfsvolumebackup_types.go

@@ -20,6 +20,7 @@ import (
 	featuretypes "github.com/kyma-project/cloud-manager/pkg/feature/types"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 type GcpNfsBackupState string
@@ -32,9 +33,6 @@ const (
 	// GcpNfsBackupError signifies the backup operation resulted in error.
 	GcpNfsBackupError GcpNfsBackupState = "Error"
 
-	// GcpNfsBackupProcessing signifies backup operation is in-progress.
-	GcpNfsBackupProcessing GcpNfsBackupState = "Processing"
-
 	// GcpNfsBackupCreating signifies backup create operation is in-progress.
 	GcpNfsBackupCreating GcpNfsBackupState = "Creating"
 
@@ -62,7 +60,7 @@ type GcpNfsVolumeRef struct {
 	Namespace string `json:"namespace,omitempty"`
 }
 
-func (v GcpNfsVolumeRef) ToNamespacedName(fallbackNamespace string) types.NamespacedName {
+func (v *GcpNfsVolumeRef) ToNamespacedName(fallbackNamespace string) types.NamespacedName {
 	ns := v.Namespace
 	if len(ns) == 0 {
 		ns = fallbackNamespace
@@ -158,3 +156,17 @@ type GcpNfsVolumeBackupList struct {
 func init() {
 	SchemeBuilder.Register(&GcpNfsVolumeBackup{}, &GcpNfsVolumeBackupList{})
 }
+
+func (in *GcpNfsVolumeBackup) CloneForPatchStatus() client.Object {
+	return &GcpNfsVolumeBackup{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "GcpNfsVolumeBackup",
+			APIVersion: GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: in.Namespace,
+			Name:      in.Name,
+		},
+		Status: in.Status,
+	}
+}

api/cloud-resources/v1beta1/gcpnfsvolumerestore_types.go

@@ -20,6 +20,7 @@ import (
 	featuretypes "github.com/kyma-project/cloud-manager/pkg/feature/types"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 type GcpNfsVolumeBackupRef struct {
@@ -31,7 +32,7 @@ type GcpNfsVolumeBackupRef struct {
 	Namespace string `json:"namespace"`
 }
 
-func (v GcpNfsVolumeBackupRef) ToNamespacedName(fallbackNamespace string) types.NamespacedName {
+func (v *GcpNfsVolumeBackupRef) ToNamespacedName(fallbackNamespace string) types.NamespacedName {
 	ns := v.Namespace
 	if len(ns) == 0 {
 		ns = fallbackNamespace
@@ -125,3 +126,17 @@ type GcpNfsVolumeRestoreList struct {
 func init() {
 	SchemeBuilder.Register(&GcpNfsVolumeRestore{}, &GcpNfsVolumeRestoreList{})
 }
+
+func (in *GcpNfsVolumeRestore) CloneForPatchStatus() client.Object {
+	return &GcpNfsVolumeRestore{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "GcpNfsVolumeRestore",
+			APIVersion: GroupVersion.String(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: in.Namespace,
+			Name:      in.Name,
+		},
+		Status: in.Status,
+	}
+}

internal/controller/cloud-resources/gcpnfsvolume_test.go

@@ -1,8 +1,9 @@
 package cloudresources
 
 import (
+	"context"
 	"fmt"
-	"os"
+	"github.com/kyma-project/cloud-manager/pkg/feature"
 	"time"
 
 	cloudcontrolv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-control/v1beta1"
@@ -31,9 +32,14 @@ var _ = Describe("Feature: SKR GcpNfsVolume", func() {
 	kcpIpRangeName := "513f20b4-7b73-4246-9397-f8dd55344479"
 	kcpIpRange := &cloudcontrolv1beta1.IpRange{}
 
+	shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled := func() (bool, string) {
+		if feature.GcpNfsVolumeAutomaticLocationAllocation.Value(context.Background()) {
+			return false, ""
+		}
+		return true, "cpNfsVolumeAutomaticLocationAllocation is disabled"
+	}
+
 	BeforeEach(func() {
-		err := os.Unsetenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION")
-		Expect(err).ToNot(HaveOccurred())
 		By("And Given SKR namespace exists", func() {
 			//Create namespace if it doesn't exist.
 			Eventually(CreateNamespace).
@@ -678,8 +684,10 @@ var _ = Describe("Feature: SKR GcpNfsVolume", func() {
 		kcpNfsInstance := &cloudcontrolv1beta1.NfsInstance{}
 		pv := &corev1.PersistentVolume{}
 		scope := &cloudcontrolv1beta1.Scope{}
-		err := os.Setenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION", "true")
-		Expect(err).ToNot(HaveOccurred())
+		shouldSkip, msg := shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled()
+		if shouldSkip {
+			Skip(msg)
+		}
 		gcpNfsVolumeName := "gcp-nfs-volume-4"
 		nfsIpAddress := "10.11.12.14"
 		pvSpec := &cloudresourcesv1beta1.GcpNfsVolumePvSpec{
@@ -898,8 +906,10 @@ var _ = Describe("Feature: SKR GcpNfsVolume", func() {
 		kcpNfsInstance := &cloudcontrolv1beta1.NfsInstance{}
 		pv := &corev1.PersistentVolume{}
 		scope := &cloudcontrolv1beta1.Scope{}
-		err := os.Setenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION", "true")
-		Expect(err).ToNot(HaveOccurred())
+		shouldSkip, msg := shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled()
+		if shouldSkip {
+			Skip(msg)
+		}
 
 		gcpNfsVolumeName := "gcp-nfs-volume-5"
 		nfsIpAddress := "10.11.12.16"
@@ -1165,8 +1175,10 @@ var _ = Describe("Feature: SKR GcpNfsVolume", func() {
 		pv := &corev1.PersistentVolume{}
 		pvc := &corev1.PersistentVolumeClaim{}
 		scope := &cloudcontrolv1beta1.Scope{}
-		err := os.Setenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION", "true")
-		Expect(err).ToNot(HaveOccurred())
+		shouldSkip, msg := shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled()
+		if shouldSkip {
+			Skip(msg)
+		}
 
 		gcpNfsVolumeName := "gcp-nfs-volume-6"
 		nfsIpAddress := "10.11.12.16"

internal/controller/cloud-resources/gcpnfsvolumebackup_test.go

@@ -1,8 +1,9 @@
 package cloudresources
 
 import (
+	"context"
 	cloudcontrolv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-control/v1beta1"
-	"os"
+	"github.com/kyma-project/cloud-manager/pkg/feature"
 	"time"
 
 	cloudresourcesv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1"
@@ -28,9 +29,14 @@ var _ = Describe("Feature: SKR GcpNfsVolumeBackup", func() {
 	skrIpRangeName := "gcp-iprange-1"
 	scope := &cloudcontrolv1beta1.Scope{}
 
+	shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled := func() (bool, string) {
+		if feature.GcpNfsVolumeAutomaticLocationAllocation.Value(context.Background()) {
+			return false, ""
+		}
+		return true, "cpNfsVolumeAutomaticLocationAllocation is disabled"
+	}
+
 	BeforeEach(func() {
-		err := os.Unsetenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION")
-		Expect(err).ToNot(HaveOccurred())
 		By("Given KCP Scope exists", func() {
 
 			// Given Scope exists
@@ -173,8 +179,10 @@ var _ = Describe("Feature: SKR GcpNfsVolumeBackup", func() {
 		//Define variables.
 		gcpNfsVolumeBackup := &cloudresourcesv1beta1.GcpNfsVolumeBackup{}
 		gcpNfsVolumeBackupName := "gcp-nfs-volume-backup-1"
-		err := os.Setenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION", "true")
-		Expect(err).ToNot(HaveOccurred())
+		shouldSkip, msg := shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled()
+		if shouldSkip {
+			Skip(msg)
+		}
 		It("When GcpNfsVolumeBackup Create is called", func() {
 			Eventually(CreateGcpNfsVolumeBackup).
 				WithArguments(
@@ -211,8 +219,10 @@ var _ = Describe("Feature: SKR GcpNfsVolumeBackup", func() {
 		//Define variables.
 		gcpNfsVolumeBackup := &cloudresourcesv1beta1.GcpNfsVolumeBackup{}
 		gcpNfsVolumeBackupName := "gcp-nfs-volume-backup-2"
-		err := os.Setenv("FF_GCP_NFS_VOLUME_AUTOMATIC_LOCATION_ALLOCATION", "true")
-		Expect(err).ToNot(HaveOccurred())
+		shouldSkip, msg := shouldSkipIfGcpNfsVolumeAutomaticLocationAllocationDisabled()
+		if shouldSkip {
+			Skip(msg)
+		}
 		BeforeEach(func() {
 			By("And Given SKR GcpNfsVolumeBackup has Ready condition", func() {
 

pkg/skr/backupschedule/addFinalizer.go

@@ -4,24 +4,20 @@ import (
 	"context"
 	cloudresourcesv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1"
 	"github.com/kyma-project/cloud-manager/pkg/composed"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 func addFinalizer(ctx context.Context, st composed.State) (error, context.Context) {
 	if composed.MarkedForDeletionPredicate(ctx, st) {
 		return nil, nil
 	}
 
-	added := controllerutil.AddFinalizer(st.Obj(), cloudresourcesv1beta1.Finalizer)
-	if !added {
-		// finalizer already added
-		return nil, nil
-	}
-
-	err := st.UpdateObj(ctx)
+	modified, err := st.PatchObjAddFinalizer(ctx, cloudresourcesv1beta1.Finalizer)
 	if err != nil {
 		return composed.LogErrorAndReturn(err, "Error saving object after finalizer added", composed.StopWithRequeue, ctx)
 	}
+	if modified {
+		composed.LoggerFromCtx(ctx).Info("Finalizer added")
+	}
 
 	return nil, nil
 }

pkg/skr/backupschedule/backupImpl.go

@@ -25,7 +25,7 @@ func loadBackupImpl(ctx context.Context, st composed.State) (error, context.Cont
 	if _, ok := schedule.(*cloudresourcesv1beta1.GcpNfsBackupSchedule); ok {
 		state.backupImpl = &backupImplGcpNfs{}
 	} else {
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SetExclusiveConditions(metav1.Condition{
 				Type:    cloudresourcesv1beta1.ConditionTypeError,
 				Status:  metav1.ConditionTrue,

pkg/skr/backupschedule/calculateOnetimeSchedule.go

@@ -44,7 +44,7 @@ func calculateOnetimeSchedule(ctx context.Context, st composed.State) (error, co
 	schedule.SetState(cloudresourcesv1beta1.JobStateActive)
 	schedule.SetNextRunTimes([]string{nextRunTime.UTC().Format(time.RFC3339)})
 
-	return composed.UpdateStatus(schedule.(composed.ObjWithConditions)).
+	return composed.PatchStatus(schedule.(composed.ObjWithConditions)).
 		SuccessError(composed.StopWithRequeue).
 		Run(ctx, state)
 }

pkg/skr/backupschedule/calculateRecurringSchedule.go

@@ -58,7 +58,7 @@ func calculateRecurringSchedule(ctx context.Context, st composed.State) (error,
 	}
 	schedule.SetNextRunTimes(runtimes)
 
-	return composed.UpdateStatus(schedule).
+	return composed.PatchStatus(schedule).
 		SuccessError(composed.StopWithRequeue).
 		Run(ctx, state)
 }

pkg/skr/backupschedule/checkCompleted.go

@@ -22,7 +22,7 @@ func checkCompleted(ctx context.Context, st composed.State) (error, context.Cont
 	//If the schedule is in Done state, stop reconciliation
 	if schedule.State() == cloudresourcesv1beta1.JobStateDone {
 		logger.WithValues("GcpNfsBackupSchedule :", schedule.GetName()).Info("Schedule already completed, stopping reconciliation.")
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SuccessError(composed.StopAndForget).
 			Run(ctx, state)
 	}
@@ -34,7 +34,7 @@ func checkCompleted(ctx context.Context, st composed.State) (error, context.Cont
 		logger.WithValues("GcpNfsBackupSchedule :", schedule.GetName()).Info("Current Time is after the EndTime. Stopping reconciliation.")
 		schedule.SetState(cloudresourcesv1beta1.JobStateDone)
 		schedule.SetNextRunTimes(nil)
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SuccessError(composed.StopAndForget).
 			Run(ctx, state)
 	}
@@ -50,7 +50,7 @@ func checkCompleted(ctx context.Context, st composed.State) (error, context.Cont
 		logger.WithValues("GcpNfsBackupSchedule :", schedule.GetName()).Info("One-time schedule already ran. Stopping reconciliation.")
 		schedule.SetState(cloudresourcesv1beta1.JobStateDone)
 		schedule.SetNextRunTimes(nil)
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SuccessError(composed.StopAndForget).
 			Run(ctx, state)
 	}

pkg/skr/backupschedule/checkSuspension.go

@@ -26,7 +26,7 @@ func checkSuspension(ctx context.Context, st composed.State) (error, context.Con
 	schedule.SetState(cloudresourcesv1beta1.JobStateSuspended)
 	schedule.SetNextRunTimes(nil)
 	schedule.SetNextDeleteTimes(nil)
-	return composed.UpdateStatus(schedule).
+	return composed.PatchStatus(schedule).
 		SuccessError(composed.StopAndForget).
 		Run(ctx, state)
 }

pkg/skr/backupschedule/createBackup.go

@@ -63,7 +63,7 @@ func createBackup(ctx context.Context, st composed.State) (error, context.Contex
 
 	if err != nil {
 		schedule.SetState(cloudresourcesv1beta1.JobStateError)
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SetExclusiveConditions(metav1.Condition{
 				Type:    cloudresourcesv1beta1.ConditionTypeError,
 				Status:  metav1.ConditionTrue,
@@ -85,7 +85,7 @@ func createBackup(ctx context.Context, st composed.State) (error, context.Contex
 		Namespace: backup.GetNamespace(),
 		Name:      backup.GetName(),
 	})
-	return composed.UpdateStatus(schedule).
+	return composed.PatchStatus(schedule).
 		SetExclusiveConditions().
 		SuccessError(composed.StopWithRequeue).
 		Run(ctx, state)

pkg/skr/backupschedule/deleteBackups.go

@@ -39,7 +39,7 @@ func deleteBackups(ctx context.Context, st composed.State) (error, context.Conte
 		schedule.SetNextDeleteTimes(nil)
 		schedule.SetLastDeletedBackups(nil)
 		schedule.SetBackupCount(len(state.Backups))
-		return composed.UpdateStatus(schedule).
+		return composed.PatchStatus(schedule).
 			SetExclusiveConditions().
 			SuccessError(composed.StopWithRequeue).
 			Run(ctx, state)
@@ -78,7 +78,7 @@ func deleteBackups(ctx context.Context, st composed.State) (error, context.Conte
 	schedule.SetLastDeletedBackups(lastDeleted)
 	schedule.SetNextDeleteTimes(nextDeleteTimes)
 	schedule.SetBackupCount(len(state.Backups) - len(lastDeleted))
-	return composed.UpdateStatus(schedule).
+	return composed.PatchStatus(schedule).
 		SetExclusiveConditions().
 		SuccessError(composed.StopWithRequeue).
 		Run(ctx, state)

pkg/skr/backupschedule/deleteCascade.go

@@ -39,7 +39,7 @@ func deleteCascade(ctx context.Context, st composed.State) (error, context.Conte
 	}
 
 	schedule.SetState(cloudresourcesv1beta1.StateDeleting)
-	return composed.UpdateStatus(schedule).
+	return composed.PatchStatus(schedule).
 		SetExclusiveConditions().
 		SuccessError(composed.StopWithRequeueDelay(util.Timing.T1000ms())).
 		Run(ctx, state)