diff --git a/pkg/sidecar-controller/snapshot_controller_base.go b/pkg/sidecar-controller/snapshot_controller_base.go index 33342a3a..640bba6e 100644 --- a/pkg/sidecar-controller/snapshot_controller_base.go +++ b/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 - // "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 + // Only enqueue updated VolumeSnapshotContent object if it contains a change that may need resync + // Ignore changes that cannot necessitate a sync and/or are caused by the sidecar itself + if utils.ShouldEnqueueContentChange(oldObj.(*crdv1.VolumeSnapshotContent), newObj.(*crdv1.VolumeSnapshotContent)) { + ctrl.enqueueContentWork(newObj) } - ctrl.enqueueContentWork(newObj) }, DeleteFunc: func(obj interface{}) { ctrl.enqueueContentWork(obj) }, }, diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 26f4ba77..3a778c5e 100644 --- a/pkg/utils/util.go +++ b/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 +} diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go index fd2127fa..701f265f 100644 --- a/pkg/utils/util_test.go +++ b/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) + } + }) + } +}