Skip to content

Commit 3177660

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1009 from ConnorJC3/fix-snapshotter-rpc-spam
Discard unnecessary VolumeSnapshotContent updates to prevent rapid RPC calls
2 parents d616d72 + 35579f2 commit 3177660

File tree

3 files changed

+280
-7
lines changed

3 files changed

+280
-7
lines changed

‎pkg/sidecar-controller/snapshot_controller_base.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import (
4242
groupsnapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumegroupsnapshot/v1alpha1"
4343
snapshotlisters "github.com/kubernetes-csi/external-snapshotter/client/v7/listers/volumesnapshot/v1"
4444
"github.com/kubernetes-csi/external-snapshotter/v7/pkg/snapshotter"
45+
"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
4546
)
4647

4748
type csiSnapshotSideCarController struct {
@@ -116,14 +117,11 @@ func NewCSISnapshotSideCarController(
116117
cache.ResourceEventHandlerFuncs{
117118
AddFunc: func(obj interface{}) { ctrl.enqueueContentWork(obj) },
118119
UpdateFunc: func(oldObj, newObj interface{}) {
119-
// Considering the object is modified more than once during the workflow we are not relying on the
120-
// "AnnVolumeSnapshotBeingCreated" annotation. Instead we will just check if newobj status has error
121-
// and avoid the immediate re-queue. This allows the retry to happen with exponential backoff.
122-
newSnapContent := newObj.(*crdv1.VolumeSnapshotContent)
123-
if newSnapContent.Status != nil && newSnapContent.Status.Error != nil {
124-
return
120+
// Only enqueue updated VolumeSnapshotContent object if it contains a change that may need resync
121+
// Ignore changes that cannot necessitate a sync and/or are caused by the sidecar itself
122+
if utils.ShouldEnqueueContentChange(oldObj.(*crdv1.VolumeSnapshotContent), newObj.(*crdv1.VolumeSnapshotContent)) {
123+
ctrl.enqueueContentWork(newObj)
125124
}
126-
ctrl.enqueueContentWork(newObj)
127125
},
128126
DeleteFunc: func(obj interface{}) { ctrl.enqueueContentWork(obj) },
129127
},

‎pkg/utils/util.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"time"
2626

2727
v1 "k8s.io/api/core/v1"
28+
"k8s.io/apimachinery/pkg/api/equality"
2829
"k8s.io/apimachinery/pkg/api/meta"
2930
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3031
"k8s.io/apimachinery/pkg/util/sets"
@@ -156,6 +157,14 @@ var SnapshotterListSecretParams = secretParamsMap{
156157
secretNamespaceKey: PrefixedSnapshotterListSecretNamespaceKey,
157158
}
158159

160+
// Annotations on VolumeSnapshotContent objects entirely controlled by csi-snapshotter
161+
// Changes to these annotations will be ignored for determining whether to sync changes to content objects
162+
// AnnVolumeSnapshotBeingCreated is managed entirely by the csi-snapshotter sidecar
163+
// AnnVolumeSnapshotBeingDeleted is applied by the snapshot-controller and thus is not sidecar-owned
164+
var sidecarControlledContentAnnotations = map[string]struct{}{
165+
AnnVolumeSnapshotBeingCreated: {},
166+
}
167+
159168
// MapContainsKey checks if a given map of string to string contains the provided string.
160169
func MapContainsKey(m map[string]string, s string) bool {
161170
_, r := m[s]
@@ -593,3 +602,52 @@ func IsGroupSnapshotCreated(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) bool
593602
func GetDynamicSnapshotContentNameForGroupSnapshot(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) string {
594603
return "groupsnapcontent-" + string(groupSnapshot.UID)
595604
}
605+
606+
// ShouldEnqueueContentChange indicated whether or not a change to a VolumeSnapshotContent object
607+
// is a change that should be enqueued for sync
608+
//
609+
// The following changes are sanitized (and thus, not considered for determining whether to sync)
610+
// - Resource Version (always changed between objects)
611+
// - Status (owned and updated only by the sidecar)
612+
// - Managed Fields (updated by sidecar, and will not change the sync status)
613+
// - Finalizers (updated by sidecar, and will not change the sync status)
614+
// - Sidecar-Owned Annotations (annotations that are owned and updated only by the sidecar)
615+
// (some annotations, such as AnnVolumeSnapshotBeingDeleted, are applied by the controller - so
616+
// only annotatinons entirely controlled by the sidecar are ignored)
617+
//
618+
// If the VolumeSnapshotContent object still contains other changes after this sanitization, the changes
619+
// are potentially meaningful and the object is enqueued to be considered for syncing
620+
func ShouldEnqueueContentChange(old *crdv1.VolumeSnapshotContent, new *crdv1.VolumeSnapshotContent) bool {
621+
sanitized := new.DeepCopy()
622+
// ResourceVersion always changes between revisions
623+
sanitized.ResourceVersion = old.ResourceVersion
624+
// Fields that should not result in a sync
625+
sanitized.Status = old.Status
626+
sanitized.ManagedFields = old.ManagedFields
627+
sanitized.Finalizers = old.Finalizers
628+
// Annotations should cause a sync, except for annotations that csi-snapshotter controls
629+
if old.Annotations != nil {
630+
// This can happen if the new version has all annotations removed
631+
if sanitized.Annotations == nil {
632+
sanitized.Annotations = map[string]string{}
633+
}
634+
for annotation, _ := range sidecarControlledContentAnnotations {
635+
if value, ok := old.Annotations[annotation]; ok {
636+
sanitized.Annotations[annotation] = value
637+
} else {
638+
delete(sanitized.Annotations, annotation)
639+
}
640+
}
641+
} else {
642+
// Old content has no annotations, so delete any sidecar-controlled annotations present on the new content
643+
for annotation, _ := range sidecarControlledContentAnnotations {
644+
delete(sanitized.Annotations, annotation)
645+
}
646+
}
647+
648+
if equality.Semantic.DeepEqual(old, sanitized) {
649+
// The only changes are in the fields we don't care about, so don't enqueue for sync
650+
return false
651+
}
652+
return true
653+
}

‎pkg/utils/util_test.go

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,3 +302,220 @@ func TestIsDefaultAnnotation(t *testing.T) {
302302
}
303303
}
304304
}
305+
306+
func TestShouldEnqueueContentChange(t *testing.T) {
307+
oldValue := "old"
308+
newValue := "new"
309+
310+
testcases := []struct {
311+
name string
312+
old *crdv1.VolumeSnapshotContent
313+
new *crdv1.VolumeSnapshotContent
314+
expectedResult bool
315+
}{
316+
{
317+
name: "basic no change",
318+
old: &crdv1.VolumeSnapshotContent{},
319+
new: &crdv1.VolumeSnapshotContent{},
320+
expectedResult: false,
321+
},
322+
{
323+
name: "basic change",
324+
old: &crdv1.VolumeSnapshotContent{
325+
Spec: crdv1.VolumeSnapshotContentSpec{
326+
VolumeSnapshotClassName: &oldValue,
327+
},
328+
},
329+
new: &crdv1.VolumeSnapshotContent{
330+
Spec: crdv1.VolumeSnapshotContentSpec{
331+
VolumeSnapshotClassName: &newValue,
332+
},
333+
},
334+
expectedResult: true,
335+
},
336+
{
337+
name: "status change",
338+
old: &crdv1.VolumeSnapshotContent{
339+
Status: &crdv1.VolumeSnapshotContentStatus{
340+
Error: &crdv1.VolumeSnapshotError{
341+
Message: &oldValue,
342+
},
343+
},
344+
},
345+
new: &crdv1.VolumeSnapshotContent{
346+
Status: &crdv1.VolumeSnapshotContentStatus{
347+
Error: &crdv1.VolumeSnapshotError{
348+
Message: &newValue,
349+
},
350+
},
351+
},
352+
expectedResult: false,
353+
},
354+
{
355+
name: "finalizers change",
356+
old: &crdv1.VolumeSnapshotContent{
357+
ObjectMeta: metav1.ObjectMeta{
358+
Finalizers: []string{
359+
oldValue,
360+
},
361+
},
362+
},
363+
new: &crdv1.VolumeSnapshotContent{
364+
ObjectMeta: metav1.ObjectMeta{
365+
Finalizers: []string{
366+
newValue,
367+
},
368+
},
369+
},
370+
expectedResult: false,
371+
},
372+
{
373+
name: "managed fields change",
374+
old: &crdv1.VolumeSnapshotContent{
375+
ObjectMeta: metav1.ObjectMeta{
376+
ManagedFields: []metav1.ManagedFieldsEntry{
377+
{
378+
Manager: oldValue,
379+
},
380+
},
381+
},
382+
},
383+
new: &crdv1.VolumeSnapshotContent{
384+
ObjectMeta: metav1.ObjectMeta{
385+
ManagedFields: []metav1.ManagedFieldsEntry{
386+
{
387+
Manager: newValue,
388+
},
389+
},
390+
},
391+
},
392+
expectedResult: false,
393+
},
394+
{
395+
name: "sidecar-managed annotation change",
396+
old: &crdv1.VolumeSnapshotContent{
397+
ObjectMeta: metav1.ObjectMeta{
398+
Annotations: map[string]string{
399+
AnnVolumeSnapshotBeingCreated: oldValue,
400+
},
401+
},
402+
},
403+
new: &crdv1.VolumeSnapshotContent{
404+
ObjectMeta: metav1.ObjectMeta{
405+
Annotations: map[string]string{
406+
AnnVolumeSnapshotBeingCreated: newValue,
407+
},
408+
},
409+
},
410+
expectedResult: false,
411+
},
412+
{
413+
name: "sidecar-unmanaged annotation change",
414+
old: &crdv1.VolumeSnapshotContent{
415+
ObjectMeta: metav1.ObjectMeta{
416+
Annotations: map[string]string{
417+
"test-annotation": oldValue,
418+
},
419+
},
420+
},
421+
new: &crdv1.VolumeSnapshotContent{
422+
ObjectMeta: metav1.ObjectMeta{
423+
Annotations: map[string]string{
424+
"test-annotation": newValue,
425+
},
426+
},
427+
},
428+
expectedResult: true,
429+
},
430+
{
431+
name: "sidecar-managed annotation created",
432+
old: &crdv1.VolumeSnapshotContent{
433+
ObjectMeta: metav1.ObjectMeta{
434+
Annotations: nil,
435+
},
436+
},
437+
new: &crdv1.VolumeSnapshotContent{
438+
ObjectMeta: metav1.ObjectMeta{
439+
Annotations: map[string]string{
440+
AnnVolumeSnapshotBeingCreated: newValue,
441+
},
442+
},
443+
},
444+
expectedResult: false,
445+
},
446+
{
447+
name: "sidecar-unmanaged annotation created",
448+
old: &crdv1.VolumeSnapshotContent{
449+
ObjectMeta: metav1.ObjectMeta{
450+
Annotations: nil,
451+
},
452+
},
453+
new: &crdv1.VolumeSnapshotContent{
454+
ObjectMeta: metav1.ObjectMeta{
455+
Annotations: map[string]string{
456+
"test-annotation": newValue,
457+
},
458+
},
459+
},
460+
expectedResult: true,
461+
},
462+
{
463+
name: "sidecar-managed annotation deleted",
464+
old: &crdv1.VolumeSnapshotContent{
465+
ObjectMeta: metav1.ObjectMeta{
466+
Annotations: map[string]string{
467+
AnnVolumeSnapshotBeingCreated: oldValue,
468+
},
469+
},
470+
},
471+
new: &crdv1.VolumeSnapshotContent{
472+
ObjectMeta: metav1.ObjectMeta{
473+
Annotations: nil,
474+
},
475+
},
476+
expectedResult: false,
477+
},
478+
{
479+
name: "sidecar-unmanaged annotation deleted",
480+
old: &crdv1.VolumeSnapshotContent{
481+
ObjectMeta: metav1.ObjectMeta{
482+
Annotations: map[string]string{
483+
"test-annotation": oldValue,
484+
},
485+
},
486+
},
487+
new: &crdv1.VolumeSnapshotContent{
488+
ObjectMeta: metav1.ObjectMeta{
489+
Annotations: nil,
490+
},
491+
},
492+
expectedResult: true,
493+
},
494+
{
495+
name: "sidecar-unmanaged annotation change (AnnVolumeSnapshotBeingDeleted)",
496+
old: &crdv1.VolumeSnapshotContent{
497+
ObjectMeta: metav1.ObjectMeta{
498+
Annotations: map[string]string{
499+
AnnVolumeSnapshotBeingDeleted: oldValue,
500+
},
501+
},
502+
},
503+
new: &crdv1.VolumeSnapshotContent{
504+
ObjectMeta: metav1.ObjectMeta{
505+
Annotations: map[string]string{
506+
AnnVolumeSnapshotBeingDeleted: newValue,
507+
},
508+
},
509+
},
510+
expectedResult: true,
511+
},
512+
}
513+
for _, tc := range testcases {
514+
t.Run(tc.name, func(t *testing.T) {
515+
result := ShouldEnqueueContentChange(tc.old, tc.new)
516+
if result != tc.expectedResult {
517+
t.Fatalf("Incorrect result: Expected %v received %v", tc.expectedResult, result)
518+
}
519+
})
520+
}
521+
}

0 commit comments

Comments
 (0)