From 7fce37a0ac47859b3b4736a9b533d7dac0d440d9 Mon Sep 17 00:00:00 2001 From: Ali Asghar <98263017+alliasgher@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:42:30 -0700 Subject: [PATCH 1/8] Fix e2e cache miss on force push by saving build artifacts explicitly (#9952) * Fix e2e cache miss on force push by saving artifacts explicitly actions/cache@v4 writes the cache in a post-job hook that runs after the job reports completion. The run-e2e-test jobs (needs: build) start as soon as build completes, before that post-hook save runs, so on a force push -- where the github.sha-keyed cache has no prior entry -- they deterministically miss the cache and fail with 'stat velero.tar: no such file or directory'. Switch the build job's lookups to actions/cache/restore and add explicit actions/cache/save steps at the end of the job (CLI, image, and MinIO), so the cache is written before build reports done. The run-e2e-test reads become actions/cache/restore. Fixes #9927 Signed-off-by: alliasgher * Add changelog for #9952 Signed-off-by: alliasgher --------- Signed-off-by: alliasgher --- .github/workflows/e2e-test-kind.yaml | 35 ++++++++++++++++++++++----- changelogs/unreleased/9952-alliasgher | 1 + 2 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/9952-alliasgher diff --git a/.github/workflows/e2e-test-kind.yaml b/.github/workflows/e2e-test-kind.yaml index 79e07fded..fcf0d37c2 100644 --- a/.github/workflows/e2e-test-kind.yaml +++ b/.github/workflows/e2e-test-kind.yaml @@ -39,14 +39,14 @@ jobs: # Look for a CLI that's made for this PR - name: Fetch built CLI id: cli-cache - uses: actions/cache@v6 + uses: actions/cache/restore@v6 with: path: ./_output/bin/linux/amd64/velero # The cache key a combination of the current PR number and the commit SHA key: velero-cli-${{ github.event.pull_request.number }}-${{ github.sha }} - name: Fetch built image id: image-cache - uses: actions/cache@v6 + uses: actions/cache/restore@v6 with: path: ./velero.tar # The cache key a combination of the current PR number and the commit SHA @@ -64,7 +64,7 @@ jobs: docker save velero:pr-test-linux-amd64 -o ./velero.tar # Build the MinIO image once for all e2e tests, from the reviewed bitnami/containers commit. - name: Cache MinIO Image - uses: actions/cache@v6 + uses: actions/cache/restore@v6 id: minio-cache with: path: ./minio-image.tar @@ -81,6 +81,29 @@ jobs: cd /tmp/bitnami-containers/bitnami/minio/2026/debian-12 docker build -t bitnami/minio:local . docker save bitnami/minio:local > ${{ github.workspace }}/minio-image.tar + # Save the freshly built artifacts to the cache explicitly, before this + # job reports completion. actions/cache saves in a post-job hook that + # runs *after* the job finishes, so the dependent run-e2e-test jobs (which + # start as soon as build completes) would race the save and miss the cache + # on a force push. See #9927. + - name: Save built CLI to cache + if: steps.cli-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v6 + with: + path: ./_output/bin/linux/amd64/velero + key: velero-cli-${{ github.event.pull_request.number }}-${{ github.sha }} + - name: Save built image to cache + if: steps.image-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v6 + with: + path: ./velero.tar + key: velero-image-${{ github.event.pull_request.number }}-${{ github.sha }} + - name: Save MinIO image to cache + if: steps.minio-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v6 + with: + path: ./minio-image.tar + key: minio-bitnami-${{ steps.minio-version.outputs.dockerfile_sha }} # Create json of k8s versions to test # from guide: https://stackoverflow.com/a/65094398/4590470 setup-test-matrix: @@ -157,7 +180,7 @@ jobs: # Fetch the pre-built MinIO image from the build job - name: Fetch built MinIO Image - uses: actions/cache@v6 + uses: actions/cache/restore@v6 id: minio-cache with: path: ./minio-image.tar @@ -176,13 +199,13 @@ jobs: node_image: "kindest/node:v${{ matrix.k8s }}" - name: Fetch built CLI id: cli-cache - uses: actions/cache@v6 + uses: actions/cache/restore@v6 with: path: ./_output/bin/linux/amd64/velero key: velero-cli-${{ github.event.pull_request.number }}-${{ github.sha }} - name: Fetch built Image id: image-cache - uses: actions/cache@v6 + uses: actions/cache/restore@v6 with: path: ./velero.tar key: velero-image-${{ github.event.pull_request.number }}-${{ github.sha }} diff --git a/changelogs/unreleased/9952-alliasgher b/changelogs/unreleased/9952-alliasgher new file mode 100644 index 000000000..0f96ef5da --- /dev/null +++ b/changelogs/unreleased/9952-alliasgher @@ -0,0 +1 @@ +Fix e2e-test-kind workflow cache miss on force push by saving build artifacts explicitly instead of relying on the actions/cache post-job hook From 5b0aa816636c316818a128e81cdbaf5d3b7208c6 Mon Sep 17 00:00:00 2001 From: Lubron Date: Tue, 25 Aug 2026 12:23:55 -0700 Subject: [PATCH 2/8] feat(cli): add velero client config set namespace-mode=auto (#10127) * Issue #3194: Add velero client set-context-as-velero-namespace command Saves the namespace of the current (or a specified) kubeconfig context into the Velero client config file, so operational commands default to it without requiring --namespace on every invocation. Co-Authored-By: Claude Sonnet 5 Signed-off-by: lubronzhan * Fix CI: rename changelog to PR number, add unit tests for coverage - changelogs/unreleased must be named -; rename from the 0000 placeholder to 10127 to satisfy hack/changelog-check.sh. - Extract the command's logic into setContextAsVeleroNamespace so it's testable without triggering os.Exit via cmd.CheckError, and add unit tests covering: namespace read from context, context with no explicit namespace, overwriting an existing config value, and invalid kubeconfig path. Addresses 0% codecov patch coverage on the PR. Co-Authored-By: Claude Sonnet 5 Signed-off-by: lubronzhan * Move set-namespace-from-context under client config Shubham suggested nesting the new command under `config` for hierarchy consistency, and renaming it since the original set-context-as-velero-namespace name was long and ambiguous. Moves it to `velero client config set-namespace-from-context`, matching the existing config get/set subcommands and my follow-up naming suggestion on the review thread. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 3 (Low) AI-Code-Category: Category 1 (Production) Signed-off-by: lubronzhan * Replace set-namespace-from-context with namespace-mode=auto kaovilai noted on #10127 that a one-shot command to snapshot the kubecontext namespace becomes redundant once a config toggle can resolve it dynamically, and isn't much simpler than the existing `config set namespace=...` alternative. Drop the dedicated set-namespace-from-context subcommand and instead teach the client Factory to resolve the operational namespace from the current kubeconfig context on every invocation when `namespace-mode=auto` is set via the existing generic `config set` command. Explicit --namespace flags and VELERO_NAMESPACE still take precedence, so the new mode only changes behavior when neither is set. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 2 (Medium) AI-Code-Category: Category 1 (Production) Signed-off-by: lubronzhan * Address PR review: doc, fallback test, t.Setenv Resolve feedback from PR #10127 review 4966054980: - Document how to disable namespace-mode=auto (namespace-mode=) and note the fallback to the static namespace, in namespace.md. - Add a factory test covering the fallback to the stored/default namespace when kubeconfig namespace resolution fails. - Switch the VELERO_NAMESPACE override test to t.Setenv, wrapped in a subtest so its cleanup runs before later tests execute. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 2 (Medium) AI-Code-Category: Category 2 (Non-Production) Signed-off-by: lubronzhan --------- Signed-off-by: lubronzhan Co-authored-by: Claude Sonnet 5 --- changelogs/unreleased/10127-lubronzhan | 1 + pkg/client/client.go | 17 +++++++++++ pkg/client/config.go | 28 +++++++++++++++--- pkg/client/factory.go | 30 ++++++++++++++------ pkg/client/factory_test.go | 39 ++++++++++++++++++++++++++ site/content/docs/main/namespace.md | 14 +++++++++ 6 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/10127-lubronzhan diff --git a/changelogs/unreleased/10127-lubronzhan b/changelogs/unreleased/10127-lubronzhan new file mode 100644 index 000000000..9ac64d9d3 --- /dev/null +++ b/changelogs/unreleased/10127-lubronzhan @@ -0,0 +1 @@ +Add `velero client config set namespace-mode=auto` to make operational commands resolve their default namespace from the current kubeconfig context on every invocation diff --git a/pkg/client/client.go b/pkg/client/client.go index 39cdc9141..e49fbd0cc 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -58,6 +58,23 @@ func Config(kubeconfig, kubecontext, baseName string, qps float32, burst int) (* return clientConfig, nil } +// NamespaceFromKubeContext returns the namespace associated with the given kubeconfig context +// (or the current context if kubecontext is empty), using the given kubeconfig file (or the +// default loading rules if kubeconfig is empty). +func NamespaceFromKubeContext(kubeconfig, kubecontext string) (string, error) { + loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() + loadingRules.ExplicitPath = kubeconfig + configOverrides := &clientcmd.ConfigOverrides{CurrentContext: kubecontext} + kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides) + + namespace, _, err := kubeConfig.Namespace() + if err != nil { + return "", errors.Wrap(err, "error finding namespace in --kubeconfig, $KUBECONFIG, or in-cluster configuration") + } + + return namespace, nil +} + // buildUserAgent builds a User-Agent string from given args. func buildUserAgent(command, version, formattedSha, os, arch string) string { return fmt.Sprintf( diff --git a/pkg/client/config.go b/pkg/client/config.go index 2a96e3467..793c4b9a0 100644 --- a/pkg/client/config.go +++ b/pkg/client/config.go @@ -27,10 +27,16 @@ import ( ) const ( - ConfigKeyNamespace = "namespace" - ConfigKeyFeatures = "features" - ConfigKeyCACert = "cacert" - ConfigKeyColorized = "colorized" + ConfigKeyNamespace = "namespace" + ConfigKeyNamespaceMode = "namespace-mode" + ConfigKeyFeatures = "features" + ConfigKeyCACert = "cacert" + ConfigKeyColorized = "colorized" + + // NamespaceModeAuto is the ConfigKeyNamespaceMode value that makes Velero resolve the + // namespace for operational commands from the current kubeconfig context on every + // invocation, instead of the static ConfigKeyNamespace value. + NamespaceModeAuto = "auto" ) // VeleroConfig is a map of strings to any for deserializing Velero client config options. @@ -99,6 +105,20 @@ func (c VeleroConfig) Namespace() string { return ns } +func (c VeleroConfig) NamespaceMode() string { + val, ok := c[ConfigKeyNamespaceMode] + if !ok { + return "" + } + + mode, ok := val.(string) + if !ok { + return "" + } + + return mode +} + func (c VeleroConfig) Features() []string { val, ok := c[ConfigKeyFeatures] if !ok { diff --git a/pkg/client/factory.go b/pkg/client/factory.go index 17e2a243a..01df4ed7b 100644 --- a/pkg/client/factory.go +++ b/pkg/client/factory.go @@ -77,20 +77,22 @@ type Factory interface { } type factory struct { - flags *pflag.FlagSet - kubeconfig string - kubecontext string - baseName string - namespace string - clientQPS float32 - clientBurst int + flags *pflag.FlagSet + kubeconfig string + kubecontext string + baseName string + namespace string + namespaceMode string + clientQPS float32 + clientBurst int } // NewFactory returns a Factory. func NewFactory(baseName string, config VeleroConfig) Factory { f := &factory{ - flags: pflag.NewFlagSet("", pflag.ContinueOnError), - baseName: baseName, + flags: pflag.NewFlagSet("", pflag.ContinueOnError), + baseName: baseName, + namespaceMode: config.NamespaceMode(), } f.namespace = os.Getenv("VELERO_NAMESPACE") @@ -242,5 +244,15 @@ func (f *factory) SetClientBurst(burst int) { } func (f *factory) Namespace() string { + // In auto mode, the namespace is resolved from the current kubeconfig context on every + // call, unless the caller explicitly overrode it with --namespace or VELERO_NAMESPACE. + if f.namespaceMode == NamespaceModeAuto && + !f.flags.Changed("namespace") && + os.Getenv("VELERO_NAMESPACE") == "" { + if namespace, err := NamespaceFromKubeContext(f.kubeconfig, f.kubecontext); err == nil && namespace != "" { + return namespace + } + } + return f.namespace } diff --git a/pkg/client/factory_test.go b/pkg/client/factory_test.go index 5b9db37f1..2f63d3a3c 100644 --- a/pkg/client/factory_test.go +++ b/pkg/client/factory_test.go @@ -64,6 +64,45 @@ func TestFactory(t *testing.T) { os.Unsetenv("VELERO_NAMESPACE") + // namespace-mode=auto should resolve the namespace from the current kubeconfig context. + f = NewFactory("velero", VeleroConfig{ConfigKeyNamespaceMode: NamespaceModeAuto}) + flags = new(flag.FlagSet) + f.BindFlags(flags) + require.NoError(t, flags.Parse([]string{"--kubeconfig", "kubeconfig", "--kubecontext", "federal-context"})) + assert.Equal(t, "chisel-ns", f.Namespace()) + + // namespace-mode=auto should track kubecontext changes dynamically. + f = NewFactory("velero", VeleroConfig{ConfigKeyNamespaceMode: NamespaceModeAuto}) + flags = new(flag.FlagSet) + f.BindFlags(flags) + require.NoError(t, flags.Parse([]string{"--kubeconfig", "kubeconfig", "--kubecontext", "queen-anne-context"})) + assert.Equal(t, "saw-ns", f.Namespace()) + + // An explicit --namespace flag overrides namespace-mode=auto. + f = NewFactory("velero", VeleroConfig{ConfigKeyNamespaceMode: NamespaceModeAuto}) + flags = new(flag.FlagSet) + f.BindFlags(flags) + require.NoError(t, flags.Parse([]string{"--kubeconfig", "kubeconfig", "--kubecontext", "federal-context", "--namespace", s})) + assert.Equal(t, s, f.Namespace()) + + // VELERO_NAMESPACE overrides namespace-mode=auto. + t.Run("VELERO_NAMESPACE overrides namespace-mode=auto", func(t *testing.T) { + t.Setenv("VELERO_NAMESPACE", "env-velero") + f := NewFactory("velero", VeleroConfig{ConfigKeyNamespaceMode: NamespaceModeAuto}) + flags := new(flag.FlagSet) + f.BindFlags(flags) + require.NoError(t, flags.Parse([]string{"--kubeconfig", "kubeconfig", "--kubecontext", "federal-context"})) + assert.Equal(t, "env-velero", f.Namespace()) + }) + + // namespace-mode=auto falls back to the stored/default namespace when the kubeconfig + // namespace can't be resolved (e.g. the kubeconfig file doesn't exist). + f = NewFactory("velero", VeleroConfig{ConfigKeyNamespace: "stored-ns", ConfigKeyNamespaceMode: NamespaceModeAuto}) + flags = new(flag.FlagSet) + f.BindFlags(flags) + require.NoError(t, flags.Parse([]string{"--kubeconfig", "nonexistent-kubeconfig"})) + assert.Equal(t, "stored-ns", f.Namespace()) + tests := []struct { name string kubeconfig string diff --git a/site/content/docs/main/namespace.md b/site/content/docs/main/namespace.md index 68561e720..70d84b09a 100644 --- a/site/content/docs/main/namespace.md +++ b/site/content/docs/main/namespace.md @@ -17,6 +17,20 @@ To have namespace consistency, specify the namespace for all Velero operational velero client config set namespace= ``` +If Velero was installed in the namespace of your current kubeconfig context, you can have operational commands automatically use that namespace, without having to type it out or update it every time you switch contexts: + +```bash +velero client config set namespace-mode=auto +``` + +With `namespace-mode=auto` set, Velero resolves the namespace from the current kubeconfig context (or the context specified with `--kubecontext`) on every command invocation, instead of using the static `namespace` value. If the namespace can't be resolved from the kubeconfig context (for example, the context has no namespace set, or the kubeconfig can't be loaded), Velero falls back to the static `namespace` value, or the `velero` default if that isn't set either. + +To disable `namespace-mode=auto` and go back to using the static `namespace` value, clear it by setting it to an empty value: + +```bash +velero client config set namespace-mode= +``` + Alternatively, you may use the global `--namespace` flag with any operational command to tell Velero where to run. [0]: basic-install.md#install-the-cli From e14ffe3e4c9fd152bac6269aac945289e3ac1a72 Mon Sep 17 00:00:00 2001 From: Joseph Antony Vaikath Date: Tue, 25 Aug 2026 12:55:56 -0700 Subject: [PATCH 3/8] chore(deps): bump golang.org/x libs to fix CVEs (#10402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: - CVE-2026-56864, CVE-2026-56865: golang.org/x/mod v0.36.0 → v0.40.0 - CVE-2026-46600: golang.org/x/net v0.55.0 → v0.58.0 - CVE-2026-56852: golang.org/x/text v0.37.0 → v0.41.0 Transitive golang.org/x dependencies (crypto, sync, sys, term, tools) updated accordingly to satisfy version constraints. Signed-off-by: Joseph --- go.mod | 16 ++++++++-------- go.sum | 32 ++++++++++++++++---------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/go.mod b/go.mod index 3e6bd840c..c38ab050a 100644 --- a/go.mod +++ b/go.mod @@ -44,10 +44,10 @@ require ( github.com/vmware-tanzu/velero/pkg/apis v0.0.0 go.uber.org/zap v1.28.0 go.yaml.in/yaml/v3 v3.0.4 - golang.org/x/mod v0.36.0 + golang.org/x/mod v0.40.0 golang.org/x/oauth2 v0.36.0 - golang.org/x/sys v0.46.0 - golang.org/x/text v0.37.0 + golang.org/x/sys v0.47.0 + golang.org/x/text v0.41.0 google.golang.org/api v0.283.0 google.golang.org/grpc v1.82.1 google.golang.org/protobuf v1.36.12 @@ -200,13 +200,13 @@ require ( go.starlark.net v0.0.0-20241226192728-8dfa5b98479f // indirect go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect - golang.org/x/crypto v0.52.0 // indirect + golang.org/x/crypto v0.55.0 // indirect golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f // indirect - golang.org/x/net v0.55.0 // indirect - golang.org/x/sync v0.21.0 // indirect - golang.org/x/term v0.43.0 // indirect + golang.org/x/net v0.58.0 // indirect + golang.org/x/sync v0.22.0 // indirect + golang.org/x/term v0.45.0 // indirect golang.org/x/time v0.15.0 // indirect - golang.org/x/tools v0.44.0 // indirect + golang.org/x/tools v0.49.0 // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect diff --git a/go.sum b/go.sum index 55741d1c5..f744bb2e4 100644 --- a/go.sum +++ b/go.sum @@ -501,27 +501,27 @@ go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= -golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= +golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= +golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f h1:W3F4c+6OLc6H2lb//N1q4WpJkhzJCK5J6kUi1NTVXfM= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f/go.mod h1:J1xhfL/vlindoeF/aINzNzt2Bket5bjo9sdOYzOsU80= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= -golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/mod v0.40.0 h1:hUv+3cXcdRHz08UmSiOob7sadHig73uo5bkXxQ/tvUs= +golang.org/x/mod v0.40.0/go.mod h1:0/weTWkPWGBikyTWAX3dkjVztMmBA5hM0DH6BElSupE= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= -golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -532,22 +532,22 @@ golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220503163025-988cb79eb6c6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= -golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= -golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= -golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= -golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From 2c6f45508c602222eb003c49dd2f537c5a12f388 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 25 Aug 2026 18:08:17 -0400 Subject: [PATCH 4/8] Report a measured zero incremental instead of erasing it (#10309) * Report a measured zero incremental instead of erasing it A CBT incremental with an exactly zero delta -- nothing changed since the parent -- was reported identically to a backup that moved the whole device. `velero backup describe --details` printed only "Moved data Size (bytes): 3221225472" with no incremental line, and status.incrementalBytes was absent, for a run that transferred nothing. The best possible CBT outcome displayed as the worst, and was indistinguishable from a genuine full, a whole-device fallback, or a backup predating incremental accounting. The zero was being erased twice. Besides the API status fields, datapath.BackupResult also carried omitempty, and that struct crosses a JSON boundary from the data mover pod to the controller (see micro_service_watcher.go), so the value was destroyed before the controller could persist it. Every uploader always reports a figure there, so 0 internally always means "transferred nothing" -- dropping omitempty is sufficient and correct for that hop. The API fields move to *int64 rather than just dropping omitempty. The field shipped in v1.18.0-v1.18.2, so backups exist whose stored volume info has no incrementalSize at all; with a plain int64 those unmarshal to 0 and would render "Incremental data Size (bytes): 0", a false claim of a perfect incremental on a run that never measured one. nil means not measured, a pointer to 0 means measured zero. Both fields already carry +optional, so the generated CRD schema is unchanged and no regeneration is required. Display gates relax from > 0 to != nil in all three places, including volumesByPod.Add, whose signature takes *int64 now; the restore describer passes nil, which is correct since restores measure no incremental. Verified live: the same zero-delta scenario that reported now reports 0 and renders "Incremental data Size (bytes): 0", while an older backup described with the new client still correctly prints no incremental line at all. Co-Authored-By: Claude Fable 5 Signed-off-by: Tiger Kaovilai (cherry picked from commit 6c7aa9d588f6d5eab134d4ce19c92b838f45557c) Signed-off-by: Tiger Kaovilai * gofmt: fix import ordering in backup_test.go Signed-off-by: Tiger Kaovilai * Regenerate CRDs for IncrementalBytes pointer type make update-crd was missed in the original commit. Regenerated with the pinned controller-gen v0.16.5 to avoid unrelated version-annotation churn across other CRDs. Signed-off-by: Tiger Kaovilai * Add changelog for #10309 Signed-off-by: Tiger Kaovilai * Address review: make IncrementalBytes a pointer to preserve backward compat Per Lyndon-Li's review on #10309: dropping omitempty on the plain int64 field breaks compatibility with a data mover from release-1.17 or earlier that predates IncrementalBytes and never writes the key -- the new controller would unmarshal a zero value ("nothing transferred") instead of recognizing the field is simply absent ("not measured"). Switch to *int64 with omitempty restored: - an old mover's omitted key unmarshals to nil ("not measured") - a current mover's genuine zero still serializes the key, unmarshaling to a non-nil pointer to 0 ("measured zero") - nonzero values work exactly as before - an old controller can still unmarshal a numeric value from a new mover pkg/controller/data_upload_controller.go and pod_volume_backup_controller.go assign the wire-struct field directly to their already-*int64,omitempty CRD status field instead of re-wrapping it with ptr.To, since both are now the same pointer type. Signed-off-by: Tiger Kaovilai * Fix CI: update marshal-fail test assertions for IncrementalBytes pointer Both backup_micro_service_test.go files hardcoded the %v-formatted zero-value BackupResult struct in an error-message assertion. Now that IncrementalBytes is *int64, its zero value prints as instead of 0. Signed-off-by: Tiger Kaovilai --------- Signed-off-by: Tiger Kaovilai Co-authored-by: Claude Fable 5 --- changelogs/unreleased/10309-kaovilai | 1 + .../v1/bases/velero.io_podvolumebackups.yaml | 8 +++- .../v2alpha1/bases/velero.io_datauploads.yaml | 8 +++- internal/volume/volumes_information.go | 12 ++++-- pkg/apis/velero/v1/pod_volume_backup_types.go | 8 +++- pkg/apis/velero/v1/zz_generated.deepcopy.go | 5 +++ pkg/apis/velero/v2alpha1/data_upload_types.go | 8 +++- .../velero/v2alpha1/zz_generated.deepcopy.go | 5 +++ pkg/backup/backup_test.go | 5 ++- pkg/builder/data_upload_builder.go | 2 +- pkg/cmd/util/output/backup_describer.go | 18 ++++++--- pkg/cmd/util/output/backup_describer_test.go | 3 +- .../output/backup_structured_describer.go | 8 +++- pkg/cmd/util/output/restore_describer.go | 2 +- pkg/datamover/backup_micro_service_test.go | 2 +- pkg/datapath/data_path.go | 3 +- pkg/datapath/data_path_test.go | 16 +++++--- pkg/datapath/micro_service_watcher_test.go | 37 +++++++++++++++++++ pkg/datapath/types.go | 14 ++++--- pkg/podvolume/backup_micro_service_test.go | 2 +- 20 files changed, 130 insertions(+), 37 deletions(-) create mode 100644 changelogs/unreleased/10309-kaovilai diff --git a/changelogs/unreleased/10309-kaovilai b/changelogs/unreleased/10309-kaovilai new file mode 100644 index 000000000..b0683330e --- /dev/null +++ b/changelogs/unreleased/10309-kaovilai @@ -0,0 +1 @@ +Report a measured zero-byte incremental instead of erasing it from status diff --git a/config/crd/v1/bases/velero.io_podvolumebackups.yaml b/config/crd/v1/bases/velero.io_podvolumebackups.yaml index 90e9f4e4a..935916db9 100644 --- a/config/crd/v1/bases/velero.io_podvolumebackups.yaml +++ b/config/crd/v1/bases/velero.io_podvolumebackups.yaml @@ -205,8 +205,12 @@ spec: nullable: true type: string incrementalBytes: - description: IncrementalBytes holds the number of bytes new or changed - since the last backup + description: |- + IncrementalBytes holds the number of bytes new or changed since the last backup. + A nil value means the uploader did not report a figure; a pointer to 0 means it + reported zero, i.e. nothing changed and nothing was transferred. The two are + distinct: erasing a measured zero makes a perfect incremental indistinguishable + from a full transfer in every downstream report. format: int64 type: integer message: diff --git a/config/crd/v2alpha1/bases/velero.io_datauploads.yaml b/config/crd/v2alpha1/bases/velero.io_datauploads.yaml index 5e1fd4124..8d03da279 100644 --- a/config/crd/v2alpha1/bases/velero.io_datauploads.yaml +++ b/config/crd/v2alpha1/bases/velero.io_datauploads.yaml @@ -192,8 +192,12 @@ spec: nullable: true type: object incrementalBytes: - description: IncrementalBytes holds the number of bytes new or changed - since the last backup + description: |- + IncrementalBytes holds the number of bytes new or changed since the last backup. + A nil value means the uploader did not report a figure; a pointer to 0 means it + reported zero, i.e. nothing changed and nothing was transferred. The two are + distinct: erasing a measured zero makes a perfect incremental indistinguishable + from a full transfer in every downstream report. format: int64 type: integer message: diff --git a/internal/volume/volumes_information.go b/internal/volume/volumes_information.go index 69214ef45..cec2922d9 100644 --- a/internal/volume/volumes_information.go +++ b/internal/volume/volumes_information.go @@ -175,8 +175,11 @@ type SnapshotDataMovementInfo struct { // Moved snapshot data size. Size int64 `json:"size"` - // Moved snapshot incremental size. - IncrementalSize int64 `json:"incrementalSize,omitempty"` + // Moved snapshot incremental size, i.e. the bytes actually transferred. Nil means + // the uploader reported no figure (including backups taken before this was + // recorded); a pointer to 0 means it transferred nothing, which is the ideal + // incremental and must stay distinguishable from "unknown". + IncrementalSize *int64 `json:"incrementalSize,omitempty"` // The DataUpload's Status.Phase value Phase velerov2alpha1.DataUploadPhase @@ -225,8 +228,9 @@ type PodVolumeInfo struct { // The snapshot corresponding volume size. Size int64 `json:"size,omitempty"` - // The incremental snapshot size. - IncrementalSize int64 `json:"incrementalSize,omitempty"` + // The incremental snapshot size, i.e. the bytes actually transferred. Nil means + // the uploader reported no figure; a pointer to 0 means it transferred nothing. + IncrementalSize *int64 `json:"incrementalSize,omitempty"` // The type of the uploader that uploads the data. The valid values are `kopia` and `restic`. UploaderType string `json:"uploaderType"` diff --git a/pkg/apis/velero/v1/pod_volume_backup_types.go b/pkg/apis/velero/v1/pod_volume_backup_types.go index 566ba3b29..c4b7f879c 100644 --- a/pkg/apis/velero/v1/pod_volume_backup_types.go +++ b/pkg/apis/velero/v1/pod_volume_backup_types.go @@ -124,9 +124,13 @@ type PodVolumeBackupStatus struct { // +optional Progress shared.DataMoveOperationProgress `json:"progress,omitempty"` - // IncrementalBytes holds the number of bytes new or changed since the last backup + // IncrementalBytes holds the number of bytes new or changed since the last backup. + // A nil value means the uploader did not report a figure; a pointer to 0 means it + // reported zero, i.e. nothing changed and nothing was transferred. The two are + // distinct: erasing a measured zero makes a perfect incremental indistinguishable + // from a full transfer in every downstream report. // +optional - IncrementalBytes int64 `json:"incrementalBytes,omitempty"` + IncrementalBytes *int64 `json:"incrementalBytes,omitempty"` // AcceptedTimestamp records the time the pod volume backup is to be prepared. // The server's time is used for AcceptedTimestamp diff --git a/pkg/apis/velero/v1/zz_generated.deepcopy.go b/pkg/apis/velero/v1/zz_generated.deepcopy.go index ffbbf0cf8..78f756640 100644 --- a/pkg/apis/velero/v1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v1/zz_generated.deepcopy.go @@ -1055,6 +1055,11 @@ func (in *PodVolumeBackupStatus) DeepCopyInto(out *PodVolumeBackupStatus) { *out = (*in).DeepCopy() } out.Progress = in.Progress + if in.IncrementalBytes != nil { + in, out := &in.IncrementalBytes, &out.IncrementalBytes + *out = new(int64) + **out = **in + } if in.AcceptedTimestamp != nil { in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp *out = (*in).DeepCopy() diff --git a/pkg/apis/velero/v2alpha1/data_upload_types.go b/pkg/apis/velero/v2alpha1/data_upload_types.go index 37e273b2b..6f28d399b 100644 --- a/pkg/apis/velero/v2alpha1/data_upload_types.go +++ b/pkg/apis/velero/v2alpha1/data_upload_types.go @@ -165,9 +165,13 @@ type DataUploadStatus struct { // +optional Progress shared.DataMoveOperationProgress `json:"progress,omitempty"` - // IncrementalBytes holds the number of bytes new or changed since the last backup + // IncrementalBytes holds the number of bytes new or changed since the last backup. + // A nil value means the uploader did not report a figure; a pointer to 0 means it + // reported zero, i.e. nothing changed and nothing was transferred. The two are + // distinct: erasing a measured zero makes a perfect incremental indistinguishable + // from a full transfer in every downstream report. // +optional - IncrementalBytes int64 `json:"incrementalBytes,omitempty"` + IncrementalBytes *int64 `json:"incrementalBytes,omitempty"` // Node is name of the node where the DataUpload is processed. // +optional diff --git a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go index b86c573d3..0513824bd 100644 --- a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go @@ -270,6 +270,11 @@ func (in *DataUploadStatus) DeepCopyInto(out *DataUploadStatus) { *out = (*in).DeepCopy() } out.Progress = in.Progress + if in.IncrementalBytes != nil { + in, out := &in.IncrementalBytes, &out.IncrementalBytes + *out = new(int64) + **out = **in + } if in.AcceptedTimestamp != nil { in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp *out = (*in).DeepCopy() diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 9574aa288..5d1ed1da2 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -43,6 +43,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/ptr" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" @@ -5681,7 +5682,7 @@ func TestUpdateVolumeInfos(t *testing.T) { RetainedSnapshot: "vs-1", SnapshotHandle: "snapshot-id", Size: 1000, - IncrementalSize: 500, + IncrementalSize: ptr.To(int64(500)), Phase: velerov2alpha1.DataUploadPhaseFailed, }, }, @@ -5721,7 +5722,7 @@ func TestUpdateVolumeInfos(t *testing.T) { RetainedSnapshot: "vs-1", SnapshotHandle: "snapshot-id", Size: 1000, - IncrementalSize: 500, + IncrementalSize: ptr.To(int64(500)), Phase: velerov2alpha1.DataUploadPhaseCompleted, }, }, diff --git a/pkg/builder/data_upload_builder.go b/pkg/builder/data_upload_builder.go index c8fa34956..9805e71a3 100644 --- a/pkg/builder/data_upload_builder.go +++ b/pkg/builder/data_upload_builder.go @@ -147,7 +147,7 @@ func (d *DataUploadBuilder) Progress(progress shared.DataMoveOperationProgress) // IncrementalBytes sets the DataUpload's IncrementalBytes. func (d *DataUploadBuilder) IncrementalBytes(incrementalBytes int64) *DataUploadBuilder { - d.object.Status.IncrementalBytes = incrementalBytes + d.object.Status.IncrementalBytes = &incrementalBytes return d } diff --git a/pkg/cmd/util/output/backup_describer.go b/pkg/cmd/util/output/backup_describer.go index 445ce3df5..a8d43b89f 100644 --- a/pkg/cmd/util/output/backup_describer.go +++ b/pkg/cmd/util/output/backup_describer.go @@ -739,8 +739,12 @@ func describeDataMovement(d *Describer, details bool, info *volume.BackupVolumeI d.Printf("\t\t\t\tData Mover: %s\n", dataMover) d.Printf("\t\t\t\tUploader Type: %s\n", info.SnapshotDataMovementInfo.UploaderType) d.Printf("\t\t\t\tMoved data Size (bytes): %d\n", info.SnapshotDataMovementInfo.Size) - if info.SnapshotDataMovementInfo.IncrementalSize > 0 { - d.Printf("\t\t\t\tIncremental data Size (bytes): %d\n", info.SnapshotDataMovementInfo.IncrementalSize) + // Print whenever the uploader measured a figure, including zero. A zero-delta + // incremental transfers nothing, which is the whole point of CBT; hiding it + // leaves only the volume size on display and makes the best possible result + // indistinguishable from a full transfer. + if info.SnapshotDataMovementInfo.IncrementalSize != nil { + d.Printf("\t\t\t\tIncremental data Size (bytes): %d\n", *info.SnapshotDataMovementInfo.IncrementalSize) } d.Printf("\t\t\t\tResult: %s\n", info.Result) } else { @@ -915,7 +919,7 @@ type volumesByPod struct { // Add adds a pod volume with the specified pod namespace, name // and volume to the appropriate group. // Used for both backup and restore -func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veleroapishared.DataMoveOperationProgress, incrementalBytes int64) { +func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veleroapishared.DataMoveOperationProgress, incrementalBytes *int64) { if v.volumesByPodMap == nil { v.volumesByPodMap = make(map[string]*podVolumeGroup) } @@ -925,8 +929,12 @@ func (v *volumesByPod) Add(namespace, name, volume, phase string, progress veler // append backup progress percentage if backup is in progress if phase == "In Progress" && progress.TotalBytes != 0 { volume = fmt.Sprintf("%s (%.2f%%)", volume, float64(progress.BytesDone)/float64(progress.TotalBytes)*100) - } else if phase == string(velerov1api.PodVolumeBackupPhaseCompleted) && incrementalBytes > 0 { - volume = fmt.Sprintf("%s (size: %v, incremental size: %v)", volume, progress.TotalBytes, incrementalBytes) + } else if phase == string(velerov1api.PodVolumeBackupPhaseCompleted) && incrementalBytes != nil { + // Report the incremental figure whenever it was measured, including zero. Zero is + // the best possible outcome - nothing changed, so nothing was transferred - and + // suppressing it leaves only the volume size on display, which reads as a full + // transfer. + volume = fmt.Sprintf("%s (size: %v, incremental size: %v)", volume, progress.TotalBytes, *incrementalBytes) } else if (phase == string(velerov1api.PodVolumeBackupPhaseCompleted) || phase == string(velerov1api.PodVolumeRestorePhaseCompleted)) && progress.TotalBytes > 0 { diff --git a/pkg/cmd/util/output/backup_describer_test.go b/pkg/cmd/util/output/backup_describer_test.go index da28f6c87..c64ae04cb 100644 --- a/pkg/cmd/util/output/backup_describer_test.go +++ b/pkg/cmd/util/output/backup_describer_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" + "k8s.io/utils/ptr" "github.com/vmware-tanzu/velero/internal/volume" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -629,7 +630,7 @@ func TestCSISnapshots(t *testing.T) { SnapshotHandle: "fake-repo-id-5", OperationID: "fake-operation-5", Size: 100, - IncrementalSize: 50, + IncrementalSize: ptr.To(int64(50)), Phase: velerov2alpha1.DataUploadPhaseFailed, }, }, diff --git a/pkg/cmd/util/output/backup_structured_describer.go b/pkg/cmd/util/output/backup_structured_describer.go index b2541df4b..1c0aefa34 100644 --- a/pkg/cmd/util/output/backup_structured_describer.go +++ b/pkg/cmd/util/output/backup_structured_describer.go @@ -467,9 +467,13 @@ func describeDataMovementInSF(details bool, info *volume.BackupVolumeInfo, snaps dataMovement["uploaderType"] = info.SnapshotDataMovementInfo.UploaderType dataMovement["result"] = string(info.Result) - if info.SnapshotDataMovementInfo.Size > 0 || info.SnapshotDataMovementInfo.IncrementalSize > 0 { + if info.SnapshotDataMovementInfo.Size > 0 { dataMovement["size"] = info.SnapshotDataMovementInfo.Size - dataMovement["incrementalSize"] = info.SnapshotDataMovementInfo.IncrementalSize + } + // Emit whenever measured, including zero - a zero-delta incremental transferred + // nothing, and that has to be reportable rather than absent. + if info.SnapshotDataMovementInfo.IncrementalSize != nil { + dataMovement["incrementalSize"] = *info.SnapshotDataMovementInfo.IncrementalSize } snapshotDetail["dataMovement"] = dataMovement diff --git a/pkg/cmd/util/output/restore_describer.go b/pkg/cmd/util/output/restore_describer.go index e94b2dedd..11e8ff4e4 100644 --- a/pkg/cmd/util/output/restore_describer.go +++ b/pkg/cmd/util/output/restore_describer.go @@ -417,7 +417,7 @@ func describePodVolumeRestores(d *Describer, restores []velerov1api.PodVolumeRes restoresByPod := new(volumesByPod) for _, restore := range restoresByPhase[phase] { - restoresByPod.Add(restore.Spec.Pod.Namespace, restore.Spec.Pod.Name, restore.Spec.Volume, phase, restore.Status.Progress, 0) + restoresByPod.Add(restore.Spec.Pod.Namespace, restore.Spec.Pod.Name, restore.Spec.Volume, phase, restore.Status.Progress, nil) } d.Printf("\t%s:\n", phase) diff --git a/pkg/datamover/backup_micro_service_test.go b/pkg/datamover/backup_micro_service_test.go index c9accdd77..69a4a1381 100644 --- a/pkg/datamover/backup_micro_service_test.go +++ b/pkg/datamover/backup_micro_service_test.go @@ -152,7 +152,7 @@ func TestOnDataUploadCompleted(t *testing.T) { { name: "marshal fail", marshalErr: errors.New("fake-marshal-error"), - expectedErr: "Failed to marshal backup result { false { } 0 0}: fake-marshal-error", + expectedErr: "Failed to marshal backup result { false { } 0 }: fake-marshal-error", }, { name: "succeed", diff --git a/pkg/datapath/data_path.go b/pkg/datapath/data_path.go index 2ec750805..647095672 100644 --- a/pkg/datapath/data_path.go +++ b/pkg/datapath/data_path.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/errors" "github.com/sirupsen/logrus" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/vmware-tanzu/velero/internal/credentials" @@ -220,7 +221,7 @@ func (dp *generalDataPath) StartBackup(source AccessPoint, uploaderConfig map[st } dp.callbacks.OnFailed(context.Background(), dp.namespace, dp.jobName, dataPathErr) } else { - dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Backup: BackupResult{snapshotID, emptySnapshot, source, totalBytes, incrementalBytes}}) + dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Backup: BackupResult{snapshotID, emptySnapshot, source, totalBytes, ptr.To(incrementalBytes)}}) } }() diff --git a/pkg/datapath/data_path_test.go b/pkg/datapath/data_path_test.go index 58df5d4e8..0c493645d 100644 --- a/pkg/datapath/data_path_test.go +++ b/pkg/datapath/data_path_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "k8s.io/utils/ptr" velerotest "github.com/vmware-tanzu/velero/pkg/test" "github.com/vmware-tanzu/velero/pkg/uploader/provider" @@ -82,10 +83,11 @@ func TestAsyncBackup(t *testing.T) { }, result: Result{ Backup: BackupResult{ - SnapshotID: "fake-snapshot", - EmptySnapshot: false, - Source: AccessPoint{ByPath: "fake-path"}, - TotalBytes: 1000, + SnapshotID: "fake-snapshot", + EmptySnapshot: false, + Source: AccessPoint{ByPath: "fake-path"}, + TotalBytes: 1000, + IncrementalBytes: ptr.To(int64(0)), }, }, path: "fake-path", @@ -96,7 +98,11 @@ func TestAsyncBackup(t *testing.T) { t.Run(test.name, func(t *testing.T) { dp := newGeneralDataPath("job-1", "test", nil, "velero", Callbacks{}, velerotest.NewLogger()).(*generalDataPath) mockProvider := providerMock.NewProvider(t) - mockProvider.On("RunBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Backup.SnapshotID, test.result.Backup.EmptySnapshot, test.result.Backup.TotalBytes, test.result.Backup.IncrementalBytes, test.err) + var incrementalBytes int64 + if test.result.Backup.IncrementalBytes != nil { + incrementalBytes = *test.result.Backup.IncrementalBytes + } + mockProvider.On("RunBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Backup.SnapshotID, test.result.Backup.EmptySnapshot, test.result.Backup.TotalBytes, incrementalBytes, test.err) mockProvider.On("Close", mock.Anything).Return(nil) dp.uploaderProv = mockProvider dp.initialized = true diff --git a/pkg/datapath/micro_service_watcher_test.go b/pkg/datapath/micro_service_watcher_test.go index dee9560ae..315c791ea 100644 --- a/pkg/datapath/micro_service_watcher_test.go +++ b/pkg/datapath/micro_service_watcher_test.go @@ -34,6 +34,7 @@ import ( "k8s.io/client-go/kubernetes" kubeclientfake "k8s.io/client-go/kubernetes/fake" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/vmware-tanzu/velero/pkg/builder" @@ -510,6 +511,42 @@ func TestGetResultFromMessage(t *testing.T) { }, }, }, + { + // An old data mover (release-1.17 and earlier) predates IncrementalBytes and + // never writes the key at all -- this pins that its absence unmarshals to nil + // ("not measured"), not a zero value. + name: "old mover message omits incrementalBytes -> nil", + taskType: TaskTypeBackup, + message: "{\"snapshotID\":\"fake-snapshot-id\",\"emptySnapshot\":false,\"source\":{\"byPath\":\"fake-path-1\",\"volumeMode\":\"Block\"}}", + expectResult: Result{ + Backup: BackupResult{ + SnapshotID: "fake-snapshot-id", + Source: AccessPoint{ + ByPath: "fake-path-1", + VolMode: uploader.PersistentVolumeBlock, + }, + IncrementalBytes: nil, + }, + }, + }, + { + // A current mover reports a genuine zero explicitly -- this pins that the key + // being present with value 0 unmarshals to a non-nil pointer to 0 ("measured + // zero"), distinguishing it from the omitted-key case above. + name: "current mover reports measured zero incrementalBytes -> non-nil zero", + taskType: TaskTypeBackup, + message: "{\"snapshotID\":\"fake-snapshot-id\",\"emptySnapshot\":false,\"source\":{\"byPath\":\"fake-path-1\",\"volumeMode\":\"Block\"},\"incrementalBytes\":0}", + expectResult: Result{ + Backup: BackupResult{ + SnapshotID: "fake-snapshot-id", + Source: AccessPoint{ + ByPath: "fake-path-1", + VolMode: uploader.PersistentVolumeBlock, + }, + IncrementalBytes: ptr.To(int64(0)), + }, + }, + }, { name: "succeed to unmarshall restore result", taskType: TaskTypeRestore, diff --git a/pkg/datapath/types.go b/pkg/datapath/types.go index 65a6be58f..339aa6ca4 100644 --- a/pkg/datapath/types.go +++ b/pkg/datapath/types.go @@ -30,11 +30,15 @@ type Result struct { // BackupResult represents the result of a backup type BackupResult struct { - SnapshotID string `json:"snapshotID"` - EmptySnapshot bool `json:"emptySnapshot"` - Source AccessPoint `json:"source,omitempty"` - TotalBytes int64 `json:"totalBytes,omitempty"` - IncrementalBytes int64 `json:"incrementalBytes,omitempty"` + SnapshotID string `json:"snapshotID"` + EmptySnapshot bool `json:"emptySnapshot"` + Source AccessPoint `json:"source,omitempty"` + TotalBytes int64 `json:"totalBytes,omitempty"` + // IncrementalBytes is a pointer so an old data mover (release-1.17 and earlier, + // which predates this field) that omits it unmarshals to nil -- "not measured" -- + // while a current mover reporting a genuine zero still serializes the key and + // unmarshals to a non-nil zero, distinguishing "measured zero" from "not measured". + IncrementalBytes *int64 `json:"incrementalBytes,omitempty"` } // RestoreResult represents the result of a restore diff --git a/pkg/podvolume/backup_micro_service_test.go b/pkg/podvolume/backup_micro_service_test.go index 2de4705af..b83bafd8a 100644 --- a/pkg/podvolume/backup_micro_service_test.go +++ b/pkg/podvolume/backup_micro_service_test.go @@ -156,7 +156,7 @@ func TestOnDataPathCompleted(t *testing.T) { { name: "marshal fail", marshalErr: errors.New("fake-marshal-error"), - expectedErr: "Failed to marshal backup result { false { } 0 0}: fake-marshal-error", + expectedErr: "Failed to marshal backup result { false { } 0 }: fake-marshal-error", }, { name: "succeed", From e33d8a3f846fb22023ca4cf50ce901d846eb21c8 Mon Sep 17 00:00:00 2001 From: Nolan Emirot Date: Tue, 25 Aug 2026 23:46:04 -0700 Subject: [PATCH 5/8] docs(aws-plugin): update version (#9773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(aws-plugin): update version Signed-off-by: emirot * Add e2e test case for issue 7725 Signed-off-by: dongqingcc Signed-off-by: emirot * Add e2e test case for PR 9452 Signed-off-by: dongqingcc Signed-off-by: emirot * fix: lint permission issue (#9740) * fix: lint permission issue Signed-off-by: emirot * fix: lint permission issue Signed-off-by: emirot * Set permissions to the actions This commit update the actions "Auto Assign Author", "Auto Label PRs", and "Auto Request Review" Signed-off-by: Daniel Jiang Signed-off-by: emirot * Fix wildcard expansion when includes is empty and excludes has wildcards (#9684) * Fix wildcard expansion when includes is empty and excludes has wildcards When a Backup CR is applied via kubectl with empty includedNamespaces and a wildcard in excludedNamespaces, ShouldExpandWildcards triggers expansion. The empty includes expands to nil, but wildcardExpanded is set to true, causing ShouldInclude to return false for all namespaces. Populate expanded includes with all active namespaces when the original includes was empty (meaning "include all") so that the wildcardExpanded check does not falsely reject everything. Signed-off-by: Joseph * Changelog Signed-off-by: Joseph * Normalize empty includes to * instead of active namespaces list This ensures consistent behavior between CLI and kubectl-apply paths for Namespace CR inclusion when excludes contain wildcards. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Joseph * Move empty includes normalization to backup controller Instead of normalizing empty IncludedNamespaces to ["*"] in the collections layer's ExpandIncludesExcludes, do it earlier in prepareBackupRequest. This ensures the spec is correct before any downstream processing. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Joseph * Update TestProcessBackupCompletions for wildcard normalization Add IncludedNamespaces: []string{"*"} to all expected BackupSpec structs, reflecting the new prepareBackupRequest normalization. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Joseph * Add checks around empty includenamespaces Signed-off-by: Joseph * gofmt Signed-off-by: Joseph --------- Signed-off-by: Joseph Co-authored-by: Claude Opus 4.6 (1M context) Signed-off-by: emirot * update hashicorp/go-hclog and go-plugin to current version (#9613) Signed-off-by: Peter Woodman Signed-off-by: emirot * fix: honor -stderrthreshold when -logtostderr is true (default) klog v2 defaults -logtostderr to true, which silently ignores the -stderrthreshold flag — all log levels are unconditionally sent to stderr. This makes it impossible for log-aggregation systems to filter by severity. Bump klog to v2.140.0 and opt into the fixed behavior by setting legacy_stderr_threshold_behavior=false and stderrthreshold=INFO (which preserves current output while letting users override via CLI flags). Ref: kubernetes/klog#212, kubernetes/klog#432 Signed-off-by: Pierluigi Lenoci Signed-off-by: emirot * fix: add changelog and nolint explanation for CI Add missing changelog entry for PR 9654 (fixes Changelog Check). Add explanation to //nolint:errcheck directives (fixes nolintlint). Signed-off-by: Pierluigi Lenoci Signed-off-by: emirot * Remove Restic code path from PodVolumeRestore. Signed-off-by: Xun Jiang Signed-off-by: emirot * Bump go.opentelemetry.io/otel from 1.40.0 to 1.41.0 Bumps [go.opentelemetry.io/otel](https://github.com/open-telemetry/opentelemetry-go) from 1.40.0 to 1.41.0. - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.40.0...v1.41.0) --- updated-dependencies: - dependency-name: go.opentelemetry.io/otel dependency-version: 1.41.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] Signed-off-by: emirot * Fix error in auto-request-review action Per action.yml of the action, the token is required. https://github.com/necojackarc/auto-request-review/blob/e89da1a8cd7c8c16d9de9c6e763290b6b0e3d424/action.yml#L8 Signed-off-by: Daniel Jiang Signed-off-by: emirot * fix go-releaser upload error Signed-off-by: Lyndon-Li Signed-off-by: emirot * add concurrency limit to go-releaser Signed-off-by: Lyndon-Li Signed-off-by: emirot * Bump go.opentelemetry.io/otel/sdk from 1.40.0 to 1.43.0 (#9692) Bumps [go.opentelemetry.io/otel/sdk](https://github.com/open-telemetry/opentelemetry-go) from 1.40.0 to 1.43.0. - [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases) - [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md) - [Commits](https://github.com/open-telemetry/opentelemetry-go/compare/v1.40.0...v1.43.0) --- updated-dependencies: - dependency-name: go.opentelemetry.io/otel/sdk dependency-version: 1.43.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: emirot * fix(lint): fix lint local Signed-off-by: emirot * Apply suggestion from @blackpiglet https://github.com/velero-io/velero/pull/9740/changes#r3151366281 Signed-off-by: Tiger Kaovilai --------- Signed-off-by: emirot Signed-off-by: Daniel Jiang Signed-off-by: Joseph Signed-off-by: Peter Woodman Signed-off-by: Pierluigi Lenoci Signed-off-by: Xun Jiang Signed-off-by: dependabot[bot] Signed-off-by: Lyndon-Li Signed-off-by: Tiger Kaovilai Co-authored-by: Daniel Jiang Co-authored-by: Joseph Antony Vaikath Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: peter woodman Co-authored-by: Pierluigi Lenoci Co-authored-by: Xun Jiang Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lyndon-Li Co-authored-by: Tiger Kaovilai Signed-off-by: emirot * Bump github.com/moby/spdystream from 0.5.0 to 0.5.1 (#9734) * Bump github.com/moby/spdystream from 0.5.0 to 0.5.1 Bumps [github.com/moby/spdystream](https://github.com/moby/spdystream) from 0.5.0 to 0.5.1. - [Release notes](https://github.com/moby/spdystream/releases) - [Commits](https://github.com/moby/spdystream/compare/v0.5.0...v0.5.1) --- updated-dependencies: - dependency-name: github.com/moby/spdystream dependency-version: 0.5.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] * fix: run go mod tidy to update module files Agent-Logs-Url: https://github.com/velero-io/velero/sessions/3537c5cb-5e31-405c-a79f-878bd146efa8 Co-authored-by: blackpiglet <59276555+blackpiglet@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] Signed-off-by: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Daniel Jiang Co-authored-by: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Signed-off-by: emirot * fix docker hub push error Signed-off-by: Lyndon-Li Signed-off-by: emirot * updating aws plugin to a matching version Signed-off-by: emirot --------- Signed-off-by: emirot Signed-off-by: dongqingcc Signed-off-by: Daniel Jiang Signed-off-by: Joseph Signed-off-by: Peter Woodman Signed-off-by: Pierluigi Lenoci Signed-off-by: Xun Jiang Signed-off-by: dependabot[bot] Signed-off-by: Lyndon-Li Signed-off-by: Tiger Kaovilai Signed-off-by: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Co-authored-by: dongqingcc Co-authored-by: Daniel Jiang Co-authored-by: Joseph Antony Vaikath Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: peter woodman Co-authored-by: Pierluigi Lenoci Co-authored-by: Xun Jiang Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lyndon-Li Co-authored-by: Tiger Kaovilai Co-authored-by: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Tiger Kaovilai --- changelogs/unreleased/9773-emirot | 1 + site/content/docs/main/contributions/minio.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/9773-emirot diff --git a/changelogs/unreleased/9773-emirot b/changelogs/unreleased/9773-emirot new file mode 100644 index 000000000..4c6f9f452 --- /dev/null +++ b/changelogs/unreleased/9773-emirot @@ -0,0 +1 @@ +docs(aws-plugin): update version diff --git a/site/content/docs/main/contributions/minio.md b/site/content/docs/main/contributions/minio.md index 41d0e997f..125f7e191 100644 --- a/site/content/docs/main/contributions/minio.md +++ b/site/content/docs/main/contributions/minio.md @@ -74,7 +74,7 @@ These instructions start the Velero server and a Minio instance that is accessib ``` velero install \ --provider aws \ - --plugins velero/velero-plugin-for-aws:v1.2.1 \ + --plugins velero/velero-plugin-for-aws:v1.14.0 \ --bucket velero \ --secret-file ./credentials-velero \ --use-volume-snapshots=false \ From a3d585f78d01ecd12810cca618b9f5933b414436 Mon Sep 17 00:00:00 2001 From: R4mbo Date: Wed, 26 Aug 2026 15:27:36 +0530 Subject: [PATCH 6/8] stop routing credential selection on AZURE_USERNAME after username/password removal (#10363) * stop routing credential selection on AZURE_USERNAME after username/password removal Signed-off-by: samay43 * add changelog entry Signed-off-by: samay43 --------- Signed-off-by: samay43 --- changelogs/unreleased/10363-samay43 | 1 + pkg/util/azure/credential.go | 3 +-- pkg/util/azure/credential_test.go | 22 ++++++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10363-samay43 diff --git a/changelogs/unreleased/10363-samay43 b/changelogs/unreleased/10363-samay43 new file mode 100644 index 000000000..b562a4cdf --- /dev/null +++ b/changelogs/unreleased/10363-samay43 @@ -0,0 +1 @@ +stop routing credential selection on AZURE_USERNAME after username/password removal diff --git a/pkg/util/azure/credential.go b/pkg/util/azure/credential.go index f36eb43a6..aeaff74f0 100644 --- a/pkg/util/azure/credential.go +++ b/pkg/util/azure/credential.go @@ -37,8 +37,7 @@ func NewCredential(creds map[string]string, options policy.ClientOptions) (azcor // config credential if len(creds[CredentialKeyClientSecret]) > 0 || len(creds[CredentialKeyClientCertificate]) > 0 || - len(creds[CredentialKeyClientCertificatePath]) > 0 || - len(creds[CredentialKeyUsername]) > 0 { + len(creds[CredentialKeyClientCertificatePath]) > 0 { return newConfigCredential(creds, configCredentialOptions{ ClientOptions: options, AdditionallyAllowedTenants: additionalTenants, diff --git a/pkg/util/azure/credential_test.go b/pkg/util/azure/credential_test.go index 40dd5e2c6..d92ee6a8f 100644 --- a/pkg/util/azure/credential_test.go +++ b/pkg/util/azure/credential_test.go @@ -69,6 +69,28 @@ func TestNewCredential(t *testing.T) { assert.IsType(t, &azidentity.WorkloadIdentityCredential{}, tokenCredential) os.Clearenv() + // a leftover AZURE_USERNAME must not hijack credential selection. Username/password + // handling was removed from newConfigCredential in #9041, so routing on it sends the + // caller into a function that cannot serve it and short-circuits the workload + // identity and managed identity branches below. + os.Setenv(CredentialKeyTenantID, "tenantid") + os.Setenv(CredentialKeyClientID, "clientid") + os.Setenv("AZURE_FEDERATED_TOKEN_FILE", "/tmp/token") + creds = map[string]string{CredentialKeyUsername: "username"} + tokenCredential, err = NewCredential(creds, options) + require.NoError(t, err) + assert.IsType(t, &azidentity.WorkloadIdentityCredential{}, tokenCredential) + os.Clearenv() + + // ... and must not short-circuit managed identity either + creds = map[string]string{ + CredentialKeyClientID: "clientid", + CredentialKeyUsername: "username", + } + tokenCredential, err = NewCredential(creds, options) + require.NoError(t, err) + assert.IsType(t, &azidentity.ManagedIdentityCredential{}, tokenCredential) + // managed identity credential creds = map[string]string{CredentialKeyClientID: "clientid"} tokenCredential, err = NewCredential(creds, options) From ac5744c7b4748893b1d43fd592351ea86e895442 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 26 Aug 2026 07:24:30 -0400 Subject: [PATCH 7/8] docs: move community meeting links to LFX Zoom, add calendar (#10409) Signed-off-by: Tiger Kaovilai --- site/content/community/_index.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/site/content/community/_index.md b/site/content/community/_index.md index cc1b63818..e8b2ca2af 100644 --- a/site/content/community/_index.md +++ b/site/content/community/_index.md @@ -16,8 +16,9 @@ You can follow the work we do via our [GitHub milestones](https://github.com/vel * Join our Kubernetes Slack channel and talk to over 800 other community members: [#velero-users](https://kubernetes.slack.com/messages/velero-users) * Join the Velero community meetings Bi-weekly community meeting alternating every week between Beijing Friendly timezone and EST/Europe Friendly Timezone - * Beijing/US friendly - we start at 8am Beijing Time(bound to CST) / 8pm EDT(7pm EST) / 5pm PDT(4pm PST) / 2am CEST(1am CET) - [Convert to your time zone](https://dateful.com/convert/beijing-china?t=8am) - [Zoom Link](https://broadcom.zoom.us/j/93945566592?pwd=rovF20vuI73kR6v67QBMpQuJOtM6sr.1&jst=2) - * US/Europe friendly - we start at 10am ET(bound to ET) / 7am PT / 3pm CET / 10pm(11pm) CST - [Convert to your time zone](https://dateful.com/convert/est-edt-eastern-time?t=10) - [Google meet link](https://meet.google.com/dyr-djtj-sko) + * Beijing/US friendly - we start at 8am Beijing Time(bound to CST) / 8pm EDT(7pm EST) / 5pm PDT(4pm PST) / 2am CEST(1am CET) - [Convert to your time zone](https://dateful.com/convert/beijing-china?t=8am) - [Zoom Link](https://zoom-lfx.platform.linuxfoundation.org/meeting/98821524848?password=579eadc1-f4aa-45aa-93c6-f7ea69d73b1a) + * US/Europe friendly - we start at 10am ET(bound to ET) / 7am PT / 3pm CET / 10pm(11pm) CST - [Convert to your time zone](https://dateful.com/convert/est-edt-eastern-time?t=10) - [Zoom Link](https://zoom-lfx.platform.linuxfoundation.org/meeting/95078224949?password=5f97cd2a-b140-4ede-add8-26a0816a8606) +* [Project meeting calendar](https://zoom-lfx.platform.linuxfoundation.org/meetings/velero?view=week) ([subscribe via iCal](https://webcal.prod.itx.linuxfoundation.org/lfx/lfpdCDzBbgNRCLpey8)) * Read and comment on the [meeting notes](https://hackmd.io/fCDVjqGuTG23CoOWQpoEVg) * See previous community meetings on our [YouTube Channel](https://www.youtube.com/playlist?list=PL7bmigfV0EqQRysvqvqOtRNk4L5S7uqwM) * Have a question to discuss in the community meeting? Please add it to our [Q&A Discussion board](https://github.com/velero-io/velero/discussions/categories/community-support-q-a) From b7d83a6f2bc68c0218ebb4d9f1efca2f3227ef11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wenkai=20Yin=28=E5=B0=B9=E6=96=87=E5=BC=80=29?= Date: Wed, 26 Aug 2026 22:21:32 +0800 Subject: [PATCH 8/8] Cherry pick the in-place restore implementation PRs from feature branch to main (#10415) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update CRDs and CLI to support in-place restore (#10038) Update CRDs(Restore, DataDownload, PodVolumeRestore) and restore create CLI to support in-place restore Signed-off-by: Wenkai Yin(尹文开) * Update Kopia(filesystem) uploader to support incremental and deleteExtraFile during restore (#10066) Update Kopia(filesystem) uploader to support incremental and deleteExtraFile during restore Signed-off-by: Wenkai Yin(尹文开) * Update Restore Exposer and PVC CSI to support in-place restore (#10104) 1. Update Restore Exposer to support exposing with existing PV for in-place restore 2. Update PVC CSI RIA to continue the restore process for in-place restore Signed-off-by: Wenkai Yin(尹文开) * Update Block uploader to support increase restore (#10244) Update Block uploader to support increase restore Signed-off-by: Wenkai Yin(尹文开) * Update Exposer to recreate the target PV if the volume mode is different with the restore PVC (#10257) Update Exposer to recreate the target PV if the volume mode is different with t he restore PVC Signed-off-by: Wenkai Yin(尹文开) * Preserve PVC selected-node annotation via carrier annotation for in-place restore For in-place volume data restore, the existing PVC is deleted and recreated. For StorageClasses with the WaitForFirstConsumer volume binding mode, losing the volume.kubernetes.io/selected-node annotation could let the scheduler place the recreated workload Pod in a different zone than the original PV, leaving it stuck in ContainerCreating. Instead of relying on RestoreItemAction execution order (the generic PVC RIA unconditionally strips the selected-node annotation), the PVC CSI RIA now captures the annotation from the existing PVC right before deleting it and carries it on the target PVC via the Velero-internal restore.velero.io/inplace-restore-selected-node annotation. The restore engine translates the carrier back to the Kubernetes annotation after all RestoreItemActions have run and always strips the carrier so it never lands on the cluster. This makes the behavior independent of RIA ordering: the Kubernetes annotation is stripped by default on every path (including when the target PVC does not exist and Velero falls back to provisioning a new PVC), and preservation only happens when the CSI RIA explicitly captured a value from the existing PVC. Signed-off-by: chlins * Update the control path to make the in-place incremental restore with block data mover work E2E (#10410) Update the control path to make the in-place incremental restore with block data mover work E2E Signed-off-by: Wenkai Yin(尹文开) --------- Signed-off-by: Wenkai Yin(尹文开) Signed-off-by: chlins Co-authored-by: chlins --- .../v1/bases/velero.io_podvolumerestores.yaml | 4 + config/crd/v1/bases/velero.io_restores.yaml | 11 + .../bases/velero.io_datadownloads.yaml | 32 ++ .../v2alpha1/bases/velero.io_datauploads.yaml | 4 + .../volume-data-inplace-restore.md | 12 +- pkg/apis/velero/v1/labels_annotations.go | 11 + pkg/apis/velero/v1/pod_volume_restore_type.go | 3 + pkg/apis/velero/v1/restore_types.go | 48 +- pkg/apis/velero/v1/restore_types_test.go | 69 +++ pkg/apis/velero/v1/zz_generated.deepcopy.go | 5 + .../velero/v2alpha1/data_download_types.go | 8 + pkg/apis/velero/v2alpha1/data_upload_types.go | 4 + .../velero/v2alpha1/zz_generated.deepcopy.go | 5 + pkg/builder/restore_builder.go | 8 +- pkg/cmd/cli/datamover/restore.go | 16 +- pkg/cmd/cli/nodeagent/server.go | 6 + pkg/cmd/cli/restore/create.go | 45 +- pkg/cmd/cli/restore/create_test.go | 6 + pkg/controller/data_download_controller.go | 110 +++-- .../data_download_controller_test.go | 32 +- pkg/controller/restore_controller.go | 7 +- pkg/controller/restore_controller_test.go | 33 ++ pkg/datamover/restore_micro_service.go | 19 +- pkg/datapath/data_path.go | 16 +- pkg/datapath/data_path_test.go | 2 +- pkg/exposer/csi_snapshot.go | 62 +-- pkg/exposer/csi_snapshot_test.go | 28 +- pkg/exposer/generic_restore.go | 166 ++++++- pkg/exposer/generic_restore_priority_test.go | 6 + pkg/exposer/generic_restore_test.go | 178 +++++++- pkg/exposer/mocks/GenericRestoreExposer.go | 18 +- pkg/podvolume/restore_micro_service.go | 4 +- pkg/podvolume/restorer.go | 4 + pkg/restore/actions/csi/pvc_action.go | 429 +++++++++++++----- pkg/restore/actions/csi/pvc_action_test.go | 180 +++++++- pkg/restore/restore.go | 38 +- pkg/restore/restore_test.go | 181 ++++++++ pkg/uploader/block/snapshot.go | 34 +- pkg/uploader/block/snapshot_test.go | 181 +++++++- pkg/uploader/kopia/snapshot.go | 23 +- pkg/uploader/kopia/snapshot_test.go | 3 +- pkg/uploader/provider/block.go | 4 +- pkg/uploader/provider/block_test.go | 10 +- pkg/uploader/provider/kopia.go | 4 +- pkg/uploader/provider/kopia_test.go | 11 +- pkg/uploader/provider/mocks/Provider.go | 48 +- pkg/uploader/provider/provider.go | 2 + pkg/uploader/util/uploader_config.go | 20 + pkg/uploader/util/uploader_config_test.go | 82 ++++ pkg/util/csi/cbt.go | 80 ++++ pkg/util/kube/pvc_pv.go | 56 +++ pkg/util/kube/pvc_pv_test.go | 102 +++++ pkg/util/velero/restore/util.go | 14 +- pkg/util/velero/restore/util_test.go | 15 +- 54 files changed, 2164 insertions(+), 335 deletions(-) create mode 100644 pkg/apis/velero/v1/restore_types_test.go create mode 100644 pkg/util/csi/cbt.go diff --git a/config/crd/v1/bases/velero.io_podvolumerestores.yaml b/config/crd/v1/bases/velero.io_podvolumerestores.yaml index 015d143fe..2eea696c2 100644 --- a/config/crd/v1/bases/velero.io_podvolumerestores.yaml +++ b/config/crd/v1/bases/velero.io_podvolumerestores.yaml @@ -132,6 +132,9 @@ spec: repoIdentifier: description: RepoIdentifier is the backup repository identifier. type: string + restoreType: + description: RestoreType indicates the type of the restore. + type: string snapshotID: description: SnapshotID is the ID of the volume snapshot to be restored. type: string @@ -167,6 +170,7 @@ spec: - backupStorageLocation - pod - repoIdentifier + - restoreType - snapshotID - sourceNamespace - volume diff --git a/config/crd/v1/bases/velero.io_restores.yaml b/config/crd/v1/bases/velero.io_restores.yaml index e12ea9b4f..b58666cca 100644 --- a/config/crd/v1/bases/velero.io_restores.yaml +++ b/config/crd/v1/bases/velero.io_restores.yaml @@ -89,6 +89,11 @@ spec: for the Kubernetes resource to be restored nullable: true type: string + existingVolumeDataPolicy: + description: ExistingVolumeDataPolicy specifies the restore behavior + for the volume data to be restored + nullable: true + type: string hooks: description: Hooks represent custom behaviors that should be executed during or post restore. @@ -499,6 +504,12 @@ spec: description: UploaderConfig specifies the configuration for the restore. nullable: true properties: + deleteExtraFiles: + description: |- + DeleteExtraFiles specifies whether to delete the extra files in the target volume that do not exist in the backup. + This setting is only applicable to File System restores (PodVolumeBackup or CSI File System Data Move) and has no effect on Block Data Move restores. + nullable: true + type: boolean parallelFilesDownload: description: ParallelFilesDownload is the concurrency number setting for restore. diff --git a/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml b/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml index fa3757a9d..71e662fe8 100644 --- a/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml +++ b/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml @@ -83,6 +83,34 @@ spec: Cancel indicates request to cancel the ongoing DataDownload. It can be set when the DataDownload is in InProgress phase type: boolean + csiSnapshot: + description: CSISnapshot provides the information of the CSI snapshot + used to do the incremental restore. + nullable: true + properties: + driver: + description: Driver is the driver used by the VolumeSnapshotContent + type: string + snapshotClass: + description: SnapshotClass is the name of the snapshot class that + the volume snapshot is created with + type: string + storageClass: + description: StorageClass is the name of the storage class of + the PVC that the volume snapshot is created from + type: string + volumeSnapshot: + description: VolumeSnapshot is the name of the volume snapshot + to be backed up + type: string + volumeSnapshotNamespace: + description: VolumeSnapshotNamespace is the namespece of the volume + snapshot to be backed up + type: string + required: + - storageClass + - volumeSnapshot + type: object dataMoverConfig: additionalProperties: type: string @@ -106,6 +134,9 @@ spec: OperationTimeout specifies the time used to wait internal operations, before returning error as timeout. type: string + restoreType: + description: RestoreType indicates the type of the restore. + type: string snapshotID: description: SnapshotID is the ID of the Velero backup snapshot to be restored from. @@ -145,6 +176,7 @@ spec: required: - backupStorageLocation - operationTimeout + - restoreType - snapshotID - sourceNamespace - targetVolume diff --git a/config/crd/v2alpha1/bases/velero.io_datauploads.yaml b/config/crd/v2alpha1/bases/velero.io_datauploads.yaml index 8d03da279..a3d7dbe80 100644 --- a/config/crd/v2alpha1/bases/velero.io_datauploads.yaml +++ b/config/crd/v2alpha1/bases/velero.io_datauploads.yaml @@ -110,6 +110,10 @@ spec: description: VolumeSnapshot is the name of the volume snapshot to be backed up type: string + volumeSnapshotNamespace: + description: VolumeSnapshotNamespace is the namespece of the volume + snapshot to be backed up + type: string required: - storageClass - volumeSnapshot diff --git a/design/volume-data-inplace-restore/volume-data-inplace-restore.md b/design/volume-data-inplace-restore/volume-data-inplace-restore.md index 664f5a654..b178b9314 100644 --- a/design/volume-data-inplace-restore/volume-data-inplace-restore.md +++ b/design/volume-data-inplace-restore/volume-data-inplace-restore.md @@ -212,7 +212,9 @@ Users must manage the lifecycle of their workloads before starting the restore. When performing an in-place restore, Velero deletes the existing target PVC and recreates it. For StorageClasses using the `WaitForFirstConsumer` volume binding mode, this recreation resets the scheduling lifecycle. Even though Velero adds a selector to the PVC spec to ensure it binds exclusively to the original PV, a scheduling issue can still occur. If the target PVC loses its node affinity, the Kubernetes Scheduler might schedule the recreated business Pod to a different availability zone. Because the original PV is physically constrained to its original zone, the Pod will fail to mount the volume and remain stuck in the `ContainerCreating` state with an attachment error. **Solution**: -During the PVC Restore Item Action (RIA), Velero must extract the `volume.kubernetes.io/selected-node` annotation from the original PVC. When Velero recreates the target PVC, it must inject this annotation back into the PVC spec. +During the PVC CSI Restore Item Action (RIA), right before deleting the existing PVC, Velero extracts the `volume.kubernetes.io/selected-node` annotation from that PVC and carries it on the PVC to be restored via a Velero-internal carrier annotation (`restore.velero.io/inplace-restore-selected-node`). After all Restore Item Actions have run, the restore engine translates the carrier back to the `volume.kubernetes.io/selected-node` annotation and strips the carrier so it never lands on the cluster. + +A carrier annotation is used instead of the Kubernetes annotation directly because the generic PVC RIA unconditionally strips the `selected-node` annotation during restore, and the execution order of Restore Item Actions is not a documented contract. With the carrier, the behavior is independent of the RIA execution order: the Kubernetes annotation is stripped by default on every path (including when the target PVC does not exist and Velero falls back to provisioning a new PVC), and preservation only happens when the PVC CSI RIA explicitly captured a value from the existing PVC. By preserving the `selected-node` annotation, the Kubernetes Scheduler is forced to schedule the recreated business Pod to the original node/zone, ensuring it successfully mounts the restored PV. ### Namespace Mapping @@ -260,10 +262,8 @@ This section outlines the step-by-step control path and data path workflows for **Control Path** -PVC RIA: -- Preserve the `volume.kubernetes.io/selected-node` annotation to ensure correct scheduling during target PVC recreation. - PVC CSI RIA: +- Capture the `volume.kubernetes.io/selected-node` annotation from the existing PVC into the Velero-internal carrier annotation before deleting the PVC, so the restore engine can re-apply it to the recreated target PVC (see [Handling Cross-Zone Scheduling](#handling-cross-zone-scheduling-waitforfirstconsumer)). - Create a snapshot of the existing `PVC` to serve as the baseline for CBT delta calculations. - Patch the existing PV's reclaim policy to `Retain`. - Delete the existing PVC. @@ -308,10 +308,8 @@ The workflow is identical to the **In-place Incremental Restore for CSI Snapshot **Control Path** -PVC RIA: -- Preserve the `volume.kubernetes.io/selected-node` annotation to ensure correct scheduling during target PVC recreation. - PVC CSI RIA: +- Capture the `volume.kubernetes.io/selected-node` annotation from the existing PVC into the Velero-internal carrier annotation before deleting the PVC, so the restore engine can re-apply it to the recreated target PVC (see [Handling Cross-Zone Scheduling](#handling-cross-zone-scheduling-waitforfirstconsumer)). - Create a snapshot of the existing `PVC` to serve as the baseline for CBT delta calculations. - Patch the existing `PV` to set its `persistentVolumeReclaimPolicy` to `Retain`. - Delete the existing `PVC`. diff --git a/pkg/apis/velero/v1/labels_annotations.go b/pkg/apis/velero/v1/labels_annotations.go index b34f05ed9..5636ecd36 100644 --- a/pkg/apis/velero/v1/labels_annotations.go +++ b/pkg/apis/velero/v1/labels_annotations.go @@ -174,6 +174,17 @@ const ( // Notice: SkipRestore on the Execute output takes precedence. If SkipRestore is true, the // annotation is never inspected and AdditionalItems are not processed. MustIncludeAdditionalItemRestoreAnnotation = "restore.velero.io/must-include-additional-items" + + // InplaceRestoreSelectedNodeAnnotation is a Velero-internal carrier annotation set by the + // PVC CSI RestoreItemAction during an in-place volume data restore. It carries the + // "volume.kubernetes.io/selected-node" value captured from the existing PVC right before + // that PVC is deleted, so the restore engine can re-apply it to the recreated target PVC + // after all RestoreItemActions have run. This keeps the recreated PVC (and the workload + // Pod, for WaitForFirstConsumer StorageClasses) scheduled to the original node/zone. + // The annotation is always translated and stripped by the restore engine; it never lands + // on the cluster. Using a carrier annotation avoids any dependency on the execution order + // of RestoreItemActions. + InplaceRestoreSelectedNodeAnnotation = "restore.velero.io/inplace-restore-selected-node" // SkippedNoCSIPVAnnotation - Velero checks this annotation on processed PVC to // find out if the snapshot was skipped b/c the PV is not provisioned via CSI SkippedNoCSIPVAnnotation = "backup.velero.io/skipped-no-csi-pv" diff --git a/pkg/apis/velero/v1/pod_volume_restore_type.go b/pkg/apis/velero/v1/pod_volume_restore_type.go index 96c1a1e4b..5ded78175 100644 --- a/pkg/apis/velero/v1/pod_volume_restore_type.go +++ b/pkg/apis/velero/v1/pod_volume_restore_type.go @@ -46,6 +46,9 @@ type PodVolumeRestoreSpec struct { // SnapshotID is the ID of the volume snapshot to be restored. SnapshotID string `json:"snapshotID"` + // RestoreType indicates the type of the restore. + RestoreType string `json:"restoreType"` + // SourceNamespace is the original namespace for namaspace mapping. SourceNamespace string `json:"sourceNamespace"` diff --git a/pkg/apis/velero/v1/restore_types.go b/pkg/apis/velero/v1/restore_types.go index 416a2b8ca..312781e2a 100644 --- a/pkg/apis/velero/v1/restore_types.go +++ b/pkg/apis/velero/v1/restore_types.go @@ -113,7 +113,12 @@ type RestoreSpec struct { // ExistingResourcePolicy specifies the restore behavior for the Kubernetes resource to be restored // +optional // +nullable - ExistingResourcePolicy PolicyType `json:"existingResourcePolicy,omitempty"` + ExistingResourcePolicy ResourcePolicyType `json:"existingResourcePolicy,omitempty"` + + // ExistingVolumeDataPolicy specifies the restore behavior for the volume data to be restored + // +optional + // +nullable + ExistingVolumeDataPolicy VolumeDataPolicyType `json:"existingVolumeDataPolicy,omitempty"` // ItemOperationTimeout specifies the time used to wait for RestoreItemAction operations // The default value is 4 hour. @@ -158,6 +163,11 @@ type UploaderConfigForRestore struct { // ParallelFilesDownload is the concurrency number setting for restore. // +optional ParallelFilesDownload int `json:"parallelFilesDownload,omitempty"` + // DeleteExtraFiles specifies whether to delete the extra files in the target volume that do not exist in the backup. + // This setting is only applicable to File System restores (PodVolumeBackup or CSI File System Data Move) and has no effect on Block Data Move restores. + // +optional + // +nullable + DeleteExtraFiles *bool `json:"deleteExtraFiles,omitempty"` } // RestoreHooks contains custom behaviors that should be executed during or post restore. @@ -324,13 +334,22 @@ const ( // The failing error is recorded in status.FailureReason. RestorePhaseFailed RestorePhase = "Failed" - // PolicyTypeNone means velero will not overwrite the resource + // ResourcePolicyTypeNone means velero will not overwrite the resource // in cluster with the one in backup whether changed/unchanged. - PolicyTypeNone PolicyType = "none" + ResourcePolicyTypeNone ResourcePolicyType = "none" - // PolicyTypeUpdate means velero will try to attempt a patch on + // ResourcePolicyTypeUpdate means velero will try to attempt a patch on // the changed resources. - PolicyTypeUpdate PolicyType = "update" + ResourcePolicyTypeUpdate ResourcePolicyType = "update" + + // VolumeDataPolicyTypeNone means velero will skip and not overwrite the volume data if the volume already exists + VolumeDataPolicyTypeNone VolumeDataPolicyType = "none" + + // VolumeDataPolicyTypeFull means velero will try to restore the volume data fully if the volume already exists. + VolumeDataPolicyTypeFull VolumeDataPolicyType = "full" + + // VolumeDataPolicyTypeIncremental means velero will try to restore the volume data incrementally if the volume already exists. + VolumeDataPolicyTypeIncremental VolumeDataPolicyType = "incremental" ) // RestoreStatus captures the current status of a Velero restore @@ -441,6 +460,18 @@ type Restore struct { Status RestoreStatus `json:"status,omitempty"` } +func (r *Restore) IsVolumeDataInplaceRestore() bool { + return r.Spec.ExistingVolumeDataPolicy == VolumeDataPolicyTypeFull || r.Spec.ExistingVolumeDataPolicy == VolumeDataPolicyTypeIncremental +} + +func (r *Restore) IsVolumeDataInplaceFullRestore() bool { + return r.Spec.ExistingVolumeDataPolicy == VolumeDataPolicyTypeFull +} + +func (r *Restore) IsVolumeDataInplaceIncrementalRestore() bool { + return r.Spec.ExistingVolumeDataPolicy == VolumeDataPolicyTypeIncremental +} + // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // RestoreList is a list of Restores. @@ -453,5 +484,8 @@ type RestoreList struct { Items []Restore `json:"items"` } -// PolicyType helps specify the ExistingResourcePolicy -type PolicyType string +// ResourcePolicyType helps specify the ExistingResourcePolicy +type ResourcePolicyType string + +// VolumeDataPolicyType helps specify the ExistingVolumeDataPolicy +type VolumeDataPolicyType string diff --git a/pkg/apis/velero/v1/restore_types_test.go b/pkg/apis/velero/v1/restore_types_test.go new file mode 100644 index 000000000..72063d6f2 --- /dev/null +++ b/pkg/apis/velero/v1/restore_types_test.go @@ -0,0 +1,69 @@ +/* +Copyright The Velero Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "testing" +) + +func TestIsVolumeDataInplaceRestore(t *testing.T) { + tests := []struct { + name string + existingVolumeDataPolicy VolumeDataPolicyType + expected bool + }{ + { + name: "empty policy", + existingVolumeDataPolicy: "", + expected: false, + }, + { + name: "none policy", + existingVolumeDataPolicy: VolumeDataPolicyTypeNone, + expected: false, + }, + { + name: "full policy", + existingVolumeDataPolicy: VolumeDataPolicyTypeFull, + expected: true, + }, + { + name: "incremental policy", + existingVolumeDataPolicy: VolumeDataPolicyTypeIncremental, + expected: true, + }, + { + name: "unknown policy", + existingVolumeDataPolicy: VolumeDataPolicyType("unknown"), + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + restore := &Restore{ + Spec: RestoreSpec{ + ExistingVolumeDataPolicy: tc.existingVolumeDataPolicy, + }, + } + actual := restore.IsVolumeDataInplaceRestore() + if actual != tc.expected { + t.Errorf("expected %v, got %v", tc.expected, actual) + } + }) + } +} diff --git a/pkg/apis/velero/v1/zz_generated.deepcopy.go b/pkg/apis/velero/v1/zz_generated.deepcopy.go index 78f756640..f4dc8a79a 100644 --- a/pkg/apis/velero/v1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v1/zz_generated.deepcopy.go @@ -1771,6 +1771,11 @@ func (in *UploaderConfigForRestore) DeepCopyInto(out *UploaderConfigForRestore) *out = new(bool) **out = **in } + if in.DeleteExtraFiles != nil { + in, out := &in.DeleteExtraFiles, &out.DeleteExtraFiles + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UploaderConfigForRestore. diff --git a/pkg/apis/velero/v2alpha1/data_download_types.go b/pkg/apis/velero/v2alpha1/data_download_types.go index 297a064b8..57827b97d 100644 --- a/pkg/apis/velero/v2alpha1/data_download_types.go +++ b/pkg/apis/velero/v2alpha1/data_download_types.go @@ -39,6 +39,14 @@ type DataDownloadSpec struct { // SnapshotID is the ID of the Velero backup snapshot to be restored from. SnapshotID string `json:"snapshotID"` + // RestoreType indicates the type of the restore. + RestoreType string `json:"restoreType"` + + // CSISnapshot provides the information of the CSI snapshot used to do the incremental restore. + // +optional + // +nullable + CSISnapshot *CSISnapshotSpec `json:"csiSnapshot"` + // SourceNamespace is the original namespace where the volume is backed up from. // It may be different from SourcePVC's namespace if namespace is remapped during restore. SourceNamespace string `json:"sourceNamespace"` diff --git a/pkg/apis/velero/v2alpha1/data_upload_types.go b/pkg/apis/velero/v2alpha1/data_upload_types.go index 6f28d399b..db4c8d3a8 100644 --- a/pkg/apis/velero/v2alpha1/data_upload_types.go +++ b/pkg/apis/velero/v2alpha1/data_upload_types.go @@ -80,6 +80,10 @@ const ( // CSISnapshotSpec is the specification for a CSI snapshot. type CSISnapshotSpec struct { + // VolumeSnapshotNamespace is the namespece of the volume snapshot to be backed up + // +optional + VolumeSnapshotNamespace string `json:"volumeSnapshotNamespace"` + // VolumeSnapshot is the name of the volume snapshot to be backed up VolumeSnapshot string `json:"volumeSnapshot"` diff --git a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go index 0513824bd..927dc531c 100644 --- a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go @@ -86,6 +86,11 @@ func (in *DataDownloadList) DeepCopyObject() runtime.Object { func (in *DataDownloadSpec) DeepCopyInto(out *DataDownloadSpec) { *out = *in out.TargetVolume = in.TargetVolume + if in.CSISnapshot != nil { + in, out := &in.CSISnapshot, &out.CSISnapshot + *out = new(CSISnapshotSpec) + **out = **in + } if in.DataMoverConfig != nil { in, out := &in.DataMoverConfig, &out.DataMoverConfig *out = make(map[string]string, len(*in)) diff --git a/pkg/builder/restore_builder.go b/pkg/builder/restore_builder.go index 472e51a21..5ef993617 100644 --- a/pkg/builder/restore_builder.go +++ b/pkg/builder/restore_builder.go @@ -98,7 +98,13 @@ func (b *RestoreBuilder) ExcludedResources(resources ...string) *RestoreBuilder // ExistingResourcePolicy sets the Restore's resource policy. func (b *RestoreBuilder) ExistingResourcePolicy(policy string) *RestoreBuilder { - b.object.Spec.ExistingResourcePolicy = velerov1api.PolicyType(policy) + b.object.Spec.ExistingResourcePolicy = velerov1api.ResourcePolicyType(policy) + return b +} + +// ExistingVolumeDataPolicy sets the Restore's volume data policy. +func (b *RestoreBuilder) ExistingVolumeDataPolicy(policy string) *RestoreBuilder { + b.object.Spec.ExistingVolumeDataPolicy = velerov1api.VolumeDataPolicyType(policy) return b } diff --git a/pkg/cmd/cli/datamover/restore.go b/pkg/cmd/cli/datamover/restore.go index ed6867e96..6b112a248 100644 --- a/pkg/cmd/cli/datamover/restore.go +++ b/pkg/cmd/cli/datamover/restore.go @@ -36,6 +36,7 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1" "github.com/vmware-tanzu/velero/pkg/buildinfo" + "github.com/vmware-tanzu/velero/pkg/cbtservice" "github.com/vmware-tanzu/velero/pkg/client" "github.com/vmware-tanzu/velero/pkg/cmd/util/signals" "github.com/vmware-tanzu/velero/pkg/datamover" @@ -56,6 +57,9 @@ type dataMoverRestoreConfig struct { ddName string cacheDir string resourceTimeout time.Duration + cbtSAName string + vsNamespace string + volumeID string } func NewRestoreCommand(f client.Factory) *cobra.Command { @@ -96,6 +100,9 @@ func NewRestoreCommand(f client.Factory) *cobra.Command { command.Flags().StringVar(&config.ddName, "data-download", config.ddName, "The data download name") command.Flags().StringVar(&config.cacheDir, "cache-volume-path", config.cacheDir, "The full path of the cache volume") command.Flags().DurationVar(&config.resourceTimeout, "resource-timeout", config.resourceTimeout, "How long to wait for resource processes which are not covered by other specific timeout parameters.") + command.Flags().StringVar(&config.cbtSAName, "cbt-sa-name", config.cbtSAName, "The name of the service account used by CSI's CBT service") + command.Flags().StringVar(&config.vsNamespace, "vs-namespace", config.vsNamespace, "The namespace of the VolumeSnapshot") + command.Flags().StringVar(&config.volumeID, "volume-id", config.volumeID, "The volume ID of the snapshot") _ = command.MarkFlagRequired("volume-path") _ = command.MarkFlagRequired("volume-mode") @@ -116,6 +123,7 @@ type dataMoverRestore struct { config dataMoverRestoreConfig kubeClient kubernetes.Interface dataPathMgr *datapath.Manager + cbtService cbtservice.Service } func newdataMoverRestore(logger logrus.FieldLogger, factory client.Factory, config dataMoverRestoreConfig) (*dataMoverRestore, error) { @@ -201,6 +209,12 @@ func newdataMoverRestore(logger logrus.FieldLogger, factory client.Factory, conf config: config, namespace: factory.Namespace(), nodeName: nodeName, + cbtService: cbtservice.NewService( + logger, + config.vsNamespace, + config.cbtSAName, + clientConfig, + ), } s.kubeClient, err = factory.KubeClient() @@ -294,5 +308,5 @@ func (s *dataMoverRestore) createDataPathService() (dataPathService, error) { return datamover.NewRestoreMicroService(s.ctx, s.client, s.kubeClient, s.config.ddName, s.namespace, s.nodeName, datapath.AccessPoint{ ByPath: s.config.volumePath, VolMode: uploader.PersistentVolumeMode(s.config.volumeMode), - }, s.dataPathMgr, repoEnsurer, credGetter, duInformer, s.config.cacheDir, s.logger), nil + }, s.dataPathMgr, repoEnsurer, credGetter, duInformer, s.config.cacheDir, s.config.volumeID, s.cbtService, s.logger), nil } diff --git a/pkg/cmd/cli/nodeagent/server.go b/pkg/cmd/cli/nodeagent/server.go index 287e45591..c1442aab0 100644 --- a/pkg/cmd/cli/nodeagent/server.go +++ b/pkg/cmd/cli/nodeagent/server.go @@ -27,6 +27,7 @@ import ( "github.com/bombsimon/logrusr/v3" "github.com/cockroachdb/errors" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" snapshotv1client "github.com/kubernetes-csi/external-snapshotter/client/v8/clientset/versioned" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/sirupsen/logrus" @@ -175,6 +176,10 @@ func newNodeAgentServer(logger logrus.FieldLogger, factory client.Factory, confi cancelFunc() return nil, err } + if err := snapshotv1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } nodeName := os.Getenv("NODE_NAME") @@ -484,6 +489,7 @@ func (s *nodeAgentServer) run() { s.repoConfigMgr, podLabels, podAnnotations, + csiSnapshotMetadataServiceConfigs, ) if err := dataDownloadReconciler.SetupWithManager(s.mgr); err != nil { diff --git a/pkg/cmd/cli/restore/create.go b/pkg/cmd/cli/restore/create.go index ac4284229..7b65407de 100644 --- a/pkg/cmd/cli/restore/create.go +++ b/pkg/cmd/cli/restore/create.go @@ -99,6 +99,7 @@ type CreateOptions struct { IncludeNamespaces flag.StringArray ExcludeNamespaces flag.StringArray ExistingResourcePolicy string + ExistingVolumeDataPolicy string IncludeResources flag.StringArray ExcludeResources flag.StringArray StatusIncludeResources flag.StringArray @@ -115,6 +116,7 @@ type CreateOptions struct { SkipDefaultResourceModifier bool WriteSparseFiles flag.OptionalBool ParallelFilesDownload int + DeleteExtraFiles flag.OptionalBool client kbclient.WithWatch } @@ -128,6 +130,7 @@ func NewCreateOptions() *CreateOptions { PreserveNodePorts: flag.NewOptionalBool(nil), IncludeClusterResources: flag.NewOptionalBool(nil), WriteSparseFiles: flag.NewOptionalBool(nil), + DeleteExtraFiles: flag.NewOptionalBool(nil), } } @@ -141,7 +144,8 @@ func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { flags.Var(&o.Annotations, "annotations", "Annotations to apply to the restore.") flags.Var(&o.IncludeResources, "include-resources", "Resources to include in the restore, formatted as resource.group, such as storageclasses.storage.k8s.io (use '*' for all resources).") flags.Var(&o.ExcludeResources, "exclude-resources", "Resources to exclude from the restore, formatted as resource.group, such as storageclasses.storage.k8s.io.") - flags.StringVar(&o.ExistingResourcePolicy, "existing-resource-policy", "", "Restore Policy to be used during the restore workflow, can be - none or update") + flags.StringVar(&o.ExistingResourcePolicy, "existing-resource-policy", "", "Restore Policy to be used during the restore workflow for Kubernetes resources, can be - none or update") + flags.StringVar(&o.ExistingVolumeDataPolicy, "existing-volume-data-policy", "", "Restore Policy to be used during the restore workflow for volume data, can be - none, full or incremental") flags.Var(&o.StatusIncludeResources, "status-include-resources", "Resources to include in the restore status, formatted as resource.group, such as storageclasses.storage.k8s.io.") flags.Var(&o.StatusExcludeResources, "status-exclude-resources", "Resources to exclude from the restore status, formatted as resource.group, such as storageclasses.storage.k8s.io.") flags.VarP(&o.Selector, "selector", "l", "Only restore resources matching this label selector.") @@ -175,6 +179,9 @@ func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { f.NoOptDefVal = cmd.TRUE flags.IntVar(&o.ParallelFilesDownload, "parallel-files-download", 0, "The number of restore operations to run in parallel. If set to 0, the default parallelism will be the number of CPUs for the node that node agent pod is running.") + + f = flags.VarPF(&o.DeleteExtraFiles, "delete-extra-files", "", "Whether to delete extra files in the target volume that do not exist in the backup during file system restore. This setting is only applicable to File System restores (PodVolumeBackup or CSI File System Data Move) and has no effect on Block Data Move restores.") + f.NoOptDefVal = cmd.TRUE } func (o *CreateOptions) Complete(args []string, f client.Factory) error { @@ -224,6 +231,10 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return errors.New("existing-resource-policy has invalid value, it accepts only none, update as value") } + if len(o.ExistingVolumeDataPolicy) > 0 && !restore.IsVolumeDataPolicyValid(o.ExistingVolumeDataPolicy) { + return errors.New("existing-volume-data-policy has invalid value, it accepts only none, full, incremental as value") + } + if o.ParallelFilesDownload < 0 { return errors.New("parallel-files-download cannot be negative") } @@ -344,27 +355,29 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { Annotations: o.Annotations.Data(), }, Spec: api.RestoreSpec{ - BackupName: o.BackupName, - ScheduleName: o.ScheduleName, - IncludedNamespaces: o.IncludeNamespaces, - ExcludedNamespaces: o.ExcludeNamespaces, - IncludedResources: o.IncludeResources, - ExcludedResources: o.ExcludeResources, - ExistingResourcePolicy: api.PolicyType(o.ExistingResourcePolicy), - NamespaceMapping: o.NamespaceMappings.Data(), - LabelSelector: o.Selector.LabelSelector, - OrLabelSelectors: o.OrSelector.OrLabelSelectors, - RestorePVs: o.RestoreVolumes.Value, - PreserveNodePorts: o.PreserveNodePorts.Value, - IncludeClusterResources: o.IncludeClusterResources.Value, - ResourceModifier: resModifiers, - ResourcePolicy: resPolicies, + BackupName: o.BackupName, + ScheduleName: o.ScheduleName, + IncludedNamespaces: o.IncludeNamespaces, + ExcludedNamespaces: o.ExcludeNamespaces, + IncludedResources: o.IncludeResources, + ExcludedResources: o.ExcludeResources, + ExistingResourcePolicy: api.ResourcePolicyType(o.ExistingResourcePolicy), + ExistingVolumeDataPolicy: api.VolumeDataPolicyType(o.ExistingVolumeDataPolicy), + NamespaceMapping: o.NamespaceMappings.Data(), + LabelSelector: o.Selector.LabelSelector, + OrLabelSelectors: o.OrSelector.OrLabelSelectors, + RestorePVs: o.RestoreVolumes.Value, + PreserveNodePorts: o.PreserveNodePorts.Value, + IncludeClusterResources: o.IncludeClusterResources.Value, + ResourceModifier: resModifiers, + ResourcePolicy: resPolicies, ItemOperationTimeout: metav1.Duration{ Duration: o.ItemOperationTimeout, }, UploaderConfig: &api.UploaderConfigForRestore{ WriteSparseFiles: o.WriteSparseFiles.Value, ParallelFilesDownload: o.ParallelFilesDownload, + DeleteExtraFiles: o.DeleteExtraFiles.Value, }, }, } diff --git a/pkg/cmd/cli/restore/create_test.go b/pkg/cmd/cli/restore/create_test.go index 643340e22..50d4aebde 100644 --- a/pkg/cmd/cli/restore/create_test.go +++ b/pkg/cmd/cli/restore/create_test.go @@ -68,6 +68,7 @@ func TestCreateCommand(t *testing.T) { includeNamespaces := "app1,app2" excludeNamespaces := "pod1,pod2,pod3" existingResourcePolicy := "none" + existingVolumeDataPolicy := "none" includeResources := "sc,sts" excludeResources := "job" statusIncludeResources := "sc,sts" @@ -80,6 +81,7 @@ func TestCreateCommand(t *testing.T) { resourceModifierConfigMap := "modifier-cm" ResourcePoliciesConfigMap := "policies-cm" writeSparseFiles := "true" + deleteExtraFiles := "true" parallel := 2 flags := new(pflag.FlagSet) o := NewCreateOptions() @@ -92,6 +94,7 @@ func TestCreateCommand(t *testing.T) { flags.Parse([]string{"--labels", labels}) flags.Parse([]string{"--annotations", annotations}) flags.Parse([]string{"--existing-resource-policy", existingResourcePolicy}) + flags.Parse([]string{"--existing-volume-data-policy", existingVolumeDataPolicy}) flags.Parse([]string{"--include-namespaces", includeNamespaces}) flags.Parse([]string{"--exclude-namespaces", excludeNamespaces}) flags.Parse([]string{"--include-resources", includeResources}) @@ -107,6 +110,7 @@ func TestCreateCommand(t *testing.T) { flags.Parse([]string{"--resource-policies-configmap", ResourcePoliciesConfigMap}) flags.Parse([]string{"--skip-default-resource-modifier"}) flags.Parse([]string{"--write-sparse-files", writeSparseFiles}) + flags.Parse([]string{"--delete-extra-files", deleteExtraFiles}) flags.Parse([]string{"--parallel-files-download", "2"}) client := velerotest.NewFakeControllerRuntimeClient(t).(kbclient.WithWatch) @@ -134,6 +138,7 @@ func TestCreateCommand(t *testing.T) { require.Equal(t, includeNamespaces, o.IncludeNamespaces.String()) require.Equal(t, excludeNamespaces, o.ExcludeNamespaces.String()) require.Equal(t, existingResourcePolicy, o.ExistingResourcePolicy) + require.Equal(t, existingVolumeDataPolicy, o.ExistingVolumeDataPolicy) require.Equal(t, includeResources, o.IncludeResources.String()) require.Equal(t, excludeResources, o.ExcludeResources.String()) @@ -149,6 +154,7 @@ func TestCreateCommand(t *testing.T) { require.True(t, o.SkipDefaultResourceModifier) require.Equal(t, writeSparseFiles, o.WriteSparseFiles.String()) require.Equal(t, parallel, o.ParallelFilesDownload) + require.Equal(t, deleteExtraFiles, o.DeleteExtraFiles.String()) }) t.Run("create a restore from schedule", func(t *testing.T) { diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 1e867fed5..d8062bc72 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -59,27 +59,28 @@ import ( // DataDownloadReconciler reconciles a DataDownload object type DataDownloadReconciler struct { - client client.Client - kubeClient kubernetes.Interface - mgr manager.Manager - logger logrus.FieldLogger - Clock clock.WithTickerAndDelayedExecution - restoreExposer exposer.GenericRestoreExposer - nodeName string - dataPathMgr *datapath.Manager - vgdpCounter *exposer.VgdpCounter - loadAffinity []*kube.LoadAffinity - restorePVCConfig velerotypes.RestorePVC - backupRepoConfigs map[string]string - cacheVolumeConfigs *velerotypes.CachePVC - podResources corev1api.ResourceRequirements - preparingTimeout time.Duration - metrics *metrics.ServerMetrics - cancelledDataDownload sync.Map - dataMovePriorityClass string - repoConfigMgr repository.ConfigManager - podLabels map[string]string - podAnnotations map[string]string + client client.Client + kubeClient kubernetes.Interface + mgr manager.Manager + logger logrus.FieldLogger + Clock clock.WithTickerAndDelayedExecution + restoreExposer exposer.GenericRestoreExposer + nodeName string + dataPathMgr *datapath.Manager + vgdpCounter *exposer.VgdpCounter + loadAffinity []*kube.LoadAffinity + restorePVCConfig velerotypes.RestorePVC + backupRepoConfigs map[string]string + cacheVolumeConfigs *velerotypes.CachePVC + podResources corev1api.ResourceRequirements + preparingTimeout time.Duration + metrics *metrics.ServerMetrics + cancelledDataDownload sync.Map + dataMovePriorityClass string + repoConfigMgr repository.ConfigManager + podLabels map[string]string + podAnnotations map[string]string + snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService } func NewDataDownloadReconciler( @@ -101,28 +102,30 @@ func NewDataDownloadReconciler( repoConfigMgr repository.ConfigManager, podLabels map[string]string, podAnnotations map[string]string, + snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService, ) *DataDownloadReconciler { return &DataDownloadReconciler{ - client: client, - kubeClient: kubeClient, - mgr: mgr, - logger: logger.WithField("controller", "DataDownload"), - Clock: &clock.RealClock{}, - nodeName: nodeName, - restoreExposer: exposer.NewGenericRestoreExposer(kubeClient, logger), - restorePVCConfig: restorePVCConfig, - backupRepoConfigs: backupRepoConfigs, - cacheVolumeConfigs: cacheVolumeConfigs, - dataPathMgr: dataPathMgr, - vgdpCounter: counter, - loadAffinity: loadAffinity, - podResources: podResources, - preparingTimeout: preparingTimeout, - metrics: metrics, - dataMovePriorityClass: dataMovePriorityClass, - repoConfigMgr: repoConfigMgr, - podLabels: podLabels, - podAnnotations: podAnnotations, + client: client, + kubeClient: kubeClient, + mgr: mgr, + logger: logger.WithField("controller", "DataDownload"), + Clock: &clock.RealClock{}, + nodeName: nodeName, + restoreExposer: exposer.NewGenericRestoreExposer(kubeClient, client, logger), + restorePVCConfig: restorePVCConfig, + backupRepoConfigs: backupRepoConfigs, + cacheVolumeConfigs: cacheVolumeConfigs, + dataPathMgr: dataPathMgr, + vgdpCounter: counter, + loadAffinity: loadAffinity, + podResources: podResources, + preparingTimeout: preparingTimeout, + metrics: metrics, + dataMovePriorityClass: dataMovePriorityClass, + repoConfigMgr: repoConfigMgr, + podLabels: podLabels, + podAnnotations: podAnnotations, + snapshotMetadataServiceConfigs: snapshotMetadataServiceConfigs, } } @@ -488,7 +491,9 @@ func (r *DataDownloadReconciler) OnDataDownloadCompleted(ctx context.Context, na } log.Info("Cleaning up exposed environment") - r.restoreExposer.CleanUp(ctx, objRef) + r.restoreExposer.CleanUp(ctx, objRef, &exposer.GenericRestoreCleanUpParam{ + Snapshot: dd.Spec.CSISnapshot, + }) if err := UpdateDataDownloadWithRetry(ctx, r.client, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, log, func(dd *velerov2alpha1api.DataDownload) bool { if isDataDownloadInFinalState(dd) { @@ -537,7 +542,9 @@ func (r *DataDownloadReconciler) OnDataDownloadCancelled(ctx context.Context, na return } // cleans up any objects generated during the snapshot expose - r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(&dd)) + r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(&dd), &exposer.GenericRestoreCleanUpParam{ + Snapshot: dd.Spec.CSISnapshot, + }) if err := UpdateDataDownloadWithRetry(ctx, r.client, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, log, func(dd *velerov2alpha1api.DataDownload) bool { if isDataDownloadInFinalState(dd) { @@ -587,7 +594,9 @@ func (r *DataDownloadReconciler) tryCancelDataDownload(ctx context.Context, dd * // success update r.metrics.RegisterDataDownloadCancel(r.nodeName) - r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd)) + r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd), &exposer.GenericRestoreCleanUpParam{ + Snapshot: dd.Spec.CSISnapshot, + }) log.Warn("data download is canceled") @@ -735,7 +744,9 @@ func (r *DataDownloadReconciler) prepareDataDownload(ssb *velerov2alpha1api.Data func (r *DataDownloadReconciler) errorOut(ctx context.Context, dd *velerov2alpha1api.DataDownload, err error, msg string, log logrus.FieldLogger) (ctrl.Result, error) { if r.restoreExposer != nil { - r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd)) + r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd), &exposer.GenericRestoreCleanUpParam{ + Snapshot: dd.Spec.CSISnapshot, + }) } return ctrl.Result{}, r.updateStatusToFailed(ctx, dd, err, msg, log) } @@ -825,7 +836,9 @@ func (r *DataDownloadReconciler) onPrepareTimeout(ctx context.Context, dd *veler log.Warnf("[Diagnose DD expose]%s", diag) } - r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd)) + r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd), &exposer.GenericRestoreCleanUpParam{ + Snapshot: dd.Spec.CSISnapshot, + }) log.Info("Datadownload has been cleaned up") @@ -937,6 +950,7 @@ func (r *DataDownloadReconciler) setupExposeParam(dd *velerov2alpha1api.DataDown return exposer.GenericRestoreExposeParam{ TargetPVCName: dd.Spec.TargetVolume.PVC, + TargetPVName: dd.Spec.TargetVolume.PV, TargetNamespace: dd.Spec.TargetVolume.Namespace, HostingPodLabels: hostingPodLabels, HostingPodAnnotations: hostingPodAnnotation, @@ -951,6 +965,10 @@ func (r *DataDownloadReconciler) setupExposeParam(dd *velerov2alpha1api.DataDown RestoreSize: dd.Spec.SnapshotSize, CacheVolume: cacheVolume, DataMover: dd.Spec.DataMover, + CSI: &exposer.GenericRestoreExposeCSI{ + Snapshot: dd.Spec.CSISnapshot, + SnapshotMetadataServiceConfigs: r.snapshotMetadataServiceConfigs, + }, }, nil } diff --git a/pkg/controller/data_download_controller_test.go b/pkg/controller/data_download_controller_test.go index 4ef79b823..72d51167b 100644 --- a/pkg/controller/data_download_controller_test.go +++ b/pkg/controller/data_download_controller_test.go @@ -64,7 +64,6 @@ func dataDownloadBuilder() *builder.DataDownloadBuilder { BackupStorageLocation("bsl-loc"). DataMover("velero"). SnapshotID("test-snapshot-id").TargetVolume(velerov2alpha1api.TargetVolumeSpec{ - PV: "test-pv", PVC: "test-pvc", Namespace: "test-ns", }) @@ -151,6 +150,7 @@ func initDataDownloadReconcilerWithError(t *testing.T, objects []any, needError nil, nil, // podLabels nil, // podAnnotations + nil, // snapshotMetadataServiceConfigs ), nil } @@ -186,6 +186,7 @@ func TestDataDownloadReconcile(t *testing.T) { dd *velerov2alpha1api.DataDownload notCreateDD bool targetPVC *corev1api.PersistentVolumeClaim + targetPV *corev1api.PersistentVolume dataMgr *datapath.Manager needErrs []bool needCreateFSBR bool @@ -197,6 +198,7 @@ func TestDataDownloadReconcile(t *testing.T) { isPeekExposeErr bool isNilExposer bool notNilExpose bool + mockExpose bool notMockCleanUp bool mockInit bool mockInitErr error @@ -354,6 +356,16 @@ func TestDataDownloadReconcile(t *testing.T) { targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").StorageClass("sc").Result(), expected: dataDownloadBuilder().Finalizers([]string{DataUploadDownloadFinalizer}).Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), }, + { + name: "dd succeeds for accepted with target PV set", + dd: dataDownloadBuilder().Finalizers([]string{DataUploadDownloadFinalizer}).TargetVolume(velerov2alpha1api.TargetVolumeSpec{PVC: "test-pvc", Namespace: "test-ns", PV: "test-pv"}).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").StorageClass("sc").Result(), + targetPV: builder.ForPersistentVolume("test-pv").Result(), + expected: dataDownloadBuilder().Finalizers([]string{DataUploadDownloadFinalizer}).TargetVolume(velerov2alpha1api.TargetVolumeSpec{PVC: "test-pvc", Namespace: "test-ns", PV: "test-pv"}).Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), + mockExpose: true, + notMockCleanUp: true, + notNilExpose: true, + }, { name: "prepare timeout on accepted", dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Finalizers([]string{DataUploadDownloadFinalizer}).AcceptedTimestamp(&metav1.Time{Time: time.Now().Add(-time.Minute * 30)}).Result(), @@ -490,6 +502,10 @@ func TestDataDownloadReconcile(t *testing.T) { objects = append(objects, test.targetPVC) } + if test.targetPV != nil { + objects = append(objects, test.targetPV) + } + r, err := initDataDownloadReconciler(t, objects, test.needErrs...) require.NoError(t, err) @@ -546,7 +562,7 @@ func TestDataDownloadReconcile(t *testing.T) { return asyncBR } - if test.isExposeErr || test.isGetExposeErr || test.isGetExposeNil || test.isPeekExposeErr || test.isNilExposer || test.notNilExpose { + if test.isExposeErr || test.isGetExposeErr || test.isGetExposeNil || test.isPeekExposeErr || test.isNilExposer || test.notNilExpose || test.mockExpose { if test.isNilExposer { r.restoreExposer = nil } else { @@ -554,6 +570,8 @@ func TestDataDownloadReconcile(t *testing.T) { ep := exposermockes.NewGenericRestoreExposer(t) if test.isExposeErr { ep.On("Expose", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("Error to expose restore exposer")) + } else if test.mockExpose { + ep.On("Expose", mock.Anything, mock.Anything, mock.Anything).Return(nil) } else if test.notNilExpose { hostingPod := builder.ForPod("test-ns", "test-name").Volumes(&corev1api.Volume{Name: "test-pvc"}).Result() hostingPod.ObjectMeta.SetUID("test-uid") @@ -568,7 +586,7 @@ func TestDataDownloadReconcile(t *testing.T) { } if !test.notMockCleanUp { - ep.On("CleanUp", mock.Anything, mock.Anything).Return() + ep.On("CleanUp", mock.Anything, mock.Anything, mock.Anything).Return() } return ep }() @@ -727,7 +745,7 @@ func TestOnDataDownloadCompleted(t *testing.T) { } else { ep.On("RebindVolume", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) } - ep.On("CleanUp", mock.Anything, mock.Anything).Return() + ep.On("CleanUp", mock.Anything, mock.Anything, mock.Anything).Return() return ep }() @@ -1105,7 +1123,8 @@ func (dt *ddResumeTestHelper) RebindVolume(context.Context, corev1api.ObjectRefe return nil } -func (dt *ddResumeTestHelper) CleanUp(context.Context, corev1api.ObjectReference) {} +func (dt *ddResumeTestHelper) CleanUp(context.Context, corev1api.ObjectReference, *exposer.GenericRestoreCleanUpParam) { +} func (dt *ddResumeTestHelper) newMicroServiceBRWatcher(kbclient.Client, kubernetes.Interface, manager.Manager, string, string, string, string, string, string, datapath.Callbacks, logrus.FieldLogger) datapath.AsyncBR { @@ -1328,6 +1347,7 @@ func TestDataDownloadSetupExposeParam(t *testing.T) { baseDataDownload := dataDownloadBuilder().Result() baseDataDownload.Namespace = velerov1api.DefaultNamespace + baseDataDownload.Spec.TargetVolume.PV = "pv-1" baseDataDownload.Spec.OperationTimeout = metav1.Duration{Duration: time.Minute * 10} baseDataDownload.Spec.SnapshotSize = 5368709120 // 5Gi @@ -1427,6 +1447,7 @@ func TestDataDownloadSetupExposeParam(t *testing.T) { nil, // repoConfigMgr (unused when cacheVolumeConfigs is nil) tt.args.customLabels, tt.args.customAnnotations, + nil, ) // Act @@ -1437,6 +1458,7 @@ func TestDataDownloadSetupExposeParam(t *testing.T) { // Core fields assert.Equal(t, baseDataDownload.Spec.TargetVolume.PVC, got.TargetPVCName) + assert.Equal(t, baseDataDownload.Spec.TargetVolume.PV, got.TargetPVName) assert.Equal(t, baseDataDownload.Spec.TargetVolume.Namespace, got.TargetNamespace) assert.Equal(t, baseDataDownload.Spec.DataMover, got.DataMover) diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index e4eb68144..69f8636b7 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -365,10 +365,15 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap } // validate ExistingResourcePolicy - if restore.Spec.ExistingResourcePolicy != "" && !pkgrestoreUtil.IsResourcePolicyValid(string(restore.Spec.ExistingResourcePolicy)) { + if !pkgrestoreUtil.IsResourcePolicyValid(string(restore.Spec.ExistingResourcePolicy)) { restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("Invalid ExistingResourcePolicy: %s", restore.Spec.ExistingResourcePolicy)) } + // validate ExistingVolumeDataPolicy + if !pkgrestoreUtil.IsVolumeDataPolicyValid(string(restore.Spec.ExistingVolumeDataPolicy)) { + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("Invalid ExistingVolumeDataPolicy: %s", restore.Spec.ExistingVolumeDataPolicy)) + } + // if ScheduleName is specified, fill in BackupName with the most recent successful backup from // the schedule if restore.Spec.ScheduleName != "" { diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 738ad43db..4fb77c8fd 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -350,6 +350,39 @@ func TestRestoreReconcile(t *testing.T) { expectedCompletedTime: ×tamp, expectedRestorerCall: nil, // this restore should fail validation and not be passed to the restorer }, + { + name: "valid restore with update existingvolumedatapolicy(full) gets executed", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingVolumeDataPolicy("full").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).ExistingVolumeDataPolicy("full").Result(), + }, + { + name: "valid restore with update existingvolumedatapolicy(incremental) gets executed", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingVolumeDataPolicy("incremental").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).ExistingVolumeDataPolicy("incremental").Result(), + }, + { + name: "invalid restore with invalid existingvolumedatapolicy errors", + location: defaultStorageLocation, + restore: NewRestore("foo", "invalidexistingvolumedatapolicy", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingVolumeDataPolicy("invalid").Result(), + backup: defaultBackup().StorageLocation("default").Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseFailedValidation), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: nil, // this restore should fail validation and not be passed to the restorer + }, { name: "valid restore gets executed", location: defaultStorageLocation, diff --git a/pkg/datamover/restore_micro_service.go b/pkg/datamover/restore_micro_service.go index a158a4216..7711fc503 100644 --- a/pkg/datamover/restore_micro_service.go +++ b/pkg/datamover/restore_micro_service.go @@ -32,6 +32,7 @@ import ( "github.com/vmware-tanzu/velero/internal/credentials" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1" + "github.com/vmware-tanzu/velero/pkg/cbtservice" "github.com/vmware-tanzu/velero/pkg/datapath" "github.com/vmware-tanzu/velero/pkg/repository" "github.com/vmware-tanzu/velero/pkg/uploader" @@ -62,11 +63,14 @@ type RestoreMicroService struct { ddHandler cachetool.ResourceEventHandlerRegistration nodeName string cacheDir string + + volumeID string + cbtService cbtservice.Service } func NewRestoreMicroService(ctx context.Context, client client.Client, kubeClient kubernetes.Interface, dataDownloadName string, namespace string, nodeName string, sourceTargetPath datapath.AccessPoint, dataPathMgr *datapath.Manager, repoEnsurer *repository.Ensurer, cred *credentials.CredentialGetter, - ddInformer cache.Informer, cacheDir string, log logrus.FieldLogger) *RestoreMicroService { + ddInformer cache.Informer, cacheDir string, volumeID string, cbtService cbtservice.Service, log logrus.FieldLogger) *RestoreMicroService { return &RestoreMicroService{ ctx: ctx, client: client, @@ -82,6 +86,8 @@ func NewRestoreMicroService(ctx context.Context, client client.Client, kubeClien resultSignal: make(chan dataPathResult), ddInformer: ddInformer, cacheDir: cacheDir, + volumeID: volumeID, + cbtService: cbtService, } } @@ -180,7 +186,16 @@ func (r *RestoreMicroService) RunCancelableDataPath(ctx context.Context) (string } log.Info("Async br init") - if err := dp.StartRestore(dd.Spec.SnapshotID, r.sourceTargetPath, dd.Spec.DataMoverConfig, &datapath.RestoreStartParam{}); err != nil { + param := &datapath.RestoreStartParam{ + Incremental: dd.Spec.RestoreType == string(velerov1api.VolumeDataPolicyTypeIncremental), + CBTService: r.cbtService, + } + if dd.Spec.CSISnapshot != nil { + param.VolumeSnapshotNamespace = dd.Spec.CSISnapshot.VolumeSnapshotNamespace + param.VolumeSnapshotName = dd.Spec.CSISnapshot.VolumeSnapshot + param.VolumeID = r.volumeID + } + if err := dp.StartRestore(dd.Spec.SnapshotID, r.sourceTargetPath, dd.Spec.DataMoverConfig, param); err != nil { return "", errors.Wrap(err, "error starting data path restore") } diff --git a/pkg/datapath/data_path.go b/pkg/datapath/data_path.go index 647095672..1e7ae948e 100644 --- a/pkg/datapath/data_path.go +++ b/pkg/datapath/data_path.go @@ -63,6 +63,11 @@ type BackupStartParam struct { // RestoreStartParam define the input param for restore start type RestoreStartParam struct { + Incremental bool + VolumeSnapshotNamespace string + VolumeSnapshotName string + VolumeID string + CBTService cbtservice.Service } type generalDataPath struct { @@ -235,6 +240,8 @@ func (dp *generalDataPath) StartRestore(snapshotID string, target AccessPoint, u dp.wgDataPath.Add(1) + restoreParam := param.(*RestoreStartParam) + go func() { dp.log.Info("Start data path restore") @@ -243,7 +250,14 @@ func (dp *generalDataPath) StartRestore(snapshotID string, target AccessPoint, u dp.wgDataPath.Done() }() - totalBytes, err := dp.uploaderProv.RunRestore(dp.ctx, snapshotID, target.ByPath, target.VolMode, uploaderConfigs, dp) + totalBytes, err := dp.uploaderProv.RunRestore(dp.ctx, snapshotID, target.ByPath, restoreParam.Incremental, + provider.CBTParam{ + Source: cbtservice.SourceInfo{ + Snapshot: restoreParam.VolumeSnapshotName, + VolumeID: restoreParam.VolumeID, + }, + Service: restoreParam.CBTService, + }, target.VolMode, uploaderConfigs, dp) if err == provider.ErrorCanceled { dp.callbacks.OnCancelled(context.Background(), dp.namespace, dp.jobName) diff --git a/pkg/datapath/data_path_test.go b/pkg/datapath/data_path_test.go index 0c493645d..34f989517 100644 --- a/pkg/datapath/data_path_test.go +++ b/pkg/datapath/data_path_test.go @@ -190,7 +190,7 @@ func TestAsyncRestore(t *testing.T) { t.Run(test.name, func(t *testing.T) { dp := newGeneralDataPath("job-1", "test", nil, "velero", Callbacks{}, velerotest.NewLogger()).(*generalDataPath) mockProvider := providerMock.NewProvider(t) - mockProvider.On("RunRestore", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Restore.TotalBytes, test.err) + mockProvider.On("RunRestore", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Restore.TotalBytes, test.err) mockProvider.On("Close", mock.Anything).Return(nil) dp.uploaderProv = mockProvider dp.initialized = true diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 30e299380..a5639537c 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "maps" - "strings" "time" "github.com/cockroachdb/errors" @@ -116,12 +115,6 @@ type CSISnapshotExposeWaitParam struct { NodeName string } -type cbtInfo struct { - changeID string - volumeID string - snapshotID string -} - // NewCSISnapshotExposer create a new instance of CSI snapshot exposer func NewCSISnapshotExposer(kubeClient kubernetes.Interface, csiSnapshotClient snapshotter.SnapshotV1Interface, log logrus.FieldLogger) SnapshotExposer { return &csiSnapshotExposer{ @@ -299,9 +292,9 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1api.O affinity := kube.GetLoadAffinityByStorageClass(csiExposeParam.Affinity, backupPVCStorageClass, curLog) - var cbtInfo cbtInfo + var cbtInfo csi.CBTInfo if csiExposeParam.DataMover == datamover.DataMoverTypeVeleroBlock { - cbtInfo, err = e.getCBTInfo(ctx, backupVS, backupVSC, csiExposeParam.SourcePVName) + cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, backupVS, backupVSC, csiExposeParam.SourcePVName) if err != nil { return errors.Wrap(err, "error to get CBT info") } @@ -341,49 +334,6 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1api.O return nil } -func (e *csiSnapshotExposer) getCBTInfo(ctx context.Context, vs *snapshotv1api.VolumeSnapshot, vsc *snapshotv1api.VolumeSnapshotContent, sourcePVName string) (cbtInfo, error) { - cbtInfo := cbtInfo{} - if vs == nil || vsc == nil { - return cbtInfo, errors.New("vs or vsc is nil") - } - - cbtInfo.snapshotID = vs.Name - - if vs.Annotations != nil && - (vs.Annotations[util.VSphereCNSChangeIDAnno] != "" || - vs.Annotations[util.VSphereCNSSnapshotAnno] != "") { - cbtInfo.changeID = vs.Annotations[util.VSphereCNSChangeIDAnno] - - splitSnapshotAnno := strings.Split(vs.Annotations[util.VSphereCNSSnapshotAnno], "+") - if len(splitSnapshotAnno) >= 2 { - cbtInfo.volumeID = splitSnapshotAnno[0] - } - - e.log.Debugf("volumeID %s and changeID %s are read from VKS annotations.", cbtInfo.volumeID, cbtInfo.changeID) - } else { - pv, err := e.kubeClient.CoreV1().PersistentVolumes().Get(ctx, sourcePVName, metav1.GetOptions{}) - if err != nil { - return cbtInfo, fmt.Errorf("failed to get pv %s: %w", sourcePVName, err) - } - - if vsc.Status != nil && vsc.Status.SnapshotHandle != nil { - cbtInfo.changeID = *vsc.Status.SnapshotHandle - } - - if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" { - cbtInfo.volumeID = pv.Spec.CSI.VolumeHandle - } - - e.log.Debugf("volumeID %s and changeID %s are read from PV and VS's handles.", cbtInfo.volumeID, cbtInfo.changeID) - } - - if cbtInfo.volumeID == "" { - return cbtInfo, fmt.Errorf("volumeID must not be empty for CBT") - } - - return cbtInfo, nil -} - func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1api.ObjectReference, timeout time.Duration, param any) (*ExposeResult, error) { exposeWaitParam := param.(*CSISnapshotExposeWaitParam) @@ -720,7 +670,7 @@ func (e *csiSnapshotExposer) createBackupPod( intoleratableNodes []string, volumeTopology *corev1api.NodeSelector, csiSnapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService, - cbtInfo *cbtInfo, + cbtInfo *csi.CBTInfo, ) (*corev1api.Pod, error) { podName := ownerObject.Name @@ -776,9 +726,9 @@ func (e *csiSnapshotExposer) createBackupPod( } if cbtInfo != nil { - args = append(args, fmt.Sprintf("--change-id=%s", cbtInfo.changeID)) - args = append(args, fmt.Sprintf("--volume-id=%s", cbtInfo.volumeID)) - args = append(args, fmt.Sprintf("--snapshot-id=%s", cbtInfo.snapshotID)) + args = append(args, fmt.Sprintf("--change-id=%s", cbtInfo.ChangeID)) + args = append(args, fmt.Sprintf("--volume-id=%s", cbtInfo.VolumeID)) + args = append(args, fmt.Sprintf("--snapshot-id=%s", cbtInfo.SnapshotID)) } args = append(args, podInfo.logFormatArgs...) diff --git a/pkg/exposer/csi_snapshot_test.go b/pkg/exposer/csi_snapshot_test.go index 7b5c2eb3e..688c439a9 100644 --- a/pkg/exposer/csi_snapshot_test.go +++ b/pkg/exposer/csi_snapshot_test.go @@ -47,6 +47,7 @@ import ( velerotypes "github.com/vmware-tanzu/velero/pkg/types" "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + "github.com/vmware-tanzu/velero/pkg/util/csi" "github.com/vmware-tanzu/velero/pkg/util/datamover" "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -1326,6 +1327,9 @@ func TestGetExpose(t *testing.T) { Spec: corev1api.PersistentVolumeClaimSpec{ VolumeName: "fake-pv-name", }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, } backupPV := &corev1api.PersistentVolume{ @@ -2213,7 +2217,7 @@ func TestGetCBTInfo(t *testing.T) { vsc *snapshotv1api.VolumeSnapshotContent pv *corev1api.PersistentVolume sourcePVName string - want cbtInfo + want csi.CBTInfo wantErrSubstr string }{ { @@ -2236,10 +2240,10 @@ func TestGetCBTInfo(t *testing.T) { }, vsc: &snapshotv1api.VolumeSnapshotContent{}, sourcePVName: "pv-ignored", - want: cbtInfo{ - changeID: "change-id-1", - volumeID: "volume-id-1", - snapshotID: "vs-anno", + want: csi.CBTInfo{ + ChangeID: "change-id-1", + VolumeID: "volume-id-1", + SnapshotID: "vs-anno", }, }, { @@ -2263,10 +2267,10 @@ func TestGetCBTInfo(t *testing.T) { }, }, sourcePVName: "pv-1", - want: cbtInfo{ - changeID: "snapshot-handle-1", - volumeID: "csi-volume-handle-1", - snapshotID: "vs-fallback", + want: csi.CBTInfo{ + ChangeID: "snapshot-handle-1", + VolumeID: "csi-volume-handle-1", + SnapshotID: "vs-fallback", }, }, { @@ -2329,7 +2333,7 @@ func TestGetCBTInfo(t *testing.T) { log: logrus.StandardLogger(), } - got, err := exposer.getCBTInfo(context.Background(), tc.vs, tc.vsc, tc.sourcePVName) + got, err := csi.GetCBTInfo(context.Background(), exposer.kubeClient, exposer.log, tc.vs, tc.vsc, tc.sourcePVName) if tc.wantErrSubstr != "" { if err == nil { @@ -2344,8 +2348,8 @@ func TestGetCBTInfo(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if got.changeID != tc.want.changeID || got.volumeID != tc.want.volumeID || got.snapshotID != tc.want.snapshotID { - t.Fatalf("unexpected cbtInfo, want %+v, got %+v", tc.want, got) + if got.ChangeID != tc.want.ChangeID || got.VolumeID != tc.want.VolumeID || got.SnapshotID != tc.want.SnapshotID { + t.Fatalf("unexpected CBTInfo, want %+v, got %+v", tc.want, got) } }) } diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index fe8e571d4..b19720389 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/errors" "github.com/google/uuid" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" "github.com/sirupsen/logrus" corev1api "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -31,18 +32,31 @@ import ( "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" + velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1" "github.com/vmware-tanzu/velero/pkg/nodeagent" velerotypes "github.com/vmware-tanzu/velero/pkg/types" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + "github.com/vmware-tanzu/velero/pkg/util/csi" "github.com/vmware-tanzu/velero/pkg/util/datamover" "github.com/vmware-tanzu/velero/pkg/util/kube" ) +// GenericRestoreExposeCSI define the CSI specific input param for Generic Restore Expose +type GenericRestoreExposeCSI struct { + // Snapshot is the CSI snapshot spec + Snapshot *velerov2alpha1api.CSISnapshotSpec + // SnapshotMetadataServiceConfigs is the config for CSI snapshot metadata service + SnapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService +} + // GenericRestoreExposeParam define the input param for Generic Restore Expose type GenericRestoreExposeParam struct { // TargetPVCName is the target volume name to be restored TargetPVCName string + // TargetPVName is the target persistent volume name to be restored + TargetPVName string + // TargetNamespace is the namespace of the volume to be restored TargetNamespace string @@ -84,6 +98,9 @@ type GenericRestoreExposeParam struct { // DataMover is the data mover type, e.g., velero-fs, velero-block DataMover string + + // SnapshotMetadataServiceConfigs is the config for CSI snapshot metadata service + CSI *GenericRestoreExposeCSI } // GenericRestoreRebindVolumeParam define the input param for Generic Restore Rebind Volume @@ -101,6 +118,11 @@ type GenericRestoreRebindVolumeParam struct { TargetFSType string } +// GenericRestoreCleanUpParam define the input param for Generic Restore CleanUp +type GenericRestoreCleanUpParam struct { + Snapshot *velerov2alpha1api.CSISnapshotSpec +} + // GenericRestoreExposer is the interfaces for a generic restore exposer type GenericRestoreExposer interface { // Expose starts the process to a restore expose, the expose process may take long time @@ -124,19 +146,21 @@ type GenericRestoreExposer interface { RebindVolume(context.Context, corev1api.ObjectReference, GenericRestoreRebindVolumeParam) error // CleanUp cleans up any objects generated during the restore expose - CleanUp(context.Context, corev1api.ObjectReference) + CleanUp(context.Context, corev1api.ObjectReference, *GenericRestoreCleanUpParam) } // NewGenericRestoreExposer creates a new instance of generic restore exposer -func NewGenericRestoreExposer(kubeClient kubernetes.Interface, log logrus.FieldLogger) GenericRestoreExposer { +func NewGenericRestoreExposer(kubeClient kubernetes.Interface, ctrlClient client.Client, log logrus.FieldLogger) GenericRestoreExposer { return &genericRestoreExposer{ kubeClient: kubeClient, + ctrlClient: ctrlClient, log: log, } } type genericRestoreExposer struct { kubeClient kubernetes.Interface + ctrlClient client.Client log logrus.FieldLogger } @@ -144,9 +168,11 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap curLog := e.log.WithFields(logrus.Fields{ "owner": ownerObject.Name, "target PVC": param.TargetPVCName, + "target PV": param.TargetPVName, "target namespace": param.TargetNamespace, }) + curLog.Info("Waiting for target PVC to be consumed") selectedNode, targetPVC, err := kube.WaitPVCConsumed( ctx, e.kubeClient.CoreV1(), @@ -226,7 +252,16 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap } }() - restorePVC, err := e.createRestorePVC(ctx, ownerObject, targetPVC, selectedNode, param.DataMover) + curLog.Info("Creating restore PVC") + + var targetPV *corev1api.PersistentVolume + if len(param.TargetPVName) > 0 { + targetPV, err = e.kubeClient.CoreV1().PersistentVolumes().Get(ctx, param.TargetPVName, metav1.GetOptions{}) + if err != nil { + return errors.Wrapf(err, "fail to get the target PV %s", param.TargetPVName) + } + } + restorePVC, err := e.createRestorePVC(ctx, ownerObject, targetPVC, targetPV, selectedNode, param.DataMover, param.ExposeTimeout) if err != nil { return errors.Wrap(err, "error to create restore pvc") } @@ -235,10 +270,44 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap defer func() { if err != nil { - kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, 0, curLog) + if len(param.TargetPVName) == 0 { + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, 0, curLog) + } else { + // cannot delete PV if param.TargetPVName is set because the PV is not created by the Expose process. + // It's the existing PV used for in-place restore. + kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, 0, curLog) + } } }() + curLog.Info("Creating restore pod") + var volumeID string + if param.CSI != nil && param.CSI.Snapshot != nil { + vs := &snapshotv1api.VolumeSnapshot{} + if err := e.ctrlClient.Get(ctx, client.ObjectKey{ + Namespace: param.CSI.Snapshot.VolumeSnapshotNamespace, + Name: param.CSI.Snapshot.VolumeSnapshot, + }, vs); err != nil { + return errors.Wrapf(err, "error to get volume snapshot %s/%s", param.CSI.Snapshot.VolumeSnapshotNamespace, param.CSI.Snapshot.VolumeSnapshot) + } + + vsc, err := csi.GetVSCForVS(ctx, vs, e.ctrlClient) + if err != nil { + return errors.Wrapf(err, "error to get volume snapshot content for volume snapshot %s/%s", vs.Namespace, vs.Name) + } + + var cbtInfo csi.CBTInfo + cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, vs, vsc, param.TargetPVName) + if err != nil { + return errors.Wrap(err, "error to get CBT info") + } + curLog.Debugf("CBT info: %+v", cbtInfo) + volumeID = cbtInfo.VolumeID + } + var csiSnapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService + if param.CSI != nil { + csiSnapshotMetadataServiceConfigs = param.CSI.SnapshotMetadataServiceConfigs + } restorePod, err := e.createRestorePod( ctx, ownerObject, @@ -253,6 +322,9 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap affinity, param.PriorityClassName, cachePVC, + param.TargetNamespace, + volumeID, + csiSnapshotMetadataServiceConfigs, ) if err != nil { return errors.Wrapf(err, "error to create restore pod") @@ -419,7 +491,7 @@ func (e *genericRestoreExposer) DiagnoseExpose(ctx context.Context, ownerObject return diag } -func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1api.ObjectReference) { +func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1api.ObjectReference, param *GenericRestoreCleanUpParam) { restorePodName := ownerObject.Name restorePVCName := ownerObject.Name cachePVCName := getCachePVCName(ownerObject) @@ -432,6 +504,11 @@ func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1a BackupPVCSecretLabel, string(ownerObject.UID), e.log) kube.DeleteConfigMapsWithLabel(ctx, e.kubeClient.CoreV1(), ownerObject.Namespace, BackupPVCSecretLabel, string(ownerObject.UID), e.log) + + if param.Snapshot != nil { + kube.EnsureDeleteVolumeSnapshotIfAny(ctx, e.ctrlClient, param.Snapshot.VolumeSnapshotNamespace, + param.Snapshot.VolumeSnapshot, 0, e.log) + } } func (e *genericRestoreExposer) RebindVolume(ctx context.Context, ownerObject corev1api.ObjectReference, param GenericRestoreRebindVolumeParam) error { @@ -645,6 +722,9 @@ func (e *genericRestoreExposer) createRestorePod( affinity *kube.LoadAffinity, priorityClassName string, cachePVC *corev1api.PersistentVolumeClaim, + volumeSnapshotNamespace string, + volumeID string, + csiSnapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService, ) (*corev1api.Pod, error) { restorePodName := ownerObject.Name restorePVCName := ownerObject.Name @@ -725,6 +805,14 @@ func (e *genericRestoreExposer) createRestorePod( fmt.Sprintf("--cache-volume-path=%s", cacheVolumePath), } + if len(volumeID) > 0 { + args = append(args, fmt.Sprintf("--vs-namespace=%s", volumeSnapshotNamespace)) + args = append(args, fmt.Sprintf("--volume-id=%s", volumeID)) + } + if csiSnapshotMetadataServiceConfigs != nil && csiSnapshotMetadataServiceConfigs.SAName != "" { + args = append(args, fmt.Sprintf("--cbt-sa-name=%s", csiSnapshotMetadataServiceConfigs.SAName)) + } + args = append(args, podInfo.logFormatArgs...) args = append(args, podInfo.logLevelArgs...) @@ -843,7 +931,7 @@ func (e *genericRestoreExposer) createRestorePod( return e.kubeClient.CoreV1().Pods(ownerObject.Namespace).Create(ctx, pod, metav1.CreateOptions{}) } -func (e *genericRestoreExposer) createRestorePVC(ctx context.Context, ownerObject corev1api.ObjectReference, targetPVC *corev1api.PersistentVolumeClaim, selectedNode string, dataMover string) (*corev1api.PersistentVolumeClaim, error) { +func (e *genericRestoreExposer) createRestorePVC(ctx context.Context, ownerObject corev1api.ObjectReference, targetPVC *corev1api.PersistentVolumeClaim, targetPV *corev1api.PersistentVolume, selectedNode string, dataMover string, operationTimeout time.Duration) (*corev1api.PersistentVolumeClaim, error) { restorePVCName := ownerObject.Name pvcObj := &corev1api.PersistentVolumeClaim{ @@ -871,9 +959,10 @@ func (e *genericRestoreExposer) createRestorePVC(ctx context.Context, ownerObjec } if selectedNode != "" { - pvcObj.Annotations = map[string]string{ - kube.KubeAnnSelectedNode: selectedNode, + if pvcObj.Annotations == nil { + pvcObj.Annotations = make(map[string]string) } + pvcObj.Annotations[kube.KubeAnnSelectedNode] = selectedNode } if dataMover == datamover.DataMoverTypeVeleroBlock { @@ -884,5 +973,64 @@ func (e *genericRestoreExposer) createRestorePVC(ctx context.Context, ownerObjec *pvcObj.Spec.VolumeMode = corev1api.PersistentVolumeBlock } - return e.kubeClient.CoreV1().PersistentVolumeClaims(pvcObj.Namespace).Create(ctx, pvcObj, metav1.CreateOptions{}) + volumeName := "" + sameVolumeMode := true + if targetPV != nil { + volumeName = targetPV.Name + sameVolumeMode = kube.GetVolumeModeByPVC(pvcObj) == kube.GetVolumeModeByPV(targetPV) + if !sameVolumeMode { + volumeName = ownerObject.Name + } + pvcObj.Spec.VolumeName = volumeName + } + + restorePVC, err := e.kubeClient.CoreV1().PersistentVolumeClaims(pvcObj.Namespace).Create(ctx, pvcObj, metav1.CreateOptions{}) + if err != nil { + return nil, errors.Wrapf(err, "fail to create the restore PVC %s in namespace %s", pvcObj.Name, pvcObj.Namespace) + } + + defer func() { + if err != nil { + kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), pvcObj.Name, pvcObj.Namespace, 0, e.log) + } + }() + + if targetPV != nil { + if !sameVolumeMode { + tmpPV := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: volumeName, + }, + Spec: *targetPV.Spec.DeepCopy(), + } + tmpPV.Spec.VolumeMode = restorePVC.Spec.VolumeMode + e.log.Infof("the volume mode is different, creating temporary PV %s with volume mode %s", tmpPV.Name, tmpPV.Spec.VolumeMode) + tmpPV, err = e.kubeClient.CoreV1().PersistentVolumes().Create(ctx, tmpPV, metav1.CreateOptions{}) + if err != nil { + return nil, errors.Wrapf(err, "fail to create the temporary PV %s", volumeName) + } + + defer func() { + if err != nil { + kube.DeletePVIfAny(ctx, e.kubeClient.CoreV1(), tmpPV.Name, e.log) + } + }() + + e.log.Infof("deleting the target PV %s", targetPV.Name) + if err = e.kubeClient.CoreV1().PersistentVolumes().Delete(ctx, targetPV.Name, metav1.DeleteOptions{}); err != nil { + return nil, errors.Wrapf(err, "fail to delete the target PV %s", targetPV.Name) + } + targetPV = tmpPV + } + + if _, err = kube.ResetPVBinding(ctx, e.kubeClient.CoreV1(), targetPV, nil, restorePVC); err != nil { + return nil, errors.Wrapf(err, "fail to reset PV %s binding to restore PVC %s/%s", targetPV.Name, restorePVC.Namespace, restorePVC.Name) + } + + if _, err = kube.WaitPVCBound(ctx, e.kubeClient.CoreV1(), e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, operationTimeout); err != nil { + return nil, errors.Wrapf(err, "fail to wait restore PVC %s/%s bound", restorePVC.Namespace, restorePVC.Name) + } + } + + return restorePVC, nil } diff --git a/pkg/exposer/generic_restore_priority_test.go b/pkg/exposer/generic_restore_priority_test.go index 642e0cc43..c8ca784ee 100644 --- a/pkg/exposer/generic_restore_priority_test.go +++ b/pkg/exposer/generic_restore_priority_test.go @@ -149,6 +149,9 @@ func TestCreateRestorePodWithPriorityClass(t *testing.T) { nil, // affinity tc.expectedPriorityClass, nil, + "", // volumeSnapshotNamespace + "", // volumeID + nil, ) require.NoError(t, err, tc.description) @@ -229,6 +232,9 @@ func TestCreateRestorePodWithMissingConfigMap(t *testing.T) { nil, // affinity "", // empty priority class since config map is missing nil, + "", // volumeSnapshotNamespace + "", // volumeID + nil, ) // Should succeed even when config map is missing diff --git a/pkg/exposer/generic_restore_test.go b/pkg/exposer/generic_restore_test.go index 6087d0f71..c08c16b60 100644 --- a/pkg/exposer/generic_restore_test.go +++ b/pkg/exposer/generic_restore_test.go @@ -62,6 +62,21 @@ func TestRestoreExpose(t *testing.T) { StorageClassName: &scName, }, } + targetPVObj := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-target-pv", + }, + } + + modeBlock := corev1api.PersistentVolumeBlock + targetPVObjWithDifferentVolumeMode := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-target-pv", + }, + Spec: corev1api.PersistentVolumeSpec{ + VolumeMode: &modeBlock, + }, + } modeFilesystem := corev1api.PersistentVolumeFilesystem targetPVCObjWithVolumeMode := &corev1api.PersistentVolumeClaim{ @@ -119,12 +134,14 @@ func TestRestoreExpose(t *testing.T) { ownerRestore *velerov1.Restore targetPVCName string targetNamespace string + targetPVName string kubeReactors []reactor cacheVolume *CacheConfigs dataMover string expectBackupPod bool expectBackupPVC bool expectCachePVC bool + expectBackupPV bool err string }{ { @@ -185,7 +202,7 @@ func TestRestoreExpose(t *testing.T) { }, }, }, - err: "error to create restore pvc: fake-create-error", + err: "error to create restore pvc: fail to create the restore PVC fake-restore in namespace velero: fake-create-error", }, { name: "succeed", @@ -200,6 +217,135 @@ func TestRestoreExpose(t *testing.T) { expectBackupPod: true, expectBackupPVC: true, }, + { + name: "succeed with target PV set", + targetPVCName: "fake-target-pvc", + targetNamespace: "fake-ns", + targetPVName: "fake-target-pv", + ownerRestore: restore, + kubeClientObj: []runtime.Object{ + targetPVCObj, + targetPVObj, + daemonSet, + storageClass, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(clientTesting.GetAction) + if getAction.GetName() == "fake-restore" { + return true, &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-restore", + Namespace: velerov1.DefaultNamespace, + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "fake-target-pv", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, nil + } + return false, nil, nil + }, + }, + }, + expectBackupPod: true, + expectBackupPVC: true, + }, + { + name: "create temporary PV fail", + targetPVCName: "fake-target-pvc", + targetNamespace: "fake-ns", + targetPVName: "fake-target-pv", + ownerRestore: restore, + kubeClientObj: []runtime.Object{ + targetPVCObj, + targetPVObjWithDifferentVolumeMode, + daemonSet, + storageClass, + }, + kubeReactors: []reactor{ + { + verb: "create", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-create-pv-error") + }, + }, + }, + err: "error to create restore pvc: fail to create the temporary PV fake-restore: fake-create-pv-error", + }, + { + name: "delete original PV fail", + targetPVCName: "fake-target-pvc", + targetNamespace: "fake-ns", + targetPVName: "fake-target-pv", + ownerRestore: restore, + kubeClientObj: []runtime.Object{ + targetPVCObj, + targetPVObjWithDifferentVolumeMode, + daemonSet, + storageClass, + }, + kubeReactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + deleteAction := action.(clientTesting.DeleteAction) + if deleteAction.GetName() == "fake-target-pv" { + return true, nil, errors.New("fake-delete-pv-error") + } + return false, nil, nil + }, + }, + }, + err: "error to create restore pvc: fail to delete the target PV fake-target-pv: fake-delete-pv-error", + }, + { + name: "succeed with target PV set and different volume mode", + targetPVCName: "fake-target-pvc", + targetNamespace: "fake-ns", + targetPVName: "fake-target-pv", + ownerRestore: restore, + kubeClientObj: []runtime.Object{ + targetPVCObj, + targetPVObjWithDifferentVolumeMode, + daemonSet, + storageClass, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(clientTesting.GetAction) + if getAction.GetName() == "fake-restore" { + return true, &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-restore", + Namespace: velerov1.DefaultNamespace, + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "fake-restore", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, nil + } + return false, nil, nil + }, + }, + }, + expectBackupPod: true, + expectBackupPVC: true, + expectBackupPV: true, + }, { name: "succeed, cache config, no cache volume", targetPVCName: "fake-target-pvc", @@ -311,6 +457,7 @@ func TestRestoreExpose(t *testing.T) { GenericRestoreExposeParam{ TargetPVCName: test.targetPVCName, TargetNamespace: test.targetNamespace, + TargetPVName: test.targetPVName, HostingPodLabels: map[string]string{}, Resources: corev1api.ResourceRequirements{}, ExposeTimeout: time.Millisecond, @@ -330,7 +477,7 @@ func TestRestoreExpose(t *testing.T) { if test.expectBackupPod { require.NoError(t, err) } else { - require.True(t, apierrors.IsNotFound(err)) + require.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got %v", err) } pvc, err := exposer.kubeClient.CoreV1().PersistentVolumeClaims(ownerObject.Namespace).Get(t.Context(), ownerObject.Name, metav1.GetOptions{}) @@ -341,14 +488,31 @@ func TestRestoreExpose(t *testing.T) { require.Equal(t, corev1api.PersistentVolumeBlock, *pvc.Spec.VolumeMode) } } else { - require.True(t, apierrors.IsNotFound(err)) + require.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got %v", err) } _, err = exposer.kubeClient.CoreV1().PersistentVolumeClaims(ownerObject.Namespace).Get(t.Context(), getCachePVCName(ownerObject), metav1.GetOptions{}) if test.expectCachePVC { require.NoError(t, err) } else { - require.True(t, apierrors.IsNotFound(err)) + require.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got %v", err) + } + + _, err = exposer.kubeClient.CoreV1().PersistentVolumes().Get(t.Context(), ownerObject.Name, metav1.GetOptions{}) + if test.expectBackupPV { + require.NoError(t, err) + } else { + require.True(t, apierrors.IsNotFound(err), "expected IsNotFound, got %v", err) + } + + if test.targetPVName != "" && !test.expectBackupPV && test.err == "" { + // if targetPVName was provided, and sameVolumeMode was true, the original PV should still exist + _, err = exposer.kubeClient.CoreV1().PersistentVolumes().Get(t.Context(), test.targetPVName, metav1.GetOptions{}) + require.NoError(t, err) + } else if test.targetPVName != "" && test.expectBackupPV { + // if targetPVName was provided, and sameVolumeMode was false (expectBackupPV is true), the original PV should be deleted + _, err = exposer.kubeClient.CoreV1().PersistentVolumes().Get(t.Context(), test.targetPVName, metav1.GetOptions{}) + require.True(t, apierrors.IsNotFound(err), "expected original PV %s to be deleted, but it still exists", test.targetPVName) } }) } @@ -480,6 +644,9 @@ func TestRebindVolume(t *testing.T) { Spec: corev1api.PersistentVolumeClaimSpec{ VolumeName: "fake-restore-pv", }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, } restorePVObj := &corev1api.PersistentVolume{ @@ -1508,6 +1675,9 @@ func TestCreateRestorePod(t *testing.T) { test.affinity, "", // priority class name nil, + "", // volumeSnapshotNamespace + "", // volumeID + nil, ) require.NoError(t, err) diff --git a/pkg/exposer/mocks/GenericRestoreExposer.go b/pkg/exposer/mocks/GenericRestoreExposer.go index a1d8943d4..30639b6a8 100644 --- a/pkg/exposer/mocks/GenericRestoreExposer.go +++ b/pkg/exposer/mocks/GenericRestoreExposer.go @@ -42,8 +42,8 @@ func (_m *GenericRestoreExposer) EXPECT() *GenericRestoreExposer_Expecter { } // CleanUp provides a mock function for the type GenericRestoreExposer -func (_mock *GenericRestoreExposer) CleanUp(context1 context.Context, objectReference v1.ObjectReference) { - _mock.Called(context1, objectReference) +func (_mock *GenericRestoreExposer) CleanUp(context1 context.Context, objectReference v1.ObjectReference, param *exposer.GenericRestoreCleanUpParam) { + _mock.Called(context1, objectReference, param) return } @@ -55,11 +55,12 @@ type GenericRestoreExposer_CleanUp_Call struct { // CleanUp is a helper method to define mock.On call // - context1 context.Context // - objectReference v1.ObjectReference -func (_e *GenericRestoreExposer_Expecter) CleanUp(context1 interface{}, objectReference interface{}) *GenericRestoreExposer_CleanUp_Call { - return &GenericRestoreExposer_CleanUp_Call{Call: _e.mock.On("CleanUp", context1, objectReference)} +// - param *exposer.GenericRestoreCleanUpParam +func (_e *GenericRestoreExposer_Expecter) CleanUp(context1 interface{}, objectReference interface{}, param interface{}) *GenericRestoreExposer_CleanUp_Call { + return &GenericRestoreExposer_CleanUp_Call{Call: _e.mock.On("CleanUp", context1, objectReference, param)} } -func (_c *GenericRestoreExposer_CleanUp_Call) Run(run func(context1 context.Context, objectReference v1.ObjectReference)) *GenericRestoreExposer_CleanUp_Call { +func (_c *GenericRestoreExposer_CleanUp_Call) Run(run func(context1 context.Context, objectReference v1.ObjectReference, param *exposer.GenericRestoreCleanUpParam)) *GenericRestoreExposer_CleanUp_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -69,9 +70,14 @@ func (_c *GenericRestoreExposer_CleanUp_Call) Run(run func(context1 context.Cont if args[1] != nil { arg1 = args[1].(v1.ObjectReference) } + var arg2 *exposer.GenericRestoreCleanUpParam + if args[2] != nil { + arg2 = args[2].(*exposer.GenericRestoreCleanUpParam) + } run( arg0, arg1, + arg2, ) }) return _c @@ -82,7 +88,7 @@ func (_c *GenericRestoreExposer_CleanUp_Call) Return() *GenericRestoreExposer_Cl return _c } -func (_c *GenericRestoreExposer_CleanUp_Call) RunAndReturn(run func(context1 context.Context, objectReference v1.ObjectReference)) *GenericRestoreExposer_CleanUp_Call { +func (_c *GenericRestoreExposer_CleanUp_Call) RunAndReturn(run func(context1 context.Context, objectReference v1.ObjectReference, param *exposer.GenericRestoreCleanUpParam)) *GenericRestoreExposer_CleanUp_Call { _c.Run(run) return _c } diff --git a/pkg/podvolume/restore_micro_service.go b/pkg/podvolume/restore_micro_service.go index b9dbd8d64..2f778a3f9 100644 --- a/pkg/podvolume/restore_micro_service.go +++ b/pkg/podvolume/restore_micro_service.go @@ -184,7 +184,9 @@ func (r *RestoreMicroService) RunCancelableDataPath(ctx context.Context) (string log.Info("Async fs br init") - if err := fsRestore.StartRestore(pvr.Spec.SnapshotID, r.sourceTargetPath, pvr.Spec.UploaderSettings, &datapath.RestoreStartParam{}); err != nil { + if err := fsRestore.StartRestore(pvr.Spec.SnapshotID, r.sourceTargetPath, pvr.Spec.UploaderSettings, &datapath.RestoreStartParam{ + Incremental: pvr.Spec.RestoreType == string(velerov1api.VolumeDataPolicyTypeIncremental), + }); err != nil { return "", errors.Wrap(err, "error starting data path restore") } diff --git a/pkg/podvolume/restorer.go b/pkg/podvolume/restorer.go index 2cc72fe5e..53d35215c 100644 --- a/pkg/podvolume/restorer.go +++ b/pkg/podvolume/restorer.go @@ -297,6 +297,10 @@ func newPodVolumeRestore(restore *velerov1api.Restore, pod *corev1api.Pod, backu pvr.Spec.UploaderSettings = uploaderutil.StoreRestoreConfig(restore.Spec.UploaderConfig) } + if restore.IsVolumeDataInplaceRestore() { + pvr.Spec.RestoreType = string(restore.Spec.ExistingVolumeDataPolicy) + } + return pvr } diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index 6026f5378..a14b985a7 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -20,17 +20,20 @@ import ( "context" "encoding/json" "fmt" - - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "time" "github.com/cockroachdb/errors" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + snapshotter "github.com/kubernetes-csi/external-snapshotter/client/v8/clientset/versioned/typed/volumesnapshot/v1" "github.com/sirupsen/logrus" corev1api "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" utilrand "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/client-go/kubernetes" crclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -44,6 +47,9 @@ import ( uploaderUtil "github.com/vmware-tanzu/velero/pkg/uploader/util" "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + "github.com/vmware-tanzu/velero/pkg/util/csi" + "github.com/vmware-tanzu/velero/pkg/util/datamover" + "github.com/vmware-tanzu/velero/pkg/util/kube" ) const ( @@ -53,12 +59,14 @@ const ( // pvcRestoreItemAction is a restore item action plugin for Velero type pvcRestoreItemAction struct { - log logrus.FieldLogger - crClient crclient.Client + log logrus.FieldLogger + crClient crclient.Client + kubeClient kubernetes.Interface + csiSnapshotClient snapshotter.SnapshotV1Interface } // AppliesTo returns information indicating that the -// PVCRestoreItemAction should be run while restoring PVCs. +// PVCCSIRestoreItemAction should be run while restoring PVCs. func (p *pvcRestoreItemAction) AppliesTo() (velero.ResourceSelector, error) { return velero.ResourceSelector{ IncludedResources: []string{"persistentvolumeclaims"}, @@ -83,28 +91,178 @@ func (p *pvcRestoreItemAction) Execute( } logger := p.log.WithFields(logrus.Fields{ - "Action": "PVCRestoreItemAction", + "Action": "PVCCSIRestoreItemAction", "PVC": pvc.Namespace + "/" + pvc.Name, "Restore": input.Restore.Namespace + "/" + input.Restore.Name, }) - logger.Info("Starting PVCRestoreItemAction for PVC") + logger.Info("Starting PVCCSIRestoreItemAction for PVC") + // make sure this RIA only runs for CSI snapshot vsName, nameOK := pvcFromBackup.Annotations[velerov1api.VolumeSnapshotLabel] if !nameOK { - logger.Info("Skipping PVCRestoreItemAction for PVC, PVC does not have a CSI VolumeSnapshot.") + logger.Info("Skipping PVCCSIRestoreItemAction for PVC, PVC does not have a CSI VolumeSnapshot.") return &velero.RestoreItemActionExecuteOutput{ UpdatedItem: input.Item, }, nil } - // If PVC already exists, returns early. - if p.isResourceExist(pvc, *input.Restore) { + pvcExists, existingPVC, err := p.isResourceExist(&pvc, *input.Restore) + if err != nil { + logger.Error(err) + return nil, errors.WithStack(err) + } + + var output *velero.RestoreItemActionExecuteOutput + if boolptr.IsSetToFalse(input.Restore.Spec.RestorePVs) { + output, err = p.executeWithoutPVRestore(logger, input, pvcExists, &pvc) + } else { + backup := new(velerov1api.Backup) + if err := p.crClient.Get(context.TODO(), crclient.ObjectKey{Namespace: input.Restore.Namespace, Name: input.Restore.Spec.BackupName}, backup); err != nil { + return nil, fmt.Errorf("fail to get backup for restore: %s", err.Error()) + } + if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { + output, err = p.executeWithDataMove(logger, input, backup, pvcExists, existingPVC, &pvc, &pvcFromBackup) + } else { + output, err = p.executeWithoutDataMove(logger, input, pvcExists, &pvc, vsName) + } + } + if err != nil { + logger.Error(err) + return nil, errors.WithStack(err) + } + + logger.Info("Returning from PVCCSIRestoreItemAction for PVC") + + return output, nil +} + +func (p *pvcRestoreItemAction) executeWithoutPVRestore(logger *logrus.Entry, input *velero.RestoreItemActionExecuteInput, pvcExists bool, pvc *corev1api.PersistentVolumeClaim) (*velero.RestoreItemActionExecuteOutput, error) { + if pvcExists { logger.Warnf("PVC already exists. Skip restore this PVC.") return &velero.RestoreItemActionExecuteOutput{ UpdatedItem: input.Item, }, nil } + logger.Info("Restore did not request for PVs to be restored from snapshot") + pvc.Spec.VolumeName = "" + pvc.Spec.DataSource = nil + pvc.Spec.DataSourceRef = nil + + unstructuredPVC, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvc) + if err != nil { + return nil, errors.WithStack(err) + } + + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: &unstructured.Unstructured{Object: unstructuredPVC}, + }, nil +} + +func (p *pvcRestoreItemAction) executeWithoutDataMove(logger *logrus.Entry, input *velero.RestoreItemActionExecuteInput, pvcExists bool, pvc *corev1api.PersistentVolumeClaim, vsName string) (*velero.RestoreItemActionExecuteOutput, error) { + if pvcExists { + logger.Warnf("PVC already exists. Skip restore this PVC.") + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: input.Item, + }, nil + } + + //To avoid confilcs, vs and vsc get a new uniq name based in restore UID + // and vs name old name + newVSName := util.GenerateSha256FromRestoreUIDAndVsName(string(input.Restore.UID), vsName) + + logger.Debugf("Setting PVC source to VolumeSnapshot new name: %s", newVSName) + resetPVCSourceToVolumeSnapshot(pvc, newVSName) + + // Force-restore the VolumeSnapshot even when restore resource filters + // would otherwise exclude it (mirrors backup-side must-include). + annotations := pvc.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.MustIncludeAdditionalItemRestoreAnnotation] = "true" + pvc.SetAnnotations(annotations) + + unstructuredPVC, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvc) + if err != nil { + return nil, errors.WithStack(err) + } + + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: &unstructured.Unstructured{Object: unstructuredPVC}, + AdditionalItems: []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.VolumeSnapshots, + Name: vsName, + Namespace: pvc.Namespace, + }, + }, + }, nil +} + +func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input *velero.RestoreItemActionExecuteInput, backup *velerov1api.Backup, pvcExists bool, existingPVC, pvc, pvcFromBackup *corev1api.PersistentVolumeClaim) (out *velero.RestoreItemActionExecuteOutput, err error) { + ctx := context.Background() + var existingPV *corev1api.PersistentVolume + + // If PVC already exists and is not in-place restore, returns early. + if pvcExists && !input.Restore.IsVolumeDataInplaceRestore() { + logger.Warnf("PVC already exists and ExistingVolumeDataPolicy is not in-place restore. Skip restore this PVC.") + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: input.Item, + }, nil + } + + logger.Info("Start DataMover restore.") + + // If PVC doesn't have a DataUploadNameLabel, which should be created + // during backup, then CSI cannot handle the volume during to restore, + // so return early to let Velero tries to fall back to Velero native snapshot. + if _, ok := pvcFromBackup.Annotations[velerov1api.DataUploadNameAnnotation]; !ok { + logger.Warnf("PVC doesn't have a DataUpload for data mover. Return.") + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: input.Item, + }, nil + } + + var dataUploadResult *velerov2alpha1.DataUploadResult + dataUploadResult, err = getDataUploadResult(ctx, input.Restore, pvc, p.crClient) + if err != nil { + return nil, errors.Wrapf(err, "fail get DataUploadResult for restore: %s", input.Restore.Name) + } + + var volumeSnapshot *snapshotv1api.VolumeSnapshot + restoreType := input.Restore.Spec.ExistingVolumeDataPolicy + if pvcExists { + if existingPVC.Status.Phase != corev1api.ClaimBound { + return nil, errors.New("ExistingVolumeDataPolicy is in-place restore, but the existing PVC is not bound.") + } + // take a CSI snapshot of the existing PVC as the baseline of CBT + if input.Restore.IsVolumeDataInplaceIncrementalRestore() && datamover.IsVeleroBlockDataMover(dataUploadResult.DataMover) { + logger.Info("ExistingVolumeDataPolicy is in-place incremental restore and data mover is velero-block. Taking a CSI snapshot of the existing PVC as the baseline of CBT...") + volumeSnapshot, err = p.createVolumeSnapshot(ctx, logger, input.Restore, *existingPVC, dataUploadResult.SnapshotClass, backup.Spec.CSISnapshotTimeout.Duration) + if err != nil { + logger.Warnf("fail to create VolumeSnapshot for existing PVC %s/%s: %s, fallback to in-place full restore", existingPVC.Namespace, existingPVC.Name, err.Error()) + restoreType = velerov1api.VolumeDataPolicyTypeFull + } else { + defer func() { + if err != nil { + csi.CleanupVolumeSnapshot(ctx, volumeSnapshot, p.crClient, logger) + } + }() + } + } + + // delete the existing PVC, otherwise the target PVC cannot be restored + existingPV, err = p.deleteExistingPVC(ctx, logger, pvc, existingPVC, backup.Spec.CSISnapshotTimeout.Duration) + if err != nil { + return nil, errors.WithStack(err) + } + } + + operationID := label.GetValidName( + string(velerov1api.AsyncOperationIDPrefixDataDownload) + + string(input.Restore.UID) + "." + string(pvcFromBackup.UID)) + // If cross-namespace restore is configured, change the namespace // for PVC object to be restored newNamespace, ok := input.Restore.Spec.NamespaceMapping[pvc.GetNamespace()] @@ -113,90 +271,26 @@ func (p *pvcRestoreItemAction) Execute( newNamespace = pvc.Namespace } - operationID := "" - - additionalItems := []velero.ResourceIdentifier{} - if boolptr.IsSetToFalse(input.Restore.Spec.RestorePVs) { - logger.Info("Restore did not request for PVs to be restored from snapshot") - pvc.Spec.VolumeName = "" - pvc.Spec.DataSource = nil - pvc.Spec.DataSourceRef = nil - } else { - backup := new(velerov1api.Backup) - err := p.crClient.Get( - context.TODO(), - crclient.ObjectKey{ - Namespace: input.Restore.Namespace, - Name: input.Restore.Spec.BackupName, - }, - backup, - ) - - if err != nil { - logger.Error("Fail to get backup for restore.") - return nil, fmt.Errorf("fail to get backup for restore: %s", err.Error()) - } - - if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { - logger.Info("Start DataMover restore.") - - // If PVC doesn't have a DataUploadNameLabel, which should be created - // during backup, then CSI cannot handle the volume during to restore, - // so return early to let Velero tries to fall back to Velero native snapshot. - if _, ok := pvcFromBackup.Annotations[velerov1api.DataUploadNameAnnotation]; !ok { - logger.Warnf("PVC doesn't have a DataUpload for data mover. Return.") - return &velero.RestoreItemActionExecuteOutput{ - UpdatedItem: input.Item, - }, nil - } - - operationID = label.GetValidName( - string(velerov1api.AsyncOperationIDPrefixDataDownload) + - string(input.Restore.UID) + "." + string(pvcFromBackup.UID)) - dataDownload, err := restoreFromDataUploadResult( - context.Background(), input.Restore, backup, &pvc, newNamespace, - operationID, p.crClient) - if err != nil { - logger.Errorf("Fail to restore from DataUploadResult: %s", err.Error()) - return nil, errors.WithStack(err) - } - logger.Infof("DataDownload %s/%s is created successfully.", - dataDownload.Namespace, dataDownload.Name) - } else { - //To avoid confilcs, vs and vsc get a new uniq name based in restore UID - // and vs name old name - newVSName := util.GenerateSha256FromRestoreUIDAndVsName(string(input.Restore.UID), vsName) - - p.log.Debugf("Setting PVC source to VolumeSnapshot new name: %s", newVSName) - resetPVCSourceToVolumeSnapshot(&pvc, newVSName) - - additionalItems = append(additionalItems, velero.ResourceIdentifier{ - GroupResource: kuberesource.VolumeSnapshots, - Name: vsName, - Namespace: pvc.Namespace, - }) - - // Force-restore the VolumeSnapshot even when restore resource filters - // would otherwise exclude it (mirrors backup-side must-include). - annotations := pvc.GetAnnotations() - if annotations == nil { - annotations = map[string]string{} - } - annotations[velerov1api.MustIncludeAdditionalItemRestoreAnnotation] = "true" - pvc.SetAnnotations(annotations) - } + var dataDownload *velerov2alpha1.DataDownload + dataDownload, err = restoreFromDataUploadResult( + context.Background(), dataUploadResult, input.Restore, backup, pvc, existingPV, newNamespace, + operationID, string(restoreType), volumeSnapshot, p.crClient) + if err != nil { + logger.Errorf("Fail to restore from DataUploadResult: %s", err.Error()) + return nil, errors.WithStack(err) } + logger.Infof("DataDownload %s/%s is created successfully.", + dataDownload.Namespace, dataDownload.Name) - pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc) + var unstructuredPVC map[string]any + unstructuredPVC, err = runtime.DefaultUnstructuredConverter.ToUnstructured(pvc) if err != nil { return nil, errors.WithStack(err) } - logger.Info("Returning from PVCRestoreItemAction for PVC") return &velero.RestoreItemActionExecuteOutput{ - UpdatedItem: &unstructured.Unstructured{Object: pvcMap}, - OperationID: operationID, - AdditionalItems: additionalItems, + UpdatedItem: &unstructured.Unstructured{Object: unstructuredPVC}, + OperationID: operationID, }, nil } @@ -406,8 +500,14 @@ func newDataDownload( backup *velerov1api.Backup, dataUploadResult *velerov2alpha1.DataUploadResult, pvc *corev1api.PersistentVolumeClaim, - newNamespace, operationID string, + pv *corev1api.PersistentVolume, + newNamespace, operationID, restoreType string, + volumeSnapshot *snapshotv1api.VolumeSnapshot, ) *velerov2alpha1.DataDownload { + pvName := "" + if pv != nil { + pvName = pv.Name + } dataDownload := &velerov2alpha1.DataDownload{ TypeMeta: metav1.TypeMeta{ APIVersion: velerov2alpha1.SchemeGroupVersion.String(), @@ -434,6 +534,7 @@ func newDataDownload( Spec: velerov2alpha1.DataDownloadSpec{ TargetVolume: velerov2alpha1.TargetVolumeSpec{ PVC: pvc.Name, + PV: pvName, Namespace: newNamespace, FSType: dataUploadResult.FSType, }, @@ -444,8 +545,15 @@ func newDataDownload( SourceNamespace: dataUploadResult.SourceNamespace, OperationTimeout: backup.Spec.CSISnapshotTimeout, NodeOS: dataUploadResult.NodeOS, + RestoreType: restoreType, }, } + if volumeSnapshot != nil { + dataDownload.Spec.CSISnapshot = &velerov2alpha1.CSISnapshotSpec{ + VolumeSnapshot: volumeSnapshot.Name, + VolumeSnapshotNamespace: volumeSnapshot.Namespace, + } + } if restore.Spec.UploaderConfig != nil { dataDownload.Spec.DataMoverConfig = uploaderUtil.StoreRestoreConfig(restore.Spec.UploaderConfig) } @@ -454,17 +562,15 @@ func newDataDownload( func restoreFromDataUploadResult( ctx context.Context, + dataUploadResult *velerov2alpha1.DataUploadResult, restore *velerov1api.Restore, backup *velerov1api.Backup, pvc *corev1api.PersistentVolumeClaim, - newNamespace, operationID string, + pv *corev1api.PersistentVolume, + newNamespace, operationID, restoreType string, + volumeSnapshot *snapshotv1api.VolumeSnapshot, crClient crclient.Client, ) (*velerov2alpha1.DataDownload, error) { - dataUploadResult, err := getDataUploadResult(ctx, restore, pvc, crClient) - if err != nil { - return nil, errors.Wrapf(err, "fail get DataUploadResult for restore: %s", - restore.Name) - } pvc.Spec.VolumeName = "" if pvc.Spec.Selector == nil { pvc.Spec.Selector = &metav1.LabelSelector{} @@ -481,10 +587,13 @@ func restoreFromDataUploadResult( backup, dataUploadResult, pvc, + pv, newNamespace, operationID, + restoreType, + volumeSnapshot, ) - err = crClient.Create(ctx, dataDownload) + err := crClient.Create(ctx, dataDownload) if err != nil { return nil, errors.Wrapf(err, "fail to create DataDownload") } @@ -493,9 +602,9 @@ func restoreFromDataUploadResult( } func (p *pvcRestoreItemAction) isResourceExist( - pvc corev1api.PersistentVolumeClaim, + pvc *corev1api.PersistentVolumeClaim, restore velerov1api.Restore, -) bool { +) (bool, *corev1api.PersistentVolumeClaim, error) { // get target namespace to restore into, if different from source namespace targetNamespace := pvc.Namespace if target, ok := restore.Spec.NamespaceMapping[pvc.Namespace]; ok { @@ -503,17 +612,115 @@ func (p *pvcRestoreItemAction) isResourceExist( } tmpPVC := new(corev1api.PersistentVolumeClaim) - if err := p.crClient.Get( + err := p.crClient.Get( context.Background(), crclient.ObjectKey{ Name: pvc.Name, Namespace: targetNamespace, }, tmpPVC, - ); err == nil { - return true + ) + if err == nil { + return true, tmpPVC, nil } - return false + if apierrors.IsNotFound(err) { + return false, nil, nil + } + return false, nil, errors.Wrapf(err, "fail to get PVC %s in namespace %s", pvc.Name, targetNamespace) +} + +func (p *pvcRestoreItemAction) deleteExistingPVC(ctx context.Context, logger *logrus.Entry, targetPVC *corev1api.PersistentVolumeClaim, existingPVC *corev1api.PersistentVolumeClaim, operationTimeout time.Duration) (*corev1api.PersistentVolume, error) { + // Capture the "selected-node" annotation from the existing PVC before it is deleted below, + // and carry it on the target PVC via a Velero-internal carrier annotation. The restore + // engine translates the carrier back to the Kubernetes "selected-node" annotation after + // all RestoreItemActions have run, so the recreated target PVC keeps the same scheduling + // constraint regardless of the order in which RestoreItemActions execute (the generic PVC + // RIA unconditionally strips the Kubernetes annotation). + selectedNode, exists := existingPVC.Annotations[kube.KubeAnnSelectedNode] + if exists { + logger.Infof("Carrying %q annotation with value %q for target PVC to keep the same selected node as the existing PVC", kube.KubeAnnSelectedNode, selectedNode) + if targetPVC.Annotations == nil { + targetPVC.Annotations = map[string]string{} + } + targetPVC.Annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation] = selectedNode + } + + var err error + logger.Info("ExistingVolumeDataPolicy is in-place restore. Deleting the existing PVC but keep the PV...") + pv := &corev1api.PersistentVolume{} + if err = p.crClient.Get(context.Background(), crclient.ObjectKey{Name: existingPVC.Spec.VolumeName}, pv); err != nil { + return nil, errors.Errorf("Fail to get PV %s: %s", existingPVC.Spec.VolumeName, err.Error()) + } + + // set reclaim policy to retain + updatedPV, err := kube.SetPVReclaimPolicy(ctx, p.kubeClient.CoreV1(), pv, corev1api.PersistentVolumeReclaimRetain) + if err != nil { + return nil, errors.Wrapf(err, "fail to set PV reclaim policy to retain for PV %s", pv.Name) + } + if updatedPV != nil { + pv = updatedPV + } + + if err = kube.EnsureDeletePVC(ctx, p.kubeClient.CoreV1(), existingPVC.Name, existingPVC.Namespace, operationTimeout); err != nil { + return nil, errors.Wrapf(err, "fail to delete the existing PVC %s in namespace %s", existingPVC.Name, existingPVC.Namespace) + } + + logger.Info("Existing PVC deleted") + + return pv, nil +} + +func (p *pvcRestoreItemAction) createVolumeSnapshot(ctx context.Context, logger *logrus.Entry, restore *velerov1api.Restore, pvc corev1api.PersistentVolumeClaim, vsClass string, operationTimeout time.Duration) (vs *snapshotv1api.VolumeSnapshot, err error) { + logger.Infof("creating VolumeSnapshot for PVC %s/%s with VolumeSnapshotClass %s", pvc.Namespace, pvc.Name, vsClass) + + labels := map[string]string{ + velerov1api.RestoreNameLabel: label.GetValidName(restore.Name), + } + for k, v := range pvc.ObjectMeta.Labels { + labels[k] = v + } + + vs = &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "velero-" + pvc.Name + "-", + Namespace: pvc.Namespace, + Labels: labels, + }, + Spec: snapshotv1api.VolumeSnapshotSpec{ + Source: snapshotv1api.VolumeSnapshotSource{ + PersistentVolumeClaimName: &pvc.Name, + }, + VolumeSnapshotClassName: &vsClass, + }, + } + + if err := p.crClient.Create(ctx, vs); err != nil { + return nil, errors.Wrapf(err, "failed to create the VolumeSnapshot for PVC %s/%s", pvc.Namespace, pvc.Name) + } + + logger.Infof("VolumeSnapshot %s for PVC %s/%s created", vs.Name, pvc.Namespace, pvc.Name) + vsName := vs.Name + vsNamespace := vs.Namespace + + _, err = csi.WaitUntilVSCHandleIsReady(vs, p.crClient, logger, operationTimeout) + if err != nil { + csi.CleanupVolumeSnapshot(ctx, vs, p.crClient, logger) + return nil, errors.Wrapf(err, "failed to wait for VolumeSnapshotContent of VolumeSnapshot %s/%s to be ready within timeout %v", + vsNamespace, vsName, operationTimeout) + } + + var updatedVS *snapshotv1api.VolumeSnapshot + updatedVS, err = csi.WaitVolumeSnapshotReady(ctx, p.csiSnapshotClient, vs.Name, vs.Namespace, operationTimeout, logger) + if err != nil { + csi.CleanupVolumeSnapshot(ctx, vs, p.crClient, logger) + return nil, errors.Wrapf(err, "failed to wait for VolumeSnapshot %s/%s to become Ready within timeout %v", + vsNamespace, vsName, operationTimeout) + } + vs = updatedVS + + logger.Infof("VolumeSnapshot %s for PVC %s/%s is ready to use", vs.Name, pvc.Namespace, pvc.Name) + + return vs, nil } func NewPvcRestoreItemAction(f client.Factory) plugincommon.HandlerInitializer { @@ -523,9 +730,25 @@ func NewPvcRestoreItemAction(f client.Factory) plugincommon.HandlerInitializer { return nil, err } + kubeClient, err := f.KubeClient() + if err != nil { + return nil, err + } + + clientConfig, err := f.ClientConfig() + if err != nil { + return nil, err + } + csiSnapshotClient, err := snapshotter.NewForConfig(clientConfig) + if err != nil { + return nil, err + } + return &pvcRestoreItemAction{ - log: logger, - crClient: crClient, + log: logger, + crClient: crClient, + kubeClient: kubeClient, + csiSnapshotClient: csiSnapshotClient, }, nil } } diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index 0e10144f6..47e8937a1 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -28,12 +28,15 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/rest" crclient "sigs.k8s.io/controller-runtime/pkg/client" "github.com/vmware-tanzu/velero/pkg/apis/velero/shared" @@ -371,6 +374,7 @@ func TestExecute(t *testing.T) { backup *velerov1api.Backup restore *velerov1api.Restore pvc *corev1api.PersistentVolumeClaim + pv *corev1api.PersistentVolume pvcFromBackup *corev1api.PersistentVolumeClaim vs *snapshotv1api.VolumeSnapshot dataUploadResult *corev1api.ConfigMap @@ -378,9 +382,11 @@ func TestExecute(t *testing.T) { expectedDataDownload *velerov2alpha1.DataDownload expectedPVC *corev1api.PersistentVolumeClaim preCreatePVC bool + kubeClientObj []runtime.Object }{ { name: "Don't restore PV", + backup: builder.ForBackup("velero", "testBackup").Result(), restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").RestorePVs(false).Result(), pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName")).Result(), expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName")).VolumeName("").Result(), @@ -486,6 +492,47 @@ func TestExecute(t *testing.T) { pvc: builder.ForPersistentVolumeClaim("restore", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), preCreatePVC: true, }, + { + name: "PVC exists and in-place restore set", + backup: builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result(), + restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").ExistingVolumeDataPolicy(string(velerov1api.VolumeDataPolicyTypeFull)).ItemOperationTimeout(time.Minute * 10).ObjectMeta(builder.WithUID("uid")).Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").Phase(corev1api.ClaimBound).ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), + pv: builder.ForPersistentVolume("testPV").ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain).Result(), + dataUploadResult: builder.ForConfigMap("velero", "testCM").Data("uid", "{}").ObjectMeta(builder.WithLabels(velerov1api.RestoreUIDLabel, "uid", velerov1api.PVCNamespaceNameLabel, "velero.testPVC", velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)))).Result(), + preCreatePVC: true, + kubeClientObj: []runtime.Object{ + builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").Phase(corev1api.ClaimBound).ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), + }, + expectedDataDownload: func() *velerov2alpha1.DataDownload { + d := builder.ForDataDownload("velero", "name").TargetVolume(velerov2alpha1.TargetVolumeSpec{PVC: "testPVC", Namespace: "velero", PV: "testPV"}). + ObjectMeta(builder.WithOwnerReference([]metav1.OwnerReference{{APIVersion: velerov1api.SchemeGroupVersion.String(), Kind: "Restore", Name: "testRestore", UID: "uid", Controller: boolptr.True()}}), + builder.WithLabelsMap(map[string]string{velerov1api.AsyncOperationIDLabel: "dd-uid.", velerov1api.RestoreNameLabel: "testRestore", velerov1api.RestoreUIDLabel: "uid"}), + builder.WithGenerateName("testRestore-")).Result() + d.Spec.RestoreType = "full" + return d + }(), + }, + { + name: "PVC exists and in-place incremental restore set, createVolumeSnapshot fails", + backup: builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result(), + restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").ExistingVolumeDataPolicy(string(velerov1api.VolumeDataPolicyTypeIncremental)).ItemOperationTimeout(time.Minute * 10).ObjectMeta(builder.WithUID("uid")).Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").Phase(corev1api.ClaimBound).ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), + pv: builder.ForPersistentVolume("testPV").ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain).Result(), + dataUploadResult: builder.ForConfigMap("velero", "testCM").Data("uid", "{\"DataMover\":\"velero-block\", \"SnapshotClass\":\"test-snapclass\"}").ObjectMeta(builder.WithLabels(velerov1api.RestoreUIDLabel, "uid", velerov1api.PVCNamespaceNameLabel, "velero.testPVC", velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)))).Result(), + preCreatePVC: true, + kubeClientObj: []runtime.Object{ + builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").Phase(corev1api.ClaimBound).ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), + }, + expectedDataDownload: func() *velerov2alpha1.DataDownload { + d := builder.ForDataDownload("velero", "name").TargetVolume(velerov2alpha1.TargetVolumeSpec{PVC: "testPVC", Namespace: "velero", PV: "testPV"}). + ObjectMeta(builder.WithOwnerReference([]metav1.OwnerReference{{APIVersion: velerov1api.SchemeGroupVersion.String(), Kind: "Restore", Name: "testRestore", UID: "uid", Controller: boolptr.True()}}), + builder.WithLabelsMap(map[string]string{velerov1api.AsyncOperationIDLabel: "dd-uid.", velerov1api.RestoreNameLabel: "testRestore", velerov1api.RestoreUIDLabel: "uid"}), + builder.WithGenerateName("testRestore-")).Result() + d.Spec.RestoreType = "full" + d.Spec.DataMover = "velero-block" + return d + }(), + }, } for _, tc := range tests { @@ -499,6 +546,10 @@ func TestExecute(t *testing.T) { object = append(object, tc.vs) } + if tc.pv != nil { + object = append(object, tc.pv) + } + input := new(velero.RestoreItemActionExecuteInput) if tc.pvc != nil { @@ -524,8 +575,9 @@ func TestExecute(t *testing.T) { } pvcRIA := pvcRestoreItemAction{ - log: logrus.New(), - crClient: velerotest.NewFakeControllerRuntimeClient(t, object...), + log: logrus.New(), + crClient: velerotest.NewFakeControllerRuntimeClient(t, object...), + kubeClient: fake.NewSimpleClientset(tc.kubeClientObj...), } output, err := pvcRIA.Execute(input) @@ -567,6 +619,128 @@ func TestExecute(t *testing.T) { } } +// TestPrepareForInplaceRestoreSelectedNode verifies that prepareForInplaceRestore captures +// the selected-node annotation from the existing PVC into the Velero-internal carrier +// annotation (not the Kubernetes annotation) on the target PVC, before deleting the PVC. +func TestPrepareForInplaceRestoreSelectedNode(t *testing.T) { + tests := []struct { + name string + existingPVC *corev1api.PersistentVolumeClaim + expectedCarrier string + expectCarrierSet bool + expectKubeAnnoSet bool + }{ + { + name: "existing PVC with selected-node sets carrier annotation only", + existingPVC: builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + ObjectMeta(builder.WithAnnotations(AnnSelectedNode, "node-1")). + VolumeName("pv-1"). + Phase(corev1api.ClaimBound).Result(), + expectedCarrier: "node-1", + expectCarrierSet: true, + expectKubeAnnoSet: false, + }, + { + name: "existing PVC without selected-node sets neither annotation", + existingPVC: builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + VolumeName("pv-1"). + Phase(corev1api.ClaimBound).Result(), + expectCarrierSet: false, + expectKubeAnnoSet: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + pv := builder.ForPersistentVolume("pv-1").Result() + kubeClient := fake.NewSimpleClientset(tc.existingPVC, pv) + pvcRIA := pvcRestoreItemAction{ + log: logrus.New(), + crClient: velerotest.NewFakeControllerRuntimeClient(t, pv), + kubeClient: kubeClient, + } + + targetPVC := builder.ForPersistentVolumeClaim("ns-1", "pvc-1").Result() + returnedPV, err := pvcRIA.deleteExistingPVC( + t.Context(), logrus.New().WithField("test", tc.name), + targetPVC, tc.existingPVC, time.Minute) + require.NoError(t, err) + require.Equal(t, "pv-1", returnedPV.Name) + + carrier, carrierOK := targetPVC.Annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation] + require.Equal(t, tc.expectCarrierSet, carrierOK) + if tc.expectCarrierSet { + require.Equal(t, tc.expectedCarrier, carrier) + } + _, kubeAnnoOK := targetPVC.Annotations[AnnSelectedNode] + require.Equal(t, tc.expectKubeAnnoSet, kubeAnnoOK) + }) + } +} + +// TestExecuteInplaceRestore exercises the public Execute() entry for an in-place restore +// with an existing PVC: the carrier annotation must be emitted on the returned item, the +// Kubernetes selected-node annotation must not be set by this RIA, the existing PVC must be +// deleted, and a DataDownload with the in-place restoreType must be created. +func TestExecuteInplaceRestore(t *testing.T) { + existingPVC := builder.ForPersistentVolumeClaim("velero", "testPVC"). + ObjectMeta(builder.WithAnnotations(AnnSelectedNode, "node-1")). + VolumeName("testPV"). + Phase(corev1api.ClaimBound).Result() + existingPV := builder.ForPersistentVolume("testPV").Result() + backup := builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result() + restore := builder.ForRestore("velero", "testRestore").Backup("testBackup"). + ObjectMeta(builder.WithUID("uid")).ExistingVolumeDataPolicy("full").Result() + pvcFromBackup := builder.ForPersistentVolumeClaim("velero", "testPVC"). + ObjectMeta(builder.WithAnnotations( + velerov1api.VolumeSnapshotLabel, "vsName", + velerov1api.DataUploadNameAnnotation, "velero/testDU", + )).Result() + dataUploadResult := builder.ForConfigMap("velero", "testCM").Data("uid", "{}"). + ObjectMeta(builder.WithLabels( + velerov1api.RestoreUIDLabel, "uid", + velerov1api.PVCNamespaceNameLabel, "velero.testPVC", + velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)), + )).Result() + + pvcRIA := pvcRestoreItemAction{ + log: logrus.New(), + crClient: velerotest.NewFakeControllerRuntimeClient(t, existingPVC, existingPV, backup, dataUploadResult), + kubeClient: fake.NewSimpleClientset(existingPVC, existingPV), + } + + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup.DeepCopy()) + require.NoError(t, err) + pvcFromBackupMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup) + require.NoError(t, err) + + output, err := pvcRIA.Execute(&velero.RestoreItemActionExecuteInput{ + Item: &unstructured.Unstructured{Object: pvcMap}, + ItemFromBackup: &unstructured.Unstructured{Object: pvcFromBackupMap}, + Restore: restore, + }) + require.NoError(t, err) + + updatedPVC := new(corev1api.PersistentVolumeClaim) + require.NoError(t, runtime.DefaultUnstructuredConverter.FromUnstructured( + output.UpdatedItem.UnstructuredContent(), updatedPVC)) + + // Carrier annotation carries the captured value; the Kubernetes annotation is not set by this RIA. + require.Equal(t, "node-1", updatedPVC.Annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]) + require.NotContains(t, updatedPVC.Annotations, AnnSelectedNode) + + // The existing PVC is deleted so the exposer can bind a temporary PVC to the PV. + _, err = pvcRIA.kubeClient.CoreV1().PersistentVolumeClaims("velero").Get(t.Context(), "testPVC", metav1.GetOptions{}) + require.True(t, apierrors.IsNotFound(err)) + + // A DataDownload with the in-place restoreType referencing the existing PV is created. + dataDownloadList := new(velerov2alpha1.DataDownloadList) + require.NoError(t, pvcRIA.crClient.List(t.Context(), dataDownloadList, &crclient.ListOptions{})) + require.Len(t, dataDownloadList.Items, 1) + require.Equal(t, "full", dataDownloadList.Items[0].Spec.RestoreType) + require.Equal(t, "testPV", dataDownloadList.Items[0].Spec.TargetVolume.PV) +} + func TestPVCAppliesTo(t *testing.T) { p := pvcRestoreItemAction{ log: logrus.StandardLogger(), @@ -596,6 +770,8 @@ func TestNewPvcRestoreItemAction(t *testing.T) { f1 := &factorymocks.Factory{} f1.On("KubebuilderClient").Return(crClient, nil) + f1.On("KubeClient").Return(nil, nil) + f1.On("ClientConfig").Return(&rest.Config{}, nil) plugin1 := NewPvcRestoreItemAction(f1) _, err1 := plugin1(logger) require.NoError(t, err1) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index a4c29d067..aec181a97 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1636,6 +1636,19 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists } + // Strip any pre-existing Velero-internal in-place restore carrier annotation coming from + // the backup metadata before RestoreItemActions run. The carrier is only trusted when it + // is set by a RestoreItemAction (the PVC CSI RIA) during this restore; a stale carrier + // baked into the backup must not be translated into the Kubernetes "selected-node" + // annotation, which could pin a newly provisioned PVC to a stale node. + if annotations := obj.GetAnnotations(); annotations != nil { + if _, present := annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]; present { + restoreLogger.Infof("Removing pre-existing %q annotation from backup metadata", velerov1api.InplaceRestoreSelectedNodeAnnotation) + delete(annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + obj.SetAnnotations(annotations) + } + } + restoreLogger.Infof("restore status includes excludes: %+v", ctx.resourceStatusIncludesExcludes) for _, action := range ctx.getApplicableActions(groupResource, namespace) { @@ -1768,6 +1781,23 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } } + // Translate the Velero-internal carrier annotation (set by the PVC CSI RestoreItemAction + // during an in-place volume data restore) back to the Kubernetes "selected-node" annotation. + // This runs after all RestoreItemActions so the result does not depend on the order in which + // the actions executed: the generic PVC RIA unconditionally strips the Kubernetes annotation, + // while the carrier annotation passes through untouched. The carrier itself is always + // stripped so it never lands on the cluster. + if annotations := obj.GetAnnotations(); annotations != nil { + if selectedNode, present := annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]; present { + if selectedNode != "" { + restoreLogger.Infof("Restoring %q annotation with value %q from in-place restore carrier annotation", kube.KubeAnnSelectedNode, selectedNode) + annotations[kube.KubeAnnSelectedNode] = selectedNode + } + delete(annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + obj.SetAnnotations(annotations) + } + } + // This comes after running item actions because we have built-in actions that restore // a PVC's associated PV (if applicable). As part of the PV being restored, the 'pvsToProvision' // set may be inserted into, and this needs to happen *before* running the following block of logic. @@ -1943,7 +1973,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso if err != nil { warnings.Add(namespace, err) // check if there is existingResourcePolicy and if it is set to update policy - if len(ctx.restore.Spec.ExistingResourcePolicy) > 0 && ctx.restore.Spec.ExistingResourcePolicy == velerov1api.PolicyTypeUpdate { + if len(ctx.restore.Spec.ExistingResourcePolicy) > 0 && ctx.restore.Spec.ExistingResourcePolicy == velerov1api.ResourcePolicyTypeUpdate { // remove restore labels so that we apply the latest backup/restore names on the object via patch removeRestoreLabels(fromCluster) //try patching just the backup/restore labels @@ -1963,14 +1993,14 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso restoreLogger.Infof("restore API has resource policy defined %s, executing restore workflow accordingly for changed resource %s %s", resourcePolicy, fromCluster.GroupVersionKind().Kind, kube.NamespaceAndName(fromCluster)) // existingResourcePolicy is set as none, add warning - if resourcePolicy == velerov1api.PolicyTypeNone { + if resourcePolicy == velerov1api.ResourcePolicyTypeNone { e := errors.Errorf("could not restore, %s %q already exists. Warning: the in-cluster version is different than the backed-up version", obj.GetKind(), obj.GetName()) warnings.Add(namespace, e) itemStatus.action = ItemRestoreResultSkipped ctx.restoredItems[itemKey] = itemStatus // existingResourcePolicy is set as update, attempt patch on the resource and add warning if it fails - } else if resourcePolicy == velerov1api.PolicyTypeUpdate { + } else if resourcePolicy == velerov1api.ResourcePolicyTypeUpdate { // processing update as existingResourcePolicy warningsFromUpdateRP, errsFromUpdateRP := ctx.processUpdateResourcePolicy(fromCluster, fromClusterWithLabels, obj, namespace, resourceClient) if warningsFromUpdateRP.IsEmpty() && errsFromUpdateRP.IsEmpty() { @@ -1993,7 +2023,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } //update backup/restore labels on the unchanged resources if existingResourcePolicy is set as update - if ctx.restore.Spec.ExistingResourcePolicy == velerov1api.PolicyTypeUpdate { + if ctx.restore.Spec.ExistingResourcePolicy == velerov1api.ResourcePolicyTypeUpdate { resourcePolicy := ctx.restore.Spec.ExistingResourcePolicy restoreLogger.Infof("restore API has resource policy defined %s, executing restore workflow accordingly for unchanged resource %s %s ", resourcePolicy, obj.GroupVersionKind().Kind, kube.NamespaceAndName(fromCluster)) // remove restore labels so that we apply the latest backup/restore names on the object via patch diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index fdb6f20c4..5c75fff42 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -44,6 +44,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/dynamic" + k8sfake "k8s.io/client-go/kubernetes/fake" kubetesting "k8s.io/client-go/testing" "github.com/vmware-tanzu/velero/internal/volume" @@ -60,6 +61,7 @@ import ( vsv1 "github.com/vmware-tanzu/velero/pkg/plugin/velero/volumesnapshotter/v1" "github.com/vmware-tanzu/velero/pkg/podvolume" uploadermocks "github.com/vmware-tanzu/velero/pkg/podvolume/mocks" + riav1 "github.com/vmware-tanzu/velero/pkg/restore/actions" "github.com/vmware-tanzu/velero/pkg/test" "github.com/vmware-tanzu/velero/pkg/types" "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -2852,6 +2854,185 @@ func TestRestoreMustIncludeAdditionalItems(t *testing.T) { }) } +// TestRestoreInplaceSelectedNodeCarrierAnnotation verifies the engine translates the +// Velero-internal in-place restore carrier annotation into the Kubernetes selected-node +// annotation after all RestoreItemActions have run, and always strips the carrier. +func TestRestoreInplaceSelectedNodeCarrierAnnotation(t *testing.T) { + t.Run("carrier annotation is translated to selected-node and stripped", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + + data := &Request{ + Log: h.log, + Restore: defaultRestore().Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1").Result()). + Done(), + } + warnings, errs := h.restorer.Restore( + data, + []riav2.RestoreItemAction{ + // Simulates the PVC CSI RIA setting the carrier during an in-place restore. + &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + annotations := item.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation] = "node-1" + item.SetAnnotations(annotations) + return &velero.RestoreItemActionExecuteOutput{UpdatedItem: item}, nil + }, + }, + // The real generic PVC RIA (velero.io/pvc), which unconditionally strips the + // Kubernetes selected-node annotation. Running it after the carrier-setting + // action proves the carrier survives the real strip regardless of action order. + &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + clientset := k8sfake.NewSimpleClientset() + return riav1.NewPVCAction( + h.log, + clientset.CoreV1().ConfigMaps("velero"), + clientset.CoreV1().Nodes(), + ).Execute(input) + }, + }, + }, + nil, + ) + + assertEmptyResults(t, warnings, errs) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + annotations := got.GetAnnotations() + assert.Equal(t, "node-1", annotations["volume.kubernetes.io/selected-node"]) + assert.NotContains(t, annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + }) + + t.Run("empty carrier annotation is stripped without setting selected-node", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + + data := &Request{ + Log: h.log, + Restore: defaultRestore().Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1").Result()). + Done(), + } + warnings, errs := h.restorer.Restore( + data, + []riav2.RestoreItemAction{ + &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + annotations := item.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation] = "" + item.SetAnnotations(annotations) + return &velero.RestoreItemActionExecuteOutput{UpdatedItem: item}, nil + }, + }, + }, + nil, + ) + + assertEmptyResults(t, warnings, errs) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + annotations := got.GetAnnotations() + assert.NotContains(t, annotations, "volume.kubernetes.io/selected-node") + assert.NotContains(t, annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + }) + + t.Run("no carrier annotation leaves selected-node stripped (PVC-absent fallback)", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + + data := &Request{ + Log: h.log, + Restore: defaultRestore().Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + ObjectMeta(builder.WithAnnotations("volume.kubernetes.io/selected-node", "stale-node")).Result()). + Done(), + } + warnings, errs := h.restorer.Restore( + data, + []riav2.RestoreItemAction{ + // Simulates the generic PVC RIA stripping the annotation; no action sets the + // carrier (as when the target PVC does not exist and Velero falls back to + // provisioning a new PVC). + &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + annotations := item.GetAnnotations() + delete(annotations, "volume.kubernetes.io/selected-node") + item.SetAnnotations(annotations) + return &velero.RestoreItemActionExecuteOutput{UpdatedItem: item}, nil + }, + }, + }, + nil, + ) + + assertEmptyResults(t, warnings, errs) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + assert.NotContains(t, got.GetAnnotations(), "volume.kubernetes.io/selected-node") + }) + + t.Run("carrier annotation baked into backup metadata is not trusted when no action sets it", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + + data := &Request{ + Log: h.log, + Restore: defaultRestore().Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + ObjectMeta(builder.WithAnnotations(velerov1api.InplaceRestoreSelectedNodeAnnotation, "stale-node")).Result()). + Done(), + } + warnings, errs := h.restorer.Restore( + data, + // No action sets the carrier during this restore (as in the PVC-absent fallback + // path where a new PVC is dynamically provisioned), so the carrier from the + // backup metadata must be stripped and never translated into selected-node. + []riav2.RestoreItemAction{ + &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + // The stale carrier from the backup must already be gone before + // RestoreItemActions execute. + assert.NotContains(t, item.GetAnnotations(), velerov1api.InplaceRestoreSelectedNodeAnnotation) + return &velero.RestoreItemActionExecuteOutput{UpdatedItem: item}, nil + }, + }, + }, + nil, + ) + + assertEmptyResults(t, warnings, errs) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + annotations := got.GetAnnotations() + assert.NotContains(t, annotations, "volume.kubernetes.io/selected-node") + assert.NotContains(t, annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + }) +} + // TestShouldRestore runs the ShouldRestore function for various permutations of // existing/nonexisting/being-deleted PVs, PVCs, and namespaces, and verifies the // result/error matches expectations. diff --git a/pkg/uploader/block/snapshot.go b/pkg/uploader/block/snapshot.go index 1ecfedbbe..ff844d197 100644 --- a/pkg/uploader/block/snapshot.go +++ b/pkg/uploader/block/snapshot.go @@ -205,18 +205,44 @@ func getParentBackupInfo(ctx context.Context, rep udmrepo.BackupRepo, forceFull } // Restore restore specific sourcePath with given snapshotID and update progress -func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapshotID, dest string, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, error) { +func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapshotID, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, error) { log.Info("Start to restore...") snapshot, err := rep.GetSnapshot(ctx, udmrepo.ID(snapshotID)) if err != nil { return 0, errors.Wrapf(err, "Unable to load snapshot %v", snapshotID) } + log.Infof("Restore from snapshot %s, incremental %v, cbt source %v, description %s, created time %v, tags %v", snapshotID, incremental, cbtSource, snapshot.Description, snapshot.EndTime, snapshot.Tags) - log.Infof("Restore from snapshot %s, description %s, created time %v, tags %v", snapshotID, snapshot.Description, snapshot.EndTime, snapshot.Tags) + var volumeSnapshot, changeID, volumeID string + if incremental { + if snapshot.Tags == nil { + log.Warnf("No tag from snapshot %s, fallback to full restore", snapshotID) + incremental = false + } else if snapshot.Tags[uploader.CBTChangeIDTag] == "" { + log.Warnf("No ChangeID tag from snapshot %s, fallback to full restore", snapshotID) + incremental = false + } else if snapshot.Tags[uploader.CBTVolumeIDTag] == "" { + log.Warnf("No VolumeID tag from snapshot %s, fallback to full restore", snapshotID) + incremental = false + } else if snapshot.Tags[uploader.CBTVolumeIDTag] != cbtSource.VolumeID { + log.Warnf("VolumeID %s from snapshot %s is not expected as %s, fallback to full restore", snapshot.Tags[uploader.CBTVolumeIDTag], snapshotID, cbtSource.VolumeID) + incremental = false + } else { + volumeSnapshot = cbtSource.Snapshot + changeID = snapshot.Tags[uploader.CBTChangeIDTag] + volumeID = snapshot.Tags[uploader.CBTVolumeIDTag] + } + } - bitmap := cbt.NewBitmap(blockSize, uint64(snapshot.TotalSize), "", "", "") - bitmap.SetFull() + bitmap := cbt.NewBitmap(blockSize, uint64(snapshot.TotalSize), volumeSnapshot, changeID, volumeID) + if incremental { + if err = cbt.SetBitmapOrFull(ctx, cbtService, bitmap); err != nil { + log.WithError(err).Warnf("Failed to create CBT with source %v, fallback to full restore", cbtSource) + } + } else { + bitmap.SetFull() + } destPath, err := filepath.Abs(dest) if err != nil { diff --git a/pkg/uploader/block/snapshot_test.go b/pkg/uploader/block/snapshot_test.go index 3cebd10bc..d7e7d2ee2 100644 --- a/pkg/uploader/block/snapshot_test.go +++ b/pkg/uploader/block/snapshot_test.go @@ -33,6 +33,7 @@ import ( "github.com/stretchr/testify/require" "github.com/vmware-tanzu/velero/pkg/cbtservice" + cbtservicemocks "github.com/vmware-tanzu/velero/pkg/cbtservice/mocks" "github.com/vmware-tanzu/velero/pkg/repository/udmrepo" udmrepomocks "github.com/vmware-tanzu/velero/pkg/repository/udmrepo/mocks" "github.com/vmware-tanzu/velero/pkg/uploader" @@ -123,6 +124,23 @@ func TestBackup(t *testing.T) { assert.Positive(t, info.Size) }, }, + { + name: "success with CBT", + setupOpenDev: func(t *testing.T) *os.File { + t.Helper() + return tempFile(t, "test-block-data") + }, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + blkup.On("Backup", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(udmrepo.Snapshot{RootObject: udmrepo.ObjectMetadata{ID: "root"}}, int64(8), nil) + repo.On("SaveSnapshot", mock.Anything, mock.Anything).Return(udmrepo.ID("snap-001"), nil) + repo.On("Flush", mock.Anything).Return(nil) + }, + checkInfo: func(t *testing.T, info uploader.SnapshotInfo) { + t.Helper() + assert.Equal(t, "snap-001", info.ID) + }, + }, } for _, tc := range testCases { @@ -186,6 +204,7 @@ func TestSnapshotSource(t *testing.T) { expectedErrStr string expectedSnapID string expectedSize int64 + cbtService func(t *testing.T) cbtservice.Service }{ { name: "uploader Backup error", @@ -218,7 +237,10 @@ func TestSnapshotSource(t *testing.T) { { name: "success with nil cbtService falls back to full bitmap", setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { - blkup.On("Backup", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + blkup.On("Backup", mock.Anything, mock.Anything, mock.MatchedBy(func(iter cbttypes.Iterator) bool { + // In full mode, the iterator should cover the whole range if it's a full backup + return iter != nil + }), mock.Anything). Return(udmrepo.Snapshot{RootObject: udmrepo.ObjectMetadata{ID: "root"}}, int64(512), nil) repo.On("SaveSnapshot", mock.Anything, mock.Anything).Return(udmrepo.ID("snap-success"), nil) repo.On("Flush", mock.Anything).Return(nil) @@ -241,6 +263,46 @@ func TestSnapshotSource(t *testing.T) { }, expectedSnapID: "snap-tags", }, + { + name: "success with cbtService getting allocated blocks", + cbtService: func(t *testing.T) cbtservice.Service { + t.Helper() + m := cbtservicemocks.NewService(t) + m.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything). + Run(func(args mock.Arguments) { + record := args.Get(2).(func([]cbtservice.Range) error) + record([]cbtservice.Range{{Offset: 0, Length: 1024}}) + }).Return(nil) + return m + }, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + blkup.On("Backup", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(udmrepo.Snapshot{RootObject: udmrepo.ObjectMetadata{ID: "root"}}, int64(1024), nil) + repo.On("SaveSnapshot", mock.Anything, mock.Anything).Return(udmrepo.ID("snap-cbt-alloc"), nil) + repo.On("Flush", mock.Anything).Return(nil) + }, + expectedSnapID: "snap-cbt-alloc", + expectedSize: 1024, + }, + { + name: "cbtService error falls back to full", + cbtService: func(t *testing.T) cbtservice.Service { + t.Helper() + m := cbtservicemocks.NewService(t) + m.On("GetAllocatedBlocks", mock.Anything, "snap-1", mock.Anything). + Return(errors.New("CBT error")) + return m + }, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + // Should be called with parentObject as empty because of fallback + blkup.On("Backup", mock.Anything, udmrepo.ID(""), mock.Anything, mock.Anything). + Return(udmrepo.Snapshot{}, int64(2048), nil) + repo.On("SaveSnapshot", mock.Anything, mock.Anything).Return(udmrepo.ID("snap-cbt-fallback"), nil) + repo.On("Flush", mock.Anything).Return(nil) + }, + expectedSnapID: "snap-cbt-fallback", + expectedSize: 2048, + }, } for _, tc := range testCases { @@ -251,14 +313,19 @@ func TestSnapshotSource(t *testing.T) { tc.setupMocks(mockBlkup, mockRepo) - cbtSrc := cbtservice.SourceInfo{ChangeID: "cid-1", VolumeID: "vid-1"} + cbtSrc := cbtservice.SourceInfo{Snapshot: "snap-1", ChangeID: "cid-1", VolumeID: "vid-1"} snapshotTags := map[string]string{"custom": "val"} + var cbtSvc cbtservice.Service + if tc.cbtService != nil { + cbtSvc = tc.cbtService(t) + } + snapID, size, err := snapshotSource( ctx, mockRepo, mockBlkup, baseSource, true, "", - cbtSrc, nil, + cbtSrc, cbtSvc, snapshotTags, map[string]string{}, testLog(), "Block Uploader", ) @@ -601,6 +668,9 @@ func TestRestore(t *testing.T) { testCases := []struct { name string + incremental bool + cbtSource cbtservice.SourceInfo + cbtService func(t *testing.T) cbtservice.Service setupMocks func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) setupOpenDev func(t *testing.T) *os.File expectedErrStr string @@ -637,7 +707,7 @@ func TestRestore(t *testing.T) { expectedErrStr: "error restoring to block dev", }, { - name: "success returns size", + name: "success returns size (full restore)", setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")). Return(storedSnap, nil) @@ -650,6 +720,102 @@ func TestRestore(t *testing.T) { }, expectedSize: 4096, }, + { + name: "incremental restore success", + incremental: true, + cbtSource: cbtservice.SourceInfo{Snapshot: "snap-cbt", VolumeID: "vol-1"}, + cbtService: func(t *testing.T) cbtservice.Service { + t.Helper() + m := cbtservicemocks.NewService(t) + m.On("GetChangedBlocks", mock.Anything, "snap-cbt", "cid-1", mock.Anything). + Run(func(args mock.Arguments) { + record := args.Get(3).(func([]cbtservice.Range) error) + record([]cbtservice.Range{{Offset: 0, Length: 512}}) + }).Return(nil) + return m + }, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + snapWithTags := udmrepo.Snapshot{ + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-1", + uploader.CBTVolumeIDTag: "vol-1", + }, + TotalSize: 1024, + } + repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")).Return(snapWithTags, nil) + blkup.On("Restore", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(int64(512), int64(512), nil) + }, + setupOpenDev: func(t *testing.T) *os.File { + t.Helper() + return tempFile(t, "") + }, + expectedSize: 512, + }, + { + name: "incremental restore fallback - missing tags", + incremental: true, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")).Return(storedSnap, nil) + blkup.On("Restore", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(int64(4096), int64(4096), nil) + }, + setupOpenDev: func(t *testing.T) *os.File { + t.Helper() + return tempFile(t, "") + }, + expectedSize: 4096, + }, + { + name: "incremental restore fallback - VolumeID mismatch", + incremental: true, + cbtSource: cbtservice.SourceInfo{VolumeID: "vol-actual"}, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + snapWithTags := udmrepo.Snapshot{ + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-1", + uploader.CBTVolumeIDTag: "vol-expected", + }, + } + repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")).Return(snapWithTags, nil) + blkup.On("Restore", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(int64(4096), int64(4096), nil) + }, + setupOpenDev: func(t *testing.T) *os.File { + t.Helper() + return tempFile(t, "") + }, + expectedSize: 4096, + }, + { + name: "incremental restore fallback - CBT service error", + incremental: true, + cbtSource: cbtservice.SourceInfo{Snapshot: "snap-cbt", VolumeID: "vol-1"}, + cbtService: func(t *testing.T) cbtservice.Service { + t.Helper() + m := cbtservicemocks.NewService(t) + m.On("GetChangedBlocks", mock.Anything, "snap-cbt", "cid-1", mock.Anything). + Return(errors.New("CBT error")) + return m + }, + setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) { + snapWithTags := udmrepo.Snapshot{ + Tags: map[string]string{ + uploader.CBTChangeIDTag: "cid-1", + uploader.CBTVolumeIDTag: "vol-1", + }, + TotalSize: 1024, + } + repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")).Return(snapWithTags, nil) + blkup.On("Restore", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(int64(1024), int64(1024), nil) + }, + setupOpenDev: func(t *testing.T) *os.File { + t.Helper() + return tempFile(t, "") + }, + expectedSize: 1024, + }, } for _, tc := range testCases { @@ -671,7 +837,12 @@ func TestRestore(t *testing.T) { } } - size, err := Restore(ctx, mockBlkup, mockRepo, "snap-001", "/dev/sdb", map[string]string{}, testLog()) + var cbtSvc cbtservice.Service + if tc.cbtService != nil { + cbtSvc = tc.cbtService(t) + } + + size, err := Restore(ctx, mockBlkup, mockRepo, "snap-001", "/dev/sdb", tc.incremental, tc.cbtSource, cbtSvc, map[string]string{}, testLog()) if tc.expectedErrStr != "" { require.Error(t, err) diff --git a/pkg/uploader/kopia/snapshot.go b/pkg/uploader/kopia/snapshot.go index 217ff531f..fae7a517c 100644 --- a/pkg/uploader/kopia/snapshot.go +++ b/pkg/uploader/kopia/snapshot.go @@ -389,7 +389,7 @@ func (o *fileSystemRestoreOutput) Terminate() error { } // Restore restore specific sourcePath with given snapshotID and update progress -func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, snapshotID, dest string, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, +func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { log.Info("Start to restore...") @@ -421,7 +421,7 @@ func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, } restoreConcurrency := runtime.NumCPU() - + deleteExtra := false if len(uploaderCfg) > 0 { writeSparseFiles, err := uploaderutil.GetWriteSparseFiles(uploaderCfg) if err != nil { @@ -438,9 +438,14 @@ func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, if concurrency > 0 { restoreConcurrency = concurrency } + + deleteExtra, err = uploaderutil.GetDeleteExtraFiles(uploaderCfg) + if err != nil { + return 0, 0, errors.Wrap(err, "failed to get delete extra files config") + } } - log.Debugf("Restore filesystem output %v, concurrency %d", fsOutput, restoreConcurrency) + log.Debugf("Restore filesystem output %v, concurrency %d, incremental %v, delete extra %v", fsOutput, restoreConcurrency, incremental, deleteExtra) err = fsOutput.Init(ctx) if err != nil { @@ -448,14 +453,22 @@ func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, } var output RestoreOutput + // kopiaOutput is the output passed to Kopia's restore.Entry function. + // We must pass the unwrapped fsOutput (*restore.FilesystemOutput) directly for file system restores. + // This is because Kopia internally uses a strict type assertion (c.output.(*FilesystemOutput)) + // to determine if it should execute the deleteExtra logic. If we pass the wrapped + // fileSystemRestoreOutput, the type assertion fails and extra files are not deleted. + var kopiaOutput restore.Output if volMode == uploader.PersistentVolumeBlock { output = &BlockOutput{ FilesystemOutput: fsOutput, } + kopiaOutput = output } else { output = &fileSystemRestoreOutput{ FilesystemOutput: fsOutput, } + kopiaOutput = fsOutput } defer func() { @@ -464,8 +477,10 @@ func Restore(ctx context.Context, rep repo.RepositoryWriter, progress *Progress, } }() - stat, err := restoreEntryFunc(kopiaCtx, rep, output, rootEntry, restore.Options{ + stat, err := restoreEntryFunc(kopiaCtx, rep, kopiaOutput, rootEntry, restore.Options{ Parallel: restoreConcurrency, + Incremental: incremental, + DeleteExtra: deleteExtra, RestoreDirEntryAtDepth: math.MaxInt32, Cancel: cancleCh, ProgressCallback: func(ctx context.Context, stats restore.Stats) { diff --git a/pkg/uploader/kopia/snapshot_test.go b/pkg/uploader/kopia/snapshot_test.go index 36f30d82c..e58c2bb88 100644 --- a/pkg/uploader/kopia/snapshot_test.go +++ b/pkg/uploader/kopia/snapshot_test.go @@ -681,6 +681,7 @@ func TestRestore(t *testing.T) { expectedCount int32 expectedError error volMode uploader.PersistentVolumeMode + incremental bool } // Define test cases @@ -818,7 +819,7 @@ func TestRestore(t *testing.T) { repoWriterMock.On("OpenObject", mock.Anything, mock.Anything).Return(em, nil) progress := new(Progress) - bytesRestored, fileCount, err := Restore(t.Context(), repoWriterMock, progress, tc.snapshotID, tc.dest, tc.volMode, map[string]string{}, logrus.New(), nil) + bytesRestored, fileCount, err := Restore(t.Context(), repoWriterMock, progress, tc.snapshotID, tc.dest, tc.incremental, tc.volMode, map[string]string{}, logrus.New(), nil) // Check if the returned error matches the expected error if tc.expectedError != nil { diff --git a/pkg/uploader/provider/block.go b/pkg/uploader/provider/block.go index e37a0c16f..2b5ad275f 100644 --- a/pkg/uploader/provider/block.go +++ b/pkg/uploader/provider/block.go @@ -163,6 +163,8 @@ func (bp *blockProvider) RunRestore( ctx context.Context, snapshotID string, volumePath string, + incremental bool, + cbtParam CBTParam, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, updater uploader.ProgressUpdater) (int64, error) { @@ -178,7 +180,7 @@ func (bp *blockProvider) RunRestore( blkUploader := block.NewUploader(ctx, bp.bkRepo, updater, log) - size, err := blockRestoreFunc(ctx, blkUploader, bp.bkRepo, snapshotID, volumePath, uploaderCfg, log) + size, err := blockRestoreFunc(ctx, blkUploader, bp.bkRepo, snapshotID, volumePath, incremental, cbtParam.Source, cbtParam.Service, uploaderCfg, log) // errors.Is, not ==: see the equivalent comment on the backup path above. if errors.Is(err, block.ErrCanceled) { diff --git a/pkg/uploader/provider/block_test.go b/pkg/uploader/provider/block_test.go index 42375be20..970fc7cf6 100644 --- a/pkg/uploader/provider/block_test.go +++ b/pkg/uploader/provider/block_test.go @@ -412,7 +412,7 @@ func TestBlockProviderCancelThroughWrappedError(t *testing.T) { t.Run("restore", func(t *testing.T) { orig := blockRestoreFunc defer func() { blockRestoreFunc = orig }() - blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ map[string]string, _ logrus.FieldLogger) (int64, error) { + blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ bool, _ cbtservice.SourceInfo, _ cbtservice.Service, _ map[string]string, _ logrus.FieldLogger) (int64, error) { return 0, errors.Wrap(block.ErrCanceled, "error restoring bdev") } @@ -422,7 +422,7 @@ func TestBlockProviderCancelThroughWrappedError(t *testing.T) { log: logrus.New(), } - _, err := bp.RunRestore(t.Context(), "snap-1", "/dev/sda", + _, err := bp.RunRestore(t.Context(), "snap-1", "/dev/sda", false, CBTParam{}, uploader.PersistentVolumeBlock, map[string]string{}, &blockMockProgressUpdater{}) require.ErrorIs(t, err, ErrorCanceled) @@ -496,9 +496,9 @@ func TestBlockProviderRunRestore(t *testing.T) { var capturedSnapshotID string var capturedVolumePath string - blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, snapshotID string, volumePath string, _ map[string]string, _ logrus.FieldLogger) (int64, error) { + blockRestoreFunc = func(ctx context.Context, blkUp block.Uploader, rep udmrepo.BackupRepo, snapshotID string, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, error) { capturedSnapshotID = snapshotID - capturedVolumePath = volumePath + capturedVolumePath = dest return tc.mockRestoreSize, tc.mockRestoreErr } @@ -511,6 +511,8 @@ func TestBlockProviderRunRestore(t *testing.T) { t.Context(), tc.snapshotID, tc.volumePath, + false, + CBTParam{}, uploader.PersistentVolumeBlock, map[string]string{}, tc.updater, diff --git a/pkg/uploader/provider/kopia.go b/pkg/uploader/provider/kopia.go index 682b2053e..c9d9948bf 100644 --- a/pkg/uploader/provider/kopia.go +++ b/pkg/uploader/provider/kopia.go @@ -211,6 +211,8 @@ func (kp *kopiaProvider) RunRestore( ctx context.Context, snapshotID string, volumePath string, + incremental bool, + _ CBTParam, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, updater uploader.ProgressUpdater) (int64, error) { @@ -234,7 +236,7 @@ func (kp *kopiaProvider) RunRestore( // We use the cancel channel to control the restore cancel, so don't pass a context with cancel to Kopia restore. // Otherwise, Kopia restore will not response to the cancel control but return an arbitrary error. // Kopia restore cancel is not designed as well as Kopia backup which uses the context to control backup cancel all the way. - size, fileCount, err := kopiaRestoreFunc(context.Background(), repoWriter, progress, snapshotID, volumePath, volMode, uploaderCfg, log, restoreCancel) + size, fileCount, err := kopiaRestoreFunc(context.Background(), repoWriter, progress, snapshotID, volumePath, incremental, volMode, uploaderCfg, log, restoreCancel) if err != nil { return 0, errors.Wrapf(err, "Failed to run kopia restore") diff --git a/pkg/uploader/provider/kopia_test.go b/pkg/uploader/provider/kopia_test.go index bfb544c26..a29a3c424 100644 --- a/pkg/uploader/provider/kopia_test.go +++ b/pkg/uploader/provider/kopia_test.go @@ -119,20 +119,21 @@ func TestRunBackup(t *testing.T) { func TestRunRestore(t *testing.T) { testCases := []struct { name string - hookRestoreFunc func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) + hookRestoreFunc func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) notError bool volMode uploader.PersistentVolumeMode + incremental bool }{ { name: "normal restore", - hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { + hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { return 0, 0, nil }, notError: true, }, { name: "normal block mode restore", - hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { + hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { return 0, 0, nil }, volMode: uploader.PersistentVolumeBlock, @@ -140,7 +141,7 @@ func TestRunRestore(t *testing.T) { }, { name: "failed to restore", - hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { + hookRestoreFunc: func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { return 0, 0, errors.New("failed to restore") }, notError: false, @@ -157,7 +158,7 @@ func TestRunRestore(t *testing.T) { tc.volMode = uploader.PersistentVolumeFilesystem } kopiaRestoreFunc = tc.hookRestoreFunc - _, err := kp.RunRestore(t.Context(), "", "/var", tc.volMode, map[string]string{}, &updater) + _, err := kp.RunRestore(t.Context(), "", "/var", tc.incremental, CBTParam{}, tc.volMode, map[string]string{}, &updater) if tc.notError { assert.NoError(t, err) } else { diff --git a/pkg/uploader/provider/mocks/Provider.go b/pkg/uploader/provider/mocks/Provider.go index 71e60b84e..5bd3dda54 100644 --- a/pkg/uploader/provider/mocks/Provider.go +++ b/pkg/uploader/provider/mocks/Provider.go @@ -223,8 +223,8 @@ func (_c *Provider_RunBackup_Call) RunAndReturn(run func(ctx context.Context, pa } // RunRestore provides a mock function for the type Provider -func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volumePath string, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error) { - ret := _mock.Called(ctx, snapshotID, volumePath, volMode, uploaderConfig, updater) +func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error) { + ret := _mock.Called(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) if len(ret) == 0 { panic("no return value specified for RunRestore") @@ -232,16 +232,16 @@ func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volume var r0 int64 var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) (int64, error)); ok { - return returnFunc(ctx, snapshotID, volumePath, volMode, uploaderConfig, updater) + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) (int64, error)); ok { + return returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) } - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) int64); ok { - r0 = returnFunc(ctx, snapshotID, volumePath, volMode, uploaderConfig, updater) + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) int64); ok { + r0 = returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) } else { r0 = ret.Get(0).(int64) } - if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) error); ok { - r1 = returnFunc(ctx, snapshotID, volumePath, volMode, uploaderConfig, updater) + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) error); ok { + r1 = returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) } else { r1 = ret.Error(1) } @@ -257,14 +257,16 @@ type Provider_RunRestore_Call struct { // - ctx context.Context // - snapshotID string // - volumePath string +// - incremental bool +// - cbtParam provider.CBTParam // - volMode uploader.PersistentVolumeMode // - uploaderConfig map[string]string // - updater uploader.ProgressUpdater -func (_e *Provider_Expecter) RunRestore(ctx interface{}, snapshotID interface{}, volumePath interface{}, volMode interface{}, uploaderConfig interface{}, updater interface{}) *Provider_RunRestore_Call { - return &Provider_RunRestore_Call{Call: _e.mock.On("RunRestore", ctx, snapshotID, volumePath, volMode, uploaderConfig, updater)} +func (_e *Provider_Expecter) RunRestore(ctx interface{}, snapshotID interface{}, volumePath interface{}, incremental interface{}, cbtParam interface{}, volMode interface{}, uploaderConfig interface{}, updater interface{}) *Provider_RunRestore_Call { + return &Provider_RunRestore_Call{Call: _e.mock.On("RunRestore", ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater)} } -func (_c *Provider_RunRestore_Call) Run(run func(ctx context.Context, snapshotID string, volumePath string, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater)) *Provider_RunRestore_Call { +func (_c *Provider_RunRestore_Call) Run(run func(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater)) *Provider_RunRestore_Call { _c.Call.Run(func(args mock.Arguments) { var arg0 context.Context if args[0] != nil { @@ -278,17 +280,25 @@ func (_c *Provider_RunRestore_Call) Run(run func(ctx context.Context, snapshotID if args[2] != nil { arg2 = args[2].(string) } - var arg3 uploader.PersistentVolumeMode + var arg3 bool if args[3] != nil { - arg3 = args[3].(uploader.PersistentVolumeMode) + arg3 = args[3].(bool) } - var arg4 map[string]string + var arg4 provider.CBTParam if args[4] != nil { - arg4 = args[4].(map[string]string) + arg4 = args[4].(provider.CBTParam) } - var arg5 uploader.ProgressUpdater + var arg5 uploader.PersistentVolumeMode if args[5] != nil { - arg5 = args[5].(uploader.ProgressUpdater) + arg5 = args[5].(uploader.PersistentVolumeMode) + } + var arg6 map[string]string + if args[6] != nil { + arg6 = args[6].(map[string]string) + } + var arg7 uploader.ProgressUpdater + if args[7] != nil { + arg7 = args[7].(uploader.ProgressUpdater) } run( arg0, @@ -297,6 +307,8 @@ func (_c *Provider_RunRestore_Call) Run(run func(ctx context.Context, snapshotID arg3, arg4, arg5, + arg6, + arg7, ) }) return _c @@ -307,7 +319,7 @@ func (_c *Provider_RunRestore_Call) Return(n int64, err error) *Provider_RunRest return _c } -func (_c *Provider_RunRestore_Call) RunAndReturn(run func(ctx context.Context, snapshotID string, volumePath string, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error)) *Provider_RunRestore_Call { +func (_c *Provider_RunRestore_Call) RunAndReturn(run func(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error)) *Provider_RunRestore_Call { _c.Call.Return(run) return _c } diff --git a/pkg/uploader/provider/provider.go b/pkg/uploader/provider/provider.go index 26b7b84f2..9d06578d8 100644 --- a/pkg/uploader/provider/provider.go +++ b/pkg/uploader/provider/provider.go @@ -64,6 +64,8 @@ type Provider interface { ctx context.Context, snapshotID string, volumePath string, + incremental bool, + cbtParam CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error) diff --git a/pkg/uploader/util/uploader_config.go b/pkg/uploader/util/uploader_config.go index c221741bf..3736bcfca 100644 --- a/pkg/uploader/util/uploader_config.go +++ b/pkg/uploader/util/uploader_config.go @@ -28,6 +28,7 @@ const ( ParallelFilesUpload = "ParallelFilesUpload" WriteSparseFiles = "WriteSparseFiles" RestoreConcurrency = "ParallelFilesDownload" + DeleteExtraFiles = "DeleteExtraFiles" ) func StoreBackupConfig(config *velerov1api.UploaderConfigForBackup) map[string]string { @@ -47,6 +48,13 @@ func StoreRestoreConfig(config *velerov1api.UploaderConfigForRestore) map[string if config.ParallelFilesDownload > 0 { data[RestoreConcurrency] = strconv.Itoa(config.ParallelFilesDownload) } + + if config.DeleteExtraFiles != nil { + data[DeleteExtraFiles] = strconv.FormatBool(*config.DeleteExtraFiles) + } else { + data[DeleteExtraFiles] = strconv.FormatBool(false) + } + return data } @@ -85,3 +93,15 @@ func GetRestoreConcurrency(uploaderCfg map[string]string) (int, error) { } return 0, nil } + +func GetDeleteExtraFiles(uploaderCfg map[string]string) (bool, error) { + deleteExtraFiles, ok := uploaderCfg[DeleteExtraFiles] + if ok { + deleteExtraFilesBool, err := strconv.ParseBool(deleteExtraFiles) + if err != nil { + return false, errors.Wrap(err, "failed to parse DeleteExtraFiles config") + } + return deleteExtraFilesBool, nil + } + return false, nil +} diff --git a/pkg/uploader/util/uploader_config_test.go b/pkg/uploader/util/uploader_config_test.go index 46df8b714..e9628d938 100644 --- a/pkg/uploader/util/uploader_config_test.go +++ b/pkg/uploader/util/uploader_config_test.go @@ -58,6 +58,7 @@ func TestStoreRestoreConfig(t *testing.T) { }, expectedData: map[string]string{ WriteSparseFiles: "true", + DeleteExtraFiles: "false", }, }, { @@ -67,6 +68,7 @@ func TestStoreRestoreConfig(t *testing.T) { }, expectedData: map[string]string{ WriteSparseFiles: "false", + DeleteExtraFiles: "false", }, }, { @@ -76,6 +78,7 @@ func TestStoreRestoreConfig(t *testing.T) { }, expectedData: map[string]string{ WriteSparseFiles: "false", // Assuming default value is false for nil case + DeleteExtraFiles: "false", }, }, { @@ -86,6 +89,37 @@ func TestStoreRestoreConfig(t *testing.T) { expectedData: map[string]string{ RestoreConcurrency: "5", WriteSparseFiles: "false", + DeleteExtraFiles: "false", + }, + }, + { + name: "DeleteExtraFiles is true", + config: &velerov1api.UploaderConfigForRestore{ + DeleteExtraFiles: &boolTrue, + }, + expectedData: map[string]string{ + WriteSparseFiles: "false", + DeleteExtraFiles: "true", + }, + }, + { + name: "DeleteExtraFiles is false", + config: &velerov1api.UploaderConfigForRestore{ + DeleteExtraFiles: &boolFalse, + }, + expectedData: map[string]string{ + WriteSparseFiles: "false", + DeleteExtraFiles: "false", + }, + }, + { + name: "DeleteExtraFiles is nil", + config: &velerov1api.UploaderConfigForRestore{ + DeleteExtraFiles: nil, + }, + expectedData: map[string]string{ + WriteSparseFiles: "false", + DeleteExtraFiles: "false", // Assuming default value is false for nil case }, }, } @@ -240,3 +274,51 @@ func TestGetRestoreConcurrency(t *testing.T) { }) } } + +func TestGetDeleteExtraFiles(t *testing.T) { + tests := []struct { + name string + uploaderCfg map[string]string + expectedResult bool + expectedError error + }{ + { + name: "Valid DeleteExtraFiles (true)", + uploaderCfg: map[string]string{DeleteExtraFiles: "true"}, + expectedResult: true, + expectedError: nil, + }, + { + name: "Valid DeleteExtraFiles (false)", + uploaderCfg: map[string]string{DeleteExtraFiles: "false"}, + expectedResult: false, + expectedError: nil, + }, + { + name: "Invalid DeleteExtraFiles (not a boolean)", + uploaderCfg: map[string]string{DeleteExtraFiles: "invalid"}, + expectedResult: false, + expectedError: errors.Wrap(errors.New("strconv.ParseBool: parsing \"invalid\": invalid syntax"), "failed to parse DeleteExtraFiles config"), + }, + { + name: "Missing DeleteExtraFiles", + uploaderCfg: map[string]string{}, + expectedResult: false, + expectedError: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := GetDeleteExtraFiles(test.uploaderCfg) + + if result != test.expectedResult { + t.Errorf("Expected result %t, but got %t", test.expectedResult, result) + } + + if (err == nil && test.expectedError != nil) || (err != nil && test.expectedError == nil) || (err != nil && test.expectedError != nil && err.Error() != test.expectedError.Error()) { + t.Errorf("Expected error '%v', but got '%v'", test.expectedError, err) + } + }) + } +} diff --git a/pkg/util/csi/cbt.go b/pkg/util/csi/cbt.go new file mode 100644 index 000000000..00342996d --- /dev/null +++ b/pkg/util/csi/cbt.go @@ -0,0 +1,80 @@ +/* +Copyright The Velero Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package csi + +import ( + "context" + "fmt" + "strings" + + "github.com/cockroachdb/errors" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + + "github.com/vmware-tanzu/velero/pkg/util" +) + +// CBTInfo define the info for CBT +type CBTInfo struct { + ChangeID string + VolumeID string + SnapshotID string +} + +// GetCBTInfo returns the CBT info for a snapshot +func GetCBTInfo(ctx context.Context, kubeClient kubernetes.Interface, log logrus.FieldLogger, vs *snapshotv1api.VolumeSnapshot, vsc *snapshotv1api.VolumeSnapshotContent, sourcePVName string) (CBTInfo, error) { + cbtInfo := CBTInfo{} + if vs == nil || vsc == nil { + return cbtInfo, errors.New("vs or vsc is nil") + } + + cbtInfo.SnapshotID = vs.Name + + if vs.Annotations != nil && + (vs.Annotations[util.VSphereCNSChangeIDAnno] != "" || + vs.Annotations[util.VSphereCNSSnapshotAnno] != "") { + cbtInfo.ChangeID = vs.Annotations[util.VSphereCNSChangeIDAnno] + + splitSnapshotAnno := strings.Split(vs.Annotations[util.VSphereCNSSnapshotAnno], "+") + if len(splitSnapshotAnno) >= 2 { + cbtInfo.VolumeID = splitSnapshotAnno[0] + } + log.Debugf("volumeID %s and changeID %s are read from VKS annotations.", cbtInfo.VolumeID, cbtInfo.ChangeID) + } else { + pv, err := kubeClient.CoreV1().PersistentVolumes().Get(ctx, sourcePVName, metav1.GetOptions{}) + if err != nil { + return cbtInfo, fmt.Errorf("failed to get pv %s: %w", sourcePVName, err) + } + + if vsc.Status != nil && vsc.Status.SnapshotHandle != nil { + cbtInfo.ChangeID = *vsc.Status.SnapshotHandle + } + + if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" { + cbtInfo.VolumeID = pv.Spec.CSI.VolumeHandle + } + log.Debugf("volumeID %s and changeID %s are read from PV and VS's handles.", cbtInfo.VolumeID, cbtInfo.ChangeID) + } + + if cbtInfo.VolumeID == "" { + return cbtInfo, fmt.Errorf("volumeID must not be empty for CBT") + } + + return cbtInfo, nil +} diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index b375ce0ba..49d0bbc60 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -35,6 +35,7 @@ import ( corev1client "k8s.io/client-go/kubernetes/typed/core/v1" crclient "sigs.k8s.io/controller-runtime/pkg/client" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" storagev1api "k8s.io/api/storage/v1" storagev1 "k8s.io/client-go/kubernetes/typed/storage/v1" ) @@ -95,6 +96,10 @@ func WaitPVCBound(ctx context.Context, pvcGetter corev1client.CoreV1Interface, return false, nil } + if tmpPVC.Status.Phase != corev1api.ClaimBound { + return false, nil + } + updated = tmpPVC return true, nil @@ -112,6 +117,16 @@ func WaitPVCBound(ctx context.Context, pvcGetter corev1client.CoreV1Interface, return pv, err } +// DeletePVCIfAny deletes a PVC by namespace and name if it exists, and log an error when the deletion fails +func DeletePVCIfAny(ctx context.Context, client corev1client.CoreV1Interface, pvcName, pvcNamespace string, ensureTimeout time.Duration, log logrus.FieldLogger) { + if err := EnsureDeletePVC(ctx, client, pvcName, pvcNamespace, ensureTimeout); err != nil { + if apierrors.IsNotFound(err) { + return + } + log.Warnf("failed to delete pvc %s/%s with err %v", pvcNamespace, pvcName, err) + } +} + // DeletePVIfAny deletes a PV by name if it exists, and log an error when the deletion fails func DeletePVIfAny(ctx context.Context, pvGetter corev1client.CoreV1Interface, pvName string, log logrus.FieldLogger) { err := pvGetter.PersistentVolumes().Delete(ctx, pvName, metav1.DeleteOptions{}) @@ -124,6 +139,47 @@ func DeletePVIfAny(ctx context.Context, pvGetter corev1client.CoreV1Interface, p } } +// EnsureDeleteVolumeSnapshotIfAny deletes a VolumeSnapshot by namespace and name if it exists, and log an error when the deletion fails +func EnsureDeleteVolumeSnapshotIfAny(ctx context.Context, client crclient.Client, namespace, name string, ensureTimeout time.Duration, log logrus.FieldLogger) { + if err := client.Delete(ctx, &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + }); err != nil && !apierrors.IsNotFound(err) { + log.WithError(err).Errorf("Failed to delete the VolumeSnapshot %s/%s", namespace, name) + } + + if ensureTimeout == 0 { + return + } + + var updated *snapshotv1api.VolumeSnapshot + err := wait.PollUntilContextTimeout(ctx, waitInternal, ensureTimeout, true, func(ctx context.Context) (bool, error) { + if err := client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, updated); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + + return false, errors.Wrapf(err, "error to get VolumeSnapshot %s/%s", namespace, name) + } + + return false, nil + }) + + if err != nil { + if errors.Is(err, context.DeadlineExceeded) { + if updated == nil { + log.WithError(err).Errorf("Timeout to assure VolumeSnapshot %s/%s is deleted", namespace, name) + } else { + log.WithError(err).Errorf("Timeout to assure VolumeSnapshot %s/%s is deleted, finalizers in VolumeSnapshot %v", namespace, name, updated.Finalizers) + } + } else { + log.WithError(err).Errorf("Error to assure VolumeSnapshot %s/%s is deleted", namespace, name) + } + } +} + // EnsureDeletePVC asserts the existence of a PVC by name, deletes it and waits for its disappearance and returns errors on any failure // If timeout is 0, it doesn't wait and return nil func EnsureDeletePVC(ctx context.Context, pvcGetter corev1client.CoreV1Interface, pvcName string, namespace string, timeout time.Duration) error { diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index c805929d7..fb6eb4947 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/pkg/util/kube/pvc_pv_test.go @@ -62,6 +62,9 @@ func TestWaitPVCBound(t *testing.T) { Spec: corev1api.PersistentVolumeClaimSpec{ VolumeName: "fake-pv", }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, } pvObj := &corev1api.PersistentVolume{ @@ -304,6 +307,105 @@ func TestWaitPVCConsumed(t *testing.T) { } func TestDeletePVCIfAny(t *testing.T) { + pvcObject := &corev1api.PersistentVolumeClaim{ + TypeMeta: metav1.TypeMeta{ + Kind: "fake-kind-1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-namespace", + Name: "fake-pvc", + }, + } + + tests := []struct { + name string + pvcName string + pvcNamespace string + kubeClientObj []runtime.Object + kubeReactors []reactor + logMessage string + logLevel string + ensureTimeout time.Duration + }{ + { + name: "pvc not found", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + }, + { + name: "failed to delete pvc", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + kubeReactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-delete-error") + }, + }, + }, + kubeClientObj: []runtime.Object{ + pvcObject, + }, + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err error to delete pvc fake-pvc: fake-delete-error", + logLevel: "level=warning", + }, + { + name: "delete pvc success", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + kubeClientObj: []runtime.Object{ + pvcObject, + }, + }, + { + name: "delete pvc success but wait fail", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + kubeClientObj: []runtime.Object{ + pvcObject, + }, + kubeReactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + }, + ensureTimeout: time.Second, + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err timeout to assure pvc fake-pvc is deleted, finalizers in pvc []", + logLevel: "level=warning", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...) + + for _, reactor := range test.kubeReactors { + fakeKubeClient.Fake.PrependReactor(reactor.verb, reactor.resource, reactor.reactorFunc) + } + + var kubeClient kubernetes.Interface = fakeKubeClient + + logMessage := "" + DeletePVCIfAny(t.Context(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, test.ensureTimeout, velerotest.NewSingleLogger(&logMessage)) + + if len(test.logMessage) > 0 { + assert.Contains(t, logMessage, test.logMessage) + } + + if len(test.logLevel) > 0 { + assert.Contains(t, logMessage, test.logLevel) + } + }) + } +} + +func TestDeletePVAndPVCIfAny(t *testing.T) { pvObject := &corev1api.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ Name: "fake-pv", diff --git a/pkg/util/velero/restore/util.go b/pkg/util/velero/restore/util.go index e0812884b..96a368e50 100644 --- a/pkg/util/velero/restore/util.go +++ b/pkg/util/velero/restore/util.go @@ -5,8 +5,14 @@ import ( ) func IsResourcePolicyValid(resourcePolicy string) bool { - if resourcePolicy == string(api.PolicyTypeNone) || resourcePolicy == string(api.PolicyTypeUpdate) { - return true - } - return false + return resourcePolicy == "" || + resourcePolicy == string(api.ResourcePolicyTypeNone) || + resourcePolicy == string(api.ResourcePolicyTypeUpdate) +} + +func IsVolumeDataPolicyValid(volumeDataPolicy string) bool { + return volumeDataPolicy == "" || + volumeDataPolicy == string(api.VolumeDataPolicyTypeNone) || + volumeDataPolicy == string(api.VolumeDataPolicyTypeFull) || + volumeDataPolicy == string(api.VolumeDataPolicyTypeIncremental) } diff --git a/pkg/util/velero/restore/util_test.go b/pkg/util/velero/restore/util_test.go index be72ff8ba..bcd447d4b 100644 --- a/pkg/util/velero/restore/util_test.go +++ b/pkg/util/velero/restore/util_test.go @@ -9,7 +9,16 @@ import ( ) func TestIsResourcePolicyValid(t *testing.T) { - require.True(t, IsResourcePolicyValid(string(velerov1api.PolicyTypeNone))) - require.True(t, IsResourcePolicyValid(string(velerov1api.PolicyTypeUpdate))) - require.False(t, IsResourcePolicyValid("")) + require.True(t, IsResourcePolicyValid(string(velerov1api.ResourcePolicyTypeNone))) + require.True(t, IsResourcePolicyValid(string(velerov1api.ResourcePolicyTypeUpdate))) + require.True(t, IsResourcePolicyValid("")) + require.False(t, IsResourcePolicyValid("invalid")) +} + +func TestIsVolumeDataPolicyValid(t *testing.T) { + require.True(t, IsVolumeDataPolicyValid(string(velerov1api.VolumeDataPolicyTypeNone))) + require.True(t, IsVolumeDataPolicyValid(string(velerov1api.VolumeDataPolicyTypeFull))) + require.True(t, IsVolumeDataPolicyValid(string(velerov1api.VolumeDataPolicyTypeIncremental))) + require.True(t, IsVolumeDataPolicyValid("")) + require.False(t, IsVolumeDataPolicyValid("invalid")) }