Merge pull request #1009 from ConnorJC3/fix-snapshotter-rpc-spam

Discard unnecessary VolumeSnapshotContent updates to prevent rapid RPC calls

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,11 @@ 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
+				// Only enqueue updated VolumeSnapshotContent object if it contains a change that may need resync
-				// "AnnVolumeSnapshotBeingCreated" annotation. Instead we will just check if newobj status has error
+				// Ignore changes that cannot necessitate a sync and/or are caused by the sidecar itself
-				// and avoid the immediate re-queue. This allows the retry to happen with exponential backoff.
+				if utils.ShouldEnqueueContentChange(oldObj.(*crdv1.VolumeSnapshotContent), newObj.(*crdv1.VolumeSnapshotContent)) {
-				newSnapContent := newObj.(*crdv1.VolumeSnapshotContent)
-				if newSnapContent.Status != nil && newSnapContent.Status.Error != nil {
-					return
-				}
 					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,14 @@ 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
+// AnnVolumeSnapshotBeingCreated is managed entirely by the csi-snapshotter sidecar
+// AnnVolumeSnapshotBeingDeleted is applied by the snapshot-controller and thus is not sidecar-owned
+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]
@@ -593,3 +602,52 @@ 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 VolumeSnapshotContent object
+// is a change that should be enqueued for sync
+//
+// The following changes are sanitized (and thus, not considered for determining whether to sync)
+//   - Resource Version (always changed between objects)
+//   - Status (owned and updated only by the sidecar)
+//   - Managed Fields (updated by sidecar, and will not change the sync status)
+//   - Finalizers (updated by sidecar, and will not change the sync status)
+//   - Sidecar-Owned Annotations (annotations that are owned and updated only by the sidecar)
+//     (some annotations, such as AnnVolumeSnapshotBeingDeleted, are applied by the controller - so
+//     only annotatinons entirely controlled by the sidecar are ignored)
+//
+// If the VolumeSnapshotContent object still contains other changes after this sanitization, the changes
+// are potentially meaningful and the object is enqueued to be considered for syncing
+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

@@ -302,3 +302,220 @@ func TestIsDefaultAnnotation(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: "finalizers change",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{
+						oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{
+						newValue,
+					},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "managed fields change",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					ManagedFields: []metav1.ManagedFieldsEntry{
+						{
+							Manager: oldValue,
+						},
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					ManagedFields: []metav1.ManagedFieldsEntry{
+						{
+							Manager: newValue,
+						},
+					},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "sidecar-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: "sidecar-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: "sidecar-managed annotation created",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: nil,
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingCreated: newValue,
+					},
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "sidecar-unmanaged 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: "sidecar-managed annotation deleted",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingCreated: oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: nil,
+				},
+			},
+			expectedResult: false,
+		},
+		{
+			name: "sidecar-unmanaged annotation deleted",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"test-annotation": oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: nil,
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "sidecar-unmanaged annotation change (AnnVolumeSnapshotBeingDeleted)",
+			old: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingDeleted: oldValue,
+					},
+				},
+			},
+			new: &crdv1.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						AnnVolumeSnapshotBeingDeleted: newValue,
+					},
+				},
+			},
+			expectedResult: true,
+		},
+	}
+	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)
+			}
+		})
+	}
+}