diff --git a/cmd/weka-capacity/plan.go b/cmd/weka-capacity/plan.go index 11ab6258c..6ecfe68b8 100644 --- a/cmd/weka-capacity/plan.go +++ b/cmd/weka-capacity/plan.go @@ -555,8 +555,8 @@ func planSummary(p *capacityplanner.CapacityPlan, result *inventory.Result, desi } idle := 0 - for _, n := range result.Inventory { - if _, used := newNodes[n.NodeName]; !used { + for i := range result.Inventory { + if _, used := newNodes[result.Inventory[i].NodeName]; !used { idle++ } } diff --git a/internal/capacityplanner/autofulldrives_compute.go b/internal/capacityplanner/autofulldrives_compute.go index 064274ba5..980bf675f 100644 --- a/internal/capacityplanner/autofulldrives_compute.go +++ b/internal/capacityplanner/autofulldrives_compute.go @@ -294,8 +294,8 @@ func autoKeptCompute(in *autoComputeInput) (kept []autoComputeEntry, pinned map[ // but which of "wait" or "intervene" is right turns on this fact, and nothing else in the report carries it. func drainingComputeClause(in *autoComputeInput) string { var nodes []string - for node, nc := range in.remaining { - if nc.HasDeletingComputeContainer { + for node := range in.remaining { + if in.remaining[node].HasDeletingComputeContainer { nodes = append(nodes, node) } } diff --git a/internal/capacityplanner/cores.go b/internal/capacityplanner/cores.go index c0d9cff14..24cfbfc2d 100644 --- a/internal/capacityplanner/cores.go +++ b/internal/capacityplanner/cores.go @@ -106,7 +106,7 @@ func finalCores(c *ExistingContainer, growth map[string]*ContainerGrowth) int { // tlcDriveCoresForContainer returns the TLC-attributable core count for a container's final state. A // TLC-only container (qlcGiB <= 0, checked before the tlcGiB<=0 short-circuit below) attributes all of // assignedCores to TLC — the only reliable figure for auto-full-drives containers, which are always -// TLC-only. Mixed containers fall back to capacity-derived TlcDriveCores(tlcGiB, cons). +// TLC-only. Mixed containers derive the share from capacity, capped at assignedCores. func tlcDriveCoresForContainer(tlcGiB, qlcGiB, assignedCores int, cons *CapacityConstraints) int { if qlcGiB <= 0 && assignedCores > 0 { return assignedCores @@ -114,7 +114,13 @@ func tlcDriveCoresForContainer(tlcGiB, qlcGiB, assignedCores int, cons *Capacity if tlcGiB <= 0 { return 0 } - return TlcDriveCores(tlcGiB, cons) + tlc := TlcDriveCores(tlcGiB, cons) + // Only an existing container can need the cap: pinCores keeps new ones at or above the derived value, + // so a shortfall means the assigned count predates the current TlcCapacityPerCoreGiB. + if assignedCores > 0 { + return min(tlc, assignedCores) + } + return tlc } // qlcDriveCoresForContainer returns the QLC-attributable core count for a single drive container in its diff --git a/internal/capacityplanner/cores_test.go b/internal/capacityplanner/cores_test.go new file mode 100644 index 000000000..9ba94f719 --- /dev/null +++ b/internal/capacityplanner/cores_test.go @@ -0,0 +1,69 @@ +package capacityplanner + +import "testing" + +// The split must add up to the cores the container actually has. An over-count reaches totalTlcDriveCores +// and from there both terms of RequiredComputeCores, over-stating a mixed-pool cluster's compute. +func TestDriveCoresForContainer_SplitSumsToAssignedCores(t *testing.T) { + cons := testCons() // TlcCapacityPerCoreGiB=5120, QlcCapacityPerCoreGiB=51200 + + tests := []struct { + name string + tlcGiB, qlcGiB, assigned int + wantTlc, wantQlc int + }{ + { + // 10240GiB derives 2 TLC cores, above the 1 assigned. + name: "mixed, assigned below capacity-derived tlc share", + tlcGiB: 10240, qlcGiB: 51200, assigned: 1, + wantTlc: 1, wantQlc: 0, + }, + { + name: "mixed, assigned above tlc share leaves remainder to qlc", + tlcGiB: 10240, qlcGiB: 51200, assigned: 5, + wantTlc: 2, wantQlc: 3, + }, + { + name: "mixed, assigned exactly equals tlc share", + tlcGiB: 10240, qlcGiB: 51200, assigned: 2, + wantTlc: 2, wantQlc: 0, + }, + { + name: "tlc-only attributes every assigned core to tlc", + tlcGiB: 10240, qlcGiB: 0, assigned: 4, + wantTlc: 4, wantQlc: 0, + }, + { + name: "qlc-only attributes every assigned core to qlc", + tlcGiB: 0, qlcGiB: 51200, assigned: 3, + wantTlc: 0, wantQlc: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotTlc := tlcDriveCoresForContainer(tt.tlcGiB, tt.qlcGiB, tt.assigned, cons) + gotQlc := qlcDriveCoresForContainer(tt.tlcGiB, tt.qlcGiB, tt.assigned, cons) + + if gotTlc != tt.wantTlc || gotQlc != tt.wantQlc { + t.Errorf("tlc/qlc = %d/%d, want %d/%d", gotTlc, gotQlc, tt.wantTlc, tt.wantQlc) + } + if got := gotTlc + gotQlc; got != tt.assigned { + t.Errorf("tlc+qlc = %d, want %d (the split must not invent or drop cores)", got, tt.assigned) + } + }) + } +} + +// With no assigned count to split, both sides fall back to their capacity-derived values and the sum is +// not bound to assignedCores — 2 TLC cores for 10240GiB plus 1 QLC core for 51200GiB. +func TestDriveCoresForContainer_UnassignedFallsBackToCapacityDerived(t *testing.T) { + cons := testCons() + + gotTlc := tlcDriveCoresForContainer(10240, 51200, 0, cons) + gotQlc := qlcDriveCoresForContainer(10240, 51200, 0, cons) + + if gotTlc != 2 || gotQlc != 1 { + t.Errorf("tlc/qlc = %d/%d, want 2/1 (capacity-derived on both sides)", gotTlc, gotQlc) + } +} diff --git a/internal/capacityplanner/inventory/collect.go b/internal/capacityplanner/inventory/collect.go index e393f1e1f..4e89c42d0 100644 --- a/internal/capacityplanner/inventory/collect.go +++ b/internal/capacityplanner/inventory/collect.go @@ -937,8 +937,8 @@ func (c Collector) nodeDetailsFromLists(ctx context.Context, cluster *weka.WekaC // Index the planner inventory by node for free-headroom + FD lookup. invByNode := make(map[string]capacityplanner.NodeCapacity, len(inv)) - for _, nc := range inv { - invByNode[nc.NodeName] = nc + for i := range inv { + invByNode[inv[i].NodeName] = inv[i] } // Union of driveNodes + computeNodeList: computeNodeList already aliases driveNodes when the @@ -1154,15 +1154,16 @@ func resolveInventoryFDValue(node *corev1.Node, fdConfig *weka.FailureDomain) (f func mergeRoleNodes(driveInv, computeInv []capacityplanner.NodeCapacity) (inventory []capacityplanner.NodeCapacity, computeNodes map[string]bool) { inventory = append([]capacityplanner.NodeCapacity(nil), driveInv...) index := make(map[string]struct{}, len(inventory)) - for _, nc := range inventory { - index[nc.NodeName] = struct{}{} + for i := range inventory { + index[inventory[i].NodeName] = struct{}{} } computeNodes = make(map[string]bool, len(computeInv)) - for _, nc := range computeInv { + for i := range computeInv { + nc := &computeInv[i] computeNodes[nc.NodeName] = nc.IneligibleReason == "" if _, ok := index[nc.NodeName]; !ok { index[nc.NodeName] = struct{}{} - inventory = append(inventory, nc) + inventory = append(inventory, *nc) } } return inventory, computeNodes diff --git a/internal/capacityplanner/planner.go b/internal/capacityplanner/planner.go index 39fea4552..eda6ec96b 100644 --- a/internal/capacityplanner/planner.go +++ b/internal/capacityplanner/planner.go @@ -516,9 +516,10 @@ func PlanCapacity( // Working per-node headroom, sorted deterministically by FD then node. states := make(map[string]*nodeState, len(inventory)) - for _, nc := range inventory { + for i := range inventory { + nc := &inventory[i] states[nc.NodeName] = &nodeState{ - nc: nc, + nc: *nc, tlcFree: nc.TlcGiB, qlcFree: nc.QlcGiB, coresFree: nc.AllocatableCPU, // physical CPU remaining diff --git a/internal/controllers/wekacluster/steps_cluster_creation.go b/internal/controllers/wekacluster/steps_cluster_creation.go index 02a4dac0c..166f0202a 100644 --- a/internal/controllers/wekacluster/steps_cluster_creation.go +++ b/internal/controllers/wekacluster/steps_cluster_creation.go @@ -641,15 +641,7 @@ func (r *wekaClusterReconcilerLoop) buildPlannerComputeContainers(ctx context.Co logger := instrumentation.CurrentSpanLogger(ctx) cluster := r.cluster - occupied := make(map[string]struct{}) - for _, c := range existing { - if c.Spec.Mode == weka.WekaContainerModeCompute { - if n := string(c.GetNodeAffinity()); n != "" { - occupied[n] = struct{}{} - } - } - } - + occupied := computeNodesOccupied(existing) totalCount := len(layout) for _, entry := range layout { @@ -687,18 +679,26 @@ func (r *wekaClusterReconcilerLoop) buildPlannerComputeContainers(ctx context.Co return built, skippedReasons } +// computeNodesOccupied indexes the nodes already hosting one of this cluster's compute containers — the +// nodes a planner layout entry creates nothing on, since in-place growth resizes them instead. +func computeNodesOccupied(containers []*weka.WekaContainer) map[string]struct{} { + occupied := make(map[string]struct{}) + for _, c := range containers { + if c.Spec.Mode != weka.WekaContainerModeCompute { + continue + } + if n := string(c.GetNodeAffinity()); n != "" { + occupied[n] = struct{}{} + } + } + return occupied +} + // unusedComputeNodes returns the planner-reserved compute nodes that are not already hosting a compute // container, preserving the planner's order. New compute containers pin to these (one each) so they never // schedule onto a drive-pinned node lacking the post-drive hugepages to host both. func unusedComputeNodes(existing []*weka.WekaContainer, planned []string) []string { - used := make(map[string]struct{}) - for _, c := range existing { - if c.Spec.Mode == weka.WekaContainerModeCompute { - if n := string(c.GetNodeAffinity()); n != "" { - used[n] = struct{}{} - } - } - } + used := computeNodesOccupied(existing) free := make([]string, 0, len(planned)) for _, n := range planned { if _, taken := used[n]; !taken { diff --git a/internal/validation/cluster_drive_compute_core_ratio.go b/internal/validation/cluster_drive_compute_core_ratio.go index 35ed3a3ec..8a5ae1b04 100644 --- a/internal/validation/cluster_drive_compute_core_ratio.go +++ b/internal/validation/cluster_drive_compute_core_ratio.go @@ -20,6 +20,9 @@ import ( // auto-derived drive cores count too — except under clusterCapacity/auto-full-drives, where the planner // assigns both sides and the template's numbers are not the ones the cluster runs on. Skips cases // already below clusterComputeDriveCoresFloor's hard 1:1 floor, which owns those exclusively. +// +// At shipped defaults only exclusive full-drives (ratio 2.0) leaves room between that floor and the +// recommendation; drive-sharing's 1.0 makes the two equal, so the advisory is inert there by design. type clusterDriveComputeCoreRatio struct{} func (clusterDriveComputeCoreRatio) ID() string { diff --git a/internal/validation/cluster_drive_compute_core_ratio_test.go b/internal/validation/cluster_drive_compute_core_ratio_test.go index 0dd7a5511..061b39c96 100644 --- a/internal/validation/cluster_drive_compute_core_ratio_test.go +++ b/internal/validation/cluster_drive_compute_core_ratio_test.go @@ -29,7 +29,8 @@ func setRatioConfig(t *testing.T, tlcRatio, fullDrivesRatio float64) { } func TestClusterDriveComputeCoreRatio(t *testing.T) { - // Ratio pinned to 2.0, matching the old hardcoded 1:2 behavior this test mirrors. + // Pinned above the shipped drive-sharing 1.0, where the advisory is inert by design, so these cases are + // observable at all. TestClusterDriveComputeCoreRatio_FullDrivesUsesFullDrivesRatio covers the shipped pair. setRatioConfig(t, 2.0, 2.0) v := &clusterDriveComputeCoreRatio{} diff --git a/internal/validation/cluster_min_drives_feasibility.go b/internal/validation/cluster_min_drives_feasibility.go index c8daf90f9..4b881fd75 100644 --- a/internal/validation/cluster_min_drives_feasibility.go +++ b/internal/validation/cluster_min_drives_feasibility.go @@ -36,14 +36,12 @@ func (clusterMinDrivesFeasibility) Validate(ctx context.Context, c client.Client fldPath := field.NewPath("spec", "startIoConditions", "minNumDrives") - // Checked before the nil guard below: a nil dynamicTemplate IS auto-full-drives mode, so it takes - // the per-node branch rather than falling through as "nothing configured". + // A nil dynamicTemplate IS auto-full-drives mode (UsesAutoFullDrives returns true on a nil receiver), + // so it takes the per-node branch rather than falling through as "nothing configured" — which is also + // what lets everything below dereference Dynamic unguarded. if cluster.Spec.Dynamic.UsesAutoFullDrives() { return validateMinDrivesAutoFullDrives(ctx, c, cluster, minNumDrives, fldPath) } - if cluster.Spec.Dynamic == nil { - return nil - } driveContainers := cluster.Spec.Dynamic.DriveContainers numDrives := cluster.Spec.Dynamic.NumDrives