[snapshot] new d8 snapshot command - #422
Open
kkozoriz wants to merge 442 commits into
Open
Conversation
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>
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
force-pushed
the
feat/d8-snapshot
branch
from
July 29, 2026 09:43
39c8671 to
2ff7511
Compare
Signed-off-by: Konstantin Kozoriz <konstantin.kozoriz@flant.com> # Conflicts: # cmd/d8/root.go # go.mod
kkozoriz
marked this pull request as ready for review
July 29, 2026 11:11
…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
force-pushed
the
feat/d8-snapshot
branch
2 times, most recently
from
July 29, 2026 14:39
d24eb58 to
cc0bead
Compare
…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>
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>
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.
Description
Adds
d8 snapshot, a command family for backing up and restoring Deckhouseworkloads without requiring direct connectivity between the source and
destination clusters. A
SnapshotCR captures a namespace's manifests andvolume 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-snapshotterandstorage-foundationDeckhousemodules 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 importcommands are updated tomatch 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 aSnapshotobject capturing a namespace's state.An empty selector captures the whole namespace;
--selectorlimits it tomatching labels. Reconciled asynchronously by state-snapshotter;
--waitblocks until it'sReady.list— listsSnapshotresources in the cluster.describe— shows a snapshot's manifest hierarchy as a tree,in-cluster.
download— downloads a snapshot (manifests + volume data) from thecluster into a local directory tree.
local describe/local get— inspect a downloaded archive's treeor get a one-line summary, fully offline.
import— uploads a downloaded archive into a namespace. Rebuildsthe tree top-down (parents before children, then manifests, then volume
data last, since a leaf's DataImport stays pending until its ancestors'
manifests exist).
--noderestricts the upload to a single subtree.restore— restores a snapshot in-place, into the same namespace itcame from. Walks the hierarchy node by node, stages and validates the
whole tree before applying anything. Cross-namespace/cluster restore is
download+import+restoreon the target, not a single command.delete— deletes one or moreSnapshotobjects (by name, by--selector, or--all), letting state-snapshotter garbage-collect theassociated 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:
Each node is checksummed independently, so downloads/imports can resume
after an interruption without re-verifying the whole tree.
Typical workflow
d8 snapshot create— capture a namespace's state in the source cluster.d8 snapshot download— pull manifests + volume data to a local directory.environment). Inspect it with
local describe/local getif needed.d8 snapshot import(into a new/different namespace or cluster) ord8 snapshot restore(in place, same namespace).list/describe/deletefor 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
client, compression codecs) — no unrelated version bumps.