refactor(scheduler): remove CubeMaster overcommit ratio - #1575
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
|
AI-generated review. Not reviewed or approved by a human maintainer. PR #1575 — refactor(scheduler): remove CubeMaster overcommit ratioVerdict: No blocking defects found; approve with minor follow-ups.I traced every call site of the removed API surface across the repo and manually Semantically, every edited expression is exactly the old computation evaluated Findings (all minor / non-blocking)1. Behavior change for existing clusters is larger than the diff suggests — document the migration. (docs/rollout)
The deprecation warning is a good mitigation for the config keys, but the docs 2. 3. Warning is skipped for an empty leftover map. (nit) Files checked
|
| return int64(hostCfg.Quota.Cpu) | ||
| quota := hostQuota(hostCfg) | ||
| if quota.Cpu > 0 { | ||
| return quota.OvercommittedCPUMilli(int64(quota.Cpu)) |
There was a problem hiding this comment.
The ratio is applied only on the explicit-config branch; the fallback branch below returns the node-status capacity verbatim. This works today only because applyHostQuota has already overcommitted node.Status.Capacity before buildRegisterRequest uses it as the fallback. That coupling is fragile — any caller that passes a raw host value as a fallback (or a code path that skips applyHostQuota) silently loses overcommit. Consider applying the ratio to fallbacks too (guarding against the registration path already carrying it), or at least documenting this asymmetry.
| q := *resource.NewQuantity(memMB*1024*1024, resource.BinarySI) | ||
| node.Status.Capacity[corev1.ResourceMemory] = q.DeepCopy() | ||
| node.Status.Allocatable[corev1.ResourceMemory] = q.DeepCopy() | ||
| } |
There was a problem hiding this comment.
Overcommitted values now flow into node.Status.Capacity/Allocatable. That means registration capacity (toResourceSnapshot), the inventory API (listinventory.go computes Memory: n.QuotaMem - n.QuotaMemUsage - reserved), and the derived affinity labels AffinityKeyMemorySize/AffinityKeyCPUCores (node.go builds them from n.QuotaMem/n.QuotaCpu) all start reporting the overcommitted quota. Previously the multiplier was applied only inside CubeMaster filters/scorers, so an 8 GiB / 8-core node that reported 10240 Mi / 8000m now reports 16384 Mi / 24000m at default ratios — this changes affinity-selector matching and displayed inventory for existing clusters. Worth calling out in the docs or deriving affinity labels from physical rather than overcommitted capacity.
| nodes := make(node.NodeList, 0, inList.Len()) | ||
| for i := range inList { | ||
| quotaMemFree := sconf.EffectiveQuotaMem(inList[i].InstanceType, inList[i].QuotaMem) - | ||
| quotaMemFree := inList[i].QuotaMem - |
There was a problem hiding this comment.
With this change n.QuotaMem is the overcommitted value, so GetEffectiveNodeMaxMemReservedInMB(instanceType, inList[i].QuotaMem) a few lines below scales the reserved amount by mem_overcommit_ratio (e.g., 10% reserve on a 2x-overcommitted node holds back 20% of physical memory). That tightens the physical check (loadMemFree <= request + reserved) beyond what the docs' "uses the reported (overcommitted) quota as the base" implies. The quota check here is equivalent to before, but the physical-reservation check gets stricter for default nodes.
| `0` or an empty value usually means Cubelet derives a default from host resources. It does not mean unlimited capacity. For load tests or larger clusters, explicitly review CPU, memory, MVM count, and create-concurrency limits. | ||
| `0` or an empty value for `mcpu_limit` / `mem_limit` / `mvm_limit` means Cubelet derives a default from host resources. It does not mean unlimited capacity. CPU and memory overcommit (`cpu_overcommit_ratio` default `3.0`, `mem_overcommit_ratio` default `2.0`) is applied to that base before Cubelet reports `quota_cpu` / `quota_mem_mb`. Set a ratio of `1.0` to report the base unchanged. For load tests or larger clusters, explicitly review CPU, memory, MVM count, create-concurrency, and overcommit. | ||
|
|
||
| ## Upgrade notes: overcommit moved to Cubelet |
There was a problem hiding this comment.
Consider adding an upgrade-ordering warning here. During a rolling upgrade the two sides are asymmetric: a new Cubelet (reporting overcommitted quota) registering with an old CubeMaster — which still multiplies by its own overcommit_ratio — inflates capacity by the product (CPU ×9, memory ×4 with defaults), while an old Cubelet under a new CubeMaster gets no overcommit at all (~1/3 CPU capacity of upgraded nodes). A documented upgrade order (e.g., upgrade CubeMaster first, or a Cubelet flag to report raw quota until the master is upgraded) would avoid a dangerous scheduling window.
d4b6581 to
e35bca9
Compare
| return 0 | ||
| } | ||
| return roundUpFraction(memMB, defaultHostQuotaMemNumerator, defaultHostQuotaMemDenominator) | ||
| return quota.OvercommittedMemMB(memMB) |
There was a problem hiding this comment.
The overcommitted value written here flows not only into the reported quota_cpu/quota_mem_mb (intended) but also into node.Status.Capacity / Allocatable. CubeOps derives CpuTotal / MemMBTotal from that Capacity (CubeOps/.../service/snapshot.go ToSchedulerNode), and those fields are the baselines for the scheduler's physical load guards that the base intentionally preserved over overcommit (see docs/changelog/v0.4.0.md: "Physical load guards (CPU utilization ceiling, real-time free memory) are intentionally preserved"):
memfilter.goloadMemFree := MemMBTotal - MemUsageis now against 2× physical memory — a request of ~10 GiB can pass the "physical" check on an 8 GiB node.prefilter.goCpuLoadUsage > CpuTotalandscore/utils.gogetCpuLoadUsageScore/getMemMbUsageScoreuse the tripledCpuTotal.
The upgrade docs say "Registration Capacity / Allocatable … now use the overcommitted quota" but never mention MemMBTotal / CpuTotal, and the docs' own loadMemFree example ("10% reserve holds back 20% of physical memory") only holds if the baseline stayed physical. Please keep physical memory/CPU for these guards reported separately from the overcommitted Capacity, or explicitly document that these physical guards are now scaled by the overcommit ratio.
| - Nodes with explicit `mcpu_limit` / `mem_limit` keep the same schedulable capacity as the old CubeMaster defaults (CPU ×3, memory ×2). | ||
| - Nodes that derive quota from host resources (the documented default) change: CPU becomes `cores × 3000m` instead of `cores × 6000m`; memory becomes `host × 2` instead of `host × 2.5`. | ||
| - When `mvm_limit` is unset, `max_mvm_num` follows overcommitted memory / 512 MiB (for example 8 GiB: 20 → 32). | ||
| - A CubeMaster `overcommit_ratio` of `1.0` or a per-type override such as `cubebox_gpu: 1.0` no longer applies. Set the matching Cubelet ratios to `1.0` on those nodes. |
There was a problem hiding this comment.
This example assumes loadMemFree stays physical, but after this PR MemMBTotal (= snap.Capacity.MemoryMB) is itself overcommitted (2× physical). So the reserve is 20% of an already-doubled baseline — the "physical" check no longer bounds by physical memory, and the effective backstop is much weaker than this sentence implies. Worth either keeping physical memory reported separately for the loadMemFree guard, or rewording this to state that the physical free-memory guard is now scaled by the overcommit ratio (see inline comment on Cubelet/pkg/cubelet/node_status.go).
|
I verified the bot's findings against the code at 1. The physical load guards' baseline is now also inflated by the overcommit ratioCubelet writes the ratio-multiplied value into Concrete effect: on an 8 GiB machine with 4 GiB used, It's less severe than the bot made it sound, though: for nodes without explicit Ideally the load guards would use a separately reported physical total. At minimum the docs should state that this baseline no longer reflects physical resources — the current upgrade notes only cover the reserve side. 2. Default (host-derived) nodes lose schedulable capacity: CPU halved, memory −20%For nodes with empty
After overcommit moved into Cubelet, the host-derived base dropped from 2000m / ×5⁄4 to 1000m / ×1. This is documented, but existing clusters hit it without touching any config, and scheduling pressure will rise noticeably after upgrade. Worth a prominent release-note callout, or aligning the new defaults with the old effective capacity. 3. Affinity labels
|
e35bca9 to
36d28d1
Compare
| labels[constants.AffinityKeyCPUType] = n.CPUType | ||
| labels[constants.AffinityKeyMemorySize] = fmt.Sprintf("%dMi", n.QuotaMem) | ||
| labels[constants.AffinityKeyCPUCores] = fmt.Sprintf("%dm", n.QuotaCpu) | ||
| labels[constants.AffinityKeyMemorySize] = fmt.Sprintf("%dMi", n.MemMBTotal) |
There was a problem hiding this comment.
Behavior change worth double-checking: the affinity labels kubernetes.io/memory-size / kubernetes.io/cpu-cores switch from the Cubelet-reported quota (QuotaMem / QuotaCpu) to the physical totals (MemMBTotal / CpuTotal).
- For nodes with an explicit
mcpu_limit/mem_limitthat differs from physical capacity, the label can move arbitrarily (up or down) relative to the old quota-derived value, so any existing node-affinity rule /large_size_affinity_confwater mark that matched the old values will silently stop matching after upgrade. - For nodes whose physical fields are not populated (older rows, synthesized/virtual nodes, or any path that sets quota but not
CpuTotal/MemMBTotal), the label becomes0m/0Mi; theGt/Lt/Inmatchers innodeaffinity.gowill then exclude those nodes from any affinity that references these keys.
The label cache is invalidated on metadata sync so the cache itself is fine, but please confirm every node source that reaches Labels() populates CpuTotal/MemMBTotal, and make sure the affinity-label semantics change is called out in upgrade notes (the scheduler-config doc covers it; the affinity/large-size configs do not).
| } | ||
|
|
||
| func resolveHostQuotaMemMB(hostCfg *config.HostConf, fallbacks ...int64) int64 { | ||
| return hostQuota(hostCfg).OvercommittedMemMB(resolveHostBaseMemMB(hostCfg, fallbacks...)) |
There was a problem hiding this comment.
resolveHostQuotaMemMB now returns the overcommitted memory, and buildRegisterRequest passes that value straight into resolveHostMaxMvmNum for the derived (unset-mvm_limit) MVM cap: max_mvm_num = overcommittedMemMB / 512. So the default MVM limit now scales with mem_overcommit_ratio — an 8 GiB host at the default ratio 2 reports max_mvm_num = 32 (32 × 512 MiB = 16 GiB of reservations on 8 GiB physical), and raising the memory ratio to gain density raises the MVM cap in lockstep.
Before this PR the derived cap was computed from the Cubelet's un-overcommitted base (host × 5/4 → 20 for 8 GiB) and never scaled with the configurable master ratio, so the overcommit knob did not relax this physical per-MVM bound. Consider deriving the unset max_mvm_num from resolveHostBaseMemMB (physical) so it remains a physical-memory safety cap while quota_mem_mb keeps the overcommitted figure — or require an explicit mvm_limit whenever mem_overcommit_ratio > 1.
| for k, v := range config.Scheduler.OvercommitRatioByType { | ||
| config.Scheduler.OvercommitRatioByType[k] = v.sanitized() | ||
| if config.Scheduler.hasDeprecatedOvercommitConfig() { | ||
| CubeLog.Warnf("scheduler.overcommit_ratio / overcommit_ratio_conf are ignored; set Cubelet host.quota.cpu_overcommit_ratio / mem_overcommit_ratio instead") |
There was a problem hiding this comment.
Silent-migration hazard: when leftover scheduler.overcommit_ratio/overcommit_ratio_conf keys are present this warning fires once at boot and the old values are ignored — but when they are absent (the common case after this PR, since the keys are removed from the shipped conf.yaml), nothing warns, and CubeMaster relies entirely on whatever each Cubelet now reports.
The old defaults were effectively master-side cpu ×3 / mem ×2 applied on top of Cubelet-reported base quota. The new Cubelet defaults are also cpu ×3 / mem ×2 — but applied per-node against physical host resources when mcpu_limit/mem_limit are empty. Two consequences operators will not see coming:
- Any node that previously ran with a master
overcommit_ratio: 1.0or a per-instance-type override (e.g. GPU nodes set to 1.0 so they don't overcommit) silently inherits the Cubelet's3.0 / 2.0defaults unless each Cubelet config is edited to setcpu_overcommit_ratio: 1.0/mem_overcommit_ratio: 1.0. The warning above does not fire for these nodes (no leftover keys on the master), so the capacity change is invisible. - The shipped
conf.yaml/cubelet.yamldiffs only add the ratio keys to sample configs as comments — they don't pin defaults anywhere authoritative.
Suggest logging the effective per-node overcommit ratio (and base vs reported quota) once per Cubelet at registration, so an unexpected ×3/×2 on a GPU/no-overcommit node is visible in the Cubelet log rather than only manifesting as changed scheduling capacity.
36d28d1 to
98b4eef
Compare
kinwin-ustc
changed the title
| for k, v := range config.Scheduler.OvercommitRatioByType { | ||
| config.Scheduler.OvercommitRatioByType[k] = v.sanitized() | ||
| if config.Scheduler.hasDeprecatedOvercommitConfig() { | ||
| CubeLog.Warnf("scheduler.overcommit_ratio / overcommit_ratio_conf are deprecated and ignored; CubeMaster no longer applies overcommit to node-reported quota") |
There was a problem hiding this comment.
Blocking/coordination concern: this is only correct if Cubelet already reports effective (overcommitted) quota. Before this PR, CubeMaster applied CPU×3 / Mem×2 implicitly — preHandleScheduler populated those defaults even when overcommit_ratio was absent from YAML — so every current deployment (including ones that never set the key) is being scheduled against quota×3 / quota×2. The replacement mechanism referenced here and in conf.yaml (host.quota.cpu_overcommit_ratio / host.quota.mem_overcommit_ratio) does not exist in this repo's Cubelet today: Cubelet/pkg/config/config.go HostConfigQuota exposes only mcpu_limit/mem_limit/mvm_limit/creation_concurrent_num, and node_status.go applies only fixed derived factors (CPU×2, Mem×5/4) when quota is not explicitly configured.
Consequences if the Cubelet counterpart isn't shipped and deployed first:
- Nodes with an explicit
host.quotasilently lose ~2/3 of CPU and ~1/2 of Mem schedulable capacity → placements that previously fit get filtered/rejected. - Because the old default applied with no YAML key present,
hasDeprecatedOvercommitConfig()returns false for the majority of affected deployments — this warning never fires, so the capacity drop is silent. - Per-instance-type overcommit (
overcommit_ratio_conf, e.g. GPU nodes at 1.0/1.0) is dropped to a single host-level setting.
Please confirm the Cubelet-side PR is part of the same release (ideally landed first), and consider emitting the warning/upgrade note even for configs that relied on the implicit default.
98b4eef to
4277315
Compare
| for i := range inList { | ||
|
|
||
| quotaCpuFree := sconf.EffectiveQuotaCpu(inList[i].InstanceType, inList[i].QuotaCpu) - | ||
| quotaCpuFree := inList[i].QuotaCpu - |
There was a problem hiding this comment.
This line now treats the Cubelet-reported QuotaCpu as the final schedulable capacity, dropping the CubeMaster-side overcommit multiplier. That is only behavior-preserving if Cubelet already reports quota scaled by its overcommit ratio — but at this PR's merge base it does not: resolveHostQuotaCPUMilli (Cubelet/pkg/cubelet/node_status.go) returns the raw configured host.quota.mcpu_limit, or a default of cpuCount*1000*2, and HostConfigQuota (Cubelet/pkg/config/config.go) has no cpu_overcommit_ratio/mem_overcommit_ratio keys. So for a node with an explicit quota, the schedulable CPU drops from quota*3 (previous default) to quota*1 as soon as this PR is deployed — roughly a 3×/2× capacity cut for CPU/mem until the Cubelet-side change lands and is configured with matching defaults. The same applies to memfilter.go:41, realtimescore.go:122-125, and score/utils.go:110,124. Please confirm the companion Cubelet change ships before/with this PR and that its defaults reproduce the old effective capacity (or that the capacity reduction is intentional and documented); otherwise this is a silent scheduling regression during any mixed-version upgrade.
CubeMaster no longer multiplies Cubelet-reported quota by overcommit_ratio. Filters and scorers use QuotaCpu/QuotaMem directly. Leftover YAML keys are parsed but ignored with a startup warning. Assisted-by: Cursor Grok 4.6:cursor-grok-4.6 Signed-off-by: Feng Jin <ronyjin@tencent.com>
4277315 to
d4705cd
Compare
| // Deprecated: overcommit_ratio is no longer used by CubeMaster. | ||
| // These fields are kept only so leftover YAML is parsed without error; | ||
| // the values are ignored at runtime. | ||
| DeprecatedOvercommitRatio *deprecatedOvercommitRatioConf `yaml:"overcommit_ratio"` |
There was a problem hiding this comment.
After this change nothing reads CPURatio/MemRatio anymore — the deprecated struct is populated only so leftover YAML decodes without error, and hasDeprecatedOvercommitConfig() keys off pointer/map presence, never these values. The fields are now write-only. Consider a placeholder struct (or a comment noting the values are intentionally unused) so future readers don't expect them to feed the startup warning or any fallback logic.
| # cubebox_gpu: | ||
| # cpu_ratio: 1.0 | ||
| # mem_ratio: 1.0 | ||
| # Deprecated: overcommit_ratio / overcommit_ratio_conf are no longer used. |
There was a problem hiding this comment.
Worth calling out that this isn't a no-op deprecation: schedulable capacity now equals the raw Cubelet-reported quota, whereas the default before was quota × 3 (CPU) / × 2 (mem). Cubelet still returns an explicitly configured host.quota.mcpu_limit/mem_limit verbatim (its overcommit factors only apply to auto-derived defaults), so a node with mcpu_limit: 24000 that used to offer 72000 milli-CPU now offers 24000 after upgrade. Suggest adding a short migration note in the docs telling operators to fold their previous ratio into the explicit host quota, or expect a one-time capacity drop (abrupt during rolling master/Cubelet upgrades).
4 participants
Remove CubeMaster-side
overcommit_ratio/overcommit_ratio_conf. Filters and scorers now use Cubelet-reportedQuotaCpu/QuotaMemdirectly. Leftover YAML keys are parsed but ignored with a startup warning.Assisted-by: Cursor Grok 4.6:cursor-grok-4.6