feat: expose staleDataUploadThreshold in DPA KubevirtDatamoverConfig - #2361
feat: expose staleDataUploadThreshold in DPA KubevirtDatamoverConfig#2361shubham-pampattiwar wants to merge 1 commit into
Conversation
Add staleDataUploadThreshold field to KubevirtDatamoverConfig so the OADP operator passes --stale-dataupload-threshold to the kubevirt-datamover-controller deployment. This allows users to configure how long a stuck DataUpload can block future backups for the same VM before being skipped. Companion to migtools/kubevirt-datamover-controller#172. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe change adds an optional stale DataUpload threshold to KubeVirt DataMover configuration, exposes it in the CRD schemas, passes it to the DataMover container, and adds controller tests for argument handling. ChangesKubeVirt stale threshold
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/kubevirt_datamover_controller.go`:
- Around line 243-246: The controller currently appends the unsupported
--stale-dataupload-threshold argument in the DPA argument-building flow; remove
this append block unless the kubevirt-datamover-controller manager is updated to
declare the flag and its 2h default first. Ensure configured
StaleDataUploadThreshold values no longer produce an unknown manager flag.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64d44322-69f8-478a-b35f-2358e55d489e
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
api/v1alpha1/dataprotectionapplication_types.gobundle/manifests/oadp.openshift.io_dataprotectionapplications.yamlconfig/crd/bases/oadp.openshift.io_dataprotectionapplications.yamlinternal/controller/kubevirt_datamover_controller.gointernal/controller/kubevirt_datamover_controller_test.go
| if dpa.Spec.Configuration.KubevirtDatamover.StaleDataUploadThreshold != nil { | ||
| args = append(args, fmt.Sprintf("--stale-dataupload-threshold=%s", | ||
| dpa.Spec.Configuration.KubevirtDatamover.StaleDataUploadThreshold.Duration.String())) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate the flag declaration and all operator-side references.
rg -n -C 5 --glob '*.go' \
-e 'stale-dataupload-threshold|staleDataUploadThreshold|StaleDataUploadThreshold' .
# Locate the manager image/version source used by this operator.
rg -n -C 3 \
--glob '*.go' --glob '*.yaml' --glob '*.yml' --glob 'go.mod' --glob 'go.sum' \
-e 'kubevirt-datamover' .Repository: openshift/oadp-operator
Length of output: 50379
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the exact deployment argument wiring and image resolution without
# emitting the repository-wide search results that were truncated.
sed -n '200,275p' internal/controller/kubevirt_datamover_controller.go
sed -n '350,395p' internal/controller/kubevirt_datamover_controller.go
rg -n -C 4 --glob '*.go' --glob '*.yaml' --glob '*.yml' \
-e 'RELATED_IMAGE_KUBEVIRT_DATAMOVER_CONTROLLER|kubevirt-datamover-controller:[^[:space:]]+' .
# Check whether the manager source is vendored or referenced through a
# dependency, and query the public source tree for the flag declaration.
git ls-files | rg '(^|/)(vendor|go.mod|go.sum|Dockerfile|Makefile|Containerfile|.*kubevirt.*datamover.*)$' || true
for url in \
'https://api.github.com/search/code?q=stale-dataupload-threshold+in:file' \
'https://api.github.com/repos/konveyor/kubevirt-datamover/contents' \
'https://api.github.com/repos/kubevirt/kubevirt-datamover/contents'
do
echo "=== $url"
curl -LfsS --max-time 20 "$url" 2>/dev/null | head -c 12000 || true
printf '\n'
doneRepository: openshift/oadp-operator
Length of output: 14654
🌐 Web query:
kubevirt-datamover-controller "stale-dataupload-threshold"
💡 Result:
The stale-dataupload-threshold refers to a proposed configuration mechanism within the kubevirt-datamover-controller designed to identify and ignore "stuck" DataUpload (DU) resources [1]. Because Velero does not apply its standard DataUpload timeout to custom data movers, an orphaned or stalled DataUpload that remains indefinitely in an active phase (e.g., Accepted, Prepared, or InProgress) can block all subsequent backups for the same Virtual Machine [1]. To resolve this, a staleness threshold was proposed to allow the controller to recognize when a blocking DataUpload has exceeded a configurable duration—such as 30 minutes—and subsequently ignore it [1]. This ensures that younger, potentially healthy DataUploads are not permanently blocked by a stale process [1]. It is important to distinguish this from standard Velero timeouts: - The kubevirt-datamover-controller mechanism targets specific issues with per-VM serialization where one stuck DU prevents others from proceeding [1]. - Standard Velero backups use parameters like --item-operation-timeout (default 4 hours) and data-mover-prepare-timeout (default 30 minutes) to manage general DataUpload lifecycle timeouts [2]. As of the current development context, this threshold is identified as a solution to prevent indefinite backup blocking for VMs [1]. Users managing KubeVirt backups via this controller should be aware that this specific threshold is intended to provide a safety net where Velero's default mechanisms do not apply [1].
Citations:
- 1: Add staleness check for per-VM DataUpload serialization migtools/kubevirt-datamover-controller#30
- 2: https://velero.io/docs/main/csi-snapshot-data-movement/
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
repo='migtools/kubevirt-datamover-controller'
echo '=== repository metadata ==='
curl -LfsS --max-time 20 "https://api.github.com/repos/$repo" |
jq '{full_name,default_branch,archived,updated_at}'
branch=$(curl -LfsS --max-time 20 "https://api.github.com/repos/$repo" |
jq -r '.default_branch')
echo "=== tree: $branch ==="
curl -LfsS --max-time 20 \
"https://api.github.com/repos/$repo/git/trees/$branch?recursive=1" |
jq -r '.tree[] | select(.path|test("\\.(go|yaml|yml|md)$")) | .path' |
head -200
echo '=== flag references in default-branch source ==='
curl -LfsS --max-time 20 \
"https://api.github.com/repos/$repo/git/trees/$branch?recursive=1" |
jq -r '.tree[] | select(.type=="blob" and (.path|test("\\.go$"))) | .url' |
while read -r blob; do
curl -LfsS --max-time 20 "$blob" |
jq -r '.content // empty' | base64 -d 2>/dev/null |
grep -n -C 5 -E 'stale-dataupload-threshold|staleDataUploadThreshold' || true
done
echo '=== issue 30 ==='
curl -LfsS --max-time 20 "https://api.github.com/repos/$repo/issues/30" |
jq '{title,state,body,html_url}'Repository: openshift/oadp-operator
Length of output: 240
🌐 Web query:
site:github.com/migtools/kubevirt-datamover-controller "stale-dataupload-threshold"
💡 Result:
The term stale-dataupload-threshold refers to a proposed or implemented configuration parameter within the kubevirt-datamover-controller project [1][2]. It is used to define a duration of time after which an active DataUpload resource that appears stuck is considered stale [1]. In the context of the kubevirt-datamover-controller, the controller implements per-VM serialization of DataUploads, where newer DataUploads are requeued if an older one for the same VM is already in an active phase [1]. If a DataUpload becomes stuck in an active phase (for example, due to a controller crash or an orphaned process), it can indefinitely block all future backups for that VM because Velero's built-in DataUpload timeouts do not apply to these custom data movers [1]. To mitigate this, the stale-dataupload-threshold allows the controller to ignore such blocking DataUploads if they have remained in their current active phase longer than the configured duration [1]. When a DataUpload is identified as having exceeded this threshold, the controller can proceed to ignore or transition it to a terminal state, such as Failed, thereby preventing it from blocking subsequent backups [1]. The implementation suggested in technical discussions uses this threshold to skip DataUploads whose creation timestamp plus the defined duration is older than the current time [1].
Citations:
- 1: Add staleness check for per-VM DataUpload serialization migtools/kubevirt-datamover-controller#30
- 2: https://github.com/migtools/kubevirt-datamover-controller/issues
🌐 Web query:
site:github.com/migtools/kubevirt-datamover-controller "StaleDataUploadThreshold"
💡 Result:
StaleDataUploadThreshold is a configuration parameter (typically a duration) proposed for the migtools/kubevirt-datamover-controller [1][2]. It is intended to manage the lifecycle of DataUpload resources by identifying and handling potentially orphaned or stuck operations [1]. In the context of the KubeVirt Data Mover for Velero, the controller implements per-VM serialization of DataUploads to prevent concurrent operations on the same virtual machine [1]. If an older DataUpload remains stuck in an active phase (such as Accepted, Prepared, or InProgress) indefinitely—potentially due to a controller crash or orphaned process—subsequent younger DataUploads for that same VM would be requeued indefinitely [1]. The StaleDataUploadThreshold is proposed as a mechanism to address this by allowing the controller to ignore a blocking DataUpload if it has remained in its current active phase for longer than this specified threshold [1]. By treating such operations as "stale," the controller can bypass the blockage and potentially transition the stuck resource to a failed state, thereby preventing future backups for that VM from being blocked indefinitely [1].
Citations:
- 1: Add staleness check for per-VM DataUpload serialization migtools/kubevirt-datamover-controller#30
- 2: https://github.com/migtools/kubevirt-datamover-controller/issues
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Use read-only Git transport and raw file retrieval because the GitHub API
# rate limit rejected the metadata request.
for remote in \
https://github.com/migtools/kubevirt-datamover-controller.git \
https://github.com/konveyor/kubevirt-datamover-controller.git
do
echo "=== $remote refs ==="
git ls-remote --heads --tags "$remote" 2>&1 | head -80 || true
done
for path in \
cmd/main.go \
main.go \
controller/main.go \
internal/controller/main.go \
pkg/controller/main.go
do
for branch in main master; do
url="https://raw.githubusercontent.com/migtools/kubevirt-datamover-controller/$branch/$path"
body=$(curl -LfsS --max-time 20 "$url" 2>/dev/null || true)
if [ -n "$body" ]; then
echo "=== $url ==="
printf '%s\n' "$body" | grep -n -C 8 \
-E 'stale-dataupload-threshold|staleDataUploadThreshold|flag\.Duration|2h' || true
fi
done
doneRepository: openshift/oadp-operator
Length of output: 2335
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
repo='https://github.com/migtools/kubevirt-datamover-controller.git'
echo '=== default branch and tags ==='
git ls-remote --symref "$repo" HEAD
git ls-remote --tags "$repo" | tail -40
branch=$(git ls-remote --symref "$repo" HEAD | awk '$1=="ref:" {sub("refs/heads/","",$2); print $2}')
[ -n "$branch" ] || branch=oadp-dev
echo "=== source files in $branch ==="
curl -LfsS --max-time 30 \
"https://codeload.github.com/migtools/kubevirt-datamover-controller/tar.gz/refs/heads/$branch" |
tar -tzf - | grep -E '\.(go|yaml|yml|md)$' | head -250
echo "=== flag and default references in $branch ==="
curl -LfsS --max-time 30 \
"https://codeload.github.com/migtools/kubevirt-datamover-controller/tar.gz/refs/heads/$branch" |
tar -xzf - --to-stdout --wildcards '*/go.mod' '*/main.go' '*/cmd/*.go' '*/internal/**/*.go' '*/pkg/**/*.go' 2>/dev/null |
grep -n -C 8 -E \
'stale-dataupload-threshold|staleDataUploadThreshold|flag\.Duration|2h' || trueRepository: openshift/oadp-operator
Length of output: 5167
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
repo='https://github.com/migtools/kubevirt-datamover-controller.git'
branch='oadp-dev'
echo '=== manager entry point ==='
curl -LfsS --max-time 30 \
"https://codeload.github.com/migtools/kubevirt-datamover-controller/tar.gz/refs/heads/$branch" |
tar -xzf - --to-stdout "kubevirt-datamover-controller-$branch/cmd/main.go" |
cat -n
echo '=== all Go-source references on the default branch ==='
curl -LfsS --max-time 30 \
"https://codeload.github.com/migtools/kubevirt-datamover-controller/tar.gz/refs/heads/$branch" |
tar -xzf - --to-stdout --wildcards \
"kubevirt-datamover-controller-$branch/*.go" \
"kubevirt-datamover-controller-$branch/**/*.go" 2>/dev/null |
grep -n -C 5 -E \
'stale-dataupload-threshold|staleDataUploadThreshold|flag\.Duration|2h' || true
echo '=== flag references across all remote branches ==='
git ls-remote --heads "$repo" |
awk '{sub("refs/heads/","",$2); print $2}' |
while IFS= read -r branch; do
encoded_branch=$(printf '%s' "$branch" | sed 's#/#%2F#g')
matches=$(
curl -LfsS --max-time 30 \
"https://codeload.github.com/migtools/kubevirt-datamover-controller/tar.gz/refs/heads/$encoded_branch" |
tar -xzf - --to-stdout --wildcards \
"kubevirt-datamover-controller-$branch/*.go" \
"kubevirt-datamover-controller-$branch/**/*.go" 2>/dev/null |
grep -n -E 'stale-dataupload-threshold|staleDataUploadThreshold|flag\.Duration|2h' || true
)
if [ -n "$matches" ]; then
echo "=== branch: $branch ==="
printf '%s\n' "$matches"
fi
doneRepository: openshift/oadp-operator
Length of output: 13396
Do not pass an unsupported manager flag.
The kubevirt-datamover-controller manager does not declare --stale-dataupload-threshold or a 2h default. When configured, this argument causes manager startup to fail with an unknown-flag error. Add the flag and default to the manager first, or remove this operator argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/kubevirt_datamover_controller.go` around lines 243 - 246,
The controller currently appends the unsupported --stale-dataupload-threshold
argument in the DPA argument-building flow; remove this append block unless the
kubevirt-datamover-controller manager is updated to declare the flag and its 2h
default first. Ensure configured StaleDataUploadThreshold values no longer
produce an unknown manager flag.
|
@shubham-pampattiwar: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shubham-pampattiwar, sseago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Summary
staleDataUploadThresholdfield toKubevirtDatamoverConfigin the DPA spec--stale-dataupload-thresholdto the kubevirt-datamover-controller deployment when configuredspec.configuration.kubevirtDatamover.staleDataUploadThresholdCompanion to migtools/kubevirt-datamover-controller#172 (adds the controller flag).
Fixes migtools/kubevirt-datamover-controller#30.
Example DPA Config
Files Changed
api/v1alpha1/dataprotectionapplication_types.go-- newStaleDataUploadThreshold *metav1.Durationfieldapi/v1alpha1/zz_generated.deepcopy.go-- regeneratedconfig/crd/bases/+bundle/manifests/-- CRD schema updatedinternal/controller/kubevirt_datamover_controller.go-- arg wiringinternal/controller/kubevirt_datamover_controller_test.go-- 3 test cases (configured, not configured, update existing)Test plan
go build ./...passesgo test ./internal/controller/ -run TestEnsureKubevirtDatamoverRequiredSpecs -v-- all 14 tests passgo vet ./...cleanstaleDataUploadThreshold: 1hin DPA, verify controller pod has--stale-dataupload-threshold=1h0m0sarg🤖 Generated with Claude Code
Summary by CodeRabbit