Skip to content

[snapshot] new d8 snapshot command - #422

Open
kkozoriz wants to merge 442 commits into
mainfrom
feat/d8-snapshot
Open

[snapshot] new d8 snapshot command#422
kkozoriz wants to merge 442 commits into
mainfrom
feat/d8-snapshot

Conversation

@kkozoriz

@kkozoriz kkozoriz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Adds d8 snapshot, a command family for backing up and restoring Deckhouse
workloads without requiring direct connectivity between the source and
destination clusters. A Snapshot CR captures a namespace's manifests and
volume data in-cluster; the CLI can then move that data offline as a plain
directory tree and restore it later into the same or a different
namespace/cluster.

Requires the state-snapshotter and storage-foundation Deckhouse
modules to be enabled on the cluster
— the CLI talks to their aggregated
API and DataExport/DataImport CRDs, it doesn't implement snapshotting
itself.

Breaking changes

The pre-existing d8 data export/d8 data import commands are updated to
match storage-foundation's current contract (API group move, an added
finalize step on upload, and a changed expiry signal). This requires a
matching storage-foundation version — clusters on an older one are not
compatible.

Commands

  • create — creates a Snapshot object capturing a namespace's state.
    An empty selector captures the whole namespace; --selector limits it to
    matching labels. Reconciled asynchronously by state-snapshotter;
    --wait blocks until it's Ready.
  • list — lists Snapshot resources in the cluster.
  • describe — shows a snapshot's manifest hierarchy as a tree,
    in-cluster.
  • download — downloads a snapshot (manifests + volume data) from the
    cluster into a local directory tree.
  • local describe / local get — inspect a downloaded archive's tree
    or get a one-line summary, fully offline.
  • import — uploads a downloaded archive into a namespace. Rebuilds
    the tree top-down (parents before children, then manifests, then volume
    data last, since a leaf's DataImport stays pending until its ancestors'
    manifests exist). --node restricts the upload to a single subtree.
  • restore — restores a snapshot in-place, into the same namespace it
    came from. Walks the hierarchy node by node, stages and validates the
    whole tree before applying anything. Cross-namespace/cluster restore is
    download + import + restore on the target, not a single command.
  • delete — deletes one or more Snapshot objects (by name, by
    --selector, or --all), letting state-snapshotter garbage-collect the
    associated data.

Archive layout

A downloaded snapshot is a plain directory tree, not a single blob —
manifests and volume data live side by side, and child resources nest
recursively:

<root>/
├── snapshot.yaml        # node metadata + checksum of this node's contents
├── manifests/
│   └── <kind>_<name>.yaml
├── data.tar / data.bin  # volume data (filesystem or block), optionally compressed
└── snapshots/           # child nodes, same structure recursively
    └── <kind>_<name>/
        └── ...

Each node is checksummed independently, so downloads/imports can resume
after an interruption without re-verifying the whole tree.

Typical workflow

  1. d8 snapshot create — capture a namespace's state in the source cluster.
  2. d8 snapshot download — pull manifests + volume data to a local directory.
  3. Move the directory wherever it needs to go (offline storage, another
    environment). Inspect it with local describe/local get if needed.
  4. d8 snapshot import (into a new/different namespace or cluster) or
    d8 snapshot restore (in place, same namespace).
  5. list / describe / delete for day-to-day lifecycle management.

Tests

Unit tests across all new packages, using fake Kubernetes clients and local
HTTP/TLS test servers — no real cluster required. Coverage includes the
download/restore/import pipeline, checksum verification, resumable/chunked
downloads across compression codecs, and the platform-specific archive
safety code that guards against symlink and mount-boundary escapes when
extracting an archive. Concurrency-sensitive packages (pipeline,
volume) are also covered under -race.

Notes

  • Builds and passes lint/tests on Linux, macOS, and Windows.
  • New dependencies are limited to what the feature needs (CSI snapshot
    client, compression codecs) — no unrelated version bumps.

kkozoriz added 30 commits July 7, 2026 18:23
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
… it forever

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…ckoff

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…er-chunk all-or-nothing

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…rable partial offset

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
… resume

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…ceeded

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
… sidecar

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…ine test

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…targets

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…le part file

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…rectory

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…pace

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
kkozoriz added 4 commits July 28, 2026 23:41
KubeconfigNamespace only served snapshot's get/create/delete/describe
commands, so keep the loading-rules duplication local to
internal/snapshot/transport instead of growing the shared
internal/utilk8s/clientset.go — restored to match main.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…eekable-format-go

zstd-seekable-format-go (and its need for klauspost/compress >= v1.18.6) was
removed when that resume design was reverted, but the go.mod bump wasn't.
Nothing in the tree needs more than v1.18.0; go.sum reconciled with go mod tidy.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
http.go was edited during snapshot work and later reverted to match main
once the logic moved into internal/snapshot/transport; the tests added
alongside those edits target a file that never actually changed here.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Guide operators to --edit only when pre-mutation Kubernetes validation or a
Bound PVC refusal can be corrected before apply. Preserve original causes and
suppress the guidance once editing ran or mutation began.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
@kkozoriz kkozoriz changed the title [snapshot] new d8 snapshot [snapshot] new d8 snapshot command Jul 29, 2026
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>

# Conflicts:
#	cmd/d8/root.go
#	go.mod
@kkozoriz
kkozoriz marked this pull request as ready for review July 29, 2026 11:11
@kkozoriz
kkozoriz requested a review from ldmonster as a code owner July 29, 2026 11:12
…onfig/build-tag gaps

golangci-lint on the CI linux runner flagged openArchiveRootUnix as unused:
it's only called from darwin/freebsd/openbsd sources, but the file defining
it was tagged //go:build unix, which also compiles on linux. Move the
function into its own file tagged darwin || freebsd || openbsd; the
remaining reflection-based mount-identity helpers stay under unix since a
linux-only test file exercises them directly with synthetic data.

Several transport/exporter/pipeline tests called transport.NewClient() with
no explicit kubeconfig, relying on the ambient $HOME/.kube/config to exist.
That happened to work on dev machines but fails on a clean CI runner with
no kubeconfig file. Added a small per-package newTestKubeconfigFlags(t)
helper (temp file + --kubeconfig flag, not t.Setenv, since these tests use
t.Parallel) so the affected tests are hermetic.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
@kkozoriz kkozoriz self-assigned this Jul 29, 2026
@kkozoriz
kkozoriz force-pushed the feat/d8-snapshot branch 2 times, most recently from d24eb58 to cc0bead Compare July 29, 2026 14:39
kkozoriz added 7 commits July 29, 2026 18:25
…nistic

Both failures were in the tests, not in DownloadFilesystemVolume.

LargeFlatInventoryBoundedHeap sampled MemStats.HeapAlloc, which counts
garbage the collector has not swept yet. That term is GOGC pacing
headroom, proportional to the whole test binary's resting live heap and to
how long a run keeps allocating -- not to what the download path retains.
Isolated it stays around 2 MiB, but in a full-package run whose resting
heap is tens of MiB, streaming 30k entries (~360 MB of churn) reads tens
of MiB larger than the 500-entry run purely because it allocates longer,
which is exactly the 30 MB delta CI reported. Reproduced locally by
holding a 40 MB balloon: 3.7 MB versus 34 MB, matching CI. Sample
/gc/heap/live:bytes instead, forcing a collection every few ticks so the
series keeps advancing; the count-dependent delta then stays well under a
MiB with every core busy, so the bound tightens to 2 MiB. smallEntries
rises to 3k so both runs are sampled comparably.

InventoryErrorCancelsBlockedWorker raced the inventory reader against the
truncation meant to corrupt the tail underneath it: with workers == 500
the job queue never applied back-pressure, so the reader could finish the
whole spool before the first HEAD truncated it. Nothing then cancelled the
workers parked in that fake transport and the test timed out, which is
what CI saw and what GOMAXPROCS=1 reproduces locally 100% of the time. Cap
workers at 2 so the queue holds the reader within its first buffer fill
until the truncation lands, and let non-blocking calls reach the server so
the reader is pulled forward to the corrupt tail. The listing now ends in
link entries, which need no staging, so reaching that tail costs 8
downloads instead of 400 (0.3s, was 19s). The hang detector is loosened to
30s since it only guards against a real deadlock.

Verified: ./internal/snapshot/... -race -count=3 green; the two tests
green at -count=25 under GOMAXPROCS 1/2/4/16, under -race, and with all
cores loaded; golangci-lint clean.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
The DataImport typed client stayed on the legacy storage.deckhouse.io
group while DataExport was already migrated to
storage-foundation.deckhouse.io in this same branch. The legacy CRD is
removed by storage-foundation's 025-migrate-legacy-crds hook and the
SVDM controller only serves the new group, so `d8 data import
create/upload/delete` would fail outright after merging main.

Also fixes stale docs/comments referencing the removed
`d8 snapshot import` command name (the actual command is
`d8 snapshot upload`), and adds a cross-check test pinning
snapimport's hardcoded DataImport GVR literal to the same group so
the two cannot drift apart again unnoticed.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
GetDataImportWithRestart still matched the legacy standalone
Type=="Expired" condition, which the storage-foundation controller no
longer sets. DataExport's equivalent restart loop was already updated
in this branch to the current contract (Ready=False,
Reason="Expired"); the DataImport side was missed, so an expired
DataImport was never detected and auto-restarted.

Also sets notReadyErr after a successful delete+recreate: previously
the function could fall through and return the stale object's
already-populated status.url/status.volumeMode as if the fresh import
were ready, before the new DataImport actually became Ready.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
alignDataImportTTL patched spec.ttl with a bare Update and no
retry.RetryOnConflict, unlike the equivalent reconcileExistingMarker
logic in the same package. A single 409 from a concurrent
status-writing controller aborted the entire `d8 snapshot upload` run
instead of retrying.

The retry re-Gets the object on conflict rather than resubmitting the
same stale revision, and re-validates identity (validateDataImport)
and the Expired condition on every freshly fetched object before
patching — a naive retry-on-the-same-object could otherwise patch the
TTL of a foreign DataImport that raced in under the same name, or of
an object that expired mid-retry. Both cases now surface
errDataImportRecheck so EnsureDataImport's outer loop re-evaluates the
object from scratch (recreate or delete-and-recreate) instead of
treating the race as a fatal error.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
An interrupted-and-resumed download can leave a child node directory
under snapshots/ with no snapshot.yaml (archive.CollisionNodeDir
redirects the resumed node elsewhere and leaves the original partial
directory in place). archive/checksum.go already tolerates this when
verifying archive integrity, but snapimport's plan builder and
localscan's tree scanner both failed hard on it, so a verified-intact
archive could not actually be uploaded (`d8 snapshot upload`) or
inspected (`d8 snapshot local get/describe`).

Both packages now skip a child directory that carries no
snapshot.yaml the same way checksum.go already does (a non-regular
snapshot.yaml, e.g. a symlink or directory, still fails hard — the
tolerance is scoped to this exact leftover shape) and report what was
skipped. Existing exported BuildPlan*/Scan* signatures are unchanged
(they have many test call sites); a new *ReportingSkips variant in
each package surfaces the skipped paths, and the command layer
(`d8 snapshot upload`, `d8 snapshot local get/describe`) logs a
warning instead of silently dropping part of the tree.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
The package comment still named the removed `d8 snapshot import`
command; the actual command is `d8 snapshot upload` (cmdUse in
cmd/snapimport/import.go). Missed when the other stale references to
the old name were fixed alongside the DataImport API group rename.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
@ldmonster
ldmonster requested a review from Copilot July 30, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

kkozoriz added 8 commits July 30, 2026 19:10
TestImportFSFromTar_StreamingIsMemoryBounded and
TestPutBlockCompressed_StreamingIsMemoryBounded each push a >=200 MiB
payload through a real httptest.Server. On a healthy machine that is
~0.1s, and neither test had a deadline of its own: they ran on
context.Background().

Without a bound, an environment whose loopback/decode throughput
collapses does not fail these two tests -- it consumes the whole
10-minute `go test` package budget. The binary is then killed on the
package timeout, and every top-level test still parked in t.Parallel()
is reported as a failure alongside the one that actually stalled. That
is what a CI run of this package produced: 18 reported failures, with
the observed upload throughput down to ~165 KiB/s and the timeout
goroutine dump the only place naming the test that really hung.

Both tests now run on a context bounded by the shared
memoryBoundedStreamingTimeout (2 minutes, ~1000x the healthy runtime,
so it can only fire on a pathological environment and never on a
merely slow one). A stall now surfaces as a `context deadline
exceeded` failure in the responsible test, and the rest of the package
keeps running.

This does not make the CI job green by itself -- it removes the
diagnostic noise only. Why throughput collapsed in that environment is
not established here, and no cause is asserted: the commits on this
branch do not touch fs.go, the compress package, or any other code on
the stalling path.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Three tests in this package override the package-level var
maxRetryAttempts to make GetDataImportWithRestart return after one
iteration instead of looping 60+ times:
TestGetDataImportWithRestart_ExpiredRecreates,
_LegacyExpiredConditionIsNotUsed and _CompletedIsNotTreatedAsExpired.
All three also called t.Parallel(), so all three wrote and restored
that shared var concurrently.

Two consequences. It is a plain data race, and it is order-dependent:
one test's t.Cleanup restores the original 60 while another is still
running, so that test stops seeing its own -1 and instead waits out
60 attempts at retryInterval=3s -- roughly three minutes -- and then
fails on a timeout-shaped error rather than the assertion it was
written for.

t.Parallel() is removed from all three, with the reason recorded at
each. The override is left as-is: it is the existing mechanism in this
package and reworking it into an injected parameter is a wider change
than this fix.

Note on why this was never caught: `task test` runs
`gotestsum -- -count=1 -tags=...` (Taskfile.yml:110) with no -race, so
CI cannot see this class of bug at all. Adding -race to the CI test
task is a separate change and is deliberately not made here.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
`d8 data import` built spec.targetRef{kind: PersistentVolumeClaim,
pvcTemplate: ...}. The current storage-foundation DataImport CRD has
no spec.targetRef at all -- the discriminator is spec.mode and
pvcTemplate sits at the top level of the spec.

What that produced, in order. The CRD's structural schema has
preserveUnknownFields: false, so the whole spec.targetRef subtree was
pruned on admission -- silently, no error. Pruning took pvcTemplate
with it, since pvcTemplate was nested inside targetRef. spec.mode was
absent, so the schema default (CreatePVC) was applied. The CEL rule
"mode CreatePVC requires pvcTemplate" then evaluated against a spec
that had mode=CreatePVC and no pvcTemplate, and rejected the object.
The resulting admission error named pvcTemplate, which is the one
field the CLI was in fact sending.

DataImportTargetRefSpec and KindPersistentVolumeClaim are removed and
replaced by a DataImportMode string type with the single constant
DataImportModeCreatePVC. Deepcopy is updated by hand to match, as this
package's zz_generated file has no generator wired up (no hack/, no
controller-gen, no //go:generate).

Mode is sent explicitly even though the CRD defaults it: mode is
immutable after creation, so leaving it to the server-side default
would bind every object this CLI creates to whatever default the
CRD revision on that particular cluster happens to ship.

Version compatibility: a CLI with this change will not create a
DataImport against a storage-foundation deployment old enough to still
expect spec.targetRef. That is accepted and is the continuation of the
API group move already committed in b7d7ab7 -- the two versions of
the CRD are not simultaneously satisfiable from one client.

`d8 snapshot upload` is unaffected: it builds its own unstructured
PopulateData spec and never went through this type.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
spec.waitForFirstConsumer carried omitempty. The CRD defaults that
field to true, so an absent key means true on the server -- and
omitempty drops precisely the zero value, which for a bool is the
false the caller asked for.

The effect was that waitForFirstConsumer could only ever be true.
`--wffc` defaults to false (create.go:70), so the common case -- not
passing the flag at all -- produced a DataImport that waits for a first
consumer, and passing `--wffc=false` explicitly did the same thing. No
error, no warning; the flag was simply inoperative in one direction.

The tag is now `json:"waitForFirstConsumer"`, matching the field in the
storage-foundation API types, with the constraint recorded at the field
so the omitempty is not re-added for consistency with its neighbours.

Publish and Mode keep their omitempty deliberately, and are not
"aligned" along with this one: publish is not defaulted true by the
CRD, so an omitted key and false mean the same thing there, and an
omitted mode is defaulted by the CRD to the same CreatePVC the CLI
sends explicitly anyway.

Kept separate from the spec restructuring in the preceding commit
because it is a different defect with a different root cause -- a json
tag, not the shape of the struct -- and so it can be reverted on its
own.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
DataImportSpec had no test of its own; the package only tested the API
group registration. The two defects fixed in the preceding commits --
the wrong spec shape and the omitempty on waitForFirstConsumer -- were
both invisible to every existing assertion, because both are properties
of the serialised object and not of the Go struct.

New internal/data/dataimport/api/v1alpha1/data_import_test.go:

- TestDataImportSpec_JSONWireShape asserts the exact set of spec keys
  json.Marshal emits, per field, because the CRD treats them
  differently: waitForFirstConsumer must be present even when false (it
  is defaulted to true server-side), publish is correctly omitted when
  false (not defaulted), and targetRef must never appear (it would be
  pruned without an error). Verified to fail if the omitempty is put
  back on waitForFirstConsumer.

- TestDataImportSpec_DeepCopy_DoesNotAliasPvcTemplate mutates every
  reference-typed member of a copied pvcTemplate (name, the accessModes
  slice, the resources.requests map, both pointers) and asserts the
  original is untouched. This package's zz_generated deepcopy is
  hand-maintained -- no controller-gen, no //go:generate -- so nothing
  else would catch a new field that gets shallow-copied. Verified to
  fail if the accessModes branch is removed from DeepCopyInto.

Extended in util_test.go:

- TestCreateDataImport_BuildsCreatePVCSpec gets a subtest asserting
  that CreateDataImport with waitForFirstConsumer=false really puts
  "waitForFirstConsumer":false on the wire.

- TestGetDataImportWithRestart_ExpiredRecreates gets the same
  wire-shape assertion on the recreated object, since the recreate path
  rebuilds the spec from the stale object's fields instead of copying it
  and can regress independently.

Both wire-shape checks are scoped to this package's json tags, and the
comments say so: the controller-runtime fake client applies neither
structural-schema pruning nor CEL, so acceptance by a real apiserver is
not what is being proven here.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
The README still described the removed spec.targetRef.kind contract
(commit 8039617 moved every Go doc comment to spec.mode: CreatePVC
but missed this file), and its create example used the space form
--wffc false, which pflag parses as a positional arg, not a value,
and create's Args validator rejects.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Pre-existing since f651f88; gofmt -l (go1.26 stdlib formatter)
flags it even though golangci-lint's bundled formatter does not.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
…copy

Three gaps survived the existing tests, each confirmed by mutation
(existing tests stayed green while the mutated code was objectively
wrong):

- No test pinned spec.pvcTemplate's own JSON keys beyond presence —
  a metadata.name/inline mixup or a renamed accessModes/resources/
  storageClassName/volumeMode key would pass every existing test
  while the server silently drops the field or rejects the object.

- Every existing test only covered CLI -> server (building a spec).
  Nothing covered server -> CLI: decoding an object already on the
  wire, including the recreate-after-expiry path, which is exactly
  where a lost waitForFirstConsumer=false or a dropped
  storageClassName would silently reintroduce this branch's bugs.

- DeepCopy coverage stopped at the spec; EnsureDataImportPublish
  builds its patch from DeepCopy().status/metadata, so an aliased
  status.conditions or metadata.labels would produce an empty patch.

Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants