Repository navigation
[llm-d] Make more reliable - #91
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 EarlyReturn control flow to the DSL runtime, new cluster toolbox scripts for LeaderWorkerSet bootstrap and operator cleanup, KServe and GPU bootstrap diagnostics, and llm_d orchestration/config updates for preflight checks, single-path testing, and cleanup behavior. ChangesDSL early-return and retry logging
Cluster operator lifecycle tooling
GPU operator and KServe diagnostics
llm_d orchestration wiring
RHOAI wait and CI text
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
/test fournos llm_d |
|
🔴 Test of 'llm_d prepare' failed after 00 hours 07 minutes 16 seconds 🔴 • Link to the test results. Test configuration: FAILURE REVIEW 001 cluster deploy operator openshift-cert-manager-operator:
|
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 10 minutes 26 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
|
/test fournos llm_d |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
🔴 Test of 'llm_d prepare' failed after 00 hours 07 minutes 42 seconds 🔴 • Link to the test results. Test configuration: FAILURE REVIEW 003 bootstrap lws operator:
|
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 09 minutes 05 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
|
/test fournos llm_d |
|
🔴 Test of 'llm_d prepare' failed after 00 hours 21 minutes 20 seconds 🔴 • Link to the test results. Test configuration: FAILURE REVIEW 010 bootstrap gpu clusterpolicy:
|
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 24 minutes 47 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
7b83bd6 to
4919605
Compare
|
/test fournos llm_d |
|
🔴 Test of 'llm_d test' failed after 00 hours 16 minutes 26 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: TEST DESCRIPTION:
FAILURE REVIEW 002 run smoke request:
|
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 23 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
3de1374 to
ef42ee4
Compare
|
/test fournos llm_d |
|
🔴 Executing of 'llm_d test' failed after 00 hours 05 minutes 46 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: TEST DESCRIPTION:
FAILURE REVIEW 002 run smoke request:
|
|
🔴 Executing of 'fournos_launcher submit' failed after 00 hours 10 minutes 22 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fournos/gitops/base/workflows/pipeline-full.yaml (1)
119-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
job-step-namedoes not match the05-export-artifactstask name.The task is named
05-export-artifactsbut itsjob-step-nameis04__export-artifacts, so the artifact directory (derived fromFOURNOS_STEP_NAMEpertask-forge-step.yaml) will be misnamed and inconsistent with the task. Should be05__export-artifacts.🐛 Proposed fix
- name: job-step-name - value: 04__export-artifacts + value: 05__export-artifacts🤖 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 `@fournos/gitops/base/workflows/pipeline-full.yaml` around lines 119 - 136, The `05-export-artifacts` task in `pipeline-full.yaml` has a mismatched `job-step-name` value, which will cause the artifact directory naming used by `forge-step` and `task-forge-step.yaml` to be inconsistent. Update the `job-step-name` parameter in the `05-export-artifacts` task to match the task’s step number so it follows the same `FOURNOS_STEP_NAME`-derived pattern as the other tasks, using the `05__export-artifacts` naming convention.
🧹 Nitpick comments (6)
fournos/gitops/base/workflows/pipeline-prepare-test.yaml (1)
20-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
job-step-namenumbering on01-prepare.The task
01-preparestill usesjob-step-name: 001__preparewhile the rest of this file moved to the two-digit0N__scheme (e.g.02__preflight,04__export-artifacts). Align it to01__preparefor a consistent artifact-dir naming.🤖 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 `@fournos/gitops/base/workflows/pipeline-prepare-test.yaml` around lines 20 - 37, The `01-prepare` task in the pipeline manifest still uses the old `job-step-name` value `001__prepare`; update it to match the file’s current two-digit naming convention used by other steps like `02__preflight` and `04__export-artifacts`. Make the change in the `job-step-name` param for `01-prepare` so artifact directory naming stays consistent across the workflow.fournos/gitops/base/workflows/pipeline-full.yaml (1)
59-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
02prefix for two distinct tasks.Both
02-preflightand02-testshare the02numbering, breaking the otherwise sequential ordering convention (comparepipeline-prepare-test.yaml, where preflight is02and test is03). Consider renaming the test task to03-testand itsjob-step-nameto03__testfor consistency.🤖 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 `@fournos/gitops/base/workflows/pipeline-full.yaml` around lines 59 - 79, The pipeline task names are using the same 02 prefix for two different steps, which breaks the sequential naming convention. Update the task identifier for the test stage from 02-test to 03-test, and adjust the corresponding job-step-name in the same task from 02__test to 03__test so it matches the ordering used by the pipeline task definitions.projects/kserve/toolbox/prepare_hf_model_cache/main.py (1)
340-343: 🔒 Security & Privacy | 🔵 TrivialPrefer the list form for
shell.runto avoid shell interpolation
shell.runacceptsstr | list[str]and defaults toshell=True. Passing an argument list withshell=Falseremoves the shell interpolation risk and is more robust to unusual characters innamespaceorjob_name.Both values are derived from
cache_spec, which is built deterministically in this module and always includesnamespaceanddownload_job_name.♻️ Proposed change
- shell.run( - f"oc get pods -n {namespace} -l job-name={job_name}", - check=False, - ) + shell.run( + ["oc", "get", "pods", "-n", namespace, "-l", f"job-name={job_name}"], + check=False, + shell=False, + )🤖 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/kserve/toolbox/prepare_hf_model_cache/main.py` around lines 340 - 343, The `shell.run` call in `prepare_hf_model_cache/main.py` is using a shell-form string, which leaves room for shell interpolation; update the `oc get pods` invocation to use the argument-list form with `shell=False`. Keep the same command semantics while passing `namespace` and `job_name` as separate arguments so `shell.run` handles them safely and robustly. Use the existing `shell.run` call site around the pod lookup logic to locate the change.projects/llm_d/toolbox/cleanup_test_resources/main.py (1)
208-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop or persist the unused JSON pod fetch.
This command discards stdout and does not feed the deletion check, so it only adds an extra API-server call on every retry. Keep the plain-text diagnostic, or write the JSON output to an artifact if it is needed.
Proposed cleanup
- shell.run( - f"oc get pods -n {args.namespace} -l app.kubernetes.io/name={args.inference_service_name} -o json", - check=False, - log_stdout=False, - ) -🤖 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/toolbox/cleanup_test_resources/main.py` around lines 208 - 212, The extra JSON pod fetch in the cleanup flow is unused and just adds another API-server call on each retry. Update the retry logic around shell.run in cleanup_test_resources/main.py so the diagnostic remains plain-text only, or persist the JSON output to an artifact if it is actually needed by the deletion check; remove the discarded json call from the retry path.projects/llm_d/orchestration/test_phase.py (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
run_finalizers’ return type with its actual contract.Line 104 returns a pair, so the annotation should describe
(primary_exc, finalizer_exc)rather than one exception tuple.Proposed cleanup
def run_finalizers( endpoint_url: str | None, primary_exc: tuple[type[BaseException], BaseException, Any] | None, finalizer_exc: tuple[type[BaseException], BaseException, Any] | None, -) -> tuple[type[BaseException], BaseException, Any] | None: +) -> tuple[ + tuple[type[BaseException], BaseException, Any] | None, + tuple[type[BaseException], BaseException, Any] | None, +]:Also applies to: 104-104
🤖 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/test_phase.py` around lines 57 - 61, `run_finalizers`’ return annotation does not match what the function actually returns: it returns both `primary_exc` and `finalizer_exc`, not a single exception tuple. Update the `run_finalizers` signature and any related type hints in `test_phase.py` to reflect a pair/tuple of the two exception tuples, and make sure the return statement at the end of `run_finalizers` matches that contract.projects/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/main.py (1)
167-208: 🔒 Security & Privacy | 🔵 TrivialReplace f-string shell commands with argv lists and
shell=FalseThe
shell.run()wrapper explicitly supports list-style arguments and theshellflag. Interpolatingnamespaceinto shell strings needlessly exposes command arguments to shell parsing. Convert all sixocinvocations in this block to use the list form.shell.run( - f"oc get daemonsets -n {namespace} -oyaml", + ["oc", "get", "daemonsets", "-n", namespace, "-oyaml"], + shell=False, stdout_dest=args.artifact_dir / "artifacts" / "gpu-operator-daemonsets.yaml", check=False, )Apply this pattern to all remaining calls in the block.
🤖 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/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/main.py` around lines 167 - 208, The `shell.run()` calls in the GPU operator artifact capture block are still using interpolated shell strings, which should be replaced with argv lists. Update each `oc` invocation in `main.py` to pass command arguments as a list using the `shell.run` wrapper’s list-style form, keeping `namespace` as a separate argument and not embedded in an f-string. Apply this change consistently to the six capture calls in the block, including the `get`, `describe`, and `-owide` commands, so they all run with `shell=False`.
🤖 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 `@fournos/gitops/base/workflows/pipeline-full.yaml`:
- Around line 79-97: The 02-test task in the pipeline is invoking the wrong job
step, so testing never runs and preflight is duplicated. Update the `job-step`
parameter in `pipeline-full.yaml` for the `02-test` task from preflight to test,
keeping the surrounding `forge-step` task configuration and `job-step-name`
as-is so the step name matches the intended test execution.
In `@fournos/gitops/base/workflows/pipeline-prepare-test.yaml`:
- Around line 59-60: The 03-test task in the pipeline workflow has an invalid
runAfter dependency because it points to a task name that does not exist. Update
the runAfter reference on 03-test to match the actual preflight task identifier,
02-preflight, so the Tekton DAG resolves correctly and the pipeline definition
validates.
In `@fournos/gitops/base/workflows/pipeline-test-only.yaml`:
- Line 39: The `01-test` task in the pipeline is missing a `runAfter` dependency
on `00-preflight`, so preflight can run in parallel instead of gating the test.
Update the Tekton workflow definition for `01-test` to depend on `00-preflight`,
following the same pattern used in `pipeline-prepare-only.yaml`, so the
preflight step completes before the test starts.
In `@projects/cluster/toolbox/bootstrap_lws_operator/main.py`:
- Around line 16-22: The `run` function’s `timeout_seconds` parameter is
currently unused because readiness waits are controlled by the fixed retry
decorators in `run` rather than by caller input. Either remove `timeout_seconds`
and its docstring from `run`, or thread it into the waiting logic by replacing
the fixed retries with timeout-based polling in the readiness checks so the
caller can actually influence the wait duration.
In `@projects/cluster/toolbox/cleanup_operators/main.py`:
- Around line 114-124: The per-operator artifact filenames in the cleanup flow
are missing the namespace, so repeated subscription names can overwrite each
other’s diagnostics. Update the filename construction in the `shell.run` calls
that write the CSV and InstallPlan artifacts to include both `subscription_name`
and `namespace`, and apply the same change in the other matching block
referenced by the comment. Keep the existing artifact destinations and command
behavior unchanged.
- Around line 161-175: The cleanup logic in the subscription check is failing
open because it treats an unsuccessful oc get subscription call as zero
subscriptions, which can trigger unsafe deletion of shared OperatorGroups.
Update the subscription-counting flow in cleanup_operators/main.py so the code
using shell.run and remaining_subscriptions first verifies the command succeeded
before inspecting result.stdout, and if it fails, skip deletion and log or raise
an error instead of proceeding to delete operator groups. Keep the deletion
guard around the existing logger.info and oc delete operatorgroup path so
OperatorGroups are only removed when the subscription query is known to be
valid.
In `@projects/cluster/toolbox/cluster_deploy_operator/main.py`:
- Around line 66-112: The early-return logic in check_existing_csv can bypass
validate_parameters, letting reused cluster state hide invalid inputs. Reorder
the task flow so validate_parameters runs before check_existing_csv, ensuring
source_name, channel, approval mode, and install mode are validated first while
keeping the existing EarlyReturn behavior in check_existing_csv unchanged.
- Around line 456-466: The Subscription failure check in the operator logic only
inspects the second entry in conditions, so it can miss a ResolutionFailed
condition when ordering changes. Update the condition handling in main.py to
iterate through all items in subscription_data["status"]["conditions"] and raise
the same ValueError as soon as any condition_type equals ResolutionFailed, using
the matching message and reason from that condition.
In `@projects/core/ci_entrypoint/prepare_ci.py`:
- Line 588: The user-visible notification text built in prepare_ci.py is
inconsistent and ungrammatical, using both “Executing of” and “Execution of”
around the notification_status formatting. Update the status message
construction at the affected notification points so they all use one consistent,
grammatical phrasing (for example, the same “Execution of '{project}
{operation}' ...” form) in the prepare_ci flow, including the logic around
notification_status.
In `@projects/core/dsl/runtime.py`:
- Around line 369-376: The early-return path in runtime.py drops any context
changes because EarlyReturnException is raised in the task handling block before
the shared_context write-back runs. Update the task execution flow around the
EarlyReturn check so the context persistence loop executes before raising
EarlyReturnException, or otherwise ensure context is saved as part of the
early-return path. Use the existing task_status, EarlyReturn, and shared_context
logic in the runtime task runner to keep values set by the task available to
downstream `@always` tasks like capture_csv.
In `@projects/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/main.py`:
- Around line 84-91: The namespace fallback in the ClusterPolicy readiness flow
is getting stuck on the default value instead of refreshing from the latest
status. Update the logic in the ClusterPolicy retry/readiness path, especially
the code that reads `result.stdout` and the loop that rechecks status on each
retry, so `ctx.gpu_operator_namespace` is reassigned whenever
`.status.namespace` becomes available. Keep the existing fallback behavior only
until a real namespace is observed, and make sure the final diagnostics use the
refreshed value from `ctx.gpu_operator_namespace`.
In `@projects/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 136-149: The pod-appearance wait in wait_pods_appear was shortened
too much, which can cause slow KServe/LLMInferenceService reconciliations to
fail before readiness checks run. Update wait_pods_appear in
deploy_llmisvc/main.py to restore a longer wait window or make the retry
duration configurable, keeping the existing service_name, selector, and
oc_get_json flow intact.
- Around line 166-178: The diagnostic artifact writing for the `oc` calls
currently saves only `stdout`, so failures from `describe` and `get replicaset`
can lose the real error details. Update the artifact generation around the
`oc(...)` result handling in `main.py` so the saved files include `stderr`
whenever `check=False` is used, and make sure this is applied to both the
`llminferenceservice` describe path and the replicaset diagnostic path. Use the
existing `result` object and artifact writes like `llmisv_desc_path` to locate
and append the error output alongside the normal output.
In `@projects/llm_d/orchestration/ci.py`:
- Around line 89-96: The pre_cleanup() step is deleting non-preserved operator
subscriptions before prepare(), which forces rhods-operator and related
operators to be torn down and reinstalled on every run. Update the cleanup flow
in pre_cleanup() so full cleanup with cleanup_subscriptions=True is moved to
post_cleanup(), or adjust runtime_config/config.yaml preserve settings to
include rhods-operator, rhcl-operator, and leader-worker-set if they must
survive pre-run cleanup. Make sure the fix is applied in the pre_cleanup and
cleanup_toolbox_run path so prepare() no longer triggers unnecessary operator
churn.
In `@projects/llm_d/orchestration/cleanup_phase.py`:
- Around line 58-74: The cleanup flow in cleanup_phase.py is currently
fail-open: when cleanup_config or preserve_operators is missing,
_identify_operators_to_delete can mark all platform operators for deletion
before cleanup_operators_command.run is called. Update the cleanup logic to
default to preserving operators unless an explicit allowlist/opt-in is present,
and make _identify_operators_to_delete (and any callers in this phase) return no
deletions when the preserve list is absent or incomplete. Ensure the guard
happens before invoking cleanup_operators_command.run so platform operators are
not removed accidentally.
In `@projects/llm_d/orchestration/config.yaml`:
- Line 71: The hardcoded model_key value is invalid and will break model
resolution; update the config entry in the orchestration config to a valid
registry key such as qwen3-0_6b/qwen3-0-6b, and make sure the default MODEL_KEY
in config_review/__init__.py matches the same valid identifier used by
runtime.yaml and the test suite.
---
Outside diff comments:
In `@fournos/gitops/base/workflows/pipeline-full.yaml`:
- Around line 119-136: The `05-export-artifacts` task in `pipeline-full.yaml`
has a mismatched `job-step-name` value, which will cause the artifact directory
naming used by `forge-step` and `task-forge-step.yaml` to be inconsistent.
Update the `job-step-name` parameter in the `05-export-artifacts` task to match
the task’s step number so it follows the same `FOURNOS_STEP_NAME`-derived
pattern as the other tasks, using the `05__export-artifacts` naming convention.
---
Nitpick comments:
In `@fournos/gitops/base/workflows/pipeline-full.yaml`:
- Around line 59-79: The pipeline task names are using the same 02 prefix for
two different steps, which breaks the sequential naming convention. Update the
task identifier for the test stage from 02-test to 03-test, and adjust the
corresponding job-step-name in the same task from 02__test to 03__test so it
matches the ordering used by the pipeline task definitions.
In `@fournos/gitops/base/workflows/pipeline-prepare-test.yaml`:
- Around line 20-37: The `01-prepare` task in the pipeline manifest still uses
the old `job-step-name` value `001__prepare`; update it to match the file’s
current two-digit naming convention used by other steps like `02__preflight` and
`04__export-artifacts`. Make the change in the `job-step-name` param for
`01-prepare` so artifact directory naming stays consistent across the workflow.
In `@projects/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/main.py`:
- Around line 167-208: The `shell.run()` calls in the GPU operator artifact
capture block are still using interpolated shell strings, which should be
replaced with argv lists. Update each `oc` invocation in `main.py` to pass
command arguments as a list using the `shell.run` wrapper’s list-style form,
keeping `namespace` as a separate argument and not embedded in an f-string.
Apply this change consistently to the six capture calls in the block, including
the `get`, `describe`, and `-owide` commands, so they all run with
`shell=False`.
In `@projects/kserve/toolbox/prepare_hf_model_cache/main.py`:
- Around line 340-343: The `shell.run` call in `prepare_hf_model_cache/main.py`
is using a shell-form string, which leaves room for shell interpolation; update
the `oc get pods` invocation to use the argument-list form with `shell=False`.
Keep the same command semantics while passing `namespace` and `job_name` as
separate arguments so `shell.run` handles them safely and robustly. Use the
existing `shell.run` call site around the pod lookup logic to locate the change.
In `@projects/llm_d/orchestration/test_phase.py`:
- Around line 57-61: `run_finalizers`’ return annotation does not match what the
function actually returns: it returns both `primary_exc` and `finalizer_exc`,
not a single exception tuple. Update the `run_finalizers` signature and any
related type hints in `test_phase.py` to reflect a pair/tuple of the two
exception tuples, and make sure the return statement at the end of
`run_finalizers` matches that contract.
In `@projects/llm_d/toolbox/cleanup_test_resources/main.py`:
- Around line 208-212: The extra JSON pod fetch in the cleanup flow is unused
and just adds another API-server call on each retry. Update the retry logic
around shell.run in cleanup_test_resources/main.py so the diagnostic remains
plain-text only, or persist the JSON output to an artifact if it is actually
needed by the deletion check; remove the discarded json call from the retry
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: 8e4e2f9a-4e5a-4b96-ad58-9a10f65452d1
📒 Files selected for processing (33)
fournos/gitops/base/workflows/pipeline-full.yamlfournos/gitops/base/workflows/pipeline-prepare-only.yamlfournos/gitops/base/workflows/pipeline-prepare-test.yamlfournos/gitops/base/workflows/pipeline-test-only.yamlprojects/cluster/toolbox/bootstrap_lws_operator/main.pyprojects/cluster/toolbox/bootstrap_lws_operator/templates/leaderworkersetoperator.yaml.j2projects/cluster/toolbox/cleanup_operators/main.pyprojects/cluster/toolbox/cluster_deploy_operator/main.pyprojects/cluster/toolbox/cluster_deploy_operator/on_failure_helpers.pyprojects/core/agentic/on_failure/__init__.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/dsl/__init__.pyprojects/core/dsl/control_flow.pyprojects/core/dsl/runtime.pyprojects/core/dsl/task.pyprojects/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/main.pyprojects/gpu_operator/toolbox/bootstrap_gpu_clusterpolicy/on_failure_helpers.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/deploy_llmisvc/on_failure_helpers.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/cleanup_phase.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/leaderworkersetoperator.yamlprojects/llm_d/orchestration/preflight_phase.pyprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_profiles.pyprojects/llm_d/toolbox/cleanup_test_resources/main.pyprojects/rhaiis/orchestration/ci.pyprojects/skeleton/orchestration/ci.py
| - name: 03-test | ||
| runAfter: [01-preflight] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
03-test references a nonexistent task in runAfter.
runAfter: [01-preflight] points to a task that does not exist — the preflight task is named 02-preflight (Line 39). Tekton will reject this pipeline with a DAG validation error ("task ... runAfter refers to nonexistent task"). Update the reference to 02-preflight.
🐛 Proposed fix
- name: 03-test
- runAfter: [01-preflight]
+ runAfter: [02-preflight]📝 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.
| - name: 03-test | |
| runAfter: [01-preflight] | |
| - name: 03-test | |
| runAfter: [02-preflight] |
🤖 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 `@fournos/gitops/base/workflows/pipeline-prepare-test.yaml` around lines 59 -
60, The 03-test task in the pipeline workflow has an invalid runAfter dependency
because it points to a task name that does not exist. Update the runAfter
reference on 03-test to match the actual preflight task identifier,
02-preflight, so the Tekton DAG resolves correctly and the pipeline definition
validates.
| - name: job-step-name | ||
| value: 00__preflight | ||
|
|
||
| - name: 01-test |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
01-test has no runAfter on 00-preflight, so preflight does not gate the test.
Without a runAfter dependency, Tekton schedules 00-preflight and 01-test in parallel. If the intent of adding preflight is to validate prerequisites before running the test, the gate is ineffective here (contrast pipeline-prepare-only.yaml, where preflight runAfter: [00-prepare]). Add the dependency.
🐛 Proposed fix
- name: 01-test
+ runAfter: [00-preflight]
taskRef:
name: forge-step📝 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.
| - name: 01-test | |
| - name: 01-test | |
| runAfter: [00-preflight] |
🤖 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 `@fournos/gitops/base/workflows/pipeline-test-only.yaml` at line 39, The
`01-test` task in the pipeline is missing a `runAfter` dependency on
`00-preflight`, so preflight can run in parallel instead of gating the test.
Update the Tekton workflow definition for `01-test` to depend on `00-preflight`,
following the same pattern used in `pipeline-prepare-only.yaml`, so the
preflight step completes before the test starts.
| stdout_dest=artifact_dir / "artifacts" / f"{subscription_name}-csvs.yaml", | ||
| check=False, | ||
| log_stdout=False, | ||
| ) | ||
|
|
||
| # Capture InstallPlans | ||
| shell.run( | ||
| f"oc get installplan -n {namespace} " | ||
| f"-l operators.coreos.com/{subscription_name}.{namespace} " | ||
| "-o yaml", | ||
| stdout_dest=artifact_dir / "artifacts" / f"{subscription_name}-installplans.yaml", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Include namespace in per-operator artifact filenames.
{subscription_name}-csvs.yaml and {subscription_name}-installplans.yaml collide when the same subscription name is cleaned from multiple namespaces, overwriting diagnostics.
🧾 Proposed fix
- stdout_dest=artifact_dir / "artifacts" / f"{subscription_name}-csvs.yaml",
+ stdout_dest=artifact_dir / "artifacts" / f"{namespace}-{subscription_name}-csvs.yaml",
...
- stdout_dest=artifact_dir / "artifacts" / f"{subscription_name}-installplans.yaml",
+ stdout_dest=artifact_dir / "artifacts" / f"{namespace}-{subscription_name}-installplans.yaml",
...
- f.write(f" - {operator_name}-csvs.yaml\n")
- f.write(f" - {operator_name}-installplans.yaml\n")
+ f.write(f" - {namespace}-{operator_name}-csvs.yaml\n")
+ f.write(f" - {namespace}-{operator_name}-installplans.yaml\n")Also applies to: 212-216
🤖 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/cluster/toolbox/cleanup_operators/main.py` around lines 114 - 124,
The per-operator artifact filenames in the cleanup flow are missing the
namespace, so repeated subscription names can overwrite each other’s
diagnostics. Update the filename construction in the `shell.run` calls that
write the CSV and InstallPlan artifacts to include both `subscription_name` and
`namespace`, and apply the same change in the other matching block referenced by
the comment. Keep the existing artifact destinations and command behavior
unchanged.
There was a problem hiding this comment.
@kpouget, understood — leaving this as-is.
| result = oc( | ||
| "describe", | ||
| "llminferenceservice", | ||
| service_name, | ||
| "-n", | ||
| args.namespace, | ||
| log_stdout=False, | ||
| check=False, | ||
| ) | ||
|
|
||
| llmisv_desc_path = artifacts_dir / "llmisv_description.txt" | ||
| with open(llmisv_desc_path, "w", encoding="utf-8") as f: | ||
| f.write(result.stdout) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Include stderr when diagnostic oc calls fail.
These calls use check=False, but the artifacts only write stdout. If oc describe / oc get replicaset fails, the saved file can be empty or say “No replicasets found” while the real cause is in stderr.
Proposed diagnostic artifact fix
- with open(llmisv_desc_path, "w", encoding="utf-8") as f:
- f.write(result.stdout)
+ with open(llmisv_desc_path, "w", encoding="utf-8") as f:
+ f.write(result.stdout)
+ if result.stderr:
+ f.write("\n\n--- stderr ---\n")
+ f.write(result.stderr)- else:
+ elif rs_result.returncode == 0:
f.write("No replicasets found for the service")
+ else:
+ f.write("Failed to list replicasets for the service")
+ if rs_result.stderr:
+ f.write("\n\n--- stderr ---\n")
+ f.write(rs_result.stderr)Also applies to: 201-236
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 176-176: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(llmisv_desc_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/kserve/toolbox/deploy_llmisvc/main.py` around lines 166 - 178, The
diagnostic artifact writing for the `oc` calls currently saves only `stdout`, so
failures from `describe` and `get replicaset` can lose the real error details.
Update the artifact generation around the `oc(...)` result handling in `main.py`
so the saved files include `stderr` whenever `check=False` is used, and make
sure this is applied to both the `llminferenceservice` describe path and the
replicaset diagnostic path. Use the existing `result` object and artifact writes
like `llmisv_desc_path` to locate and append the error output alongside the
normal output.
|
/test fournos llm_d |
|
🔴 Executing of 'llm_d test' failed after 00 hours 06 minutes 16 seconds 🔴 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: TEST DESCRIPTION:
FAILURE REVIEW 002 run smoke request:
|
|
🔴 Executing of 'fournos_launcher submit' failed after 00 hours 10 minutes 38 seconds 🔴 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
|
/test fournos llm_d |
|
🟢 Executing of 'llm_d test' succeeded after 00 hours 03 minutes 10 seconds 🟢 • Link to the test results. • Caliper postprocess completed but no reports generated. Test configuration: TEST DESCRIPTION:
|
…the manual InstallPlan
…ait_for_installplan failure
…ator are ready before deploying
|
🟢 Execution of 'llm_d test' succeeded after 00 hours 06 minutes 54 seconds 🟢 • Link to the test results. • Generated 1 Caliper report(s):
Test configuration: TEST DESCRIPTION:
|
|
🟢 Execution of 'fournos_launcher submit' succeeded after 00 hours 13 minutes 08 seconds 🟢 • Link to the test results. • FOURNOS Job Status:
• MLflow Tracking: • FOURNOS Job Details: • Task Logs:
Test configuration: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: albertoperdomo2 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