Skip to content

Conversation

@kaovilai
Copy link
Member

@kaovilai kaovilai commented Nov 10, 2025

Problem

When spec.configuration.velero.args is used to customize Velero server arguments, the --uploader-type flag gets lost. This happens because:

  1. --uploader-type is set via install.WithUploaderType() based on spec.configuration.nodeAgent.uploaderType
  2. When spec.configuration.velero.args is specified, veleroserver.GetArgs() completely rebuilds the args array from scratch
  3. Since --uploader-type comes from NodeAgent config (not Velero.Args), it gets overwritten

This was previously fixed in commit 3739f23 (May 2024) but regressed in commit b51ce21 (August 2024).

Solution

After calling GetArgs(), check if NodeAgent.UploaderType is configured and re-append the --uploader-type flag to preserve it.

Verification

Tested with:

go test -v -run TestDPAReconciler_buildVeleroDeployment/valid_DPA_CR_with_Velero_Args_burst_and_qps

✅ All 58 tests passed. The test case now correctly verifies that --uploader-type=kopia appears in Velero container args even when custom Velero.Args are specified.

Expected Args Order

- server
- --client-burst=321
- --client-qps=321  
- --fs-backup-timeout=4h0m0s
- --restore-resource-priorities=...
- --uploader-type=kopia  # ✅ Now preserved
- --disable-informer-cache=false

https://redhat-internal.slack.com/archives/C0144ECKUJ0/p1760474313952109

…nfiguration.args is used.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

The PR adds logic to re-append the uploader-type argument to the Velero container invocation when NodeAgent.UploaderType is configured, since GetArgs() does not include this value. The test is updated to expect this argument in the final Velero deployment configuration.

Changes

Cohort / File(s) Summary
Velero uploader-type argument handling
internal/controller/velero.go, internal/controller/velero_test.go
Implementation adds logic to re-append --uploader-type argument after GetArgs() is called, ensuring the final Velero invocation includes the configured uploader type. Test expectations updated to include --uploader-type=kopia in the expected server args list.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the argument re-append logic executes at the correct point in the Velero deployment configuration flow
  • Confirm the placement and formatting of the --uploader-type argument in the test expectations
  • Check that this change does not interfere with other argument handling or deployment scenarios
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link

openshift-ci bot commented Nov 10, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2025
@kaovilai
Copy link
Member Author

/cherry-pick oadp-1.5

@openshift-cherrypick-robot
Copy link
Contributor

@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick oadp-1.5

Instructions 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 038659f and 63b25d8.

📒 Files selected for processing (2)
  • internal/controller/velero.go (1 hunks)
  • internal/controller/velero_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • internal/controller/velero.go
  • internal/controller/velero_test.go
🔇 Additional comments (1)
internal/controller/velero.go (1)

682-689: ****

The review comment is based on an incorrect assumption about GetArgs() behavior. Verification confirms:

  1. GetArgs() (in pkg/velero/server/args.go) only processes dpa.Spec.Configuration.Velero.Args and contains no uploader-type handling whatsoever.
  2. uploader-type comes from dpa.Spec.Configuration.NodeAgent.UploaderType — a separate configuration source.
  3. The code comment at lines 682-683 correctly explains this separation.
  4. Test cases confirm the current argument ordering and inclusion of --uploader-type is expected behavior.

The duplicate check concern is unfounded since these are from different config sources, and argument ordering is correct.

Likely an incorrect or invalid review comment.

@openshift-ci
Copy link

openshift-ci bot commented Nov 10, 2025

@kaovilai: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.19-e2e-test-aws 63b25d8 link true /test 4.19-e2e-test-aws
ci/prow/4.20-e2e-test-aws 63b25d8 link false /test 4.20-e2e-test-aws

Full PR test history. Your PR dashboard.

Instructions 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants