feat: add prepare and cleanup steps for rhaiis project - #106
Closed
MML-coder wants to merge 17 commits into
Closed
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 |
|
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:
📝 WalkthroughWalkthroughThis PR adds a cluster preflight validator, expands RHAIIS platform/Vault config, and replaces the orchestration entrypoints with operator bootstrap, namespace provisioning, secret/PVC creation, and cleanup of benchmark and operator resources. ChangesRHAIIS platform preparation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ci.py
participant runtime_config
participant OpenShift Cluster
ci.py->>runtime_config: get_platform_config()
ci.py->>OpenShift Cluster: oc_resource_exists for CRDs and namespace
ci.py->>OpenShift Cluster: oc_resource_exists for image pull secret and PVC
ci.py->>ci.py: collect errors, return status
sequenceDiagram
participant prepare_rhaiis.py
participant OpenShift Cluster
participant Vault
prepare_rhaiis.py->>OpenShift Cluster: oc whoami
prepare_rhaiis.py->>OpenShift Cluster: install/bootstrap operators
prepare_rhaiis.py->>OpenShift Cluster: create namespace, service account, SCC binding
prepare_rhaiis.py->>Vault: resolve image pull secret content path
prepare_rhaiis.py->>OpenShift Cluster: create dockerconfigjson secret and model PVC
prepare_rhaiis.py->>OpenShift Cluster: delete runtime resources during cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🤖 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/rhaiis/orchestration/config.d/platform.yaml`:
- Around line 31-34: The SCC is currently bound to a hardcoded service account
that may never be created because `rhaiis.deploy.service_account_name` can be
empty and `ensure_service_account()` skips creation in that case. Update
`prepare.scc.service_account` to use the same source of truth as the deploy
service account, or give `rhaiis.deploy.service_account_name` a non-empty
default so both `prepare.scc` and `ensure_service_account()` reference a real,
consistent service account.
In `@projects/rhaiis/orchestration/prepare_rhaiis.py`:
- Around line 71-80: The _operator_csv_exists helper is treating any matching
CSV name as installed, which can skip operator installation before the CSV is
ready. Update _operator_csv_exists in prepare_rhaiis.py to inspect CSV status
and only return true when the matching CSV is in Succeeded phase, then make the
operator-installation check around the current package/CSV handling use that
readiness check instead of name matching alone.
- Around line 93-101: The call to cluster_deploy_operator.run in
prepare_rhaiis.py passes an unsupported artifact_dirname_suffix keyword, which
will raise TypeError before operator installation starts. Remove that argument
from the run invocation and keep the remaining parameters aligned with the
actual cluster_deploy_operator.run signature, using the existing package,
namespace, and operator_spec fields.
- Around line 216-217: The success log in prepare_rhaiis.py is unconditional
because the `oc("adm", "policy", "add-scc-to-user", ...)` call uses
`check=False`, so failures can still be reported as applied. Update the
`prepare_rhaiis` flow around the SCC binding call to either let the command
raise on failure or explicitly inspect the return status and only emit
`logger.info("Applied SCC %s to SA %s in %s", ...)` when the `oc adm policy`
operation succeeds.
- Around line 291-298: The `prepare_rhaiis` flow currently skips PVC creation
and still returns success when `deploy_cfg.storage_pvc` is set but
`model_pvc.storage_class` is missing, leaving the deployment without its
required volume. Update the PVC handling in `prepare_rhaiis` so that a
configured model PVC is treated as required: if the PVC is expected but cannot
be provisioned because `storage_class` is absent, raise an error or fail the
prepare step instead of logging a warning and returning. Keep the early-exit
only for cases where no model PVC was requested.
- Around line 240-248: The image pull secret lookup in prepare_rhaiis.py
currently logs and returns when vault content is missing, which allows prepare
to succeed even though deploy_cfg.image_pull_secret was requested. Update the
secret-creation flow around the vault.get_vault_content_path check to treat
missing Vault content or an absent dockerconfig_path as a hard failure for the
configured image pull secret, and make the prepare_rhaiis path raise/abort
instead of returning silently. Use the existing vault_name, vault_content, and
deploy_cfg.image_pull_secret handling in this function to locate the change.
🪄 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: a733fb85-6249-44b6-af25-ed6f83154feb
📒 Files selected for processing (7)
projects/rhaiis/orchestration/ci.pyprojects/rhaiis/orchestration/config.d/platform.yamlprojects/rhaiis/orchestration/config.d/rhaiis.yamlprojects/rhaiis/orchestration/config.yamlprojects/rhaiis/orchestration/prepare_rhaiis.pyprojects/rhaiis/orchestration/runtime_config.pyvaults/psap-rhaiis-image-pull.yaml
Comment on lines
+291
to
+298
| pvc_cfg = prepare_cfg.get("model_pvc", {}) | ||
| storage_class = pvc_cfg.get("storage_class", "") | ||
| size = pvc_cfg.get("size", "300Gi") | ||
| access_mode = pvc_cfg.get("access_mode", "ReadWriteOnce") | ||
|
|
||
| if not storage_class: | ||
| logger.warning("No storage_class configured for model PVC, skipping creation") | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail prepare when a configured model PVC cannot be provisioned.
When deploy_cfg.storage_pvc is set and the PVC does not already exist, missing model_pvc.storage_class leaves the run without its required volume while prepare still exits 0.
Proposed fix
if not storage_class:
- logger.warning("No storage_class configured for model PVC, skipping creation")
- return
+ raise ValueError(
+ "platform.prepare.model_pvc.storage_class is required when rhaiis.deploy.storage_pvc is configured"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pvc_cfg = prepare_cfg.get("model_pvc", {}) | |
| storage_class = pvc_cfg.get("storage_class", "") | |
| size = pvc_cfg.get("size", "300Gi") | |
| access_mode = pvc_cfg.get("access_mode", "ReadWriteOnce") | |
| if not storage_class: | |
| logger.warning("No storage_class configured for model PVC, skipping creation") | |
| return | |
| pvc_cfg = prepare_cfg.get("model_pvc", {}) | |
| storage_class = pvc_cfg.get("storage_class", "") | |
| size = pvc_cfg.get("size", "300Gi") | |
| access_mode = pvc_cfg.get("access_mode", "ReadWriteOnce") | |
| if not storage_class: | |
| raise ValueError( | |
| "platform.prepare.model_pvc.storage_class is required when rhaiis.deploy.storage_pvc is configured" | |
| ) |
🤖 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/rhaiis/orchestration/prepare_rhaiis.py` around lines 291 - 298, The
`prepare_rhaiis` flow currently skips PVC creation and still returns success
when `deploy_cfg.storage_pvc` is set but `model_pvc.storage_class` is missing,
leaving the deployment without its required volume. Update the PVC handling in
`prepare_rhaiis` so that a configured model PVC is treated as required: if the
PVC is expected but cannot be provisioned because `storage_class` is absent,
raise an error or fail the prepare step instead of logging a warning and
returning. Keep the early-exit only for cases where no model PVC was requested.
kpouget
reviewed
Jul 2, 2026
Comment on lines
+91
to
+94
| REQUIRED_CRDS = [ | ||
| "inferenceservices.serving.kserve.io", | ||
| "servingruntimes.serving.kserve.io", | ||
| ] |
Contributor
There was a problem hiding this comment.
this should be in the config file (it's easier to control the list from there)
kpouget
reviewed
Jul 2, 2026
| from projects.core.dsl.utils.k8s import oc_resource_exists | ||
|
|
||
| logger.info("Starting preflight checks") | ||
| missing = [crd for crd in REQUIRED_CRDS if not oc_resource_exists("crd", crd)] |
Contributor
There was a problem hiding this comment.
will be updated with config.project.get_config("somewhere.required_crds") instead of REQUIRED_CRDS
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
| return 0 | ||
|
|
||
|
|
||
| def cleanup() -> int: |
Contributor
There was a problem hiding this comment.
the cleanup should delete all the components that have been installed, to bring the cluster back to a fresh state
More or less at least. See in llm-d, for time sake I don't uninstall the GPU operator and a few other core operators
Contributor
There was a problem hiding this comment.
I din't think this has been addressed
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
kpouget
reviewed
Jul 2, 2026
MML-coder
added a commit
to MML-coder/forge-1
that referenced
this pull request
Jul 7, 2026
- Remove check=False on SCC command (fail on error, not silently) - Fail hard when vault/secret is missing (no silent swallow) - Use oc create secret generic --from-file instead of base64 manifest - Check CSV phase is Succeeded before skipping operator install - SCC uses deploy_cfg.service_account_name as single source of truth - Move REQUIRED_CRDS to platform.yaml config - Move namespace/PVC labels to platform.yaml config - Remove hardcoded defaults for PVC size/access_mode - Let K8s use default storage class when not configured - Add preflight validation for namespace, secret, PVC
Rewrite prepare_rhaiis.py with a 9-step prepare sequence: - Cluster-level: NFD, GPU operator, KServe (via RHOAI) installation - Per-run: namespace, SA, SCC policy, vault-based image pull secret, PVC Add config.d/platform.yaml with operator specs, DSC config, and prepare settings (SCC, PVC storage class, vault references). Add vault definition for rhaiis image pull credentials (psap-rhaiis-image-pull) following existing Fournos vault pattern. Update ci.py with @agent_review_on_failure decorators and preflight CRD validation for KServe InferenceService/ServingRuntime. Update cleanup to also delete InferenceServices and ServingRuntimes.
The decorator requires agentic config (model_key etc) that rhaiis does not use. Remove it to unblock the prepare step.
Avoids triggering operator upgrades on clusters where operators are already installed and working. Checks for existing CSV by package name before calling cluster_deploy_operator.
- Remove check=False on SCC command (fail on error, not silently) - Fail hard when vault/secret is missing (no silent swallow) - Use oc create secret generic --from-file instead of base64 manifest - Check CSV phase is Succeeded before skipping operator install - SCC uses deploy_cfg.service_account_name as single source of truth - Move REQUIRED_CRDS to platform.yaml config - Move namespace/PVC labels to platform.yaml config - Remove hardcoded defaults for PVC size/access_mode - Let K8s use default storage class when not configured - Add preflight validation for namespace, secret, PVC
MML-coder
force-pushed
the
rhaiis-prepare-steps
branch
from
July 7, 2026 21:43
60811ad to
a133fc9
Compare
kpouget
reviewed
Jul 8, 2026
kpouget
reviewed
Jul 8, 2026
kpouget
reviewed
Jul 8, 2026
| errors.append(f"PVC not found: {pvc_name} in {ns}") | ||
| elif pvc_name: | ||
| logger.info("PVC found: %s in %s", pvc_name, ns) | ||
|
|
Contributor
There was a problem hiding this comment.
the PVC can be created & populated in the test step (in addition to the prepare step)
Contributor
|
thanks, looks good overall, added some comments |
- Skip operator install with warning when CSV exists but is not Succeeded (Pending/Failed), avoiding 15-min timeout on stuck upgrades - Add preflight validation for namespace, secret, PVC existence - Move required CRDs to platform.yaml config - Move namespace/PVC labels to platform.yaml config - Remove hardcoded defaults for PVC size/access_mode - Remove check=False on SCC (fail on error) - Fail hard on missing vault content - Use oc create secret generic --from-file - Let K8s use default storage class when not configured - SCC uses deploy_cfg.service_account_name as source of truth
Tab/newline escape sequences in jsonpath may not work across all oc versions. Use = and ; as delimiters instead. Add logging when a CSV is found to aid debugging.
kpouget
reviewed
Jul 8, 2026
Per Kevin's review: if an operator CSV is not Succeeded, prepare must fail — continuing will just delay the failure to the test step. Also fix CSV jsonpath parsing to use reliable separators.
- Add operator cleanup following llm-d pattern: deletes operator subscriptions/CSVs while respecting cleanup.preserve_operators config - post_cleanup now runs both namespace and operator cleanup - Add log message when service account is not configured
The cleanup was reading preserve_operators from root config path which doesn't exist (it's under platform.cleanup). This caused ALL operators to be deleted including NFD and GPU which should have been preserved.
ServiceMesh is a cluster prerequisite, not a project concern. Must not be deleted during cleanup.
Contributor
|
@MML-coder can you disable the cleanup of the GPU Operator? |
Pinned channel installed an old GPU operator version incompatible with the cluster's NVIDIA driver. Use stable channel to match llm-d's approach.
All operators (NFD, GPU, ServiceMesh, RHCL, RHOAI) are cluster prerequisites and must not be deleted during pipeline cleanup. Only namespace-level test resources should be cleaned up.
Per Kevin's feedback: RHOAI should be cleaned up so other tests can install their own version. Only GPU operator, NFD, and ServiceMesh are preserved (slow to reinstall / cluster prerequisites).
Contributor
|
@Harshith-umesh I think this one can be closed |
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
3 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.
Summary
prepare_rhaiis.pywith a 9-step prepare sequence:anyuid), vault-based image pull secret, model PVCconfig.d/platform.yamlwith operator specs, DSC config, SCC/PVC/vault settingspsap-rhaiis-image-pullfor container registry credentialsci.pywith preflight CRD validation for KServeimage_pull_secret: rhaiis-image-pull— no longer needs FournosJob overrideCluster-side prerequisite
The vault secret
vault-psap-rhaiis-image-pullmust exist inpsap-secretson psap-automation with thefournos.dev/vault-entry=truelabel. See Fournos README "Adding a project vault secret" section.Test plan
rhaiis-mlflow-full-tfsnxsucceeded on forge-smoke-testingSummary by CodeRabbit
The completed pipellne https://mlflow.apps.psap-automation.ibm.rhperfscale.org/#/experiments/233/runs/34cd9812b0a84004a6b2cfd58d23b436/artifacts?workspace=default
On the forge-smoke-testing, all operators were already installed, so the prepare step validated their presence by checking for existing CSVs and skipped installation: