Discard unnecessary VolumeSnapshotContent updates to prevent rapid RP…

…C calls

Signed-off-by: Connor Catlett <[email protected]>

pkg/sidecar-controller/snapshot_controller_base.go

@@ -42,6 +42,7 @@ import (
 	groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1"
 	snapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/snapshotter"
+	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
 )
 
 type csiSnapshotSideCarController struct {
@@ -116,14 +117,9 @@ func NewCSISnapshotSideCarController(
 		cache.ResourceEventHandlerFuncs{
 			AddFunc: func(obj interface{}) { ctrl.enqueueContentWork(obj) },
 			UpdateFunc: func(oldObj, newObj interface{}) {
-				// Considering the object is modified more than once during the workflow we are not relying on the
-				// "AnnVolumeSnapshotBeingCreated" annotation. Instead we will just check if newobj status has error
-				// and avoid the immediate re-queue. This allows the retry to happen with exponential backoff.
-				newSnapContent := newObj.(*crdv1.VolumeSnapshotContent)
-				if newSnapContent.Status != nil && newSnapContent.Status.Error != nil {
-					return
+				if utils.ShouldEnqueueContentChange(oldObj.(*crdv1.VolumeSnapshotContent), newObj.(*crdv1.VolumeSnapshotContent)) {
+					ctrl.enqueueContentWork(newObj)
 				}
-				ctrl.enqueueContentWork(newObj)
 			},
 			DeleteFunc: func(obj interface{}) { ctrl.enqueueContentWork(obj) },
 		},

pkg/utils/util.go

@@ -25,6 +25,7 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -156,6 +157,12 @@ var SnapshotterListSecretParams = secretParamsMap{
 	secretNamespaceKey: PrefixedSnapshotterListSecretNamespaceKey,
 }
 
+// Annotations on VolumeSnapshotContent objects entirely controlled by csi-snapshotter
+// Changes to these annotations will be ignored for determining whether to sync changes to content objects
+var sidecarControlledContentAnnotations = map[string]struct{}{
+	AnnVolumeSnapshotBeingCreated: {},
+}
+
 // MapContainsKey checks if a given map of string to string contains the provided string.
 func MapContainsKey(m map[string]string, s string) bool {
 	_, r := m[s]
@@ -586,3 +593,40 @@ func IsGroupSnapshotCreated(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) bool
 func GetDynamicSnapshotContentNameForGroupSnapshot(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) string {
 	return "groupsnapcontent-" + string(groupSnapshot.UID)
 }
+
+// ShouldEnqueueContentChange indicated whether or not a change to a SnapshotContent object
+// is a change that should be enqueued for sync
+func ShouldEnqueueContentChange(old *crdv1.VolumeSnapshotContent, new *crdv1.VolumeSnapshotContent) bool {
+	sanitized := new.DeepCopy()
+	// ResourceVersion always changes between revisions
+	sanitized.ResourceVersion = old.ResourceVersion
+	// Fields that should not result in a sync
+	sanitized.Status = old.Status
+	sanitized.ManagedFields = old.ManagedFields
+	sanitized.Finalizers = old.Finalizers
+	// Annotations should cause a sync, except for annotations that csi-snapshotter controls
+	if old.Annotations != nil {
+		// This can happen if the new version has all annotations removed
+		if sanitized.Annotations == nil {
+			sanitized.Annotations = map[string]string{}
+		}
+		for annotation, _ := range sidecarControlledContentAnnotations {
+			if value, ok := old.Annotations[annotation]; ok {
+				sanitized.Annotations[annotation] = value
+			} else {
+				delete(sanitized.Annotations, annotation)
+			}
+		}
+	} else {
+		// Old content has no annotations, so delete any sidecar-controlled annotations present on the new content
+		for annotation, _ := range sidecarControlledContentAnnotations {
+			delete(sanitized.Annotations, annotation)
+		}
+	}
+
+	if equality.Semantic.DeepEqual(old, sanitized) {
+		// The only changes are in the fields we don't care about, so don't enqueue for sync
+		return false
+	}
+	return true
+}

pkg/utils/util_test.go

@@ -207,3 +207,130 @@ func TestRemovePrefixedCSIParams(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldEnqueueContentChange(t *testing.T) {
+	oldValue := "old"
+	newValue := "new"
+
+	testcases := []struct {
+		name           string
+		old            *crdv1.VolumeSnapshotContent
+		new            *crdv1.VolumeSnapshotContent
+		expectedResult bool
+	}{
+		{
+			name:           "basic no change",
+			old:            &crdv1.VolumeSnapshotContent{},
+			new:            &crdv1.VolumeSnapshotContent{},
+			expectedResult: false,
+		},
+		{
+			name: "basic change",
+			old: &crdv1.VolumeSnapshotContent{
+				Spec: crdv1.VolumeSnapshotContentSpec{
+					VolumeSnapshotClassName: &oldValue,
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				Spec: crdv1.VolumeSnapshotContentSpec{
+					VolumeSnapshotClassName: &newValue,
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "status change",
+			old: &crdv1.VolumeSnapshotContent{
+				Status: &crdv1.VolumeSnapshotContentStatus{
+					Error: &crdv1.VolumeSnapshotError{
+						Message: &oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				Status: &crdv1.VolumeSnapshotContentStatus{
+					Error: &crdv1.VolumeSnapshotError{
+						Message: &newValue,
+					},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "managed annotation change",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingCreated: oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingCreated: newValue,
+					},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "unmanaged annotation change",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"test-annotation": oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"test-annotation": newValue,
+					},
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "annotation created",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: nil,
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"test-annotation": newValue,
+					},
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "annotation deleted",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingCreated: oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: nil,
+				},
+			},
+			expectedResult: false,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := ShouldEnqueueContentChange(tc.old, tc.new)
+			if result != tc.expectedResult {
+				t.Fatalf("Incorrect result: Expected %v received %v", tc.expectedResult, result)
+			}
+		})
+	}
+}