SR: Phase 5: Node-builder scripts and quality fixups - #558
Cameron E Baird (Camelron) merged 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical factory-sizing and moderate custom-root snapshot issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates runtime-rs snapshot restoration to use a configured snapshot root and adds Azure Linux kata-v2 configurations.
Changes:
- Resolves named snapshots beneath a validated
snapshot_root. - Adds snapshot-path, manifest, and annotation validation.
- Generates and installs runtime-rs kata-v2 configurations.
Review findings:
- Critical: Factory templates may ignore pod-specific static CPU and memory sizing.
- Moderate: Snapshot finalization hard-codes the default root instead of the configured root.
- Nit: Update the restore design document and examples for the renamed annotation.
File summaries
| File | Description |
|---|---|
tools/osbuilder/node-builder/azure-linux/package_install.sh |
Installs kata-v2 configurations. |
tools/osbuilder/node-builder/azure-linux/package_build.sh |
Generates kata-v2 variants; factory sizing requires correction. |
tools/osbuilder/node-builder/azure-linux/common.sh |
Defines kata-v2 configuration names. |
src/tools/kata-ctl/src/args.rs |
Updates the snapshot-path fixture. |
src/runtime-rs/crates/runtimes/virt_container/src/sandbox.rs |
Resolves and validates named snapshots. |
src/runtime-rs/crates/runtimes/virt_container/src/lib.rs |
Detects the new restore annotation. |
src/runtime-rs/crates/runtimes/src/manager.rs |
Validates snapshot-name usage. |
src/runtime-rs/crates/resource/src/rootfs/snapshot.rs |
Identifies restored disks; must honor configured snapshot roots. |
src/runtime-rs/config/configuration-clh-runtime-rs.toml.in |
Adds the default snapshot root. |
src/runtime-rs/config/configuration-clh-azure-runtime-rs.toml.in |
Adds the Azure snapshot root. |
src/libs/kata-types/src/config/runtime.rs |
Adds snapshot-root configuration and defaults. |
src/libs/kata-types/src/config/mod.rs |
Exports the default-root constant. |
src/libs/kata-types/src/annotations/mod.rs |
Renames the restore annotation; documentation needs updating. |
Review details
Suppressed comments (1)
src/libs/kata-types/src/annotations/mod.rs:38
- The public annotation rename leaves
RUNTIME-RS-SNAPSHOT-RESTORE-PORT-DESIGN.mddocumenting and illustratingio.katacontainers.restore-from(including its implementation checklist). Users following that document will send a key the runtime now ignores and get a cold boot instead of restore. Update the design document and examples to the new annotation in the same change.
/// Selects a packaged VM snapshot beneath the configured snapshot root.
pub const KATA_ANNO_SNAPSHOT_NAME: &str = "io.katacontainers.snapshot-name";
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3d2bb82 to
86494b1
Compare
86494b1 to
7f4ef21
Compare
7f4ef21 to
cf4f03c
Compare
cf4f03c to
2e1634d
Compare
2e1634d to
29f2472
Compare
29f2472 to
6abc240
Compare
6abc240 to
fca89e8
Compare
|
|
||
| # Root directory containing packaged VM snapshots selected by the | ||
| # io.katacontainers.snapshot-name annotation. | ||
| snapshot_root = "/var/lib/kata/snapshots" |
There was a problem hiding this comment.
Is this path an AKS requirement? If not we should use one of the existing Kath paths e.g. /run/kata-containers/...
There was a problem hiding this comment.
The problem with /run is that it is tmpfs ram on the AKS host.
There was a problem hiding this comment.
Whereas we'd prefer to utilize real disk space instead. Having it in ram is faster, but only marginally (10-15% speedup in restore timings) and introduces a lot of questions about host resource hogging, running out of space, etc.
419fd9e to
6dec5c9
Compare
6dec5c9 to
d8707a2
Compare
d8707a2 to
657d39a
Compare
1591ba2 to
c754b55
Compare
1426a92 to
00726c2
Compare
3e8aff3 to
dc31465
Compare
dc31465 to
a21024f
Compare
a21024f to
cd38523
Compare
41d732c to
c0cc9e3
Compare
| echo "Installing runtime-rs kata-v2 configuration" | ||
| cp -a --backup=numbered "${CONFIG_DIR_RUNTIME_RS}/${SHIM_V2_CONFIG_FILE_NAME_RUNTIME_RS}" "${PREFIX}/${SHIM_CONFIG_PATH}/${SHIM_V2_CONFIG_FILE_NAME_RUNTIME_RS}" | ||
| cp -a --backup=numbered "${CONFIG_DIR_RUNTIME_RS}/${SHIM_V2_DBG_CONFIG_FILE_NAME_RUNTIME_RS}" "${PREFIX}/${SHIM_CONFIG_PATH}/${SHIM_V2_DBG_CONFIG_FILE_NAME_RUNTIME_RS}" | ||
|
|
There was a problem hiding this comment.
nit: Should in the Installing configurations side by side block above.
There was a problem hiding this comment.
We effectively are in that block; it is just broken down by sections:
echo "Installing configurations side by side"
..
echo "Installing preview (VM templating) configurations"
..
echo "Installing runtime-rs kata-v2 configuration"
..
All are gated by [[ "${SHIM_REDEPLOY_CONFIG}" == "yes" ]]
| .PHONY: force-generated-code | ||
| force-generated-code: | ||
|
|
||
| $(GENERATED_CODE): force-generated-code |
There was a problem hiding this comment.
Unclear why we need this - isn't TARGET_PATH being phony enough?
There was a problem hiding this comment.
TARGET_PATH being phony reruns Cargo, but Make does not track changes to PACKAGE_VERSION, so config.rs could retain the previous value when config.rs.in is unchanged. You could accidentally build with whatever the old PACKAGE_VERSION was you passed in a previous build.
Now, GENERATED_CODE is phony and making TARGET_PATH depends on it, which guarantees correct ordering.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved security, data-integrity, and snapshot-format issues span protocol, guest filesystem, and restore paths.
Review details
Files not reviewed (1)
- src/runtime/virtcontainers/pkg/agent/protocols/grpc/agent.pb.go: Generated file
- Files reviewed: 30/31 changed files
- Comments generated: 4
- Review effort level: Balanced
c0cc9e3 to
39028de
Compare
restore-from accepted arbitrary absolute host paths as the snapshot
location. To harden the security aspects of this feature,
instead expose two things:
- kata config [runtime] snapshot_root config. An absolute path,
default /var/lib/kata-containers/snapshots
- io.katacontainers.snapshot-name annotation path. The result is that
the requested snapshot should be found under
{snapshot_root}/{snapshot_name}
Enforce that the resolved path is a canonical (no ../) absolute
path with no symlinks, ending in a directory containing a proper
snapshot manifest.
Signed-off-by: Cameron Baird <cameronbaird@microsoft.com>
Assisted-by: Sol:5.6
In the node-builder, prepare the config for kata-v2. It: - Supports VM templating - Supports Snapshot/Restore - Consequentially, turns off shared_fs - Turns off virtio-mem resizing - Currently relies on the static_default_* configs for VM size Signed-off-by: Cameron Baird <cameronbaird@microsoft.com> Assisted-by: Sol:5.6
Populate in the SnapshotManifest format_version (int, denotes the version of the SnapshotManifest schema for backwards compat). Also populate in the SnapshotManifest runtime_version (string, denotes what version of kata created the snapshot). On the restore path, the runtime must parse the runtime_version, check for exact equality, and reject immediately mismatched runtime_versions. It then introduces a match on format_version so that handling for older snapshot manifests can be added in the future. Signed-off-by: Cameron Baird <cameronbaird@microsoft.com> Assisted-by: Sol:5.6
The existing mechanism for forwarding new forward api volumes (/etc/hosts, /etc/resolv.conf, etc) works, but the restored bind mount still references the original, now-empty inode. Here we add a preserve_inode parameter to CopyFileRequest, allowing a file to be replaced in-line. Signed-off-by: Cameron Baird <cameronbaird@microsoft.com> Assisted-by: Sol:5.6
39028de to
364e6fb
Compare
e40d1e5
into
msft-main
Quality Fixups:
Node-builder: