feat: add prepare and cleanup steps for rhaiis project#106
Conversation
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.
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a CRD preflight gate, new platform and Vault configuration, and a rewritten RHAIIS orchestration flow that prepares operators and per-run namespace, secret, SCC, and PVC resources, then cleans up KServe and benchmark resources. ChangesRHAIIS platform preparation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 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
| prepare: | ||
| scc: | ||
| policy: anyuid | ||
| service_account: sa |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "def ensure_service_account" -A 20 projects/rhaiis/orchestration/prepare_rhaiis.pyRepository: openshift-psap/forge
Length of output: 1016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the SCC application logic
sed -n '208,250p' projects/rhaiis/orchestration/prepare_rhaiis.py
# Find where service_account_name is defined/consumed
rg -n "service_account_name|scc:" projects/rhaiis -g '*.yaml' -g '*.py'
# Read the relevant config file section
sed -n '1,120p' projects/rhaiis/orchestration/config.d/platform.yamlRepository: openshift-psap/forge
Length of output: 3905
Align the SCC service account with the deploy SA
In projects/rhaiis/orchestration/config.d/platform.yaml, prepare.scc.service_account is hardcoded to sa, but rhaiis.deploy.service_account_name defaults to empty and ensure_service_account() skips creation when it’s unset. Use one shared source of truth, or set a non-empty default, so the SCC isn’t applied to a service account that never exists.
🤖 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/config.d/platform.yaml` around lines 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.
| def _operator_csv_exists(namespace: str, package: str) -> bool: | ||
| result = oc( | ||
| "get", "csv", "-n", namespace, | ||
| "-o", "jsonpath={.items[*].metadata.name}", | ||
| check=False, log_stdout=False, | ||
| ) | ||
| if result.returncode != 0: | ||
| oc("create", "namespace", ns) | ||
| logger.info(f"Created namespace {ns}") | ||
| else: | ||
| logger.info(f"Namespace {ns} already exists") | ||
| return False | ||
| csv_names = result.stdout.strip().split() | ||
| return any(package in name for name in csv_names) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Only skip operator installation when the matching CSV is actually Succeeded.
A CSV can exist while still Pending, Installing, or Failed. Lines 87-89 currently treat any name match as installed, bypassing the helper that waits for readiness.
Proposed fix
-def _operator_csv_exists(namespace: str, package: str) -> bool:
+def _operator_csv_succeeded(namespace: str, package: str) -> bool:
result = oc(
"get", "csv", "-n", namespace,
- "-o", "jsonpath={.items[*].metadata.name}",
+ "-o", 'jsonpath={range .items[*]}{.metadata.name}{"\t"}{.status.phase}{"\n"}{end}',
check=False, log_stdout=False,
)
if result.returncode != 0:
return False
- csv_names = result.stdout.strip().split()
- return any(package in name for name in csv_names)
+ for line in result.stdout.strip().splitlines():
+ name, _, phase = line.partition("\t")
+ if package in name and phase == "Succeeded":
+ return True
+ return False
@@
- if _operator_csv_exists(namespace, package):
+ if _operator_csv_succeeded(namespace, package):Also applies to: 87-89
🤖 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 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.
| cluster_deploy_operator.run( | ||
| package_name=package, | ||
| target_namespace=namespace, | ||
| source_name=operator_spec["source"], | ||
| channel=operator_spec["channel"], | ||
| source_namespace=operator_spec.get("source_namespace", "openshift-marketplace"), | ||
| display_name=operator_spec.get("display_name", package), | ||
| artifact_dirname_suffix=f"_{package}", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the unsupported artifact_dirname_suffix argument.
Line 100 passes artifact_dirname_suffix, but the provided cluster_deploy_operator.run() signature does not accept that keyword, so the first missing operator install will raise TypeError before any subscription is created.
Proposed fix
cluster_deploy_operator.run(
package_name=package,
target_namespace=namespace,
source_name=operator_spec["source"],
channel=operator_spec["channel"],
source_namespace=operator_spec.get("source_namespace", "openshift-marketplace"),
display_name=operator_spec.get("display_name", package),
- artifact_dirname_suffix=f"_{package}",
)📝 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.
| cluster_deploy_operator.run( | |
| package_name=package, | |
| target_namespace=namespace, | |
| source_name=operator_spec["source"], | |
| channel=operator_spec["channel"], | |
| source_namespace=operator_spec.get("source_namespace", "openshift-marketplace"), | |
| display_name=operator_spec.get("display_name", package), | |
| artifact_dirname_suffix=f"_{package}", | |
| ) | |
| cluster_deploy_operator.run( | |
| package_name=package, | |
| target_namespace=namespace, | |
| source_name=operator_spec["source"], | |
| channel=operator_spec["channel"], | |
| source_namespace=operator_spec.get("source_namespace", "openshift-marketplace"), | |
| display_name=operator_spec.get("display_name", package), | |
| ) |
🤖 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 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.
There was a problem hiding this comment.
@coderabbitai, this is supported, these parameters are dynamically appended to the function signature
see there:
projects/core/dsl/task.py:514: "artifact_dirname_suffix", inspect.Parameter.KEYWORD_ONLY, default=None, annotation=str
projects/core/dsl/task.py:520: "artifact_dirname_prefix", inspect.Parameter.KEYWORD_ONLY, default=None, annotation=str
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '480,540p' projects/core/dsl/task.pyLength of output: 4301
🏁 Script executed:
#!/bin/bash
sed -n '1,30p' projects/cluster/toolbox/cluster_deploy_operator/main.pyLength of output: 656
Confirmed — run() is decorated with @entrypoint, which dynamically appends artifact_dirname_suffix/artifact_dirname_prefix as keyword-only parameters to the signature (per projects/core/dsl/task.py Lines 514-520). So the call is valid, and my original comment was mistaken. Thanks for the clarification!
🧬 Code Graph Analysis Results
projects/rhaiis/orchestration/prepare_rhaiis.py
Lines 84-102 (calls cluster_deploy_operator.run with artifact_dirname_suffix=...)
def _ensure_operator_subscription(operator_spec: dict[str, str]) -> None:
package = operator_spec["package"]
namespace = operator_spec["namespace"]
if _operator_csv_exists(namespace, package):
logger.info("Operator %s already installed in %s, skipping", package, namespace)
return
from projects.cluster.toolbox.cluster_deploy_operator import main as cluster_deploy_operator
cluster_deploy_operator.run(
package_name=package,
target_namespace=namespace,
source_name=operator_spec["source"],
channel=operator_spec["channel"],
source_namespace=operator_spec.get("source_namespace", "openshift-marketplace"),
display_name=operator_spec.get("display_name", package),
artifact_dirname_suffix=f"_{package}",
)projects/cluster/toolbox/cluster_deploy_operator/main.py
Lines 27-53 (shows run(...) signature as defined; does not list artifact_dirname_suffix explicitly)
def run(
package_name: str,
target_namespace: str,
source_name: str,
channel: str,
*,
source_namespace: str = "openshift-marketplace",
installplan_approval: str = "Automatic",
display_name: str = "",
install_mode: str = "auto",
) -> int:
"""
Deploy an OLM operator and wait for its CSV to succeed.
Args:
package_name: Operator package/subscription name
target_namespace: Namespace where the operator will be installed
source_name: CatalogSource name providing the operator
channel: Subscription channel to use
source_namespace: CatalogSource namespace
installplan_approval: InstallPlan approval mode
display_name: Optional human-friendly name used in logs
install_mode: Install mode - "auto" (default), "namespace-scoped", or "cluster-wide"
"""
execute_tasks(locals())
return 0| oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace, check=False) | ||
| logger.info("Applied SCC %s to SA %s in %s", policy, sa, namespace) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not report SCC application success after a failed oc adm policy command.
Line 216 disables checking, then Line 217 logs success unconditionally. If RBAC/SCC binding fails, prepare still returns success and workloads can later fail admission.
Proposed fix
- oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace, check=False)
+ oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace)
logger.info("Applied SCC %s to SA %s in %s", policy, sa, namespace)📝 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.
| oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace, check=False) | |
| logger.info("Applied SCC %s to SA %s in %s", policy, sa, namespace) | |
| oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace) | |
| logger.info("Applied SCC %s to SA %s in %s", policy, sa, namespace) |
🤖 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 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.
| try: | ||
| dockerconfig_path = vault.get_vault_content_path(vault_name, vault_content) | ||
| except Exception: | ||
| logger.warning("Vault %s not available — cannot create image pull secret", vault_name) | ||
| return | ||
|
|
||
| if not dockerconfig_path or not dockerconfig_path.exists(): | ||
| logger.warning("Vault content %s/%s not found", vault_name, vault_content) | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail prepare when the configured image pull secret cannot be created.
If deploy_cfg.image_pull_secret is set and the secret is absent, missing Vault content is not optional; returning here makes prepare succeed while later image pulls fail.
Proposed fix
try:
dockerconfig_path = vault.get_vault_content_path(vault_name, vault_content)
- except Exception:
- logger.warning("Vault %s not available — cannot create image pull secret", vault_name)
- return
+ except Exception as exc:
+ raise RuntimeError(
+ f"Vault {vault_name} not available — cannot create image pull secret {secret_name}"
+ ) from exc
if not dockerconfig_path or not dockerconfig_path.exists():
- logger.warning("Vault content %s/%s not found", vault_name, vault_content)
- return
+ raise FileNotFoundError(f"Vault content {vault_name}/{vault_content} not found")📝 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.
| try: | |
| dockerconfig_path = vault.get_vault_content_path(vault_name, vault_content) | |
| except Exception: | |
| logger.warning("Vault %s not available — cannot create image pull secret", vault_name) | |
| return | |
| if not dockerconfig_path or not dockerconfig_path.exists(): | |
| logger.warning("Vault content %s/%s not found", vault_name, vault_content) | |
| return | |
| try: | |
| dockerconfig_path = vault.get_vault_content_path(vault_name, vault_content) | |
| except Exception as exc: | |
| raise RuntimeError( | |
| f"Vault {vault_name} not available — cannot create image pull secret {secret_name}" | |
| ) from exc | |
| if not dockerconfig_path or not dockerconfig_path.exists(): | |
| raise FileNotFoundError(f"Vault content {vault_name}/{vault_content} not found") |
🤖 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 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.
| 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.
| 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.
| REQUIRED_CRDS = [ | ||
| "inferenceservices.serving.kserve.io", | ||
| "servingruntimes.serving.kserve.io", | ||
| ] |
There was a problem hiding this comment.
this should be in the config file (it's easier to control the list from there)
| 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)] |
There was a problem hiding this comment.
will be updated with config.project.get_config("somewhere.required_crds") instead of REQUIRED_CRDS
| vaults: | ||
| - psap-forge-notifications | ||
| - psap-forge-mlflow-export | ||
| - psap-rhaiis-image-pull |
There was a problem hiding this comment.
did you define the vault in the OCPCI vault already?
| return 0 | ||
|
|
||
|
|
||
| def cleanup() -> int: |
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
| logger.info("SCC policy not configured, skipping") | ||
| return | ||
|
|
||
| oc("adm", "policy", "add-scc-to-user", policy, "-z", sa, "-n", namespace, check=False) |
There was a problem hiding this comment.
check=False
@MML-coder @Harshith-umesh I'd like to draw your attention on this (aside from the slack comment)
this check=False will swallow an error. If the command fails because the KUBECONFIG isn't kubeadmin or alike, the automation will silently continue, just with an ignored error message lost in the logs, and the test won't fail until an image anyuid image tries to run
(+ @acalhounRH for awareness, as this is a important aspect for the reliability of the overall test harnesses development)
| if not secret_name: | ||
| return |
There was a problem hiding this comment.
you can log here that no secret is configured
| try: | ||
| dockerconfig_path = vault.get_vault_content_path(vault_name, vault_content) | ||
| except Exception: | ||
| logger.warning("Vault %s not available — cannot create image pull secret", vault_name) | ||
| return |
There was a problem hiding this comment.
don't swallow the error.
If the vault is optional, log a message telling that the optional vault is not available
if it's mandatory, fail here
(same for the lines below)
| src_dir = env.ARTIFACT_DIR / "src" | ||
| src_dir.mkdir(parents=True, exist_ok=True) | ||
| oc_apply(src_dir / "image-pull-secret.yaml", manifest) | ||
| logger.info( | ||
| "Created image pull secret %s in %s from vault %s", secret_name, namespace, vault_name | ||
| ) |
There was a problem hiding this comment.
@MML-coder @Harshith-umesh ,
please use oc create secret generic --from-file=...
(will be discussed further on slack)
| 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") |
There was a problem hiding this comment.
please remove the hardcoded default. The values must be in the configuration.
| if not storage_class: | ||
| logger.warning("No storage_class configured for model PVC, skipping creation") | ||
| return |
There was a problem hiding this comment.
remove this guard, make it more explicit, eg with a enabled field.
If enabled, do it and all the fields exist, if not enabled, skip.
This ^^^ is true in the general cases, but here: please don't bail out if there's no storage class. There's a default storage class mechanism in OCP/K8s, so if this field is empty, the default PVC will be picked up by the cluster
| "app.kubernetes.io/managed-by": "forge", | ||
| "forge.openshift.io/project": "rhaiis", | ||
| "forge.openshift.io/preserve": "true", |
There was a problem hiding this comment.
this should be defined in the config file
| ) | ||
| vault_content = prepare_cfg.get("image_pull_secret", {}).get( | ||
| "vault_content", ".dockerconfigjson" | ||
| ) |
There was a problem hiding this comment.
please remove the hardcoded default values
| labels={ | ||
| "app.kubernetes.io/managed-by": "forge", | ||
| "forge.openshift.io/project": "rhaiis", | ||
| }, |
There was a problem hiding this comment.
please move this to the config file
| ensure_test_namespace(ns) | ||
| ensure_service_account(ns, deploy_cfg) | ||
| ensure_scc_policy(ns, prepare_cfg) | ||
| ensure_image_pull_secret(ns, deploy_cfg, prepare_cfg) | ||
| ensure_model_pvc(ns, deploy_cfg, prepare_cfg) |
There was a problem hiding this comment.
those could be:
- either included in the
preflightchecks (in a check-only mode) - or moved to the beginning of the test step, so that all the tests ensure that the cluster has that well prepared. Makes it easier to test on a cluster not prepared by Forge
2 participants
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
Summary by CodeRabbit
New Features
Bug Fixes
Chores
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: