fix(manifest): parse same-indent block sequences instead of dropping them (issue #77) - #90
Merged
Merged
Conversation
parseYAMLLines only recognized a block sequence as a key's value when the "-" markers were indented deeper than the key. A sequence at the SAME indentation (e.g. `containers:` immediately followed by `- name: runner` with no extra indent -- valid, common YAML/K8s style) fell into the "key has no value" branch, silently zeroing the key and truncating the rest of the document parse. For issue #77's manifest this dropped spec.containers to empty while spec.restartPolicy (parsed earlier in the same block) survived, so CreatePod's container loop had nothing to iterate: podman only ever received "pod create", never a "podman run", and the pod reported completed/exitCode=0 within seconds with no workload container ever having existed. Every existing YAML test fixture in this package used the extra-indented style, so this path had no coverage. Distinct from #73/#85, which corrupt Command/Args after a successful parse rather than dropping Containers before one. fixes #77
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.
Summary
POST /api/v1/podswith a Pod manifest whosecontainers:list uses the"dash at the same indentation as its key" YAML style (very common,
including in hand-written K8s manifests copy-pasted from docs) silently
dropped the entire container list. The pod was admitted,
podman pod createran, but no container was ever started -- the pod reportedcompleted/exitCode=0within seconds with nothing having actually run.This fixes
internal/manifest's hand-rolled YAML parser to recognize thatstyle.
Changes
internal/manifest/yaml.go:parseYAMLLines's "key with no inlinevalue" branch only treated the next line as a list when it was indented
deeper than the key (
nextIndent > baseIndent). A sequence at thesame indentation as its key (
nextIndent == baseIndent) fell throughto the
nextIndent <= baseIndentcase, which means "key has no value" --silently setting the key to
""and, because the top-level parse loopthen also saw the now-unconsumed
-line as a dedent, truncating therest of the document parse entirely. Added an explicit branch for
nextIndent == baseIndent && strings.HasPrefix(nextTrimmed, "- ")aheadof the existing catch-all, routing it through the same
parseYAMLListalready used for the deeper-indented case. Purely additive: it only
changes behavior for the previously-mishandled case, so it can't affect
any manifest that parsed correctly before.
internal/manifest/yaml_test.go:TestParseYAML_SameIndentBlockList(bare-scalar sequence) and
TestParseYAML_SameIndentBlockList_Nested(map-valued items under a nested key, matching the issue's actual
shape) at the
ParseYAMLunit level.internal/manifest/same_indent_list_test.go:TestIssue77_SameIndentContainersList, an end-to-endParse()reproduction using the exact manifest text from the issue (the
GitHub token/URL placeholders scrubbed; irrelevant to the parse path
under test).
Root cause vs #73/#85
internal/executor/podman.go'sbuildRunArgs/GPU-env-injection family(#73, #85) corrupts
Command/Argsafter a successful parse. This bugis upstream of that:
spec.Containerswas already empty by the timeCreatePodran, so its per-container loop had nothing to iterate --podman run/podman container createwas never invoked for theworkload container, only
podman pod createfor the pod shell. Confirmsdocs/plan.md's R2/T2.4 question that #77 does not share #73/#85's root
cause.
Every existing YAML test fixture in this package used the extra-indented
containers:\n - name: ...style, so the same-indent style had zerocoverage before this change -- same "silently defaults instead of
erroring" pattern previously seen in #43, #44, #52, #66, #74.
Testing
go build ./...-- clean.go vet ./...-- clean.staticcheck ./...-- clean.go test ./... -race -timeout 120s -count=1-- all 13 packages pass,no regressions (
internal/manifest1.9s, full suite ~37s dominated byinternal/watcher).go test ./internal/manifest/... -run 'TestIssue77|TestParseYAML_SameIndent' -v-- confirmed red before the fix (
containers=0) and green after(
containers=1,restartPolicy=Always) on all three new tests.merge/release per this wave's operating procedure. The exact repro
manifest from the issue is now covered by an automated test; DGX
live-verify just needs the real manifest (with the real token/URL)
submitted once the release carrying this fix is deployed.
Linked issues / tasks
Pod completes instantly (exitCode=0) with zero container startup — container spec silently dropped): root-caused and fixed asdescribed above.
and fix); E2's remaining Wave 1 items (Pod command args: long quoted scalar mangled — bash -c receives no argument #73, podman pod stop/rm on a GPU-attached pod hangs indefinitely, and the stuck child becomes a zombie under the spark PID #88) are separate tasks in
this same wave.