Validate desired StatefulSet before deleting it on recreate (#1420)

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)

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)

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)

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")
+}

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