[Storage/Network] Specify architecture for internal HTTP server for multiarch clusters - #5813
Conversation
On multi-arch clusters with a single --cpu-arch target, the internal HTTP server pod must be scheduled on a node matching the target architecture. Add a nodeSelector using kubernetes.io/arch label when both cpu_arch is set and cluster_type is multiarch. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
…zation-tests into internal_http_server_arch
…ted multiarch check
The pattern `py_config.get("cpu_arch") + cluster_type == MULTIARCH` was
duplicated across utilities/storage.py, tests/storage/conftest.py, and
libs/vm/factory.py. Extract it into a single function in
utilities/architecture.py and update all call sites.
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
The constants modules (architecture.py, cluster.py) have no imports from utilities/, so there is no circular dependency risk. Remove the lazy imports and TODO comments. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
📝 WalkthroughWalkthroughChangesMulti-architecture architecture handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ 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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
📌 Additional InformationCustom Commands:
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
tests/storage/conftest.py (2)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMEDIUM: Use a specific import for
deepcopy.The new
import copyviolates the repository import rule. Usefrom copy import deepcopyand update the deep-copy call. This preserves the behavior and complies with the required import style.As per coding guidelines, prefer specific imports and do not use bare
import modulestatements.🤖 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 `@tests/storage/conftest.py` at line 6, Replace the bare copy import with a specific deepcopy import, then update the deep-copy call sites in the test fixture to use the imported symbol directly while preserving existing behavior.Source: Coding guidelines
238-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Pass the configured HCO namespace to
ResourceEditorValidateHCOReconcile.
hco_namespaceis injected here, but the constructor call does not pass it.ResourceEditorValidateHCOReconciledefaults to"openshift-cnv", whiletests/conftest.py::hco_namespaceusespy_config["hco_namespace"]. When the configured namespace differs, reconciliation checks run against the wrong namespace.Pass
hco_namespace=hco_namespace.name.Proposed fix
with ResourceEditorValidateHCOReconcile( admin_client=admin_client, + hco_namespace=hco_namespace.name, patches={The unused-argument warning from Ruff is valid here because this fixture should supply the namespace to the reconciliation helper.
🤖 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 `@tests/storage/conftest.py` around lines 238 - 246, Update the ResourceEditorValidateHCOReconcile constructor call in cdi_config_upload_proxy_overridden to pass the injected configured namespace as hco_namespace=hco_namespace.name, ensuring reconciliation uses the test configuration instead of the helper’s default.Source: Linters/SAST tools
utilities/architecture.py (1)
25-27: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMEDIUM: Resolve the import cycle instead of adding linter suppressions.
These imports are inside functions and add
# noqa: PLC0415. The PR objective identifies the circular dependency as the reason. Refactor the dependency boundary soKUBERNETES_ARCH_LABELandMULTIARCHcan be imported at module scope. If the lazy imports must remain, obtain explicit approval for the exception.As per coding guidelines, Python files must not add linter suppressions and imports must remain at module scope. Based on learnings, the temporary
# noqa: PLC0415exception applies only to documented lint-rule-enablement work for pre-existing violations.Also applies to: 58-60
🤖 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/architecture.py` around lines 25 - 27, Refactor the dependency boundary in utilities/architecture.py so KUBERNETES_ARCH_LABEL and MULTIARCH are importable at module scope, then move their imports out of the affected functions and remove the PLC0415 suppressions and temporary TODO. Preserve the existing architecture behavior while ensuring no new in-function imports remain.Sources: Coding guidelines, Learnings
🤖 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 `@tests/storage/conftest.py`:
- Line 6: Replace the bare copy import with a specific deepcopy import, then
update the deep-copy call sites in the test fixture to use the imported symbol
directly while preserving existing behavior.
- Around line 238-246: Update the ResourceEditorValidateHCOReconcile constructor
call in cdi_config_upload_proxy_overridden to pass the injected configured
namespace as hco_namespace=hco_namespace.name, ensuring reconciliation uses the
test configuration instead of the helper’s default.
In `@utilities/architecture.py`:
- Around line 25-27: Refactor the dependency boundary in
utilities/architecture.py so KUBERNETES_ARCH_LABEL and MULTIARCH are importable
at module scope, then move their imports out of the affected functions and
remove the PLC0415 suppressions and temporary TODO. Preserve the existing
architecture behavior while ensuring no new in-function imports remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48fda206-e7c2-4fa4-bfee-24746af56150
📒 Files selected for processing (4)
libs/vm/factory.pytests/storage/conftest.pyutilities/architecture.pyutilities/unittests/test_storage.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
|
/retest all |
|
/retest tox |
|
/verified (tox is broken on main; need to retest once it's fixed there) |
|
/retest tox |
|
/verified |
|
/rerun-smoke |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5813. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
|
/cherry-pick cnv-4.22 |
|
Cherry-pick conflicts were resolved by AI Cherry-picked PR [Storage/Network] Specify architecture for internal HTTP server for multiarch clusters into cnv-4.22: #5980 @jpeimer Manual verification is required — please review the changes and test before merging. |
11 participants
What this PR does / why we need it:
On multi-arch clusters with a single
--cpu-archtarget, the internal HTTP server pod could be scheduled on a node with the wrong architecture.This PR:
nodeSelectorwithkubernetes.io/archto the internal HTTP server Deployment so it lands on the correct architecture node.py_config["cpu_arch"] + cluster_type == MULTIARCHcheck (repeated inutilities/storage.py,tests/storage/conftest.py, andlibs/vm/factory.py) into a singleget_multiarch_cpu_arch()function inutilities/architecture.py.Which issue(s) this PR fixes:
Failures on multi-arch clusters that run wit single requested arch.
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit