feat: add MTV operator upgrade/update migration testing#487
Merged
Conversation
Add upgrade test infrastructure: - test_upgrade_migration.py: incremental test class that creates migration resources, upgrades MTV operator, verifies post-upgrade state, and runs cold migration - utilities/upgrade.py: subprocess-based upgrade runner with fd 3 support for mtv-autodeploy logging, plus pod readiness and plan verification - Add @pytest.mark.upgrade to warm migration for post-upgrade warm test - Remove tier0 from cold migration (covered by upgrade test) - Add upgrade_script_path, mtv_upgrade_to_version, mtv_upgrade_to_source, mtv_upgrade_image_index config parameters - Add 'upgrade' marker to pytest.ini - Add MtvUpgradeError and ForkliftPodsNotRunningError exceptions
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds MTV operator upgrade testing: new ChangesMTV Operator Upgrade Test Support
Sequence Diagram(s)sequenceDiagram
participant Test as PyTest
participant Fixture as upgrade_script_path
participant Runner as run_mtv_upgrade
participant UpgradeScript as ExternalScript
participant MTV as MTV_Operator
participant OCP as OCP_API
Test->>Fixture: request upgrade script path (repo_url, repo_ref, script_path)
Fixture->>UpgradeScript: clone repo and resolve script path
Test->>Runner: call(script_path, mtv_version, mtv_source, image_index)
Runner->>UpgradeScript: execute script (env includes MTV_VERSION, MTV_SOURCE, IMAGE_INDEX, CLUSTER_*)
UpgradeScript-->>Runner: exit code or timeout (stdout/stderr)
Runner->>MTV: operator upgraded (applied by script)
MTV->>OCP: report new version
Test->>OCP: verify MTV version and Plan READY=true
Test->>OCP: execute migration and run check_vms
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/upgrade/test_upgrade_migration.py`:
- Line 203: Remove the unused fixture parameter target_namespace from the
test_check_vms signature because it is not referenced inside the test nor passed
to check_vms(); edit the test function definition (test_check_vms) to only
include the actual fixtures it uses and update any calls or references
accordingly so the test requests only necessary fixtures.
In `@utilities/upgrade.py`:
- Line 33: The Generator type annotation on function _duplicate_stdout_to_fd3 is
incomplete; change its return type from Generator[None] to Generator[None, None,
None] so the full type parameters (YieldType, SendType, ReturnType) are
specified for proper static analysis and IDE support.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fef25392-bc9f-4341-91f6-21262fdfb22b
📒 Files selected for processing (6)
exceptions/exceptions.pypytest.initests/tests_config/config.pytests/upgrade/test_upgrade_migration.pytests/warm/test_mtv_warm_migration.pyutilities/upgrade.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 `@utilities/upgrade.py`:
- Around line 42-94: Add a pre-flight validation in run_mtv_upgrade to verify
the provided script_path exists and is a file (and optionally executable) before
calling subprocess.run; if not, raise a clear exception (e.g., FileNotFoundError
or ValueError) with a message like "script_path not found: <path>" so the
failure is immediate and informative; place this check after the existing
empty-parameter guards and before constructing env/calling
_duplicate_stdout_to_fd3()/subprocess.run to ensure the function fails fast when
script_path is invalid.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b502be4-26e8-404a-a214-6f8a305403c9
📒 Files selected for processing (3)
tests/tests_config/config.pytests/upgrade/test_upgrade_migration.pyutilities/upgrade.py
redhat-qe-bot2
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Code Review by Qodo
1. Missing upgrade config keys
|
|
Persistent review updated to latest commit 0cd8377 |
Redact sensitive fields (password, token, thumbprint) in upgrade log output. Add git package to Dockerfile runtime stage for upgrade test branch operations. Document upgrade test usage in README.
|
Persistent review updated to latest commit 3d6341b |
1 similar comment
|
Persistent review updated to latest commit 3d6341b |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utilities/upgrade.py (2)
37-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Add explicit fail-fast validation for required inputs and config values.
mtv_source.upper()and env assembly assume non-Nonevalues; missing/Nonevalues will fail with indirect runtime errors instead of actionable messages. Validatescript_path,mtv_version,mtv_source, and requiredpy_configkeys before buildingenvand running subprocess.Proposed fix
def run_mtv_upgrade( script_path: str, mtv_version: str, mtv_source: str, image_index: str = "", ) -> None: @@ + if not script_path: + raise ValueError("script_path must be provided via --tc=upgrade_script_path:<path>") + if not mtv_version: + raise ValueError("mtv_version must be provided via --tc=mtv_upgrade_to_version:<version>") + if mtv_source is None or not mtv_source: + raise ValueError("mtv_source must be provided via --tc=mtv_upgrade_to_source:<source>") + + cluster_username = py_config.get("cluster_username") + cluster_password = py_config.get("cluster_password") + cluster_host = py_config.get("cluster_host") + if cluster_username is None: + raise ValueError("cluster_username must be provided via --tc=cluster_username:<username>") + if cluster_password is None: + raise ValueError("cluster_password must be provided via --tc=cluster_password:<password>") + if cluster_host is None: + raise ValueError("cluster_host must be provided via --tc=cluster_host:<api-url>") + env = os.environ.copy() env.update({ "MTV_VERSION": mtv_version, "MTV_SOURCE": mtv_source.upper(), "IMAGE_INDEX": image_index, - "CLUSTER_USERNAME": py_config["cluster_username"], - "CLUSTER_PASSWORD": py_config["cluster_password"], - "CLUSTER_API_URL": py_config["cluster_host"], + "CLUSTER_USERNAME": cluster_username, + "CLUSTER_PASSWORD": cluster_password, + "CLUSTER_API_URL": cluster_host, })As per coding guidelines: "Code must never result in
NonewhenNoneis not valid - fail early with clear errors usingif value is None:checks".🤖 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 `@utilities/upgrade.py` around lines 37 - 45, Validate inputs before building env: explicitly check that script_path, mtv_version, mtv_source, and py_config are not None (and that mtv_source is a string) and that py_config contains required keys "cluster_username", "cluster_password", and "cluster_host"; if any check fails raise a clear ValueError mentioning the missing/invalid field. Do these checks in the same scope where env is constructed (the block that references mtv_version, mtv_source, image_index and py_config) so you never call mtv_source.upper() or access py_config[...] when values may be None or missing; after validation you can safely build env as shown.
37-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCRITICAL: Do not pass the full parent environment to an external upgrade script.
Using
os.environ.copy()forwards all CI/runtime secrets to a script sourced from configurable repo/ref. Build a minimal allowlist environment to reduce credential leakage risk.Proposed fix
- env = os.environ.copy() - env.update({ + env = { "MTV_VERSION": mtv_version, "MTV_SOURCE": mtv_source.upper(), "IMAGE_INDEX": image_index, - "CLUSTER_USERNAME": py_config["cluster_username"], - "CLUSTER_PASSWORD": py_config["cluster_password"], - "CLUSTER_API_URL": py_config["cluster_host"], - }) + "CLUSTER_USERNAME": cluster_username, + "CLUSTER_PASSWORD": cluster_password, + "CLUSTER_API_URL": cluster_host, + # Optional pass-throughs only if needed by script runtime: + "PATH": os.environ.get("PATH", ""), + "HOME": os.environ.get("HOME", ""), + }Also applies to: 50-57
🤖 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 `@utilities/upgrade.py` around lines 37 - 45, Replace the os.environ.copy() usage with a minimal allowlist environment instead of forwarding the full parent env: create a new dict (e.g., env = {}) and explicitly set only the required keys (MTV_VERSION, MTV_SOURCE, IMAGE_INDEX, CLUSTER_USERNAME, CLUSTER_PASSWORD, CLUSTER_API_URL) and, if necessary, a safe PATH from os.environ.get("PATH") rather than copying everything; update both the env construction shown (the env = os.environ.copy(); env.update({...}) block) and the similar block at lines 50-57 to use this explicit allowlist approach so no other CI/runtime secrets are passed to the external upgrade script.
🤖 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.
Outside diff comments:
In `@utilities/upgrade.py`:
- Around line 37-45: Validate inputs before building env: explicitly check that
script_path, mtv_version, mtv_source, and py_config are not None (and that
mtv_source is a string) and that py_config contains required keys
"cluster_username", "cluster_password", and "cluster_host"; if any check fails
raise a clear ValueError mentioning the missing/invalid field. Do these checks
in the same scope where env is constructed (the block that references
mtv_version, mtv_source, image_index and py_config) so you never call
mtv_source.upper() or access py_config[...] when values may be None or missing;
after validation you can safely build env as shown.
- Around line 37-45: Replace the os.environ.copy() usage with a minimal
allowlist environment instead of forwarding the full parent env: create a new
dict (e.g., env = {}) and explicitly set only the required keys (MTV_VERSION,
MTV_SOURCE, IMAGE_INDEX, CLUSTER_USERNAME, CLUSTER_PASSWORD, CLUSTER_API_URL)
and, if necessary, a safe PATH from os.environ.get("PATH") rather than copying
everything; update both the env construction shown (the env = os.environ.copy();
env.update({...}) block) and the similar block at lines 50-57 to use this
explicit allowlist approach so no other CI/runtime secrets are passed to the
external upgrade script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a9a960a-de41-4c5d-9006-fd626c9d51ea
📒 Files selected for processing (3)
DockerfileREADME.mdutilities/upgrade.py
Contributor
Author
|
/verified |
|
Code review by qodo was updated up to the latest commit 31a264c |
|
Code review by qodo was updated up to the latest commit 31a264c |
Contributor
Author
|
/verified |
myakove
approved these changes
May 28, 2026
Collaborator
|
/lgtm |
|
Cherry-pick conflicts were resolved by AI Cherry-picked PR feat: add MTV operator upgrade/update migration testing into v2.11: #521 Manual verification is required — please review the changes and test before merging. |
|
Cherry-pick conflicts were resolved by AI Cherry-picked PR feat: add MTV operator upgrade/update migration testing into v2.10: #522 Manual verification is required — please review the changes and test before merging. |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
This was referenced May 29, 2026
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
6 participants
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.
Add upgrade test infrastructure:
Summary by CodeRabbit
New Features
Tests
Chores