Fix datamover backup arg mismatch for CSI CBT service account name - #10318
Draft
kaovilai wants to merge 2 commits into
Draft
Fix datamover backup arg mismatch for CSI CBT service account name#10318kaovilai wants to merge 2 commits into
kaovilai wants to merge 2 commits into
Conversation
The exposer built the pod command with --csi-snapshot-metadata-service-sa, but the datamover backup command only registered --cbt-sa-name. cobra rejects unknown flags, so the data mover pod exited immediately whenever a dedicated CBT service account was configured -- and the reverse also held: since the flags never matched, the SA name never actually reached the pod, so any code path depending on it stayed unreachable. Not limited to the block data mover: this line sits outside the DataMoverTypeVeleroBlock gate and the cbtInfo != nil gate, so it fires for any CSI snapshot data-movement backup. Fix: emit --cbt-sa-name (already consumed by the backup command), naming it consistently with the other CBT flags on the same line (--change-id, --volume-id, --snapshot-id). Add a regression test asserting the emitted flag string parses cleanly against NewBackupCommand's own flag set, so the two sides can't drift apart again without a test failure. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
✅ Deploy Preview for velero canceled.
|
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Responses generated with Claude
Does your change fix a particular issue?
Fixes #10298
Summary
The exposer built the datamover backup pod's command with
--csi-snapshot-metadata-service-sa, but the datamover backup command only registered--cbt-sa-name. cobra rejects unknown flags, so the data mover pod exited immediately whenever a dedicated CBT service account was configured (csiSnapshotMetadataServiceConfigs.saNameset in the node-agent ConfigMap) — and the reverse also held: since the two sides never agreed on a flag name, the SA name never actually reached the pod, socbtservice.ServiceImpl.SAName/SANamespace(pkg/cbtservice/csi_service_impl.go:89-93,119-123) were unreachable regardless of configuration.Not limited to the block data mover. The line that emits this flag (
pkg/exposer/csi_snapshot.go:746) sits outside both theDataMoverTypeVeleroBlockgate (:266-272) and thecbtInfo != nilgate (:735-739), so it fires for any CSI snapshot data-movement backup, not justvelero-block.Fix
Emit
--cbt-sa-name— already registered and consumed byNewBackupCommand(pkg/cmd/cli/datamover/backup.go:104, plumbed through topkg/cbtserviceat:217) — instead of the flag no consumer ever listened for. This also brings the naming in line with the other CBT flags built on adjacent lines (--change-id,--volume-id,--snapshot-id).Test
TestBackupPodCBTServiceSAFlagMatchesDatamoverBackupFlagsasserts the flag string the exposer builds parses cleanly againstNewBackupCommand's own flag set and round-trips the SA name correctly — so the two sides can't silently drift apart again the way they did here.Please indicate you have done the following:
make new-changelog) — will add once this PR has a number.site/content/docs/main— n/a, no user-facing flag change; this fixes an internal pod-to-pod arg contract.