fix(agent): bundle a statically linked tar for restore #98
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,11 @@ import ( | |
| const ( | ||
| rootfsDiffFilename = "rootfs-diff.tar" | ||
| deletedFilesFilename = "deleted-files.json" | ||
|
|
||
| // RestoreTarBinaryName is the file name of the statically linked GNU tar | ||
| // in the agent binary bundle. Must match the name the Dockerfile bundle | ||
| // stage copies into /snapshot-binaries/. | ||
| RestoreTarBinaryName = "tar" | ||
| ) | ||
|
|
||
| // GetRootFS returns the container's root filesystem path via /host/proc. | ||
|
|
@@ -158,7 +163,13 @@ func CaptureDeletedFiles(upperDir, checkpointDir string) (bool, error) { | |
| // The archive is copied to local disk first. tar walks members with many small | ||
| // reads; doing that directly from NFS is much slower than one sequential copy | ||
| // plus a local extract. | ||
| func ApplyRootfsDiff(checkpointPath, targetRoot, tarBinary string, log logr.Logger) error { | ||
| // A relative path would resolve through PATH inside the placeholder — | ||
| // exactly the fallback the bundled static tar exists to prevent. | ||
| if !filepath.IsAbs(tarBinary) { | ||
| return fmt.Errorf("tar binary path %q is not absolute", tarBinary) | ||
| } | ||
|
|
||
| rootfsDiffPath := filepath.Join(checkpointPath, rootfsDiffFilename) | ||
| info, err := os.Stat(rootfsDiffPath) | ||
| if os.IsNotExist(err) { | ||
|
|
@@ -182,12 +193,20 @@ func ApplyRootfsDiff(checkpointPath, targetRoot string, log logr.Logger) error { | |
| // --skip-old-files: silently skip files that already exist in the restore target. | ||
| // The rootfs diff only contains overlay upperdir changes (runtime-generated files | ||
| // like triton caches, tmp files) — base image files should not be overwritten. | ||
| // | ||
| // The capture side archives with --xattrs; restore them, but only the user | ||
| // and security namespaces. The archive is cut from the overlay upperdir, so | ||
| // it can carry trusted.overlay.* entries that must never be re-applied | ||
| // through the restored container's overlay mount. These flags are mirrored | ||
| // by the smoke test in scripts/build-gnu-tar.sh — keep them in sync. | ||
| log.Info("Applying rootfs diff", "target", targetRoot, "bytes", info.Size()) | ||
| cmd := exec.Command(tarBinary, "--skip-old-files", "--blocking-factor=2048", | ||
| "--xattrs", "--xattrs-include=user.*", "--xattrs-include=security.*", | ||
|
Collaborator
There was a problem hiding this comment. i'm not sure i understand the need for added xattrs... i checked what i know and these additions aren't useful in this case from what i can gather
Contributor
Author
There was a problem hiding this comment. We restore |
||
| "-C", targetRoot, "-xf", localPath) | ||
|
Comment on lines
+196
to
+205
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: # Inspect the reviewed extraction path, its direct callers/contracts, and the
# repository conventions relevant to rootfs restore and container compatibility.
set -eu
printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/ai-dynamo-snapshot-44b65c85 -maxdepth 2 -type f -name '*.md' -print \
| sort \
| while read -r f; do
printf '\n--- %s ---\n' "$f"
head -80 "$f"
done
printf '%s\n' '--- overlay.go relevant source ---'
sed -n '130,225p' agent/internal/runtime/overlay.go
printf '%s\n' '--- direct references ---'
rg -n -C 3 'ApplyRootfsDiff|xattrs-include|snapshot-smoke|tar-builder' agent Dockerfile* .github 2>/dev/null || trueRepository: ai-dynamo/snapshot Length of output: 19117 🏁 Script executed: # Inspect the tar build/runtime contract and the capture-side archive options,
# then read the existing extraction tests around xattrs.
set -eu
printf '%s\n' '--- tar builder and smoke test ---'
sed -n '90,160p' agent/Dockerfile
printf '%s\n' '--- capture-side tar references ---'
rg -n -C 5 --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob 'Dockerfile*' \
'rootfs-diff|tar .*xattrs|--xattrs|upperdir|rootfs diff' agent
printf '%s\n' '--- extraction tests ---'
sed -n '1,80p' agent/internal/runtime/overlay_test.go
sed -n '175,320p' agent/internal/runtime/overlay_test.goRepository: ai-dynamo/snapshot Length of output: 36514 🌐 Web query:
💡 Result: When using GNU tar, the --xattrs option does not extract all extended attributes by default, even if they are present in the archive [1][2]. Specifically, GNU tar restricts default extraction to the user.* namespace because other namespaces, particularly security.* (such as security.capability and security.selinux), may contain data with binary formats or security contexts that are specific to the source system and could be incompatible or unsafe when applied to the destination system [1][3][4]. To extract attributes outside the user.* namespace, you must explicitly use the --xattrs-include=PATTERN option [1][2]. For example, to include all extended attributes, use: tar --xattrs --xattrs-include='*' -xf archive.tar For security.capability specifically, this approach requires you to ensure the binary format is compatible with the target system [3]. Note that for security.selinux, GNU tar provides a dedicated --selinux option, which is the preferred method for handling SELinux contexts rather than relying on generic xattr inclusion [1][5][4]. Regarding restore failures on unsupported filesystems: If the destination filesystem does not support extended attributes (e.g., certain configurations of FAT, older filesystems, or specific network mounts), attempting to restore them will trigger an error (typically Operation not supported or ENOTSUP) [6][7][8][9]. GNU tar's behavior when encountering these errors can cause the extraction process to fail or abort, depending on the version and environment [7][8]. In some workflows, tools may need to wrap syscalls to handle these failures gracefully (e.g., ignoring them if they are unsupported) rather than treating them as fatal errors [7][8]. If you encounter "unsupported filesystem" errors, you may need to disable extended attribute extraction entirely using the --no-xattrs option if the metadata is not strictly required for your specific use case [6][5]. Citations:
🏁 Script executed: # Resolve the exact GNU tar version and restore-target assumptions before
# deciding whether the generic security.* pattern creates a supported failure
# path.
set -eu
printf '%s\n' '--- GNU tar version inputs ---'
rg -n -C 3 'GNU_TAR_(VERSION|SHA256)|tar-[0-9]' agent Makefile* .github 2>/dev/null || true
printf '%s\n' '--- restore setup around target root ---'
sed -n '90,155p' agent/internal/executor/nsrestore.go
printf '%s\n' '--- image/runtime mount configuration ---'
rg -n -C 4 'placeholder|BundleDir|mount.*root|overlay|rootfs|security.selinux|security.capability' agent/internal config deploy charts 2>/dev/null | head -240Repository: ai-dynamo/snapshot Length of output: 20440 🌐 Web query:
💡 Result: GNU tar’s manual states:
Example: tar --xattrs --xattrs-include='user.*' -xf archive.tarThe manual also advises paying attention to tar’s diagnostics and exit status during extraction. [2] Citations:
Define compatibility before restoring
🤖 Prompt for AI AgentsSource: MCP tools |
||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("tar extract with %s failed: %w", tarBinary, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| #!/bin/sh | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Build the statically linked GNU tar used for restore-time rootfs-diff | ||
| # extraction, assert it is fully static, and smoke-test the exact invocation | ||
| # runtime.ApplyRootfsDiff uses. Installs the binary and its license text to | ||
| # /out. Run from the unpacked GNU tar source directory. | ||
| # | ||
| # Usage: build-gnu-tar.sh | ||
| set -eu | ||
|
|
||
| # FORCE_UNSAFE_CONFIGURE: GNU tar's configure refuses to run as root otherwise. | ||
| FORCE_UNSAFE_CONFIGURE=1 LDFLAGS=-static ./configure \ | ||
| --disable-nls \ | ||
| --without-posix-acls \ | ||
| --without-selinux | ||
| make -j"$(nproc)" | ||
|
|
||
| mkdir -p /out | ||
| cp src/tar /out/tar | ||
| cp COPYING /out/COPYING | ||
| chmod 0755 /out/tar | ||
| strip /out/tar | ||
|
|
||
| # A static binary has neither an ELF interpreter nor NEEDED entries; either | ||
| # one means the placeholder's userspace would leak back into restore. | ||
| if readelf -l /out/tar | grep -q 'INTERP'; then | ||
| echo "ERROR: restore tar has a dynamic ELF interpreter" >&2 | ||
| exit 1 | ||
| fi | ||
| if readelf -d /out/tar | grep -q '(NEEDED)'; then | ||
| echo "ERROR: restore tar has shared-library dependencies" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Smoke test mirroring the restore invocation in runtime.ApplyRootfsDiff — | ||
| # keep these flags in sync with that call site. | ||
| mkdir -p /tmp/tar-smoke/source /tmp/tar-smoke/target | ||
| echo preserved > /tmp/tar-smoke/target/existing | ||
| echo replaced > /tmp/tar-smoke/source/existing | ||
| echo created > /tmp/tar-smoke/source/created | ||
| setfattr -n user.snapshot-smoke -v smoke /tmp/tar-smoke/source/created | ||
| /out/tar --xattrs -C /tmp/tar-smoke/source -cf /tmp/tar-smoke/archive.tar . | ||
| /out/tar --skip-old-files --blocking-factor=2048 \ | ||
| --xattrs --xattrs-include='user.*' --xattrs-include='security.*' \ | ||
| -C /tmp/tar-smoke/target -xf /tmp/tar-smoke/archive.tar | ||
| grep -qx preserved /tmp/tar-smoke/target/existing | ||
| grep -qx created /tmp/tar-smoke/target/created | ||
| test "$(getfattr --only-values -n user.snapshot-smoke /tmp/tar-smoke/target/created)" = smoke |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #!/bin/sh | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Download and verify the pinned GNU tar source tarball, keep a pristine copy | ||
| # at /gnu-tar-source.tar.xz for the image's corresponding-source staging, and | ||
| # unpack the source into the current directory for the build. | ||
| # | ||
| # Usage: fetch-gnu-tar.sh <version> <sha256> | ||
| set -eu | ||
|
|
||
| VERSION="$1" | ||
| SHA256="$2" | ||
|
|
||
| curl -fsSLo gnu-tar.tar.xz "https://ftp.gnu.org/gnu/tar/tar-${VERSION}.tar.xz" | ||
| echo "${SHA256} gnu-tar.tar.xz" | sha256sum -c - | ||
| cp gnu-tar.tar.xz /gnu-tar-source.tar.xz | ||
| xz -d gnu-tar.tar.xz | ||
| tar -xf gnu-tar.tar |
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: ai-dynamo/snapshot
Length of output: 16907
🏁 Script executed:
Repository: ai-dynamo/snapshot
Length of output: 42670
🏁 Script executed:
Repository: ai-dynamo/snapshot
Length of output: 36043
Validate
BundleDirat every restore entry point.If
RestoreOptions.BundleDiris empty or relative,executeRestorepasses a relativetarBinarytoApplyRootfsDiff, which rejects it before checking forrootfs-diff.tar. The host controller passes/tmp/snapshot-binaries, butRestoreInNamespaceand the--bundle-diroverride do not enforce this contract.🤖 Prompt for AI Agents