Skip to content

Pass DATAMOVER_IMAGE env var to kubevirt-datamover-controller deployment#2172

Open
shubham-pampattiwar wants to merge 1 commit intoopenshift:oadp-devfrom
shubham-pampattiwar:fix/pass-datamover-image-env-var
Open

Pass DATAMOVER_IMAGE env var to kubevirt-datamover-controller deployment#2172
shubham-pampattiwar wants to merge 1 commit intoopenshift:oadp-devfrom
shubham-pampattiwar:fix/pass-datamover-image-env-var

Conversation

@shubham-pampattiwar
Copy link
Copy Markdown
Member

@shubham-pampattiwar shubham-pampattiwar commented Apr 21, 2026

Summary

  • Sets DATAMOVER_IMAGE env var on the kubevirt-datamover-controller deployment so mover pods use the same resolved image as the controller
  • The value respects the existing 3-tier priority: unsupportedOverrides > RELATED_IMAGE env > default
  • Adds test coverage verifying the env var is set correctly across all image override scenarios

Fixes: migtools/kubevirt-datamover-controller#53

Test plan

  • Unit tests pass (TestEnsureKubevirtDatamoverRequiredSpecs, TestBuildKubevirtDatamoverDeployment)
  • Verified DATAMOVER_IMAGE value matches resolved image for default, unsupportedOverrides, and env var cases
  • E2E: deploy with unsupportedOverrides["kubevirtDatamoverControllerImageFqin"] and verify mover pods use overridden image

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores

    • Improved kubevirt-datamover container configuration with image reference setup.
  • Tests

    • Extended test coverage to validate environment variable injection in datamover container configuration.

The kubevirt-datamover-controller already supports the DATAMOVER_IMAGE
env var to configure the image used for mover pods, but the OADP operator
was not setting it. This caused mover pods to always use the hardcoded
default image even when the controller image was overridden via
unsupportedOverrides.

Fixes: migtools/kubevirt-datamover-controller#53

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 21, 2026

Walkthrough

The changes add the DATAMOVER_IMAGE environment variable to the kubevirt-datamover controller deployment, injecting the resolved controller image address so spawned mover pods use the correct image when overridden.

Changes

Cohort / File(s) Summary
Controller logic
internal/controller/kubevirt_datamover_controller.go
Added DATAMOVER_IMAGE environment variable injection with the resolved controller image value in ensureKubevirtDatamoverRequiredSpecs().
Controller tests
internal/controller/kubevirt_datamover_controller_test.go
Updated test expectations to account for the new DATAMOVER_IMAGE env var; increased expected env counts by 1 and added explicit assertions verifying the variable is present with correct values across test scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 10 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning Ginkgo tests lack meaningful failure messages on gomega assertions, violating requirement #4 for assertion messages. Add meaningful failure messages to all gomega assertions by including a second argument string parameter to improve test maintainability and debugging.
✅ Passed checks (10 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: passing the DATAMOVER_IMAGE environment variable to the kubevirt-datamover-controller deployment.
Linked Issues check ✅ Passed The PR fully addresses issue #53 requirements: adds DATAMOVER_IMAGE env var with correct resolution priority [#53], implements changes in the specified files [#53], and includes test coverage [#53].
Out of Scope Changes check ✅ Passed All changes are directly related to the objective of adding DATAMOVER_IMAGE environment variable to the kubevirt-datamover-controller deployment; no out-of-scope modifications detected.
Stable And Deterministic Test Names ✅ Passed All 13 test names in the file use static, descriptive strings without dynamic content such as timestamps, generated IDs, or pod/namespace names.
Microshift Test Compatibility ✅ Passed This PR modifies only standard Go unit tests, not Ginkgo e2e tests; the custom check for new Ginkgo tests does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed This PR does not add new Ginkgo e2e tests. The changes are to unit tests in internal/controller/kubevirt_datamover_controller_test.go that use the envtest framework, not e2e tests deployed against real clusters.
Topology-Aware Scheduling Compatibility ✅ Passed This PR adds a DATAMOVER_IMAGE environment variable with no topology-incompatible scheduling constraints introduced.
Ote Binary Stdout Contract ✅ Passed Pull request changes are limited to the ensureKubevirtDatamoverRequiredSpecs function and associated unit tests, adding only an environment variable to a deployment specification without emitting stdout output.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR modifies unit tests, not Ginkgo e2e tests. The check specifically targets new Ginkgo e2e tests which are not present.
Description check ✅ Passed The PR description provides a clear summary of changes, explains the 3-tier priority logic, mentions test coverage, and references the fixed issue.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci Bot requested review from mrnold and sseago April 21, 2026 22:09
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 21, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shubham-pampattiwar

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [shubham-pampattiwar]

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 Apr 21, 2026
@shubham-pampattiwar
Copy link
Copy Markdown
Member Author

/cherry-pick oadp-1.6

@openshift-cherrypick-robot
Copy link
Copy Markdown
Contributor

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

Details

In response to this:

/cherry-pick oadp-1.6

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.

@shubham-pampattiwar
Copy link
Copy Markdown
Member Author

/retest

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 22, 2026

@shubham-pampattiwar: The following test 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.23-e2e-test-aws 4d638e8 link false /test 4.23-e2e-test-aws

Full PR test history. Your PR dashboard.

Details

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.

Value: deploymentObject.Namespace,
},
{
Name: "DATAMOVER_IMAGE",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we have KUBEVIRT_DATAMOVER_IMAGE ? to not confuse with DM ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The env var lives on the kubevirt-datamover-controller deployment, so DATAMOVER_IMAGE is unambiguous in that context, adding a KUBEVIRT_ prefix would be redundant. But I get your point. Happy to rename if you still prefer it though.

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.

Pass DATAMOVER_IMAGE env var to kubevirt-datamover-controller deployment

3 participants