diff --git a/pkg/controller/chi/kube/statesfulset.go b/pkg/controller/chi/kube/statesfulset.go index dec5d784a..b654a7cb8 100644 --- a/pkg/controller/chi/kube/statesfulset.go +++ b/pkg/controller/chi/kube/statesfulset.go @@ -80,6 +80,15 @@ func (c *STS) Create(ctx context.Context, statefulSet *apps.StatefulSet) (*apps. return c.kubeClient.AppsV1().StatefulSets(statefulSet.Namespace).Create(ctx, statefulSet, controller.NewCreateOptions()) } +// ValidateCreate runs a server-side dry-run create: validation runs, nothing is persisted. +func (c *STS) ValidateCreate(ctx context.Context, statefulSet *apps.StatefulSet) error { + ctx = k8sCtx(ctx) + opts := controller.NewCreateOptions() + opts.DryRun = []string{meta.DryRunAll} + _, err := c.kubeClient.AppsV1().StatefulSets(statefulSet.Namespace).Create(ctx, statefulSet, opts) + return err +} + // Update is an internal function, used in reconcileStatefulSet only func (c *STS) Update(ctx context.Context, sts *apps.StatefulSet) (*apps.StatefulSet, error) { ctx = k8sCtx(ctx) diff --git a/pkg/controller/chk/kube/statesfulset.go b/pkg/controller/chk/kube/statesfulset.go index d6916b064..bf67052f7 100644 --- a/pkg/controller/chk/kube/statesfulset.go +++ b/pkg/controller/chk/kube/statesfulset.go @@ -107,6 +107,11 @@ func (c *STS) Create(ctx context.Context, sts *apps.StatefulSet) (*apps.Stateful return sts, err } +// ValidateCreate runs a server-side dry-run create: validation runs, nothing is persisted. +func (c *STS) ValidateCreate(ctx context.Context, sts *apps.StatefulSet) error { + return c.kubeClient.Create(ctx, sts.DeepCopy(), client.DryRunAll) +} + func (c *STS) Update(ctx context.Context, sts *apps.StatefulSet) (*apps.StatefulSet, error) { log.V(3).M(sts).Info("Going to update STS: %s", util.NamespaceNameString(sts)) err := c.kubeClient.Update(ctx, sts) diff --git a/pkg/controller/common/statefulset/statefulset-reconciler.go b/pkg/controller/common/statefulset/statefulset-reconciler.go index 9d1596aa8..5f9730021 100644 --- a/pkg/controller/common/statefulset/statefulset-reconciler.go +++ b/pkg/controller/common/statefulset/statefulset-reconciler.go @@ -192,6 +192,25 @@ func (r *Reconciler) recreateStatefulSet(ctx context.Context, host *api.Host, re r.a.V(2).M(host).S().Info(util.NamespaceNameString(host.GetCR())) defer r.a.V(2).M(host).E().Info(util.NamespaceNameString(host.GetCR())) + // Recreate deletes the existing StatefulSet before creating the desired one, so a desired spec + // the API server rejects would leave the host with no StatefulSet (#1420). Dry-run the desired + // StatefulSet first and, if it is rejected on validation, return without deleting the existing + // one. The current StatefulSet is still present at this point, so a dry-run create reports + // AlreadyExists once the spec is valid; that is the normal recreate precondition, not a + // validation failure, so let the recreate proceed in that case. + if err := r.sts.ValidateCreate(ctx, host.Runtime.DesiredStatefulSet); err != nil && !apiErrors.IsAlreadyExists(err) { + namespace := host.Runtime.Address.Namespace + name := r.namer.Name(interfaces.NameStatefulSet, host) + r.a.V(1). + WithEvent(host.GetCR(), a.EventActionUpdate, a.EventReasonUpdateFailed). + WithAction(host.GetCR()). + WithError(host.GetCR()). + M(host).F(). + Warning("Recreate aborted: desired StatefulSet is invalid, keeping existing one %s/%s", namespace, name) + log.V(1).M(host).F().Error("Recreate aborted: desired StatefulSet dry-run rejected, keeping existing StatefulSet %s/%s err: %v", namespace, name, err) + return err + } + if err := r.doDeleteStatefulSet(ctx, host); err != nil { namespace := host.Runtime.Address.Namespace name := r.namer.Name(interfaces.NameStatefulSet, host) diff --git a/pkg/controller/common/statefulset/statefulset-reconciler_test.go b/pkg/controller/common/statefulset/statefulset-reconciler_test.go index 6dc7f4a43..5267acf7f 100644 --- a/pkg/controller/common/statefulset/statefulset-reconciler_test.go +++ b/pkg/controller/common/statefulset/statefulset-reconciler_test.go @@ -48,17 +48,19 @@ func minimalCR(namespace, name string) *api.ClickHouseInstallation { // fakeSTS is a minimal IKubeSTS test double recording every call and returning // injected results so each test scenario can exercise a specific code path. type fakeSTS struct { - getCalls int - createCalls int - updateCalls int - deleteCalls int - - getReturn *apps.StatefulSet - getErr error - createErr error - updateErr error - deleteErr error - updateReturn *apps.StatefulSet + getCalls int + createCalls int + validateCreateCalls int + updateCalls int + deleteCalls int + + getReturn *apps.StatefulSet + getErr error + createErr error + validateCreateErr error + updateErr error + deleteErr error + updateReturn *apps.StatefulSet lastDeleteNamespace string lastDeleteName string @@ -77,6 +79,11 @@ func (f *fakeSTS) Create(ctx context.Context, sts *apps.StatefulSet) (*apps.Stat return sts, nil } +func (f *fakeSTS) ValidateCreate(ctx context.Context, sts *apps.StatefulSet) error { + f.validateCreateCalls++ + return f.validateCreateErr +} + func (f *fakeSTS) Update(ctx context.Context, sts *apps.StatefulSet) (*apps.StatefulSet, error) { f.updateCalls++ if f.updateErr != nil { @@ -289,6 +296,7 @@ func TestRecreateStatefulSet_HappyPath(t *testing.T) { err := r.recreateStatefulSet(context.Background(), h, false /*register*/, NewReconcileStatefulSetOptions()) require.NoError(t, err) + assert.Equal(t, 1, fake.validateCreateCalls, "desired StatefulSet must be dry-run validated before delete") assert.Equal(t, 1, fake.deleteCalls, "delete should be invoked once") assert.Equal(t, 1, fake.createCalls, "create should follow a successful delete") } @@ -341,3 +349,46 @@ func TestCreateStatefulSet_AlreadyExistsPropagatesAsRecreate(t *testing.T) { "createStatefulSet must propagate ErrCRUDRecreate so the caller retries on the next reconcile pass") assert.Equal(t, 1, fake.createCalls, "Create should be attempted exactly once") } + +// TestRecreateStatefulSet_InvalidDesiredSkipsDelete: when the dry-run rejects the desired +// StatefulSet, recreate returns the error and does not delete the existing one (#1420). +func TestRecreateStatefulSet_InvalidDesiredSkipsDelete(t *testing.T) { + validateErr := errors.New(`StatefulSet is invalid: spec.template.labels: Invalid value: "/metrics"`) + fake := &fakeSTS{ + getReturn: stsWithReplicas(int32Ptr(3)), + validateCreateErr: validateErr, + } + r := newReconciler(fake, "chi-test-cluster-0-0") + + h := hostWithCR("ns", "test-chi") + h.Runtime.DesiredStatefulSet = stsWithReplicas(int32Ptr(1)) + + err := r.recreateStatefulSet(context.Background(), h, false /*register*/, NewReconcileStatefulSetOptions()) + + require.Error(t, err, "an invalid desired StatefulSet must fail the recreate") + assert.Equal(t, validateErr, err, "the dry-run validation error must propagate verbatim") + assert.Equal(t, 1, fake.validateCreateCalls, "ValidateCreate must run once, before any delete") + assert.Equal(t, 0, fake.deleteCalls, "the running StatefulSet must NOT be deleted when the desired spec is invalid") + assert.Equal(t, 0, fake.createCalls, "no create should be attempted when validation fails") +} + +// TestRecreateStatefulSet_AlreadyExistsFromDryRunProceeds: the existing StatefulSet is still +// present when recreate runs, so a dry-run create of a valid desired spec reports AlreadyExists. +// That is the normal recreate precondition, not a validation failure, so the recreate must +// proceed (delete then create) rather than abort. +func TestRecreateStatefulSet_AlreadyExistsFromDryRunProceeds(t *testing.T) { + fake := &fakeSTS{ + getReturn: stsWithReplicas(int32Ptr(0)), + validateCreateErr: apiErrors.NewAlreadyExists(stsResource, "chi-test-cluster-0-0"), + } + r := newReconciler(fake, "chi-test-cluster-0-0") + h := hostWithCR("ns", "test-chi") + h.Runtime.DesiredStatefulSet = stsWithReplicas(int32Ptr(1)) + + err := r.recreateStatefulSet(context.Background(), h, false /*register*/, NewReconcileStatefulSetOptions()) + + require.NoError(t, err, "AlreadyExists from the dry-run must not abort a valid recreate") + assert.Equal(t, 1, fake.validateCreateCalls, "ValidateCreate must run once") + assert.Equal(t, 1, fake.deleteCalls, "the existing StatefulSet must be deleted so the recreate proceeds") + assert.Equal(t, 1, fake.createCalls, "the desired StatefulSet must be created") +} diff --git a/pkg/interfaces/interfaces-kube.go b/pkg/interfaces/interfaces-kube.go index b22b4aeba..8e9dad6a8 100644 --- a/pkg/interfaces/interfaces-kube.go +++ b/pkg/interfaces/interfaces-kube.go @@ -118,6 +118,9 @@ type IKubeService interface { type IKubeSTS interface { Get(ctx context.Context, params ...any) (*apps.StatefulSet, error) Create(ctx context.Context, statefulSet *apps.StatefulSet) (*apps.StatefulSet, error) + // ValidateCreate runs a server-side dry-run create and returns the API server's validation + // error (nil if the object would be accepted); nothing is persisted. + ValidateCreate(ctx context.Context, statefulSet *apps.StatefulSet) error Update(ctx context.Context, sts *apps.StatefulSet) (*apps.StatefulSet, error) // Delete removes the StatefulSet and MUST block until it is fully gone from the API server, // i.e. until a subsequent Get returns IsNotFound. Implementations are expected to poll until