Repository navigation
chore: sync upstream PageBroker staging cap and MIG restore guard - #17
Merged
Merged
Conversation
…ynamo#640) * fix(chart): keep staging below the agent memory limit The staging emptyDir is memory-backed and written from the agent container, so its pages are charged to the agent's cgroup alongside the agent's own allocations. Shipping sizeLimit and daemonset.resources.limits.memory both at 64Gi meant the two could reach the container limit while the volume was still under its cap: the kernel OOM-kills the agent, and every capture and restore on that node dies with it. The values comment already asked for headroom that the defaults did not leave. Defaults now leave an 8Gi reserve, and the chart refuses to render when the gap is absent, so a release cannot be installed in the configuration that OOM-kills. The comparison converts both quantities to bytes rather than matching suffixes, so 65536Mi against 64Gi is caught, and an unparseable quantity fails loudly instead of silently disabling the check. Two existing tests pinned the old behaviour — one asserted the equal defaults, another set 128Gi against the 64Gi limit — and now assert the gap instead. Refs ai-dynamo#394 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> * fix(chart): answer the upgrade case and stop repeating the rationale The reason staging has to stay below the agent memory limit was written out three times — values.yaml, the helper, and storage.md — so it now lives in storage.md alone and the other two point at it. storage.md gains what none of them said: what happens on upgrade. A release on chart defaults upgrades cleanly, as does one carried forward with --reuse-values from before the key existed. Two configurations stop rendering, and both are ones that could already OOM-kill the agent: a sizeLimit pinned to the previous 64Gi default against a 64Gi limit, and an agent memory limit lowered without lowering the cap with it. Failing the upgrade is the point — neither was safe before, and the alternative is waiting for a checkpoint large enough to kill the agent. The error is the only thing an operator hitting this sees, so it now names both ways to resolve it rather than only explaining the problem. Refs ai-dynamo#394 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> * fix(chart): cap the staging volume instead of refusing to render Rejecting a staging cap that cannot fit broke installs that work today. The CPU e2e cluster is the proof: it sets the agent memory limit to 2Gi and never touches staging, so the chart's own default exceeded the limit and the install failed. The cap is now lowered to fit — the agent memory limit less 8Gi, or an eighth of a smaller limit — so every configuration that installed before still installs, and none of them can OOM-kill the agent. Defaults and any configuration that already leaves headroom render unchanged. A quantity the chart cannot compare is honoured rather than rejected, for the same reason: an exotic but valid quantity must not stop an install. Refs ai-dynamo#632 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> * fix(chart): cut the comments back Also drops a values.yaml line left saying the chart refuses to render, which capping replaced. Refs ai-dynamo#632 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> * fix(chart): cap staging against both writers, and say why it needs capping The cap only considered the agent's memory limit, but PageBroker writes the same volume on staged restore against its own. Lowering the PageBroker limit left the volume able to outgrow it, so a single restore could OOM-kill PageBroker and take checkpoint and restore off the node. The cap now follows whichever limit is smaller. The docs described how to size the field without saying what it is: a RAM-backed emptyDir with no size of its own, unbounded without this cap, whose pages are charged to whichever container wrote them. That is the part that explains why one oversized request kills a container rather than failing itself. Refs ai-dynamo#632 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> --------- Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 67664df) Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
* feat(compat): refuse a restore that changes MIG partitioning or shape nvidia-smi reports a MIG slice under its parent card's product name, so a whole H100 and a 1g.10gb slice of one are the same model and the same count. gpu-model and gpu-count both pass, and the restore fails later inside CRIU or the CUDA runtime on memory or SM capacity instead. Two rules rather than one, because they rest on different recorded data. mig-partitioning reads the GPU UUIDs, which have been recorded since the first release and carry a MIG- prefix only for a slice, so it refuses a whole GPU against a slice on artifacts captured by any version. mig-profile compares the slice shape, which needs a value nothing records yet; until one is recorded the shape stays unknown, and an unknown value admits, as mustMatch already documents. Both summarise which values are present and not how many devices share each, so losing a GPU is reported once by gpu-count rather than twice. Signed-off-by: liran elisha <lirane@nvidia.com> * feat(cuda): record the MIG profile of each captured GPU --query-gpu exposes no MIG field at all, so the slice shape has to come from nvidia-smi -L, which lists it beside each slice's own UUID. Discovery makes that second call and joins the profiles onto the devices it already found, keyed on UUID rather than on position. A device no profile was listed for keeps none, which leaves the shape unknown instead of guessing at it. For the same reason a failed listing returns the GPUs it did read: the profile is a description, so losing it costs the shape comparison rather than the checkpoint. Signed-off-by: liran elisha <lirane@nvidia.com> * feat(api): publish the captured MIG profile on the content status status.source publishes what a checkpoint was captured on, and the model name alone cannot say whether that was a whole card or a slice of one. The profile is the only published value that distinguishes them, so it belongs next to the model rather than being left to inference. CRD change. Absent when the capture had a whole GPU, and absent for a slice captured before the profile was recorded. Signed-off-by: liran elisha <lirane@nvidia.com> * fix(compat): count the slice shapes so a redistribution is refused gpu-count compares only the total, so dropping the counts from the shape summary left two slices of one shape and one of another indistinguishable from the reverse: three devices either way, one parent model either way, and the same set of shapes either way. mig-profile admitted it. The shapes are counted again. The partitioning summary stays categorical, because which kinds are present is a yes-or-no question and counting it there would refuse every plain change in GPU count on a cluster with no MIG at all, restating gpu-count under a name mentioning MIG. Signed-off-by: liran elisha <lirane@nvidia.com> * refactor(compat): model GPU partitioning as a typed value Which kinds of device a set of GPUs holds is two independent booleans, not a list: each device is either a whole card or a slice, and a set can hold both. The slice the summary was built from carried order and multiplicity that the renderer immediately discarded, and its distinctSorted helper had no other caller. The rendered value is unchanged, so refusal messages and conditions read exactly as before and the existing tests pass untouched. Signed-off-by: liran elisha <lirane@nvidia.com> * refactor(cuda): read the MIG listing by named column, not by regex nvidia-smi -L writes each MIG device as a fixed run of whitespace-padded columns, and strings.Fields collapses that padding on its own, so the pattern's repeated whitespace groups were doing work the standard library already does. Naming each column says what the line is made of in a way the pattern could only imply, and the device ordinal is still checked to be a number rather than merely present. Both forms were run over the existing inputs, the listing from the T4 this was validated on, the older MIG-GPU-<parent>/<gi>/<ci> UUID spelling, a media-extension profile, tab and single-space padding, and trailing text, and they agree throughout. Those inputs are now test cases. Signed-off-by: liran elisha <lirane@nvidia.com> * docs(api): say that a missing MIG profile means unknown, not whole GPU An absent profile has two causes, and only one of them is a fact about the GPU: a whole-GPU capture never had a shape to record, while an agent released before this field captured slices without recording theirs. The wording led with the first and buried the second, and the API reference gave only the first, so it implied exactly the inference the gates are built to avoid. Whole against slice is decided from the UUID for this reason, and an absent shape admits a restore rather than refusing one. Regenerated both CRD copies, since the field description ships in them. Signed-off-by: liran elisha <lirane@nvidia.com> * docs(api): drop the ProductName aside from the MIG profile description It explained why the model name cannot stand in for the profile, which is motivation for the field rather than a description of it, and reading it on the field itself invited the question of what ProductName had to do with the value at hand. Signed-off-by: liran elisha <lirane@nvidia.com> * refactor(compat): compare device sets as multisets, not as rendered text gpu-model and mig-profile compared the string summariseByCount returned, which made its sort load-bearing for correctness rather than cosmetic: it iterates a map, so without the sort two identical sets could render in different orders and compare unequal. Nothing at the call site said so. The comparison is now its own function that says what it ignores, and summariseByCount drops to rendering the refusal message, where the count still matters because a node holding eight of one GPU should not print eight entries. Every existing assertion passes untouched, including the two that pin order independence. Signed-off-by: liran elisha <lirane@nvidia.com> * refactor(compat): name the multiset comparison elementsMatch testify's ElementsMatch answers the same question and is already what this repository's tests call it, and the name sits beside mustMatch in this file as its multi-value counterpart. Signed-off-by: liran elisha <lirane@nvidia.com> * fix(cuda): log the MIG listing failure instead of dropping it A failed nvidia-smi -L left every capture on the node recording no slice shape, with nothing to say why. Threading a logger into DiscoverVisibleGPUs reports it at V(1) with the pid and the error, the same form the failed PodResources description already uses. Signed-off-by: liran elisha <lirane@nvidia.com> * fix(cuda): key the MIG profile join on the parent GPU UUID --query-gpu enumerates GPUs, and a MIG slice is not one: a container holding a slice sees the parent card there. The join keyed the listing on that query's UUID, so it met the slice UUIDs from nvidia-smi -L never once, every device kept an empty profile, and mig-profile admitted every shape change it exists to refuse. The listing nests each slice under the card it was carved from, which is the only output tying the two UUIDs together. Grouping by that parent lets each captured GPU expand into the slices it actually holds, with the parent's model carried over as the only name nvidia-smi gives a slice. The fixture hid this: its --query-gpu printed a MIG UUID, which real nvidia-smi does not do, while its own -L output named a different card as the parent. It reports the parent now, so the test covers what the node reports. Signed-off-by: liran elisha <lirane@nvidia.com> * fix(cuda): pass the logger to the visible-device discovery call The NVIDIA_VISIBLE_DEVICES path added on main calls DiscoverVisibleGPUs directly, which now needs a logger to report a failed MIG listing. Signed-off-by: liran elisha <lirane@nvidia.com> * revert: drop the MIG profile comparison MIG is unsupported, so a MIG slice is refused outright rather than compared against the other side's slice shape. Restores every file this branch touched to the main it last merged, including the unreleased migProfile CRD field. Signed-off-by: liran elisha <lirane@nvidia.com> * feat(agent): refuse a MIG slice at checkpoint and at restore Snapshot supports whole GPUs only, so a container that sees a MIG slice is refused under the new mig-slice check: a checkpoint fails before anything is locked, and a restore is refused as incompatible at the inspect gate, waivable by the skip-compat annotation like any other check. Both run nvidia-smi -L inside the container ahead of GPU discovery, because discovery cannot report a slice: under DRA it fails on the slice's MIG- UUID, and elsewhere it records the parent card. Signed-off-by: liran elisha <lirane@nvidia.com> * refactor(agent): call the MIG probe directly from the refusals Both refusals were handed the same cuda.DiscoverMIGSlices, so the parameter only served the tests. They now call it themselves, and the tests answer the nvidia-smi -L probe with a fake nsenter on PATH. Signed-off-by: liran elisha <lirane@nvidia.com> * test(cuda): cover the nsenter nvidia-smi helper directly Signed-off-by: liran elisha <lirane@nvidia.com> * test(agent): move the MIG refusal tests next to compat.go Signed-off-by: liran elisha <lirane@nvidia.com> --------- Signed-off-by: liran elisha <lirane@nvidia.com> (cherry picked from commit 13dc5a7) Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
rst0git
added
the
skip-issue-check
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Review follow-ups on the two upstream cherry-picks. MIG gate: when the container's slices cannot be listed (nvidia-smi -L fails or times out), refuse the checkpoint or restore instead of treating the GPUs as whole. The gate runs only for CUDA containers, whose GPU discovery needs the same nvidia-smi, so a healthy node is unaffected. A capture is refused before anything is locked; a restore is refused but not as incompatible, since nothing showed the target to be a slice, and the skip-compat annotation still waives it. Staging cap: the quantity parser now understands every form Kubernetes accepts for a memory size, including decimal exponents (64e9), the larger binary and decimal suffixes (Pi, Ei, P, E) and fractional or milli values, so none bypasses the headroom calculation. A value that is not a quantity at all fails the render with a clear message instead of leaving the volume uncapped. Valid configurations are still lowered, never rejected. Docs: the chart README and API reference now give the 56Gi default and its relation to the 64Gi agent limit, and storage.md says an uncapped volume grows until the writer's cgroup limit or the node's memory runs out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Member
Author
|
Addressed all four findings in 4c9f7ae: MIG check fails closed on a failed listing (restore refusal is not an incompatibility verdict; skip-compat still waives it); the quantity parser covers every Kubernetes form and rejects non-quantities; README, API reference and storage.md updated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-picks ai-dynamo/snapshot 67664df (ai-dynamo#640: keep
the memory-backed PageBroker staging volume below the agent and PageBroker memory limits) and 13dc5a7
(ai-dynamo#318: refuse a checkpoint or restore on a MIG slice). Our 64Gi/64Gi defaults let a large capture
OOM-kill the agent; dende runs 2g.48gb MIG slices.
4c9f7ae addresses the review on top of the unchanged upstream commits: the MIG check fails closed when
nvidia-smi -L cannot list slices; the staging cap parses every Kubernetes quantity form (64e9, Pi, Ei,
milli) and rejects non-quantities instead of leaving the volume uncapped; the README, API reference and
storage.md give the 56Gi default and correct wording. Both fixes are candidates for upstream.
Unit and chart tests pass (74/74).