* 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 <none> 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 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
(cherry picked from commit 6c7aa9d588f6d5eab134d4ce19c92b838f45557c)
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* gofmt: fix import ordering in backup_test.go
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* 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 <tkaovila@redhat.com>
* Add changelog for #10309
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* 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 <tkaovila@redhat.com>
* 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 <nil> instead of 0.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
* Fix CI: rename changelog to PR number, add unit tests for coverage
- changelogs/unreleased must be named <pr-number>-<username>; 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 <noreply@anthropic.com>
Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
* 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 <lubron.zhan@broadcom.com>
* 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 <lubron.zhan@broadcom.com>
* 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 <lubron.zhan@broadcom.com>
---------
Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Add readWriteOncePod backupPVC config to enable mount-level SELinux labeling
On SELinux-enabled clusters the kubelet recursively relabels every file of
the backupPVC at mount time, which can take hours on volumes with a high
file count. Kubernetes avoids this when the volume is ReadWriteOncePod and
the CSI driver advertises SELinux mount support, by mounting with
-o context= instead.
Add an opt-in per-storage-class 'readWriteOncePod' backupPVC config option
that creates the backupPVC with the ReadWriteOncePod access mode and sets
the backup pod's SecurityContext.SELinuxChangePolicy to MountOption. It is
mutually exclusive with 'readOnly', which takes precedence.
Fixes#9873
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Add changelog file
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Do not set SELinuxChangePolicy for the backup pod
Live testing on OCP 4.22 (k8s 1.35) showed that setting
SecurityContext.SELinuxChangePolicy to MountOption makes backup pod
creation fail outright when the SELinuxMount feature gate is disabled,
which is the default on current clusters:
Pod is invalid: spec.securityContext.seLinuxChangePolicy:
Unsupported value: "MountOption": supported values: "Recursive"
The field is also unnecessary. For ReadWriteOncePod volumes the kubelet
already performs mount-level SELinux labeling via the
SELinuxMountReadWriteOncePod feature gate, which has been on by default
since k8s 1.28. Setting the backupPVC access mode to ReadWriteOncePod is
sufficient on its own, and is portable to clusters where the broader
SELinuxMount gate is still off.
Verified on-cluster that the backupPVC is mounted with
context="system_u:object_r:container_file_t:s0:c22,c28" instead of the
recursive seclabel mount used without the flag.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
---------
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
config/crd/{v1,v2alpha1}/crds/crds.go were generated files that
gzip-compressed the CRD YAML bases into committed []byte literals via
hack/crd-gen, requiring `go generate` and a dedicated CI drift check
(hack/verify-generated-crd-code.sh). This made the files large,
unreviewable in diffs, and a frequent source of merge conflicts.
Replace the generated files with config/crd/{v1,v2alpha1}/crds.go
using `//go:embed bases/*.yaml` to embed the already-committed YAML
manifests directly, decoding them the same way at init. Since Go's
go:embed can't reach outside a file's own directory tree, the crds
package now lives alongside bases/ instead of in a bases-sibling
subdirectory; import paths in pkg/install and pkg/controller were
updated accordingly.
Drop hack/crd-gen and hack/verify-generated-crd-code.sh entirely, and
trim their references from update-3generated-crd-code.sh and the
codespell skip-list. No codegen step remains, so no drift is possible.
Fixes#10328
AI-Tool-Used: Claude Code
AI-Tool-Use-Level: Category 1 (High)
AI-Code-Category: Category 1 (Production)
Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
Co-authored-by: Daniel Jiang <daniel.jiang@broadcom.com>
When CreateSnapshot fails (e.g. quota limit), the snapshot is recorded
with an empty ProviderSnapshotID. During backup deletion, velero was
calling DeleteSnapshot("") which produces unnecessary 404 API calls.
Skip the DeleteSnapshot call when ProviderSnapshotID is empty and log
a warning instead.
Fixes#9429
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>
getParentBackupInfo's parent-selection log lines interpolated the
parentSnapshot parameter. On the discovery branch (no explicit parent
passed by the caller) that parameter is empty by definition, so every
message about which parent was chosen, or why a run fell back to full,
printed no identifier at all -- e.g. "Using parent snapshot , start
time ...". This is the normal path for scheduled/incremental backups,
so the omission hit the common case, not an edge one.
Bind a parentID local that starts as the parameter but is overwritten
once a parent is actually resolved (explicit or discovered), and log
that instead. No behavior change.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Backups stuck in New state have no Status.Expiration yet. The CLI
was estimating expiration from CreationTimestamp + TTL, which made
long-queued backups appear already expired.
Fixes#3555
Signed-off-by: PranjalManhgaye <manhgayepranjal@gmail.com>
* Skip signing a download URL when no artifacts can exist yet
Reported in #10232: a DownloadRequest for a backup that never ran still
reaches Processed with a signed URL, and fetching it returns 404.
The controller already has the backup, and the restore for restore
targets, in hand before it signs, so checking the phase costs no extra
call to the object store.
The check is deliberately narrow. It refuses only the pre-execution
phases, where nothing has been written for any target kind: New, Queued,
ReadyToStart and FailedValidation for backups, New and FailedValidation
for restores. InProgress onwards may hold a partial log or other
artifacts, and Deleting may still hold all of them, so those keep the
behaviour callers have today.
That matters because velero backup download has no client side phase
check of its own, unlike backup logs and restore logs. Reusing the
allowlist from pkg/cmd/cli/backup/logs.go would have changed what
backup download can fetch; this does not.
A backup with an empty phase is left alone as well, since that state is
transient and the caller can retry.
Refs #10232
Signed-off-by: saral <ilovegojo2580@gmail.com>
* Derive the phase coverage test from the generated CRDs
The previous test built a slice of phases by hand and asserted its own
length, so it passed no matter what the API did. Adding a fourteenth
backup phase would not have failed it.
This reads the status.phase enum out of the generated CRDs, via the
exported v1crds.CRDs that pkg/install already uses. The enum comes from
the same kubebuilder markers as the Go constants, so a phase added to
the API fails here until it is classified.
Verified by removing Deleting from the expectations, which now fails with
'BackupPhase "Deleting" is served by the CRD but not classified'.
Signed-off-by: saral <ilovegojo2580@gmail.com>
* Use US spelling in comments to satisfy the misspell linter
golangci-lint runs misspell, which flags behaviour as a misspelling of
behavior. Comments only, no functional change.
Signed-off-by: saral <ilovegojo2580@gmail.com>
* Set a Failed phase with a reason when the guard refuses to sign
The guard added in the previous commit left the request at New with no URL, so
the CLI polled until its own timeout and then reported that the backup storage
location may be unavailable. The BSL is fine; the backup never ran.
DownloadRequestPhase gains Failed and DownloadRequestStatus gains Message. The
controller sets both where it refuses, and the CLI stops as soon as it sees the
phase and surfaces the message instead of its generic timeout error.
Adding an enum value is additive, per the direction on the PR discussion.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: saral <ilovegojo2580@gmail.com>
---------
Signed-off-by: saral <ilovegojo2580@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Fix LoadAffinity mutation accumulating OS node selector terms
The node-agent parses the loadAffinity configuration once at startup and
keeps it in memory. GetLoadAffinityByStorageClass returned a pointer to one
of the elements of that cached list rather than a copy, so the exposers,
which append a kubernetes.io/os match expression to the returned affinity,
were mutating the shared configuration. Every DataUpload or DataDownload
appended another OS term, growing the data mover pod spec until it could
eventually exceed the object size limit.
Return a deep copy from GetLoadAffinityByStorageClass so that callers can
safely modify the result. A shallow copy is not enough because the
MatchExpressions slice header would still be shared with the source.
Fixes#10341
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Add changelog
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
---------
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Backup metadata with an empty or New phase was synced into the cluster as a pending backup, which the queue controller then ran as if it were newly requested. Hooks are dropped as well, since a synced backup never executes them.
Signed-off-by: chlins <chlins.zhang@gmail.com>
This commit ensures the resources in the set "resourceMustHave" can only
be created in the namespace of velero deployment if it's namespace
scoped.
Signed-off-by: Daniel Jiang <daniel.jiang@broadcom.com>
* Detect block uploader cancellation through wrapped errors
Cancelling a block data mover backup was reported as a failure: the
DataUpload ended Failed with an error message and the Backup went
PartiallyFailed, for a user-requested cancel.
The cause is a sentinel equality check. block.ErrCanceled is raised in
the write loop and then wrapped twice before it reaches the provider --
once in block/uploader.go ("error backing up bdev %s") and again in
block/snapshot.go ("Failed to run uploader backup for si %v") -- so
`err == block.ErrCanceled` can never be true and the ErrorCanceled
returns are unreachable. The filesystem provider avoids this by asking
the uploader for its state (kpUploader.IsCanceled()) rather than
inspecting the error.
Use errors.Is at both the backup and restore sites.
Adds TestBlockProviderCancelThroughWrappedError, which injects the
doubly-wrapped sentinel exactly as production builds it. Note the
assertion is require.ErrorIs, not ErrorContains: provider.ErrorCanceled
and block.ErrCanceled carry identical message text, so a substring
assertion passes whether or not the sentinel was recognised -- which is
why the existing test, injecting the bare sentinel, did not catch this.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
(cherry picked from commit 9d6c5da7a893068d424b0c7896638787c636e213)
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Add changelog for #10308
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* lint: fix misspelling (recognised -> recognized)
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The DataUpload and DataDownload controllers now copy and delete
namespace-scoped secrets/configmaps for backup/restore PVC provisioning.
Add the corresponding kubebuilder RBAC markers (get;list;create;delete
on secrets and configmaps) and regenerate the ClusterRole.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
The copied secret/configmap label value was the owner (DataUpload/
DataDownload) name, which is derived from the Backup/Restore name and
can exceed the 63-char Kubernetes label-value limit or contain invalid
characters. That would make the copy label and the cleanup selector
diverge and orphan the copied resources.
Use string(ownerObject.UID) consistently for the label value in both
copy and cleanup (backup and restore exposers). The UID is a stable,
always-valid label value.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Mirror the backup-side fix on the restore path. The generic restore
exposer creates the intermediate restore PVC in the Velero namespace
using the target PVC's StorageClass. For encrypted volumes this fails
because ceph-csi looks up the KMS token secret in the PVC's namespace
(the Velero namespace), where it does not exist.
Add SecretNames/ConfigMapNames to the RestorePVC config. When set, the
generic restore exposer copies the named secrets/configmaps from the
target namespace to the Velero namespace before creating the restore
PVC, and cleans them up in CleanUp(). Reuses the same copy/delete
helpers and label as the backup path.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
These single-object delete helpers were introduced earlier but are no
longer called in production code: DeleteSecretsWithLabel and
DeleteConfigMapsWithLabel now delete inline with UID preconditions.
Remove the dead functions and their tests.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
- Make CopySecret/CopyConfigMap accept a generic labels map instead of
hardcoding the backup-pvc-secret label, aligning with the generic
DeleteSecretsWithLabel helper. Move the BackupPVCSecretLabel constant
from util/kube to the exposer package where it is used.
- Move the secret/configmap copy in Expose() to after
WaitVolumeSnapshotReady and before createBackupVS. That is the most
likely failure point, and nothing needs cleanup before it.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
- Fix premature deletion of shared secrets/configmaps: check owner label
in addition to data equality. Same data + different owner is now a
collision, preventing one DataUpload's CleanUp from removing resources
another DataUpload is still using.
- Copy BinaryData in CopyConfigMap and include it in the equality check,
so configmaps with binary payloads (e.g., CA bundles) are not silently
truncated.
- Add UID preconditions to DeleteSecretsWithLabel and
DeleteConfigMapsWithLabel to avoid TOCTOU races where a recreated
object with the same name could be deleted.
- Move secret/configmap copy to the beginning of Expose(), before any
intermediate objects are created, so failure doesn't require cleanup.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add unit tests for CopyConfigMap, DeleteConfigMapIfAny, and
DeleteConfigMapsWithLabel mirroring the existing secret tests.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Move the secret and configmap copy logic from the DataUpload controller
into the CSI snapshot exposer's Expose() method. This keeps all
CSI-specific logic in the exposer and maintains symmetry with CleanUp()
which already handles the cleanup of copied resources.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
- Fix import ordering in test file (gofmt)
- Add nolint:gosec for BackupPVCSecretLabel constant (not a credential)
- Use assert.Error instead of assert.True(err != nil) (testifylint)
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add label-based secret cleanup in CleanUp() to delete any secrets
that were copied to the Velero namespace for backup PVC provisioning.
Uses the velero.io/backup-pvc-secret label to find secrets associated
with the DataUpload being cleaned up.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Copy configured secrets from the source namespace to the Velero
namespace in the New phase of the DataUpload reconcile loop, before
calling Expose(). This is done in the controller rather than the
exposer because Expose() errors are non-retryable (marked as permanent
failure), while the controller can requeue on collision.
On secret collision (same name, different data from another
DataUpload), the controller requeues with a 5s delay, matching the
existing pattern used for VGDP constraint checking.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add CopySecret, DeleteSecretIfAny, and DeleteSecretsWithLabel utilities
for copying namespace-scoped secrets to the Velero namespace during
datamover backup PVC creation.
CopySecret handles three cases:
- Secret does not exist in target: copies it with a tracking label
- Secret exists with same data: no-op (same source namespace)
- Secret exists with different data: returns ErrSecretCollision so the
caller can requeue
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
Add a SecretNames field to the BackupPVC type to allow users to specify
secrets that need to be copied from the source PVC namespace to the
Velero namespace before creating the backup PVC. This is needed for CSI
drivers that require namespace-scoped secrets for volume provisioning,
such as encrypted volumes with KMS.
Fixes#9879
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
This commit updates the func buildFinalTarball so it won't use
io.ReadAll, in order to optimize memory usage.
Signed-off-by: Daniel Jiang <daniel.jiang@broadcom.com>
Wenkai Yin(尹文开)
lyndon-liandGitHub
Shubham PampattiwarandGitHub
Xun Jiang/Bruce JiangandGitHub
R4mboandGitHub
PranjalandGitHub
Xun Jiang
Krishna AwasthiandGitHub
Chlins ZhangandGitHub
Lyndon-Li
Joseph Antony VaikathandGitHub