* Add e2e test for namespace selection by label in resource policy
Covers design step 8 of #9772: a backup with no explicit
--include-namespaces (the same shape a Schedule with no
includedNamespaces produces), relying entirely on a ResourcePolicy
ConfigMap's includedNamespacesByLabel to select which namespaces to
back up.
Creates labeled and unlabeled namespaces, backs up with a
ResourcePolicy ConfigMap setting includedNamespacesByLabel, and
verifies only the labeled namespaces are restored - closing the e2e
coverage gap #10275 deferred to velero-io/velero#10564.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Add changelog entry for e2e test PR
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix e2e test: create Backup directly, not via CLI's implicit --include-namespaces=*
The kind e2e run showed every unlabeled namespace getting backed up
and restored anyway. velero backup create's --include-namespaces flag
defaults to ["*"] when omitted (pkg/cmd/cli/backup/create.go), so
skipping the flag still sent an *explicit* wildcard - and
mergeNamespacesByLabel deliberately leaves an explicit "*" untouched
rather than narrowing it, so includedNamespacesByLabel never got a
chance to replace anything.
Create the Backup object directly via the controller-runtime client
instead, leaving BackupSpec.IncludedNamespaces genuinely unset - the
only way to exercise the "defaulted empty" narrowing path the CLI's
own default makes unreachable.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit fixes the issue that when a namespace with label velero.io/exclude-from-backup: "true"
is added to the exludeNamespaces of a backup CR. There will be
duplicated entries of the namespace in the spec of the backup.
Signed-off-by: Daniel Jiang <daniel.jiang@broadcom.com>
Links to plugin-versioning.md and general-progress-monitoring.md broke when
those docs moved to design/Implemented/; two other relative paths had one
directory level wrong.
Signed-off-by: Zain <43629888+ZainnQureshii@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Add the maintainer information required by the CNCF Incubation criteria:
- State that all maintainers share collective responsibility for the entire
project, consistent with CODEOWNERS assigning ownership to the maintainer
group as a whole (no siloed per-area owners).
- Add a Contacting the maintainers section listing GitHub, Slack, the mailing
list, and the security disclosure process.
This makes MAINTAINERS.md cover names, contact information, domain of
responsibility, and affiliation as required for the Incubation application.
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
* Fix backup-finalizer: do not set backup phase to Completed before PutBackupMetadata succeeds
Previously, the backup finalizer controller set backup.Status.Phase to
Completed/PartiallyFailed in-memory BEFORE calling PutBackupMetadata and
PutBackupContents. When these uploads failed (e.g., due to object lock
or immutability), the deferred patch function still wrote the terminal
phase to the Kubernetes API server, preventing the controller from
retrying the upload on the next reconcile.
This fix moves the phase assignment to AFTER both uploads succeed. A
DeepCopy of the backup is used to encode the JSON with the final phase
for object storage, while the in-memory backup object retains the
Finalizing phase until uploads complete.
Caveats:
- CompletionTimestamp is now captured before upload but only committed to
the API server after upload succeeds. On retry after a transient
failure, a new timestamp is generated, so the completion time reflects
when the upload finally succeeded rather than when finalization
processing completed.
- Metrics (RegisterBackupSuccess/RegisterBackupPartialFailure) are now
recorded after uploads succeed, so they accurately reflect only fully
persisted backups.
- The metadata uploaded to object storage contains the final phase and
completion timestamp via DeepCopy, so storage state is correct even
before the API server is patched.
Fixes#9645
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Add changelog for #9646
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix testifylint: use require.Error instead of assert.Error
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Address review feedback on backup-finalizer fix
- Add default guard for unhandled phase values in finalPhase switch
- Add retry with DefaultBackoff for PutBackupMetadata per reviewer request
- Replace brittle framework.BackupItemActionResolverV2{} mock with mock.Anything
- Add FinalizingPartiallyFailed test case for PutBackupContents failure
Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Use bounded, object-storage-tuned backoff for backup-finalizer uploads
retry.DefaultBackoff is tuned for API server optimistic-concurrency
conflicts (4 steps, ~1.25s total) and gives up far too quickly for
object storage calls, which can see longer transient outages or
throttling (review feedback from blackpiglet). Replace it with a
dedicated, bounded backoff (1s base, 2x factor, 5 steps, ~31s total)
applied to both PutBackupMetadata and PutBackupContents.
Being bounded (rather than retrying forever) means a persistent
failure, e.g. an object-lock/immutability policy denying every write,
surfaces as an error within a bounded time instead of hanging the
reconcile indefinitely; controller-runtime requeues on error, so
retries continue across reconciles (review feedback from priyansh17).
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix PutBackupMetadata retry to re-read backupJSON each attempt
backupJSON is a bytes.Buffer, so passing it directly to
PutBackupMetadata drains it on the first read attempt. A retry after
a transient failure would then upload empty content instead of the
backup metadata. Wrap it in bytes.NewReader(backupJSON.Bytes()) inside
the retry closure so every attempt gets a fresh reader.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>
Update Request struct initialization in backup_test.go to use
SkippedVolumeTracker and NewSkipVolumeTracker following the rename
from SkippedPVTracker.
Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>
PodVolumeRestores are only created for pods that Velero creates, so when
the pod already exists in the cluster the PVC-not-in-use pre-flight check
never runs and the volume data restore is skipped silently, while the
existing pod keeps consuming the PVC. Report an explicit pre-flight error
for such pods, aligned with the PVC CSI RIA behavior.
Signed-off-by: chlins <chlins.zhang@gmail.com>
CI's changelog check requires a changelogs/unreleased/<PR#>-<login>
file; this PR didn't have one since the PR number wasn't known until
after it was opened.
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Motivation:
Forget and BatchForget in pkg/repository/manager/manager.go accept a
caller-provided context.Context but ignore it, hardcoding
context.Background() when calling into the repository provider. This
means cancellation and timeouts set by callers (e.g. the backup
deletion controller) are silently dropped during repository connection
and snapshot deletion. Additionally, BatchForget returned a wrapped nil
instead of the real connection error when prd.BoostRepoConnect failed,
because it referenced an unrelated, already-nil err variable instead of
connectErr.
Approach:
Pass the caller's ctx through to prd.BoostRepoConnect, prd.Forget, and
prd.BatchForget in both Forget and BatchForget, instead of substituting
context.Background(). Fix BatchForget's connection-failure branch to
wrap and return connectErr instead of the stale err. Other methods on
manager (InitRepo, ConnectToRepo, PrepareRepo, PruneRepo, UnlockRepo)
don't accept a ctx parameter at all, so they are unaffected and out of
scope for this change.
Validation:
- go build ./pkg/repository/... and go build ./... pass.
- go vet ./pkg/repository/... is clean.
- go test ./pkg/repository/... passes, including three new tests added
to pkg/repository/manager/manager_test.go.
- golangci-lint run ./pkg/repository/... is clean.
- Confirmed the new tests reproduce both bugs: temporarily reverting
only manager.go and re-running go test ./pkg/repository/manager/...
made all three new tests fail (missing propagated context value and
cancellation, and a nil error returned where the real connect error
was expected); re-applying the fix makes them pass. This is a silent
behavior bug (broken context propagation and a swallowed error), not
a crash.
Report: https://github.com/velero-io/velero/issues/10551
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
velero schedule create registers --annotations through BackupOptions.BindFlags,
but the Schedule ObjectMeta it builds only sets Labels, so the flag was accepted
and silently discarded.
This matters beyond the Schedule object itself: BackupBuilder.FromSchedule
falls back to schedule.Annotations when the template carries none, so every
backup generated by the schedule lost the annotations too. velero schedule
describe already prints these fields and the Schedule CRD already carries them,
so the create path was the only gap.
Same shape as #10526, which fixed the backup type being dropped on the same
struct literal.
Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com>
Add includedNamespacesByLabel, excludedNamespacesByLabel, and
labelSelectorLogic to IncludeExcludePolicy in the ResourcePolicy
ConfigMap (realizes design in velero-io/velero#9772), letting a backup
select or exclude namespaces by label instead of (or in addition to)
name/wildcard.
The backup controller resolves label selectors against the live
namespace list once per backup, merges the results into
spec.includedNamespaces/excludedNamespaces, then proceeds through the
existing name-based filtering unchanged. A defaulted "*" include list
is replaced by the resolved set; an explicitly-configured include list
(including an explicit "*") is unioned with it instead, and stays
canonical rather than widening. Namespaces matching an exclude
selector are always subtracted from the merged includes, regardless of
how the includes were populated.
Because Velero's namespace-includes/excludes model requires at least
one name (an empty list means "match everything"), a selector that
resolves to zero namespaces is represented with a sentinel glob
pattern ("[-]*") guaranteed to match no real namespace, rather than an
empty list that would silently fall back to including/excluding
everything.
labelSelectorLogic ("AND"/"OR", case-insensitive) controls whether
multiple included/excluded label selectors are combined by
intersection or union; it is validated up front, including inside
ResolveNamespacesByLabel itself, so an invalid value fails fast instead
of silently falling through to OR semantics.
Namespace-selection-by-label and resource-selection-by-label act as
independent axes and do not affect each other, matching the design
discussion in #9772.
Known limitations:
- Selectors are evaluated once per backup against the namespace list
at that point in time; namespaces created or relabeled mid-backup
are not picked up.
- Backup-only for now; restore-side namespace mapping is unaffected.
Testing:
- Unit coverage in internal/resourcepolicies for validation, selector
resolution (including AND/OR logic, case-insensitivity, and
malformed-selector/invalid-logic error paths), and the no-match
sentinel.
- Unit coverage in pkg/controller for the merge logic between resolved
label selections and explicit/defaulted includes and excludes.
- End-to-end coverage in pkg/backup exercising the full backup
pipeline with label-selected namespaces, including the
velero.io/exclude-from-backup hard-exclusion interaction and the
zero-match/fully-excluded sentinel path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Modify according to comments.
* Rename the pvSkipTracker and related fields to indicate both PVC and PV are supported.
Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
- Added cross-references in troubleshooting.md and file-system-backup.md to the Write Sparse files documentation
- Documented how to use --write-sparse-files flag when restores fail due to disk space constraints
- Clarified important limitation: only works if PV had sparse files during backup that would free up space
- Removed enhanced error messages and code changes (documentation-only approach per feedback)
This addresses issue #2812 by providing clear guidance to users when restores fail due to disk space constraints.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
* Design: namespace selection by label in resource policy
Extends includeExcludePolicy in the ResourcePolicy ConfigMap with
includedNamespacesByLabel and excludedNamespacesByLabel, letting users
dynamically include/exclude namespaces by Kubernetes label selector
without touching BackupSpec or schedule specs.
Covers precedence rules against BackupSpec.IncludedNamespaces/
ExcludedNamespaces (including the empty-IncludedNamespaces "all
namespaces" baseline and a configured selector matching zero
namespaces), observability via logging (backup.status.includedNamespaces
is deferred, see Open Issues), and rejected alternatives (extending
BackupSpec.LabelSelector, a new CRD field, a standalone ConfigMap,
matching the fine-grained filters' map[string]string selector shape).
resolveNamespacesByLabel resolves included/excluded selectors
independently and returns both sets uncombined; the caller merges them
against BackupSpec.IncludedNamespaces/ExcludedNamespaces via an
explicit labelIncludeActive flag, so exclude-only policies and
zero-match include selectors both behave correctly instead of
collapsing to "all namespaces".
Cross-checked against the fine-grained backup/restore filter policies
merged from issue #9448 (clusterScopedFilterPolicy, namespacedFilterPolicies
in internal/resourcepolicies/resource_policies.go): confirms no field/key
collisions, documents that a ConfigMap using these fields can't be reused
for Restore (ValidateForRestore rejects any non-nil IncludeExcludePolicy)
or the global backup volume policies ConfigMap (volumePolicies-only).
Fixes#9771
> [!Note]
> Responses generated with Claude
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Design: address review feedback from adam-jian-zhang
- Add markdown hard-line-break trailing spaces to Glossary entries so
each term renders on its own line.
- Add labelSelectorLogic ("OR" default | "AND") to IncludeExcludePolicy
instead of deferring AND-vs-OR to a future enhancement, per review
comment. AND combines entries within includedNamespacesByLabel (and
independently within excludedNamespacesByLabel) via intersection
instead of union. Threaded through resolveNamespacesByLabel,
validation, and a new worked example.
- Change observability logging from "log the full resolved namespace
list at info" to "log the count at info, full list at debug" to
avoid spamming backup logs on clusters with large namespace counts
where a selector matches a large fraction of them.
> [!Note]
> Responses generated with Claude
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Design: clarify LabelSelector interaction with label-resolved namespaces
Non-Goals previously asserted BackupSpec.LabelSelector/OrLabelSelectors
are unaffected by this design without qualification. Add the missing
trade-off: a namespace resolved into the effective set via
includedNamespacesByLabel still gets its Namespace object written even
if it doesn't separately match LabelSelector/OrLabelSelectors, mirroring
existing behavior for namespaces named explicitly in
BackupSpec.IncludedNamespaces (the nsTracker guard in
pkg/backup/item_collector.go only suppresses the Namespace object when
the namespace filter is at its default, per #7105).
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix schedule reconciler to compare SkipImmediately and LastSkipped by value
The reconciler compared spec.skipImmediately (*bool) and
status.lastSkipped (*metav1.Time) between the live object and its
DeepCopy by pointer, so both checks always reported a change once the
fields were set, and every reconcile issued a redundant Patch. Compare
by value instead (ptr.Equal / equality.Semantic.DeepEqual) and add a
regression test pinning that the skip-once flip persists with exactly
one patch, later reconciles of persisted state patch zero times, and
an explicit false is a no-op.
Signed-off-by: HeonJe LEE <lhjnano@gmail.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>
---------
Signed-off-by: HeonJe LEE <lhjnano@gmail.com>
Signed-off-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>
Co-authored-by: lyndon-li <98304688+Lyndon-Li@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The pre-flight check that verifies the existing PVC is still bound to the
backed-up volume compared PV names. A block data mover restore of a file
system volume recreates the PV under a new name (the volumeMode field is
immutable), so a second in-place restore of the same workload failed the
check even though the PVC was bound to the very same volume.
Record the CSI volume handle in the backup volume info (PVInfo) and
compare handles when both the backup and the bound PV record one; the
PV name remains the fallback for non-CSI volumes and for backups taken
before the handle was recorded. The handle reaches the PVC CSI RIA
through the same carrier annotation mechanism as the source size.
Signed-off-by: chlins <chlins.zhang@gmail.com>
* Fix backup queue permanently stuck when a dequeued backup completes during the patch
Motivation: backupQueueReconciler patched a dequeued backup to ReadyToStart and
only called backupTracker.AddReadyToStart on the next line. If backupReconciler
picked up that patch and completed the backup (e.g. immediate FailedValidation
while a BackupStorageLocation is briefly unavailable) before the queue
controller reached that line, backupReconciler's Add + deferred Delete ran
first, and the later AddReadyToStart re-inserted a tracker key nothing would
ever delete again. backupTracker is in-memory and never reconciled against
actual Backup phases, so RunningCount() stayed stuck at concurrentBackups and
every later reconcile, including the periodic recheck, was refused at that
gate -- the queue stopped dequeuing permanently until the deployment restarted.
Approach: record the backup as ReadyToStart in the tracker before patching it,
and roll that back if the patch itself fails, so the tracker entry always
exists before the backup can become visible to any other reconciler. Also
folds in two related fixes: the concurrency-refusal log line is now Info
instead of Debug so a stuck queue is visible at the default log level, and the
queue-position renumbering loop's error log (which built a logrus.Entry via
log.WithError(errors.Wrapf(...)) but never called a terminal method on it, so
it never actually logged anything) now emits properly.
Validation: go build ./pkg/controller/..., go vet ./pkg/controller/..., and
gofmt -l on both changed files are all clean. golangci-lint run
./pkg/controller/... reports no findings. go mod tidy produces a zero diff to
go.mod/go.sum, matching this repo's verify-modules check. Mirrored this repo's
own hack/test.sh invocation for this package (-short -vet=... -skip TestAPIs)
and it passes; TestAPIs is a separate envtest suite that needs a local
kubebuilder etcd binary not installed on this machine and fails identically on
an unmodified checkout, so it is a pre-existing environment gap, not a
regression. Added TestBackupQueueReconcilerTrackerNotLeakedWhenBackupCompletesDuringPatch,
which uses a controller-runtime fake client with a Patch interceptor to
simulate a racing reconciler completing the backup right after the
ReadyToStart patch lands; it fails (RunningCount leaks to 1) against the
pre-fix ordering and passes (RunningCount returns to 0) against the fix.
Report: https://github.com/velero-io/velero/issues/10519
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
* Add changelog file for PR #10521
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
* Add test covering tracker rollback when ReadyToStart patch fails
Addresses review comment: verify backupTracker.RunningCount() returns
to 0 when the ReadyToStart patch itself errors, covering the Delete
rollback path alongside the existing race-condition regression test.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
---------
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Co-authored-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Co-authored-by: Tiger Kaovilai <tkaovila@redhat.com>
* Update code to support namespace mapping when perform the in-place restore with block data mover
Update code to support namespace mapping when perform the in-p
lace restore with block data mover
Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
* Do not assume a port name is a string when clearing node ports
deleteNodePorts reads the last-applied-configuration annotation, which
is free-form JSON controlled by whoever produced the backup, and cast
p["name"] to string without checking. A port whose name is a number
crashed the restore of that Service with an interface conversion panic.
Every sibling in the same loop already uses the comma-ok form.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
* Add changelog file
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
* Convert name to string by Sprint.
Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
---------
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
Co-authored-by: Xun Jiang <xun.jiang@broadcom.com>
Keep the existing opt-in behavior (consistent with velero backup create and velero restore create, where waiting is disabled by default and enabled with --wait), and clarify it in the command help text. Add tests pinning the --wait default (false) and flag parsing.
Signed-off-by: Mustafa Senoglu <mmustafasenoglu0@gmail.com>
Daniel JiangandGitHub
Xun Jiang/Bruce JiangandGitHub
xinjun.jiang
Shubham PampattiwarandGitHub
Adam ZhangandGitHub
lyndon-liandGitHub
Chlins ZhangandGitHub
Pujitha Paladugu
Abhayraj Jaiswal
Xun Jiang
Jeremy Schoemaker
Wenkai Yin(尹文开)andGitHub
Lyndon-Li
Mustafa Senoglu