From b455a1a567693490f427917ed48a9e57624de3c5 Mon Sep 17 00:00:00 2001 From: atbagan Date: Sun, 30 Aug 2026 14:32:43 -0500 Subject: [PATCH 1/2] fix(controller): omit control plane replicas when the tenant omits them The StewardControlPlane builder hardcoded a replicas fallback of 1 and always emitted spec.replicas, which overrode the provider's own default for any TenantCluster that did not set control plane replicas. Emit spec.replicas only when the TenantCluster explicitly sets it (nil stays unset downstream), so Steward / capi-steward applies its own default. The StewardControlPlane drift reconciler is updated the same way. Requires the pointer-typed ControlPlaneSpec.Replicas from butler-api. --- go.mod | 2 +- go.sum | 4 +- internal/capi/builder.go | 14 ++--- internal/capi/builder_test.go | 53 ++++++++++++++++++- .../tenantcluster/reconcile_scaling.go | 17 +++--- .../tenantcluster/tenantcluster_scp_test.go | 32 +++++++---- 6 files changed, 92 insertions(+), 30 deletions(-) diff --git a/go.mod b/go.mod index b5134e4..f964b03 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/butlerdotdev/butler-controller go 1.24.6 require ( - github.com/butlerdotdev/butler-api v0.22.0 + github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce github.com/onsi/ginkgo/v2 v2.22.0 github.com/onsi/gomega v1.36.1 github.com/prometheus/client_golang v1.22.0 diff --git a/go.sum b/go.sum index 8ac96a6..03041cf 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,7 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/butlerdotdev/butler-api v0.22.0 h1:NbojKU2xDeNal1Jp3Wc2BJiysNGTnDgF8cywf2Tklak= -github.com/butlerdotdev/butler-api v0.22.0/go.mod h1:q/RjPFM/r2ELq4DqR78OiAxerKBvH5D4nTGsVLKbjqY= +github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce h1:wDhJkQePpJ08M+lEAA+PLhXcx8rqo6PPrLHlwN0SCYY= +github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce/go.mod h1:q/RjPFM/r2ELq4DqR78OiAxerKBvH5D4nTGsVLKbjqY= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= diff --git a/internal/capi/builder.go b/internal/capi/builder.go index 7359b02..7420e08 100644 --- a/internal/capi/builder.go +++ b/internal/capi/builder.go @@ -398,12 +398,6 @@ func (b *Builder) buildStewardControlPlane(name string) *unstructured.Unstructur kcp.SetNamespace(b.namespace) b.applyCommonMetadata(kcp) - // Get control plane replicas from spec - replicas := int64(1) - if b.tc.Spec.ControlPlane.Replicas > 0 { - replicas = int64(b.tc.Spec.ControlPlane.Replicas) - } - // Get datastore name dataStoreName := "default" if b.tc.Spec.ControlPlane.DataStoreRef != nil && b.tc.Spec.ControlPlane.DataStoreRef.Name != "" { @@ -515,7 +509,6 @@ func (b *Builder) buildStewardControlPlane(name string) *unstructured.Unstructur // See: https://steward.butlerlabs.dev/cluster-api/kamaji-control-plane-provider/ spec := map[string]interface{}{ "version": b.tc.Spec.KubernetesVersion, - "replicas": replicas, "dataStoreName": dataStoreName, "addons": addons, @@ -533,6 +526,13 @@ func (b *Builder) buildStewardControlPlane(name string) *unstructured.Unstructur "network": network, } + // Control plane replicas: preserve provider ownership. When the tenant + // omits replicas (nil), leave the field unset downstream so Steward / + // capi-steward applies its own default. Only set it when explicit. + if b.tc.Spec.ControlPlane.Replicas != nil { + spec["replicas"] = int64(*b.tc.Spec.ControlPlane.Replicas) + } + // Add control plane resources if configured (ButlerConfig defaults + TenantCluster overrides) resources := b.resolveControlPlaneResources() if resources != nil { diff --git a/internal/capi/builder_test.go b/internal/capi/builder_test.go index f0d12d2..7687da0 100644 --- a/internal/capi/builder_test.go +++ b/internal/capi/builder_test.go @@ -256,14 +256,63 @@ func TestBuildStewardControlPlane(t *testing.T) { if spec["version"] != "v1.30.2" { t.Errorf("expected version 'v1.30.2', got '%v'", spec["version"]) } - if spec["replicas"].(int64) != 1 { - t.Errorf("expected replicas 1, got '%v'", spec["replicas"]) + // Replicas omitted on the TenantCluster must be omitted downstream so the + // provider (Steward / capi-steward) applies its own default. + if v, ok := spec["replicas"]; ok { + t.Errorf("expected replicas to be omitted when unset on the TenantCluster, got '%v'", v) } if spec["dataStoreName"] != "default" { t.Errorf("expected dataStoreName 'default', got '%v'", spec["dataStoreName"]) } } +// TestBuildStewardControlPlane_Replicas proves the provider-owned replicas +// contract at the point where Butler builds the StewardControlPlane spec: +// an omitted (nil) value is omitted downstream so the provider chooses its +// own default; explicit values are copied exactly; and an explicit zero is +// preserved (never synthesized from nil). +func TestBuildStewardControlPlane_Replicas(t *testing.T) { + ptrI32 := func(v int32) *int32 { return &v } + pc := newTestProviderConfig("harvester") + + scpSpec := func(t *testing.T, r *int32) map[string]interface{} { + t.Helper() + tc := newTestTenantCluster("cp-replicas", "default") + tc.Spec.ControlPlane.Replicas = r + rs, err := NewBuilder(tc, pc, "cp-replicas-12345678").Build() + if err != nil { + t.Fatalf("build: %v", err) + } + return rs.ControlPlane.Object["spec"].(map[string]interface{}) + } + + t.Run("omitted is omitted downstream", func(t *testing.T) { + if v, ok := scpSpec(t, nil)["replicas"]; ok { + t.Fatalf("replicas must be omitted when unset, got %v", v) + } + }) + + for _, n := range []int32{1, 2, 3} { + n := n + t.Run("explicit preserved", func(t *testing.T) { + got, ok := scpSpec(t, ptrI32(n))["replicas"] + if !ok { + t.Fatalf("replicas %d must be present", n) + } + if got.(int64) != int64(n) { + t.Fatalf("replicas = %v, want %d", got, n) + } + }) + } + + t.Run("explicit zero preserved, never from nil", func(t *testing.T) { + got, ok := scpSpec(t, ptrI32(0))["replicas"] + if !ok || got.(int64) != 0 { + t.Fatalf("explicit zero must emit 0, got present=%v val=%v", ok, got) + } + }) +} + func TestBuildMachineDeployment(t *testing.T) { tc := newTestTenantCluster("test-cluster", "default") tc.Spec.Workers.Replicas = 3 diff --git a/internal/controller/tenantcluster/reconcile_scaling.go b/internal/controller/tenantcluster/reconcile_scaling.go index 05d3b21..851c77e 100644 --- a/internal/controller/tenantcluster/reconcile_scaling.go +++ b/internal/controller/tenantcluster/reconcile_scaling.go @@ -343,13 +343,16 @@ func (r *Reconciler) reconcileStewardControlPlane(ctx context.Context, tc *butle changes = append(changes, fmt.Sprintf("version %s->%s", currentVersion, tc.Spec.KubernetesVersion)) } - // Replicas drift. Only patch if TC explicitly sets replicas (> 0). - // SCP defaults to 2 via kubebuilder; patching 0 -> 1 would fight the default. - currentReplicas, _, _ := unstructured.NestedInt64(scp.Object, "spec", "replicas") - desiredReplicas := int64(tc.Spec.ControlPlane.Replicas) - if desiredReplicas > 0 && desiredReplicas != currentReplicas { - specPatch["replicas"] = desiredReplicas - changes = append(changes, fmt.Sprintf("replicas %d->%d", currentReplicas, desiredReplicas)) + // Replicas drift. Only patch if the TenantCluster explicitly sets replicas. + // When it is omitted (nil), leave the SCP replicas alone so the provider + // default is preserved; patching it would fight that default. + if tc.Spec.ControlPlane.Replicas != nil { + currentReplicas, _, _ := unstructured.NestedInt64(scp.Object, "spec", "replicas") + desiredReplicas := int64(*tc.Spec.ControlPlane.Replicas) + if desiredReplicas != currentReplicas { + specPatch["replicas"] = desiredReplicas + changes = append(changes, fmt.Sprintf("replicas %d->%d", currentReplicas, desiredReplicas)) + } } // CP resource drift diff --git a/internal/controller/tenantcluster/tenantcluster_scp_test.go b/internal/controller/tenantcluster/tenantcluster_scp_test.go index 8d7c1d0..85e3632 100644 --- a/internal/controller/tenantcluster/tenantcluster_scp_test.go +++ b/internal/controller/tenantcluster/tenantcluster_scp_test.go @@ -134,7 +134,7 @@ func TestReconcileStewardControlPlane_PatchContent(t *testing.T) { tests := []struct { name string tcVersion string - tcReplicas int32 + tcReplicas *int32 tcResources *butlerv1alpha1.ControlPlaneResourcesSpec scpVersion string scpReplicas int64 @@ -148,15 +148,23 @@ func TestReconcileStewardControlPlane_PatchContent(t *testing.T) { { name: "no drift produces no patch", tcVersion: "v1.31.0", - tcReplicas: 1, + tcReplicas: int32Ptr(1), scpVersion: "v1.31.0", scpReplicas: 1, wantNoPatch: true, }, + { + name: "omitted replicas never patched (provider owns default)", + tcVersion: "v1.31.0", + tcReplicas: nil, + scpVersion: "v1.31.0", + scpReplicas: 2, + wantNoPatch: true, + }, { name: "version drift", tcVersion: "v1.32.0", - tcReplicas: 1, + tcReplicas: int32Ptr(1), scpVersion: "v1.31.0", scpReplicas: 1, wantVersion: "v1.32.0", @@ -164,7 +172,7 @@ func TestReconcileStewardControlPlane_PatchContent(t *testing.T) { { name: "replicas drift", tcVersion: "v1.31.0", - tcReplicas: 3, + tcReplicas: int32Ptr(3), scpVersion: "v1.31.0", scpReplicas: 1, wantReplicas: int64Ptr(3), @@ -172,7 +180,7 @@ func TestReconcileStewardControlPlane_PatchContent(t *testing.T) { { name: "resource drift", tcVersion: "v1.31.0", - tcReplicas: 1, + tcReplicas: int32Ptr(1), tcResources: &butlerv1alpha1.ControlPlaneResourcesSpec{ APIServer: &butlerv1alpha1.ComponentResources{ Requests: &butlerv1alpha1.ResourceQuantities{ @@ -190,7 +198,7 @@ func TestReconcileStewardControlPlane_PatchContent(t *testing.T) { { name: "equivalent resources no drift", tcVersion: "v1.31.0", - tcReplicas: 1, + tcReplicas: int32Ptr(1), tcResources: &butlerv1alpha1.ControlPlaneResourcesSpec{ APIServer: &butlerv1alpha1.ComponentResources{ Requests: &butlerv1alpha1.ResourceQuantities{ @@ -291,11 +299,13 @@ func buildSCPPatch(tc *butlerv1alpha1.TenantCluster, scp *unstructured.Unstructu changes = append(changes, "version") } - currentReplicas, _, _ := unstructured.NestedInt64(scp.Object, "spec", "replicas") - desiredReplicas := int64(tc.Spec.ControlPlane.Replicas) - if desiredReplicas > 0 && desiredReplicas != currentReplicas { - specPatch["replicas"] = desiredReplicas - changes = append(changes, "replicas") + if tc.Spec.ControlPlane.Replicas != nil { + currentReplicas, _, _ := unstructured.NestedInt64(scp.Object, "spec", "replicas") + desiredReplicas := int64(*tc.Spec.ControlPlane.Replicas) + if desiredReplicas != currentReplicas { + specPatch["replicas"] = desiredReplicas + changes = append(changes, "replicas") + } } desired := capi.ResolveControlPlaneResources(tc, butlerConfig) From ac986791c01dc3c2e8c5fa9a9cc3d489f48c3c91 Mon Sep 17 00:00:00 2001 From: atbagan Date: Sun, 30 Aug 2026 16:12:04 -0500 Subject: [PATCH 2/2] chore(deps): use released butler-api v0.23.0 Replace the temporary butler-api feature-branch pseudo-version with the released v0.23.0 tag that contains the pointer-typed ControlPlaneSpec.Replicas. --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index f964b03..b930726 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/butlerdotdev/butler-controller go 1.24.6 require ( - github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce + github.com/butlerdotdev/butler-api v0.23.0 github.com/onsi/ginkgo/v2 v2.22.0 github.com/onsi/gomega v1.36.1 github.com/prometheus/client_golang v1.22.0 diff --git a/go.sum b/go.sum index 03041cf..2fc0d46 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,7 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce h1:wDhJkQePpJ08M+lEAA+PLhXcx8rqo6PPrLHlwN0SCYY= -github.com/butlerdotdev/butler-api v0.22.1-0.20260830192233-a58c04aa05ce/go.mod h1:q/RjPFM/r2ELq4DqR78OiAxerKBvH5D4nTGsVLKbjqY= +github.com/butlerdotdev/butler-api v0.23.0 h1:t6NSgZLg5e7t/xE18yfw1sOrAp3vCfQncYsgbLVCzGQ= +github.com/butlerdotdev/butler-api v0.23.0/go.mod h1:q/RjPFM/r2ELq4DqR78OiAxerKBvH5D4nTGsVLKbjqY= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=