From 32b1c43182c08e14944d8dc561fab71d48e0b324 Mon Sep 17 00:00:00 2001 From: Maciej Zimnoch Date: Wed, 13 Dec 2023 19:23:08 +0100 Subject: [PATCH] Support update of StatefulSet selector When Pods managed by existing StatefulSet matches new selector, StatefulSet is orphan deleted and recreated. --- pkg/resourceapply/apps.go | 36 +++++++++++-- pkg/resourceapply/apps_test.go | 98 +++++++++++++++++++++++++++++++++- pkg/resourceapply/batch.go | 15 +++--- pkg/resourceapply/helpers.go | 15 ++++-- pkg/resourceapply/rbac.go | 13 ++--- 5 files changed, 155 insertions(+), 22 deletions(-) diff --git a/pkg/resourceapply/apps.go b/pkg/resourceapply/apps.go index 820a2910027..b6949a23ce2 100644 --- a/pkg/resourceapply/apps.go +++ b/pkg/resourceapply/apps.go @@ -2,9 +2,13 @@ package resourceapply import ( "context" + "fmt" + "github.com/scylladb/scylla-operator/pkg/pointer" appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" appsv1client "k8s.io/client-go/kubernetes/typed/apps/v1" appsv1listers "k8s.io/client-go/listers/apps/v1" "k8s.io/client-go/tools/record" @@ -17,7 +21,31 @@ func ApplyStatefulSetWithControl( required *appsv1.StatefulSet, options ApplyOptions, ) (*appsv1.StatefulSet, bool, error) { - return ApplyGeneric[*appsv1.StatefulSet](ctx, control, recorder, required, options) + return ApplyGenericWithHandlers[*appsv1.StatefulSet]( + ctx, + control, + recorder, + required, + options, + nil, + func(required *appsv1.StatefulSet, existing *appsv1.StatefulSet) (string, *metav1.DeletionPropagation, error) { + if !equality.Semantic.DeepEqual(existing.Spec.Selector, required.Spec.Selector) { + existingPodLabels := existing.Spec.Template.Labels + requiredSelector, err := metav1.LabelSelectorAsSelector(required.Spec.Selector) + if err != nil { + return "", nil, fmt.Errorf("can't parse required StatefulSet selector: %w", err) + } + + if !requiredSelector.Matches(labels.Set(existingPodLabels)) { + return "", nil, fmt.Errorf("required StatefulSet selector %q doesn't match existing Pod Labels set %v", requiredSelector, existingPodLabels) + } + + return "spec.selector is immutable", pointer.Ptr(metav1.DeletePropagationOrphan), nil + } + + return "", nil, nil + }, + ) } func ApplyStatefulSet( @@ -56,11 +84,11 @@ func ApplyDaemonSetWithControl( required, options, nil, - func(required *appsv1.DaemonSet, existing *appsv1.DaemonSet) string { + func(required *appsv1.DaemonSet, existing *appsv1.DaemonSet) (string, *metav1.DeletionPropagation, error) { if !equality.Semantic.DeepEqual(existing.Spec.Selector, required.Spec.Selector) { - return "spec.selector is immutable" + return "spec.selector is immutable", nil, nil } - return "" + return "", nil, nil }, ) } diff --git a/pkg/resourceapply/apps_test.go b/pkg/resourceapply/apps_test.go index a52d23b0edf..a4c107fd3e7 100644 --- a/pkg/resourceapply/apps_test.go +++ b/pkg/resourceapply/apps_test.go @@ -46,8 +46,17 @@ func TestApplyStatefulSet(t *testing.T) { }, Spec: appsv1.StatefulSetSpec{ Replicas: pointer.Ptr(int32(3)), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + }, Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "foo": "bar", + }, + }, Spec: corev1.PodSpec{ Containers: []corev1.Container{ { @@ -493,6 +502,93 @@ func TestApplyStatefulSet(t *testing.T) { expectedErr: nil, expectedEvents: []string{"Normal StatefulSetUpdated StatefulSet default/test updated"}, }, + { + name: "deletes and creates the StatefulSet when selector is changed and still matches the old pods", + existing: []runtime.Object{ + func() *appsv1.StatefulSet { + sts := newSts() + sts.Spec.Template.Labels = map[string]string{ + "foo": "bar", + "bar": "foo", + } + sts.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + "bar": "foo", + }, + } + return sts + }(), + }, + required: func() *appsv1.StatefulSet { + sts := newSts() + sts.Spec.Template.Labels = map[string]string{ + "foo": "bar", + "bar": "foo", + } + sts.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + } + return sts + }(), + expectedSts: func() *appsv1.StatefulSet { + sts := newSts() + sts.Spec.Template.Labels = map[string]string{ + "foo": "bar", + "bar": "foo", + } + sts.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + } + utilruntime.Must(SetHashAnnotation(sts)) + return sts + }(), + expectedChanged: true, + expectedErr: nil, + expectedEvents: []string{ + "Normal StatefulSetDeleted StatefulSet default/test deleted", + "Normal StatefulSetCreated StatefulSet default/test created", + }, + }, + { + name: "apply fails when StatefulSet selector differs and existing Pod labels doesn't match new selector", + existing: []runtime.Object{ + func() *appsv1.StatefulSet { + sts := newSts() + sts.Spec.Template.Labels = map[string]string{ + "foo": "bar", + } + sts.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + } + return sts + }(), + }, + required: func() *appsv1.StatefulSet { + sts := newSts() + sts.Spec.Template.Labels = map[string]string{ + "foo": "bar", + "bar": "foo", + } + sts.Spec.Selector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + "bar": "foo", + }, + } + return sts + }(), + expectedSts: nil, + expectedChanged: false, + expectedErr: fmt.Errorf("can't get recreate reason: %w", fmt.Errorf(`required StatefulSet selector "bar=foo,foo=bar" doesn't match existing Pod Labels set map[foo:bar]`)), + expectedEvents: nil, + }, } for _, tc := range tt { diff --git a/pkg/resourceapply/batch.go b/pkg/resourceapply/batch.go index 38f6a5572e7..264f8b41133 100644 --- a/pkg/resourceapply/batch.go +++ b/pkg/resourceapply/batch.go @@ -5,6 +5,7 @@ import ( batchv1 "k8s.io/api/batch/v1" "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" batchv1client "k8s.io/client-go/kubernetes/typed/batch/v1" batchv1listers "k8s.io/client-go/listers/batch/v1" "k8s.io/client-go/tools/record" @@ -24,23 +25,23 @@ func ApplyJobWithControl( required, options, nil, - func(required *batchv1.Job, existing *batchv1.Job) string { + func(required *batchv1.Job, existing *batchv1.Job) (string, *metav1.DeletionPropagation, error) { if !equality.Semantic.DeepEqual(existing.Spec.Completions, required.Spec.Completions) { - return "spec.completions is immutable" + return "spec.completions is immutable", nil, nil } if !equality.Semantic.DeepEqual(existing.Spec.Selector, required.Spec.Selector) { - return "spec.selector is immutable" + return "spec.selector is immutable", nil, nil } if !equality.Semantic.DeepEqual(existing.Spec.Template, required.Spec.Template) { - return "spec.template is immutable" + return "spec.template is immutable", nil, nil } if !equality.Semantic.DeepEqual(existing.Spec.CompletionMode, required.Spec.CompletionMode) { - return "spec.completionMode is immutable" + return "spec.completionMode is immutable", nil, nil } if !equality.Semantic.DeepEqual(existing.Spec.PodFailurePolicy, required.Spec.PodFailurePolicy) { - return "spec.podFailurePolicy is immutable" + return "spec.podFailurePolicy is immutable", nil, nil } - return "" + return "", nil, nil }, ) } diff --git a/pkg/resourceapply/helpers.go b/pkg/resourceapply/helpers.go index 2de8fedb3cf..f3a3ff995a9 100644 --- a/pkg/resourceapply/helpers.go +++ b/pkg/resourceapply/helpers.go @@ -244,7 +244,7 @@ func ApplyGenericWithHandlers[T kubeinterfaces.ObjectInterface]( required T, options ApplyOptions, projectFunc func(required *T, existing T), - getRecreateReasonFunc func(required T, existing T) string, + getRecreateReasonFunc func(required T, existing T) (string, *metav1.DeletionPropagation, error), ) (T, bool, error) { gvk := resource.GetObjectGVKOrUnknown(required) @@ -315,8 +315,12 @@ func ApplyGenericWithHandlers[T kubeinterfaces.ObjectInterface]( } var recreateReason string + var propagationPolicy *metav1.DeletionPropagation if getRecreateReasonFunc != nil { - recreateReason = getRecreateReasonFunc(requiredCopy, existing) + recreateReason, propagationPolicy, err = getRecreateReasonFunc(requiredCopy, existing) + if err != nil { + return *new(T), false, fmt.Errorf("can't get recreate reason: %w", err) + } } if len(recreateReason) > 0 { klog.V(2).InfoS( @@ -326,9 +330,12 @@ func ApplyGenericWithHandlers[T kubeinterfaces.ObjectInterface]( "Ref", naming.ObjRefWithUID(existing), ) - propagationPolicy := metav1.DeletePropagationBackground + if propagationPolicy == nil { + propagationPolicy = pointer.Ptr(metav1.DeletePropagationBackground) + } + err := control.Delete(ctx, existing.GetName(), metav1.DeleteOptions{ - PropagationPolicy: &propagationPolicy, + PropagationPolicy: propagationPolicy, }) ReportDeleteEvent(recorder, existing, err) if err != nil { diff --git a/pkg/resourceapply/rbac.go b/pkg/resourceapply/rbac.go index c24c16fac01..79f4e588d49 100644 --- a/pkg/resourceapply/rbac.go +++ b/pkg/resourceapply/rbac.go @@ -5,6 +5,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" rbacv1client "k8s.io/client-go/kubernetes/typed/rbac/v1" rbacv1listers "k8s.io/client-go/listers/rbac/v1" "k8s.io/client-go/tools/record" @@ -56,11 +57,11 @@ func ApplyRoleBindingWithControl( required, options, nil, - func(required *rbacv1.RoleBinding, existing *rbacv1.RoleBinding) string { + func(required *rbacv1.RoleBinding, existing *rbacv1.RoleBinding) (string, *metav1.DeletionPropagation, error) { if !equality.Semantic.DeepEqual(existing.RoleRef, (*required).RoleRef) { - return "roleRef is immutable" + return "roleRef is immutable", nil, nil } - return "" + return "", nil, nil }, ) } @@ -101,11 +102,11 @@ func ApplyClusterRoleBindingWithControl( required, options, nil, - func(required *rbacv1.ClusterRoleBinding, existing *rbacv1.ClusterRoleBinding) string { + func(required *rbacv1.ClusterRoleBinding, existing *rbacv1.ClusterRoleBinding) (string, *metav1.DeletionPropagation, error) { if !equality.Semantic.DeepEqual(existing.RoleRef, (*required).RoleRef) { - return "roleRef is immutable" + return "roleRef is immutable", nil, nil } - return "" + return "", nil, nil }, ) }