diff --git a/go.mod b/go.mod index b5134e4..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.0 + 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 8ac96a6..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.0 h1:NbojKU2xDeNal1Jp3Wc2BJiysNGTnDgF8cywf2Tklak= -github.com/butlerdotdev/butler-api v0.22.0/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= 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)