Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
PR Summary by QodoDeploy RHOSO on a KubeVirt hosted OpenShift cluster
AI Description
Diagram
High-Level Assessment
Files changed (31)
|
Code Review by Qodo
1. Worker VMs and OpenStack pods get the same IPs
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| {{ | ||
| cifmw_hosted_cluster_networks | | ||
| map('regex_replace', '^(.*)$', | ||
| '--additional-network=name:' ~ cifmw_hosted_cluster_hosted_namespace ~ '/\1') | |
There was a problem hiding this comment.
1. Hosted workers lose isolated networks 🐞 Bug ≡ Correctness
cluster.yml passes '/\1' as a Jinja string to regex_replace, where the single backslash is interpreted as an escape rather than surviving as a capture-group reference. Each generated --additional-network argument therefore names the same invalid attachment instead of the ctlplane, internalapi, storage, or tenant attachment.
Agent Prompt
## Issue description
The Jinja string consumes the backslash intended for the regex replacement, corrupting every isolated-network argument.
## Fix Focus Areas
- roles/hosted_cluster/tasks/cluster.yml[119-125]
## Recommended Fix
Escape the backslash for Jinja, or construct each argument directly from its network name without a regex backreference. Test the resulting command arguments for all four default networks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # The hcp CLI is served from a management cluster `apps.*` route, normally | ||
| # behind a self-signed ingress certificate (the default unless a custom | ||
| # ingress CA is configured). Set to true once that CA is trusted. | ||
| cifmw_hosted_cluster_cli_validate_certs: false |
There was a problem hiding this comment.
2. A replaced cli can steal credentials 🐞 Bug ⛨ Security
cluster.yml downloads an hcp archive with certificate validation disabled by default, performs no integrity check, and executes its binary. An attacker able to intercept that download can substitute a program that receives the management kubeconfig through its environment and the pull-secret path through its arguments.
Agent Prompt
## Issue description
The default download accepts an untrusted TLS endpoint before executing the retrieved binary with cluster credentials.
## Fix Focus Areas
- roles/hosted_cluster/defaults/main.yml[139-142]
- roles/hosted_cluster/tasks/cluster.yml[41-67]
## Recommended Fix
Validate TLS using the management ingress CA and verify the downloaded archive against a trusted signature or checksum before extraction and execution.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| nodeSelector: "{{ cifmw_hosted_cluster_management_node_selector }}" | ||
| desiredState: | ||
| interfaces: "{{ _hosted_cluster_nncp_interfaces }}" |
There was a problem hiding this comment.
5. Worker vms can start without bridges 🐞 Bug ☼ Reliability
management_network.yml creates the isolated-network bridges only on nodes selected by cifmw_hosted_cluster_management_node_selector, whose default selects control-plane nodes. The hcp creation command does not constrain worker-VM placement to those nodes, so a VM scheduled on a dedicated management-cluster worker lacks the bridges required by its additional-network attachments.
Agent Prompt
## Issue description
Network bridges are restricted to control-plane nodes while hosted worker VMs are not restricted to those nodes.
## Fix Focus Areas
- roles/hosted_cluster/defaults/main.yml[42-47]
- roles/hosted_cluster/tasks/management_network.yml[108-122]
- roles/hosted_cluster/tasks/cluster.yml[139-160]
## Recommended Fix
Apply the network policy to every node eligible to run the hosted VMs, or constrain the NodePool to nodes selected by the policy. Validate the configuration on management clusters with dedicated workers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Retarget OpenShift access at the hosted cluster | ||
| ansible.builtin.set_fact: | ||
| cifmw_openshift_kubeconfig: "{{ cifmw_hosted_cluster_artifacts_dir }}/kubeconfig-{{ cifmw_hosted_cluster_name }}" | ||
| cifmw_openshift_api: "{{ _hosted_cluster_kubeconfig_dict.clusters[0].cluster.server }}" | ||
| cifmw_openshift_context: "{{ _hosted_cluster_kubeconfig_dict.contexts[0].name }}" | ||
| cifmw_openshift_user: "{{ _hosted_cluster_kubeconfig_dict.users[0].name }}" |
There was a problem hiding this comment.
3. Classic flow installs openstack on the wrong cluster 🐞 Bug ≡ Correctness
retarget.yml moves cifmw_openshift_kubeconfig to the hosted cluster but leaves cifmw_install_yamls_environment.KUBECONFIG pointing at the management cluster, which openshift_login set earlier. In the classic EDPM flow (which the README lists as supported), edpm_prepare uses that environment for its make input/make openstack calls, so the operator gets installed on the management cluster while the following oc waits poll the hosted cluster and time out.
Agent Prompt
## Issue description
After retarget, `cifmw_install_yamls_environment.KUBECONFIG` still points at the management cluster, so the install_yamls make targets in edpm_prepare act on the wrong cluster.
## Fix Focus Areas
- roles/hosted_cluster/tasks/retarget.yml[30-65]
## Recommended Fix
In the retarget set_fact, also set `cifmw_install_yamls_environment: "{{ cifmw_install_yamls_environment | combine({'KUBECONFIG': <hosted kubeconfig>}) }}"` when it is defined (cacheable: true). Also write it to zz-hosted-cluster-params.yml so separate invocations pick it up.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Create per-network NetworkAttachmentDefinitions for the hosted worker VMs | ||
| vars: | ||
| cifmw_ci_multus_namespace: "{{ cifmw_hosted_cluster_hosted_namespace }}" | ||
| ansible.builtin.include_role: | ||
| name: ci_multus |
There was a problem hiding this comment.
4. Worker vms and openstack pods get the same ips 🐞 Bug ≡ Correctness
The ci_multus NADs created on the management cluster for the worker VMs use whereabouts with tools.multus.ipv4_ranges[0]. The RHOSO NADs rendered inside the hosted cluster by the hci network-values template use the same range on the same bridged L2 segment, and each side keeps its own whereabouts datastore. Both allocators start handing out addresses from the bottom of the range, so the worker VMs' ctlplane/internalapi/storage/tenant IPs will clash with the first RHOSO pod attachments and break traffic on those networks.
Agent Prompt
## Issue description
The management-side whereabouts NADs for the hosted worker VMs and the hosted-side RHOSO NADs allocate from the same tools.multus range on the same L2, so their IPs collide.
## Fix Focus Areas
- roles/hosted_cluster/tasks/management_network.yml[147-166]
- roles/hosted_cluster/defaults/main.yml[53-55]
## Recommended Fix
In cifmw_ci_multus_net_info_patch_1, override each network's tools.multus ipv4_ranges with a dedicated range that does not overlap the RHOSO multus/metallb ranges. For example, derive it from cifmw_hosted_cluster_worker_net_range_start/_length so the VM IPs also match what ci_net_map records.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Resolve hosted worker networking via a scoped networking_mapper run | ||
| cifmw.general.ci_net_map: | ||
| networking_definition: "{{ _hosted_worker_networking_definition }}" | ||
| interfaces_info: "{{ _hosted_cluster_worker_ifaces_info }}" | ||
| register: _hosted_cluster_worker_net_map |
There was a problem hiding this comment.
6. Recorded worker ips differ from real vm ips 🐞 Bug ≡ Correctness
worker_network.yml captures and checks each VMI-assigned IP but passes only {mac, network} to
ci_net_map, which assigns ip_v4 from the group-template pool instead. When the environment
definition is merged and persisted, the HCI template renders those pool addresses as
node_N.<net>_ip even though the hosted workers have different addresses; the final assertion
checks only interface_name and misses the mismatch.
Agent Prompt
## Issue description
Hosted-worker mapping discards verified VMI IPs and persists pool-allocated `ip_v4` values instead, causing generated HCI node IPs to differ from the running workers' addresses.
## Fix Focus Areas
- roles/hosted_cluster/tasks/worker_network.yml[190-269]
## Recommended Fix
Supply the verified VMI addresses through a mapper-supported instance definition or reservation, or overwrite `instances[<node>].networks[<net>].ip_v4` with `_hosted_cluster_worker_iface_info[<node>][<net>].ip` after `ci_net_map` runs and before merging and persisting the environment definition. Alternatively, align the NAD ranges with the mapper pool so the assigned addresses agree. Test that generated `node_*_ip` values match VMI status.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ansible.builtin.copy: | ||
| dest: "{{ cifmw_basedir }}/artifacts/parameters/zz-hosted-cluster-params.yml" | ||
| content: "{{ _hosted_cluster_params_content | from_yaml | to_nice_yaml }}" | ||
| mode: "0600" |
There was a problem hiding this comment.
7. Re-deploying after cleanup targets the deleted cluster 🐞 Bug ☼ Reliability
retarget.yml writes the hosted kubeconfig, API, and user to artifacts/parameters/zz-hosted-cluster-params.yml, but cleanup.yml deletes the HostedCluster without removing that file. On a subsequent deploy, infra.yml loads the stale parameters before openshift_login, which attempts to log in to the deleted cluster; if login succeeds, management-side hosted-cluster tasks can run against the wrong kubeconfig, and separately invoked phases that load the directory can also inherit the stale access parameters.
Agent Prompt
## Issue description
Cleanup leaves the hosted cluster's persisted access parameters in the shared parameters directory. A later deploy or separately invoked phase can load credentials for the deleted cluster.
## Fix Focus Areas
- roles/hosted_cluster/tasks/cleanup.yml[46-82]
- roles/hosted_cluster/tasks/retarget.yml[51-64]
## Recommended Fix
Add a final cleanup task using `ansible.builtin.file` with `path: {{ cifmw_basedir }}/artifacts/parameters/zz-hosted-cluster-params.yml` and `state: absent`. Retain the explicitly supplied management-cluster access needed to finish cleanup; the saved hosted kubeconfig can also be removed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| {% if cifmw_openshift_token %} | ||
| cifmw_openshift_token: "{{ cifmw_openshift_token }}" | ||
| {% endif %} |
There was a problem hiding this comment.
8. Management token still sent to hosted cluster 🐞 Bug ≡ Correctness
retarget.yml conditionally omits cifmw_openshift_token from zz-hosted-cluster-params.yml when the hosted admin kubeconfig uses certificate authentication and the hosted token is None. In a later separate invocation that reloads the parameters directory, the earlier management token survives alongside the hosted kubeconfig and is passed as api_key to Kubernetes calls against the hosted API.
Agent Prompt
## Issue description
For certificate-auth hosted clusters, `zz-hosted-cluster-params.yml` omits `cifmw_openshift_token`, leaving the management token from `openshift-login-params.yml` in effect in later invocations.
## Fix Focus Areas
- roles/hosted_cluster/tasks/retarget.yml[51-65]
## Recommended Fix
Always emit `cifmw_openshift_token:` in the later-sorting hosted parameters file, using YAML null (`~`) when no hosted token exists so it overrides the earlier management value. Ensure the YAML round-trip preserves null as None rather than the string `'None'`, and verify a separate `deploy-edpm` invocation after token-based management login.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| patch: | ||
| - op: replace | ||
| path: /spec/defaultNetwork/ovnKubernetesConfig/gatewayConfig/ipForwarding | ||
| value: Global |
There was a problem hiding this comment.
9. Ip forwarding patch can fail on a fresh cluster 🐞 Bug ☼ Reliability
hosted_setup.yml uses a JSON Patch replace on /spec/defaultNetwork/ovnKubernetesConfig/gatewayConfig/ipForwarding, and RFC 6902 replace fails when the target member doesn't already exist. On a freshly created hosted cluster where gatewayConfig or ipForwarding hasn't been set, the patch errors out, the role's rescue block runs, and the whole hosted deployment stops.
Agent Prompt
## Issue description
JSON Patch `replace` on a nested path that may not exist fails on fresh hosted clusters.
## Fix Focus Areas
- roles/hosted_cluster/tasks/hosted_setup.yml[34-46]
## Recommended Fix
Use `kubernetes.core.k8s` with `state: patched` and a definition of `spec.defaultNetwork.ovnKubernetesConfig.gatewayConfig.ipForwarding: Global` (merge patch), which creates the intermediate keys.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| cifmw_ci_gen_kustomize_values_storage_class_prefix: >- | ||
| {{ | ||
| 'lvms-' | ||
| if (cifmw_use_lvms | default(false) | bool) and not | ||
| ((cifmw_deploy_hosted_cluster | default(false) | bool) and | ||
| (cifmw_hosted_cluster_map_infra_storage_class | default(true) | bool)) | ||
| else '' | ||
| }} |
There was a problem hiding this comment.
10. Mapped guest storage class is never used 🐞 Bug ≡ Correctness
With hosted mode and cifmw_hosted_cluster_map_infra_storage_class on, the new prefix logic sets the RHOSO storage class to local-storage, and deploy_architecture.yml/deploy_edpm.yml now run ci_local_storage against the guest cluster. The guest SC that --infra-storage-class-mapping creates is named after the infra class (lvms-local-storage), so no PVC ever uses it. RHOSO PVCs end up on hostPath local PVs carved from each worker's 85 GiB root disk, not the KubeVirt-CSI storage the README describes.
Agent Prompt
## Issue description
In hosted mode, RHOSO uses ci_local_storage's `local-storage` SC instead of the guest storage class mapped via --infra-storage-class-mapping.
## Fix Focus Areas
- roles/ci_gen_kustomize_values/defaults/main.yml[67-75]
- roles/test_operator/defaults/main.yml[40-48]
- roles/cifmw_setup/tasks/deploy_architecture.yml[200-209]
- roles/cifmw_setup/tasks/deploy_edpm.yml[15-29]
## Recommended Fix
When hosted mapping is enabled, set the RHOSO storage class to cifmw_hosted_cluster_resolved_root_volume_storage_class (or keep the 'lvms-' prefix) and skip ci_local_storage. Alternatively, update the README to say that ci_local_storage hostPath volumes are what's actually used.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 17m 52s |
02862d9 to
e283129
Compare
|
Build succeeded (check pipeline). ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 25m 47s Warning: |
e283129 to
f49364b
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 23m 08s Warning: |
Allow RHOSO to run on an opt-in KubeVirt hosted cluster so multiple environments can share one management cluster. Add cluster creation and cleanup, isolated worker networking, and integration with the existing deployment flow. Select the hosted workers' interfaces for HCI and OVN, preserve EDPM VLAN IDs, and map management storage into the guest without disabling management-cluster LVMS. Handle hosted-cluster reapplication and skip unsupported Metal3 and guest NNCP steps. Co-Authored-By: Andrew Bays <abays@redhat.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Dan Prince <dprince@redhat.com>
f49364b to
eab87ed
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 38m 34s Warning: |
Enable deployment of the RHOSO controlplane on an OCP Hosted Controlplane by added this role and setting a few additional extra_vars:
cifmw_deploy_hosted_cluster: true
cifmw_hosted_cluster_management_interface: enp8s0
cifmw_hosted_cluster_kubeapi_address_pool:
cifmw_hosted_cluster_lvms_disk_list:
This is meant to facilitate CI testing of this feature/topology (soon to be GA) and also help dev/test other features using OCP Hosted Controlplane style deployments.
Kubevirt was choosen as a OCP hosted provider as it closely follows the most likely use case for our customers. Other providers (like baremetal) might be useful in the future.