From d0997fd3b50882404eb4b4e10773203e7d4b9cd1 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 28 Apr 2026 17:36:03 -0300 Subject: [PATCH] model,cli,docs: medium-effort polish for v0.1.0 * model.ParseSize / FormatSizeBytes: pinned with table tests in internal/model/types_test.go (TestParseSize 22 cases, TestFormatSizeBytes 11 cases, TestParseSizeFormatRoundTrip 7 boundaries). Fixed the long-suffix regression: "4GiB", "512MiB", "4KiB" now parse correctly (parser strips trailing IB before inspecting the unit byte). Pinned current behaviour for no-suffix input ("1024" treated as MiB) and FormatSizeBytes(0). commands_image.go --size flag-help updated to show 4GiB now that the parser accepts it. * vm ports --json: matches the JSON-vs-table inconsistency between vm stats (always JSON) and vm ports (always table). --json on vm ports flips to the same printJSON path as vm stats. Default table output unchanged. Other vm subcommands (show, stats, logs, health, ping) didn't fit the identical pattern; left alone. * docs/oci-import.md architecture section moved to a new docs/oci-import-internals.md (precedent: internal/daemon/ ARCHITECTURE.md). User-facing oci-import.md keeps a one-line pointer for advanced reading. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/oci-import-internals.md | 44 +++++++++++ docs/oci-import.md | 36 +-------- internal/cli/commands_image.go | 2 +- internal/cli/commands_vm.go | 8 +- internal/model/types.go | 13 ++-- internal/model/types_test.go | 135 +++++++++++++++++++++++++++++++++ 6 files changed, 196 insertions(+), 42 deletions(-) create mode 100644 docs/oci-import-internals.md create mode 100644 internal/model/types_test.go diff --git a/docs/oci-import-internals.md b/docs/oci-import-internals.md new file mode 100644 index 0000000..434a01e --- /dev/null +++ b/docs/oci-import-internals.md @@ -0,0 +1,44 @@ +# OCI import — internals + +> **Advanced reading.** This document describes implementation details of the +> OCI import pipeline. It is not needed for day-to-day use of +> `banger image pull`. User-facing documentation is in +> [`docs/oci-import.md`](oci-import.md). + +## Architecture + +`internal/imagepull/` owns the mechanics: + +- **`Pull`** wraps `go-containerregistry`'s `remote.Image` with the + `linux/amd64` platform pinned. Layer blobs cache under + `~/.cache/banger/oci/blobs/` and populate lazily during flatten. +- **`Flatten`** replays layers oldest-first into a staging directory, + applies whiteouts, rejects unsafe paths plus filenames that banger's + debugfs ownership fixup cannot encode safely. Returns a `Metadata` + map of per-file uid/gid/mode from tar headers. +- **`BuildExt4`** runs `mkfs.ext4 -F -d -E root_owner=0:0` + at the size of the pre-truncated file — no mount, no sudo, no + loopback. Requires `e2fsprogs ≥ 1.43`. +- **`ApplyOwnership`** streams a batched `set_inode_field` script to + `debugfs -w` to rewrite per-file uid/gid/mode to the captured tar- + header values. +- **`InjectGuestAgents`** uses the same `debugfs` scripting to drop + banger's guest assets into the ext4 with root ownership: + vsock agent binary, network bootstrap + unit, first-boot script + + unit, `multi-user.target.wants` symlinks, vsock modules-load + config, `/var/lib/banger/first-boot-pending` marker. + +`internal/daemon/images_pull.go` orchestrates `pullFromOCI`: + +1. Parse + validate the OCI ref, derive a default name when `--name` + is omitted (`debian-bookworm` from + `docker.io/library/debian:bookworm`). +2. Resolve kernel info via `resolveKernelInputs` (auto-pulls from + `kernelcat` if `--kernel-ref` names a catalog entry that isn't + yet local). +3. Stage at `/.staging`; extract layers to a temp + tree under `$TMPDIR`. +4. `BuildExt4` → `ApplyOwnership` → `InjectGuestAgents`. +5. `imagemgr.StageBootArtifacts` stages the kernel triple alongside. +6. Atomic `os.Rename` publishes the artifact dir. +7. Persist a `model.Image{Managed: true, …}` record. diff --git a/docs/oci-import.md b/docs/oci-import.md index d06c14b..6160b7c 100644 --- a/docs/oci-import.md +++ b/docs/oci-import.md @@ -61,41 +61,7 @@ banger image pull ghcr.io/myorg/devimg:v2 --kernel-ref generic-6.12 ## Architecture -`internal/imagepull/` owns the mechanics: - -- **`Pull`** wraps `go-containerregistry`'s `remote.Image` with the - `linux/amd64` platform pinned. Layer blobs cache under - `~/.cache/banger/oci/blobs/` and populate lazily during flatten. -- **`Flatten`** replays layers oldest-first into a staging directory, - applies whiteouts, rejects unsafe paths plus filenames that banger's - debugfs ownership fixup cannot encode safely. Returns a `Metadata` - map of per-file uid/gid/mode from tar headers. -- **`BuildExt4`** runs `mkfs.ext4 -F -d -E root_owner=0:0` - at the size of the pre-truncated file — no mount, no sudo, no - loopback. Requires `e2fsprogs ≥ 1.43`. -- **`ApplyOwnership`** streams a batched `set_inode_field` script to - `debugfs -w` to rewrite per-file uid/gid/mode to the captured tar- - header values. -- **`InjectGuestAgents`** uses the same `debugfs` scripting to drop - banger's guest assets into the ext4 with root ownership: - vsock agent binary, network bootstrap + unit, first-boot script + - unit, `multi-user.target.wants` symlinks, vsock modules-load - config, `/var/lib/banger/first-boot-pending` marker. - -`internal/daemon/images_pull.go` orchestrates `pullFromOCI`: - -1. Parse + validate the OCI ref, derive a default name when `--name` - is omitted (`debian-bookworm` from - `docker.io/library/debian:bookworm`). -2. Resolve kernel info via `resolveKernelInputs` (auto-pulls from - `kernelcat` if `--kernel-ref` names a catalog entry that isn't - yet local). -3. Stage at `/.staging`; extract layers to a temp - tree under `$TMPDIR`. -4. `BuildExt4` → `ApplyOwnership` → `InjectGuestAgents`. -5. `imagemgr.StageBootArtifacts` stages the kernel triple alongside. -6. Atomic `os.Rename` publishes the artifact dir. -7. Persist a `model.Image{Managed: true, …}` record. +> Implementation details live in [`docs/oci-import-internals.md`](oci-import-internals.md). ## Guest-side boot sequence diff --git a/internal/cli/commands_image.go b/internal/cli/commands_image.go index fd9c65d..4cd4911 100644 --- a/internal/cli/commands_image.go +++ b/internal/cli/commands_image.go @@ -235,7 +235,7 @@ subcommand lands). cmd.Flags().StringVar(¶ms.InitrdPath, "initrd", "", "initrd path") cmd.Flags().StringVar(¶ms.ModulesDir, "modules", "", "modules dir") cmd.Flags().StringVar(¶ms.KernelRef, "kernel-ref", "", "name of a cataloged kernel (see 'banger kernel list')") - cmd.Flags().StringVar(&sizeRaw, "size", "", "ext4 image size (e.g. 4GiB); defaults to content + 25%, min 1GiB") + cmd.Flags().StringVar(&sizeRaw, "size", "", "ext4 image size, e.g. 4GiB, 512M, 2G (defaults to content + 25%, min 1GiB)") _ = cmd.RegisterFlagCompletionFunc("kernel-ref", d.completeKernelNames) return cmd } diff --git a/internal/cli/commands_vm.go b/internal/cli/commands_vm.go index bfda996..2ed950c 100644 --- a/internal/cli/commands_vm.go +++ b/internal/cli/commands_vm.go @@ -875,7 +875,8 @@ func (d *deps) newVMStatsCommand() *cobra.Command { } func (d *deps) newVMPortsCommand() *cobra.Command { - return &cobra.Command{ + var jsonOut bool + cmd := &cobra.Command{ Use: "ports ", Short: "Show host-reachable listening guest ports", Args: exactArgsUsage(1, "usage: banger vm ports "), @@ -889,9 +890,14 @@ func (d *deps) newVMPortsCommand() *cobra.Command { if err != nil { return err } + if jsonOut { + return printJSON(cmd.OutOrStdout(), result) + } return printVMPortsTable(cmd.OutOrStdout(), result) }, } + cmd.Flags().BoolVar(&jsonOut, "json", false, "print ports as JSON instead of a table") + return cmd } type resolvedVMTarget struct { diff --git a/internal/model/types.go b/internal/model/types.go index 1121b3a..e533602 100644 --- a/internal/model/types.go +++ b/internal/model/types.go @@ -240,23 +240,26 @@ func ParseSize(raw string) (int64, error) { if raw == "" { return 0, errors.New("size is required") } - unit := raw[len(raw)-1] + // Strip an optional "IB" suffix so that "GiB", "MiB", "KiB" work the + // same as "G", "M", "K" (case-insensitive after ToUpper). + number := strings.TrimSuffix(raw, "IB") + unit := number[len(number)-1] multiplier := int64(1024 * 1024) - number := raw switch unit { case 'K': multiplier = 1024 - number = raw[:len(raw)-1] + number = number[:len(number)-1] case 'M': multiplier = 1024 * 1024 - number = raw[:len(raw)-1] + number = number[:len(number)-1] case 'G': multiplier = 1024 * 1024 * 1024 - number = raw[:len(raw)-1] + number = number[:len(number)-1] default: if unit < '0' || unit > '9' { return 0, fmt.Errorf("unsupported size suffix: %q", string(unit)) } + number = raw // no suffix stripped — keep original digits-only string } value, err := strconv.ParseInt(number, 10, 64) if err != nil { diff --git a/internal/model/types_test.go b/internal/model/types_test.go new file mode 100644 index 0000000..d2c0de4 --- /dev/null +++ b/internal/model/types_test.go @@ -0,0 +1,135 @@ +package model + +import ( + "strings" + "testing" +) + +func TestParseSize(t *testing.T) { + const ( + kib = int64(1024) + mib = int64(1024 * 1024) + gib = int64(1024 * 1024 * 1024) + ) + + cases := []struct { + name string + input string + want int64 + wantErrSub string + }{ + // Happy path — short suffixes. + {"1G", "1G", gib, ""}, + {"512M", "512M", 512 * mib, ""}, + {"4K", "4K", 4 * kib, ""}, + {"4G", "4G", 4 * gib, ""}, + + // GiB/MiB/KiB suffixes — parser now accepts these. + {"4GiB", "4GiB", 4 * gib, ""}, + {"512MiB", "512MiB", 512 * mib, ""}, + {"4KiB", "4KiB", 4 * kib, ""}, + + // Lowercase — ToUpper normalises; should work like uppercase. + {"lowercase 1g", "1g", gib, ""}, + {"lowercase 512m", "512m", 512 * mib, ""}, + {"lowercase 4gib", "4gib", 4 * gib, ""}, + + // No-suffix — treated as MiB (the parser's default multiplier is 1 MiB). + // "1024" → 1024 MiB, "1" → 1 MiB. + {"no-suffix 1024", "1024", 1024 * mib, ""}, + {"no-suffix 1", "1", mib, ""}, + + // Whitespace trimming. + {"leading space", " 2G", 2 * gib, ""}, + {"trailing space", "2G ", 2 * gib, ""}, + {"both spaces", " 2G ", 2 * gib, ""}, + + // Error cases. + {"empty string", "", 0, "required"}, + {"whitespace only", " ", 0, "required"}, + {"unknown suffix B", "512B", 0, "unsupported size suffix"}, + {"negative", "-1G", 0, "positive"}, + {"zero", "0G", 0, "positive"}, + {"overflow MaxDiskBytes", "129G", 0, "exceeds max"}, + {"non-numeric", "xG", 0, "parse size"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := ParseSize(tc.input) + if tc.wantErrSub != "" { + if err == nil { + t.Fatalf("ParseSize(%q) = %d, want error containing %q", tc.input, got, tc.wantErrSub) + } + if !strings.Contains(err.Error(), tc.wantErrSub) { + t.Fatalf("ParseSize(%q) error = %q, want substring %q", tc.input, err.Error(), tc.wantErrSub) + } + return + } + if err != nil { + t.Fatalf("ParseSize(%q) unexpected error: %v", tc.input, err) + } + if got != tc.want { + t.Fatalf("ParseSize(%q) = %d, want %d", tc.input, got, tc.want) + } + }) + } +} + +func TestFormatSizeBytes(t *testing.T) { + const ( + kib = int64(1024) + mib = int64(1024 * 1024) + gib = int64(1024 * 1024 * 1024) + ) + + cases := []struct { + name string + input int64 + want string + }{ + // FormatSizeBytes(0): 0 is divisible by GiB so it formats as "0G". + {"0", 0, "0G"}, + {"1 byte", 1, "1"}, + {"1 KiB", kib, "1K"}, + {"4 KiB", 4 * kib, "4K"}, + {"1 MiB", mib, "1M"}, + {"512 MiB", 512 * mib, "512M"}, + {"1 GiB", gib, "1G"}, + {"4 GiB", 4 * gib, "4G"}, + {"128 GiB (max disk)", 128 * gib, "128G"}, + // Non-round: falls through to raw bytes. + {"non-round bytes", 1500, "1500"}, + {"non-round MiB", 3*mib + 1, "3145729"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := FormatSizeBytes(tc.input) + if got != tc.want { + t.Fatalf("FormatSizeBytes(%d) = %q, want %q", tc.input, got, tc.want) + } + }) + } +} + +func TestParseSizeFormatRoundTrip(t *testing.T) { + const ( + kib = int64(1024) + mib = int64(1024 * 1024) + gib = int64(1024 * 1024 * 1024) + ) + + boundaries := []int64{kib, 4 * kib, mib, 512 * mib, gib, 4 * gib, 8 * gib} + for _, n := range boundaries { + formatted := FormatSizeBytes(n) + parsed, err := ParseSize(formatted) + if err != nil { + t.Errorf("ParseSize(FormatSizeBytes(%d) = %q): %v", n, formatted, err) + continue + } + if parsed != n { + t.Errorf("round-trip(%d): FormatSizeBytes → %q → ParseSize → %d", n, formatted, parsed) + } + } +}