-
Notifications
You must be signed in to change notification settings - Fork 7
fix: tlc drive core capping and address linting issues #2792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
| } | ||
|
Comment on lines
+39
to
44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct — Two things worth being aware of rather than changing:
|
||
| if cluster.Spec.Dynamic == nil { | ||
| return nil | ||
| } | ||
|
|
||
| driveContainers := cluster.Spec.Dynamic.DriveContainers | ||
| numDrives := cluster.Spec.Dynamic.NumDrives | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cap itself is right — it restores
tlc + qlc == assignedCoresfor mixed containers, which is whattotalTlcDriveCores/totalQlcDriveCoresfeed into both terms ofRequiredComputeCores. Two notes:1. The comment is slightly narrower than reality. Grown containers also go through
pinCores(planner.go:600), so the precise statement is "only an existing un-grown container can need the cap" — a grown one is re-pinned at or aboveRequiredDriveCores(newTlc, newQlc), which already includes the TLC share. Suggest tightening the wording so a reader doesn't go looking for a growth-path hole that isn't there.2. When the cap binds, QLC gets attributed zero cores.
qlcDriveCoresForContainercomputesmax(assignedCores-tlc, 0)against the uncappedtlc, so forassigned < tlcDerivedthe split is(assigned, 0)— the QLC pool contributes nothing to compute sizing despite holding capacity. That's harmless at shipped defaults (ComputeToQlcDriveCoreRatio = 0.0,internal/config/env.go:538), and it's the only split that preserves the sum invariant, so I think it's the right call. But if an operator ever setsCAPACITY_COMPUTE_TO_QLC_DRIVE_CORE_RATIOabove the TLC ratio, this under-states required compute rather than over-stating it. Worth one line in the comment recording that the whole shortfall is charged to TLC deliberately.