Debugf messages named which rule tracked/skipped a namespace but not
its actual value, making the (glob-pattern) namespace filter and
label-selector matches hard to verify from logs alone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Address review feedback from adam-jian-zhang: rule b's doc comment
still described union semantics, which no longer matches the
precedence-based implementation (explicit exclusion > label-selector
match). Also hoist the repeated ShouldInclude check to the top of the
loop instead of duplicating it in each branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.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>
* Remove toleration whitelist for PodVolumeBackup and data mover pods
Instead of filtering tolerations through a hardcoded allowlist
(ThirdPartyTolerations), inherit all tolerations from the node-agent
daemonset for PodVolumeBackup/Restore and DataUpload/Download pods,
and from the Velero deployment for maintenance jobs.
This enables backups and restores on nodes with custom NoExecute taints,
which was previously impossible since only two specific toleration keys
were whitelisted.
Fixes#9476
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
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>
* Fix codespell: replace 'whitelist' with 'allowlist' in changelog
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
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>
* Implement deduplication of tolerations and add unit tests for the new function
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Merge node-agent-configmap tolerations with third-party allowlist
Add a `tolerations` field to the node-agent-configmap so operators can
declare hosting-pod tolerations explicitly, per blackpiglet's review
feedback that tolerations shouldn't be read from the DaemonSet alone.
These are merged with (and deduplicated against) DaemonSet tolerations
matching the existing third-party allowlist
(kubernetes.azure.com/scalesetpriority, CriticalAddonsOnly), restoring
that allowlist per the follow-up suggestion to keep inheriting it
alongside the new config option.
The toleration dedup helper is moved from pkg/exposer to
pkg/util/kube (exported as DeduplicateTolerations) so it can be
shared with pkg/nodeagent without an import cycle.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Fix testifylint finding in TestGetTolerations
golangci-lint v2.12.0 (pinned in pr-linter-check.yml) flagged the
shared assert.Equal after the if/else as require-error: use require
for the error assertion so each branch is self-contained, matching
the pattern used elsewhere in this file.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
* Document toleration merge priority in GetTolerations
Per blackpiglet's review feedback: clarify that configured tolerations
take priority over allowlisted daemonset tolerations because they're
appended first and DeduplicateTolerations keeps only the first
occurrence of each exact (Key, Operator, Value, Effect) combination.
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>
* test: use the Kind constant instead of the string literal
test/types.go defines `const Kind = "kind"` and most of the suite compares
against it, but three sites still use the bare string. deletion.go is
inconsistent with itself: the BeforeEach skip uses Kind while the one in
runBackupDeletionTests uses "kind", and its skip message hardcodes the
provider name where the other formats it.
namespace-mapping.go dot-imports test/e2e/test rather than test, so the
constant was not in scope there. Import the test package by name, as
test/e2e/migration/migration.go already does alongside its framework
import, and reference test.Kind.
No behavioural change: the constant's value is the string being replaced.
Signed-off-by: krishhna24 <krishhnatupedev@gmail.com>
* Add changelog for #10524
Signed-off-by: krishhna24 <krishhnatupedev@gmail.com>
---------
Signed-off-by: krishhna24 <krishhnatupedev@gmail.com>
Compare the existing PVC's capacity against the source volume size
recorded in the backup volume info (#10506) before any side effect and
skip the volume when it is too small, so the restore fails early instead
of running out of space midway. For the block data mover the source size
is the device size; for the file system data movers it is the logical
size of the backed-up files, a lower bound since file system metadata is
not accounted for.
The file system path reads the size from the volume info already carried
in RestoreData. The PVC CSI RIA has no access to the volume info, so the
restore engine carries the size on the PVC item through a Velero-internal
annotation, the same mechanism as the selected-node carrier; both carrier
annotations are stripped before the item is created in the cluster.
The check is skipped when the source size is unknown (backups taken
before it was recorded) or the PVC's capacity is not reported.
Signed-off-by: chlins <chlins.zhang@gmail.com>
The example VolumeGroupSnapshotClass cannot be applied as written. It
uses apiVersion v1alpha1, which external-snapshotter v8.2.0+ does not
serve, and nests driver and deletionPolicy under spec. Both fields are
top level and required on the CRD, which has no spec field at all.
Applying the documented example fails with:
no matches for kind "VolumeGroupSnapshotClass" in version
"groupsnapshot.storage.k8s.io/v1alpha1"
The same page already states that Velero 1.18.1+ uses the v1beta2 API
and requires external-snapshotter v8.2.0 or later, so the example also
contradicts its own prerequisites section.
Verified against a kind cluster running external-snapshotter v8.6.0:
the corrected example passes kubectl apply --dry-run=server, and the
CRD reports driver and deletionPolicy as top level and required across
v1, v1beta1 and v1beta2.
Signed-off-by: krishhna24 <krishhnatupedev@gmail.com>
#10506 added SourceSize to the PodVolumeBackup and DataUpload status
types but did not regenerate the CRD manifests. Without the field in the
CRD schema, the API server silently drops status.sourceSize on write, so
the recorded source size never reaches the cluster objects.
Generated with hack/update-3generated-crd-code.sh (controller-gen
v0.16.5).
Signed-off-by: chlins <chlins.zhang@gmail.com>
Tiger KaovilaiandClaude Sonnet 5
Wenkai Yin(尹文开)andGitHub
Xun Jiang/Bruce JiangandGitHub
lyndon-liandGitHub
Lyndon-Li
Chlins ZhangandGitHub
Mustafa Senoglu
mrchatam
Abhayraj JaiswalandGitHub
Daniel Mungai
KrishhnaTandGitHub
Wenkai Yin (尹文开)