feat: Support RHOAI RC images - #113
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RHOAI custom catalog deployment orchestration with vault-backed pull secrets, registry mirroring, phase-specific vault initialization, publisher metadata rendering, shared deployment helpers, and expanded tests. ChangesRHOAI custom catalog and publisher wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant prepare_phase
participant RHOAI_deploy
participant Vault
participant OpenShift
CI->>Vault: initialize phase vaults
CI->>prepare_phase: start RHOAI preparation
prepare_phase->>RHOAI_deploy: prepare_rhoai_operator(...)
RHOAI_deploy->>Vault: resolve pull-secret content
RHOAI_deploy->>OpenShift: update cluster pull-secret
RHOAI_deploy->>prepare_phase: apply ICSP
prepare_phase->>OpenShift: apply ImageContentSourcePolicy
RHOAI_deploy->>OpenShift: deploy catalog and subscriptions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@projects/llm_d/orchestration/prepare_phase.py`:
- Around line 161-171: The MCP readiness check in prepare_phase.py can
incorrectly pass when oc get mcp returns an empty jsonpath result. Update the
logic around the mcp_status handling in the prepare phase so that the code only
considers MachineConfigPools updated when mcp_status is non-empty and does not
contain "False". If the status is empty, treat it as not ready and keep waiting
or raise the same RuntimeError from the existing MCP status check path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 487b5437-4363-4566-aae4-40b7c217c460
📒 Files selected for processing (8)
projects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/cluster/toolbox/deploy_custom_catalog/templates/catalogsource.yaml.j2projects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/manifests/quay-registry-icsp.yamlprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/tests/test_profiles.pyvaults/psap-rhoai-rc.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/rhoai/library/deploy.py (1)
178-179: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMCP status check still passes on empty output.
This issue was previously flagged on
prepare_phase.pyand persists in the new location after the code was moved. Ifoc get mcpreturns an empty string (no MachineConfigPools exist yet, or the API is temporarily unavailable),"False" in ""evaluates toFalse, so the function treats MCPs as fully updated and returns prematurely.🔒️ Proposed fix
- if "False" in mcp_status: - raise RuntimeError("machine config pools are still updating") + if not mcp_status or "False" in mcp_status: + raise RuntimeError("machine config pools are still updating")🤖 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 `@projects/rhoai/library/deploy.py` around lines 178 - 179, The MCP readiness check in deploy.py can incorrectly pass when the `oc get mcp` output is empty, because the current `mcp_status` test only looks for `"False"`. Update the MCP status handling in the same block to treat empty output from the `mcp_status`/`oc get mcp` command as not ready and raise the existing `RuntimeError` until valid MCP data is present. Keep the fix localized around the MCP check logic so the moved behavior remains consistent with the previous `prepare_phase.py` implementation.
🧹 Nitpick comments (1)
projects/rhoai/library/deploy.py (1)
196-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated vault path resolution logic.
The vault content path lookup and
None-check with error message are duplicated betweencustom_catalog_pull_secret_path(lines 68-80) andprepare_rhoai_pull_secret(lines 196-206). Extract a shared helper to eliminate the duplication.♻️ Proposed refactor
+def _resolve_vault_content_path(vault_name: str, content_name: str) -> Path: + secret_path = vault.get_vault_content_path(vault_name, content_name) + if secret_path is None: + raise RuntimeError( + "RHOAI pull secret content " + f"'{content_name}' was not found in vault " + f"'{vault_name}'" + ) + return secret_path + + def custom_catalog_pull_secret_path(custom_catalog: dict[str, Any]) -> Path: catalog = _RhoaiCustomCatalogPullSecretInput.model_validate(custom_catalog) - secret_path = vault.get_vault_content_path( - catalog.pull_secret.vault.name, - catalog.pull_secret.vault.content, - ) - if secret_path is None: - raise RuntimeError( - "RHOAI pull secret content " - f"'{catalog.pull_secret.vault.content}' was not found in vault " - f"'{catalog.pull_secret.vault.name}'" - ) - return secret_path + return _resolve_vault_content_path( + catalog.pull_secret.vault.name, + catalog.pull_secret.vault.content, + )Then update
prepare_rhoai_pull_secretto use the same helper:def prepare_rhoai_pull_secret(custom_catalog: RhoaiCustomCatalogConfig) -> None: - pull_secret_path = vault.get_vault_content_path( + pull_secret_path = _resolve_vault_content_path( custom_catalog.pull_secret.vault.name, custom_catalog.pull_secret.vault.content, ) - if pull_secret_path is None: - raise RuntimeError( - "RHOAI pull secret content " - f"'{custom_catalog.pull_secret.vault.content}' was not found in vault " - f"'{custom_catalog.pull_secret.vault.name}'" - ) auth_basic = pull_secret_path.read_text(encoding="utf-8").strip()🤖 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 `@projects/rhoai/library/deploy.py` around lines 196 - 206, The vault content path lookup and missing-path handling in prepare_rhoai_pull_secret duplicates the logic already present in custom_catalog_pull_secret_path. Extract the shared resolution-and-error handling into a common helper, then update prepare_rhoai_pull_secret to call that helper instead of repeating the get_vault_content_path and RuntimeError logic.
🤖 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.
Duplicate comments:
In `@projects/rhoai/library/deploy.py`:
- Around line 178-179: The MCP readiness check in deploy.py can incorrectly pass
when the `oc get mcp` output is empty, because the current `mcp_status` test
only looks for `"False"`. Update the MCP status handling in the same block to
treat empty output from the `mcp_status`/`oc get mcp` command as not ready and
raise the existing `RuntimeError` until valid MCP data is present. Keep the fix
localized around the MCP check logic so the moved behavior remains consistent
with the previous `prepare_phase.py` implementation.
---
Nitpick comments:
In `@projects/rhoai/library/deploy.py`:
- Around line 196-206: The vault content path lookup and missing-path handling
in prepare_rhoai_pull_secret duplicates the logic already present in
custom_catalog_pull_secret_path. Extract the shared resolution-and-error
handling into a common helper, then update prepare_rhoai_pull_secret to call
that helper instead of repeating the get_vault_content_path and RuntimeError
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eec9ccc-9377-4821-9e54-42a13197607c
📒 Files selected for processing (5)
projects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/tests/test_profiles.pyprojects/rhoai/library/deploy.pyvaults/psap-rhoai-rc.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/llm_d/orchestration/config.d/platform.yaml
- vaults/psap-rhoai-rc.yaml
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
8069ae3 to
87b0035
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
projects/llm_d/orchestration/ci.py (1)
139-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
vault.phase_vault_list_allis redundant invault_list_funcs.
list_vaultsalready callsvault.phase_vault_list_all()internally and returns its result (pluspsap-rhoai-rcwhen enabled). Including both means the base vault list is collected and deduplicated away downstream — harmless but unnecessary.🤖 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 `@projects/llm_d/orchestration/ci.py` around lines 139 - 149, The vault list passed to create_fournos_resolve_entrypoint is redundant because list_vaults already includes vault.phase_vault_list_all internally. Remove the extra vault.phase_vault_list_all entry from vault_list_funcs in ci.py and keep list_vaults as the single source for the base vault list so the command setup stays minimal and avoids duplicate collection.projects/llm_d/tests/test_profiles.py (1)
336-365: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify that empty MCP status causes continued polling.
The test name asserts "empty MCP status is treated as not ready," but if the
not mcp_statusguard were removed, the function would return on the first iteration (empty treated as ready) and the test would still pass — no error is raised either way. Adding an assertion that both MCP outputs were consumed would make the test meaningful:rhoai_deploy.wait_for_rhoai_pull_secret_ready(timeout_seconds=1, poll_interval_seconds=0) + + assert not list(mcp_outputs), "both MCP outputs should have been consumed"🤖 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 `@projects/llm_d/tests/test_profiles.py` around lines 336 - 365, The test for wait_for_rhoai_pull_secret_ready currently only calls the function and does not prove that an empty MCP status is treated as not ready. Update test_wait_for_rhoai_pull_secret_ready_treats_empty_mcp_status_as_not_ready to assert the polling continued past the empty first response by verifying both mocked mcp outputs were consumed (or otherwise checking multiple oc get mcp calls occurred). Keep the focus on the rhoai_deploy.wait_for_rhoai_pull_secret_ready path and the _fake_oc / mcp_outputs setup so the test fails if the not mcp_status guard is removed.
🤖 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.
Nitpick comments:
In `@projects/llm_d/orchestration/ci.py`:
- Around line 139-149: The vault list passed to
create_fournos_resolve_entrypoint is redundant because list_vaults already
includes vault.phase_vault_list_all internally. Remove the extra
vault.phase_vault_list_all entry from vault_list_funcs in ci.py and keep
list_vaults as the single source for the base vault list so the command setup
stays minimal and avoids duplicate collection.
In `@projects/llm_d/tests/test_profiles.py`:
- Around line 336-365: The test for wait_for_rhoai_pull_secret_ready currently
only calls the function and does not prove that an empty MCP status is treated
as not ready. Update
test_wait_for_rhoai_pull_secret_ready_treats_empty_mcp_status_as_not_ready to
assert the polling continued past the empty first response by verifying both
mocked mcp outputs were consumed (or otherwise checking multiple oc get mcp
calls occurred). Keep the focus on the
rhoai_deploy.wait_for_rhoai_pull_secret_ready path and the _fake_oc /
mcp_outputs setup so the test fails if the not mcp_status guard is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5513c22b-d4c2-4ece-bb91-bd86616e8fc7
📒 Files selected for processing (9)
projects/cluster/toolbox/deploy_custom_catalog/main.pyprojects/cluster/toolbox/deploy_custom_catalog/templates/catalogsource.yaml.j2projects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/manifests/quay-registry-icsp.yamlprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/tests/test_profiles.pyprojects/rhoai/library/deploy.pyvaults/psap-rhoai-rc.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- projects/cluster/toolbox/deploy_custom_catalog/templates/catalogsource.yaml.j2
- projects/llm_d/orchestration/config.d/platform.yaml
- projects/llm_d/orchestration/manifests/quay-registry-icsp.yaml
- projects/cluster/toolbox/deploy_custom_catalog/main.py
- vaults/psap-rhoai-rc.yaml
- projects/rhoai/library/deploy.py
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/rhoai/library/deploy.py (1)
273-344: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the readiness check depend on staging config
wait_for_rhoai_pull_secret_ready()still requiresRHOAI_REGISTRIESunconditionally, so a catalog-only deployment can write the catalog creds and then wait forever. Pass the required registry tuple fromprepare_rhoai_pull_secret()instead of hardcoding both sets.🤖 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 `@projects/rhoai/library/deploy.py` around lines 273 - 344, Update prepare_rhoai_pull_secret and wait_for_rhoai_pull_secret_ready so readiness validation receives the registries required by the current configuration. Use RHOAI_CATALOG_REGISTRIES for catalog-only deployments and include RHOAI_REGISTRIES only when staging_pull_secret is configured, passing the resulting tuple to the readiness check instead of hardcoding both sets.projects/llm_d/tests/test_profiles.py (1)
311-324: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for MCP after applying the ICSP before deploying the custom catalog. The pull-secret wait only covers the secret update;
icsp_applier()here just applies the manifest, anddeploy_rhoai_custom_catalog()runs immediately after. If the ICSP triggers a new MachineConfigPool rollout, the catalog can start before the mirror policy reaches all nodes.🤖 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 `@projects/llm_d/tests/test_profiles.py` around lines 311 - 324, Update the deployment flow exercised by prepare_rhoai_operator so it waits for MCP readiness after icsp_applier() completes and before deploy_rhoai_custom_catalog() starts. Add or invoke the existing MCP-wait mechanism at that boundary, preserving the current call ordering and behavior for the remaining catalog, subscription, and CRD steps.
🧹 Nitpick comments (1)
projects/llm_d/tests/test_profiles.py (1)
395-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the idempotent "already present" merge path.
This test always starts from an empty
{"auths": {}}current secret, so it never exercisesprepare_rhoai_pull_secret's early-return branch when the required RHOAI/staging registries are already present (per the_registries_present(...)check in the upstream implementation). Worth adding a companion case to guard against accidental re-merges/MCP-rollouts on repeat runs.🤖 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 `@projects/llm_d/tests/test_profiles.py` around lines 395 - 498, The test coverage only exercises merging into an empty current secret and misses the idempotent path guarded by _registries_present. Add a companion test for prepare_rhoai_pull_secret with current_secret already containing the required RHOAI and staging registry entries, assert the function returns without invoking merge operations or triggering MCP rollout behavior, and preserve the existing merge-case coverage.
🤖 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 `@projects/rhoai/library/deploy.py`:
- Around line 213-216: Update _registries_present to parse decoded_secret as
JSON and check registry keys for exact matches rather than using substring
membership, ensuring rhaii and rhaii-early-access remain distinct. Preserve the
existing all-registries requirement so the readiness check and
prepare_rhoai_pull_secret short-circuit both reflect actual credential entries.
---
Outside diff comments:
In `@projects/llm_d/tests/test_profiles.py`:
- Around line 311-324: Update the deployment flow exercised by
prepare_rhoai_operator so it waits for MCP readiness after icsp_applier()
completes and before deploy_rhoai_custom_catalog() starts. Add or invoke the
existing MCP-wait mechanism at that boundary, preserving the current call
ordering and behavior for the remaining catalog, subscription, and CRD steps.
In `@projects/rhoai/library/deploy.py`:
- Around line 273-344: Update prepare_rhoai_pull_secret and
wait_for_rhoai_pull_secret_ready so readiness validation receives the registries
required by the current configuration. Use RHOAI_CATALOG_REGISTRIES for
catalog-only deployments and include RHOAI_REGISTRIES only when
staging_pull_secret is configured, passing the resulting tuple to the readiness
check instead of hardcoding both sets.
---
Nitpick comments:
In `@projects/llm_d/tests/test_profiles.py`:
- Around line 395-498: The test coverage only exercises merging into an empty
current secret and misses the idempotent path guarded by _registries_present.
Add a companion test for prepare_rhoai_pull_secret with current_secret already
containing the required RHOAI and staging registry entries, assert the function
returns without invoking merge operations or triggering MCP rollout behavior,
and preserve the existing merge-case coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2af1880-67eb-40c5-b8fa-37289e246ec1
📒 Files selected for processing (6)
projects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/manifests/rhoai-registry-icsp.yamlprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/tests/test_profiles.pyprojects/rhoai/library/deploy.pyvaults/psap-forge-staging-image-pull.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/llm_d/orchestration/config.d/platform.yaml
- projects/llm_d/orchestration/prepare_phase.py
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…orge into feat/support-rc-images
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
| for deployment_name in SERVING_CONTROL_PLANE_DEPLOYMENTS: | ||
| result = oc( | ||
| "wait", | ||
| "--for=condition=Available", | ||
| "--timeout=300s", | ||
| f"deployment/{deployment_name}", | ||
| "-n", | ||
| args.namespace, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
I'll see how to rewrite that in another PR,
I'd like to:
- remove the
forloop, turn it into a@loop(SERVING_CONTROL_PLANE_DEPLOYMENTS) - remove the
oc wait --timeout=300s, turn it into a polling with@retry
but that will wait :)
There was a problem hiding this comment.
I can try to give it a go 😄
There was a problem hiding this comment.
nice :)
second part should be straightforward,
first part, my idea is to have something like Ansible loop
https://github.com/openshift-psap/topsail/blob/main/projects/fine_tuning/toolbox/fine_tuning_run_fine_tuning_job/tasks/main.yml#L123
so that the task body can focus on a single element
something like this [but I don't know how complex this would be to implement]
@retry
@loop(SERVING_CONTROL_PLANE_DEPLOYMENTS, "deploy")
@task
def wait_for_deployments(args, ctx):
"""Wait for all serving control plane deployments to be available"""
check if ctx.deploy is ready
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
|
thanks @albertoperdomo2 , let's merge this! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kpouget The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests