test(e2e): introduces e2e test skeleton#774
Conversation
bartoszmajsak
requested review from
Jooho,
brettmthompson,
israel-hdez and
pierDipi
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a comprehensive end-to-end testing framework for the LLMInferenceService, including new test modules, fixtures, diagnostics, and documentation. It also enhances GitHub Actions workflows for CI, modifies the Makefile for CRD readiness, and updates linting and configuration files. Several new utility modules and documentation improvements are included. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Minikube Cluster
participant KServe Controller
participant Test Suite
Developer->>GitHub Actions: Push PR / Manual Dispatch
GitHub Actions->>Minikube Cluster: Setup cluster (with optional MetalLB)
GitHub Actions->>KServe Controller: Deploy CRDs and dependencies
GitHub Actions->>GitHub Actions: Build and upload KServe images (artifacts)
GitHub Actions->>Test Suite: Run E2E tests for LLMInferenceService
Test Suite->>KServe Controller: Create LLMInferenceService resource
KServe Controller->>Minikube Cluster: Deploy model
Test Suite->>Minikube Cluster: Poll service endpoint for response
Test Suite->>KServe Controller: Delete LLMInferenceService resource
Test Suite->>Minikube Cluster: Collect diagnostics if errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-project-automation
Bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
github-project-automation
Bot
moved this from Done
to New/Backlog
in ODH Model Serving Planning
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
|
Not quite sure why the new job is cancelled. I'm on mobile now so it's hard to investigate. Anyone knows? |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
pkg/controller/llmisvc/DEV.md (2)
10-12:kind create clusteruses an invalid flag
kindexpects--name, not-n. The current form fails with “unknown shorthand flag”.-kind create cluster -n "kserve-llm-d" +kind create cluster --name "kserve-llm-d"
234-237: Nested heredoc is syntactically invalid and swaps arg orderThe first
<<EOFnever terminates andjqoperates on a raw string, not JSON. Re-structure into two independent commands:-kubectl apply -n ${NS} -f - <<EOF -STORAGE_INIT_IMG=$(kubectl get cm -n kserve -o jsonpath="{.data.storageInitializer}" inferenceservice-config | jq -r '.image') - -kubectl apply -n ${NS} -f - <<EOF +STORAGE_INIT_IMG=$(kubectl get cm inferenceservice-config -n kserve \ + -ojsonpath='{.data.storageInitializer}') + +kubectl apply -n ${NS} -f - <<EOFClose the YAML heredoc later with a matching
EOF.
♻️ Duplicate comments (1)
test/e2e/llmisvc/test_llm_inference_service.py (1)
83-141: Consider moving helper functions to a dedicated test_utils package.These CRUD helper functions could be reused across different test modules.
🧹 Nitpick comments (15)
.flake8 (1)
19-19: Re-evaluate blanket exclusion
Excluding an entire test tree from linting hides genuine issues (e.g., unused imports, undefined names) that surface only at runtime. Consider narrowing the ignore list or adding a project-level# noqaper file instead..github/actions/kserve-dep-setup/action.yml (1)
16-19: Input added without docs update
enable-lwsis introduced butREADME.mdin.github/actions/kserve-dep-setup/(or equivalent docs) still lists only the original three inputs. Documenting the new flag prevents silent mis-configuration..github/actions/minikube-setup/action.yml (3)
36-36: Missing quotes may break empty default
If the caller leavesaddonsempty, the composite action passes an empty string unquoted to the underlying action. While most shells handle this, YAML → Go template expansion sometimes drops the key. Safer:addons: "${{ inputs.addons }}"
41-63: Trailing spaces & shellcheck nits
Static analysis flagged trailing spaces on the blank line afterEND=${PREFIX}.235(line 49). It’s harmless but fails strict linters. Also consider quoting vars and usingset -euo pipefailinside the run block to surface errors.- END=${PREFIX}.235 - + END=${PREFIX}.235
41-63: Race: MetalLB controller may not be ready
Immediately applying the ConfigMap can fail withno matches for kind "ConfigMap" in version "v1"if the addon pods aren’t up. Insert a readiness wait:+ kubectl wait --for=condition=Available --timeout=120s -n metallb-system deployment/controllerbefore applying the ConfigMap.
test/e2e/pytest.ini (1)
20-20: Marker added – remember to gate CI jobs
llminferenceservicemarker is in place. Ensure the new GH workflow only selects this marker, otherwise unrelated jobs may start running the heavy e2e suite.test/e2e/llmisvc/conftest.py (2)
18-21: Add typing / scope to improve fixture clarityMinor polish:
-@pytest.fixture -def config_names(request): - return request.param +from typing import Any, List + +@pytest.fixture(scope="function") +def config_names(request) -> Any: # or -> str | List[str] once decided + """Param-driven fixture that yields the config name(s) under test.""" + return request.param– Typing aids IDE completion.
– Explicitscope="function"communicates intent.
23-26: Avoid redefining markers provided by plugins
pytest-asyncioalready registers theasynciomarker; redefining it can trigger
duplicate-registration warnings when--strict-markersis enabled globally.
Since only the customllminferenceservicemarker is needed here, drop the extra one:- "markers", "llminferenceservice: mark test as an LLM inference service test" + "markers", "llminferenceservice: mark test as an LLM inference service test"test/e2e/llmisvc/pytest.ini (1)
11-14: Redundantasynciomarker & trailing space
pytest-asyncioships the marker; listing it here is unnecessary and may go
out of sync with the plugin’s description. Also, there’s a stray trailing space
on theasyncio_modeline that could annoy strict linters.-markers = - llminferenceservice: LLM inference service tests - asyncio: AsyncIO tests +markers = + llminferenceservice: LLM inference service tests @@ -asyncio_mode = auto +asyncio_mode = autotest/e2e/llmisvc/test_configs.py (1)
121-127: Use contextlib.suppress for cleaner exception handling.The teardown code can be simplified using contextlib.suppress.
+import contextlib import os import pytest- # teardown: best‑effort cleanup - for name, namespace in created: - try: - delete_llmisvc_config(client, name, namespace) - except Exception: - pass + # teardown: best‑effort cleanup + for name, namespace in created: + with contextlib.suppress(Exception): + delete_llmisvc_config(client, name, namespace).github/workflows/e2e-test-llmisvc.yml (1)
69-69: Remove trailing spaces.Line 69 contains trailing spaces that should be removed.
- go-version-file: go.mod - + go-version-file: go.mod +test/e2e/llmisvc/test_llm_inference_service.py (1)
24-29: Remove unused import.The
llm_config_factoryis passed as a fixture parameter, so the import is unnecessary.from .test_configs import ( LLMINFERENCESERVICE_CONFIGS, generate_test_id, - llm_config_factory, KSERVE_TEST_NAMESPACE, )test/e2e/llmisvc/README.md (3)
5-9: Example lacks context on where the fragments liveReaders unfamiliar with the repo won’t know where to find
router-managed,workload-single-cpu, etc. Add a short sentence pointing totest/e2e/llmisvc/test_configs.pyso newcomers can locate and inspect the fragments quickly.
14-16: Shell example will break on un-escaped quotesThe command string contains nested single quotes inside single quotes:
pytest -m "llminferenceservice and cpu"When users copy-paste this into bash the inner quotes are fine, but in
run-e2e-tests.shthe marker is injected into another single-quoted string. The safe form is:-pytest -m "llminferenceservice and cpu" +pytest -m 'llminferenceservice and cpu'
27-28: Mention the auto-stripped prefix rule only onceLines 27-28 repeat information already given in “Config Naming Convention”. Consider removing to avoid drift when one list is updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.flake8(1 hunks).github/actions/kserve-dep-setup/action.yml(2 hunks).github/actions/minikube-setup/action.yml(2 hunks).github/workflows/e2e-test-llmisvc.yml(1 hunks)Makefile(1 hunks)pkg/controller/llmisvc/DEV.md(2 hunks)test/e2e/llmisvc/README.md(1 hunks)test/e2e/llmisvc/conftest.py(1 hunks)test/e2e/llmisvc/pytest.ini(1 hunks)test/e2e/llmisvc/test_configs.py(1 hunks)test/e2e/llmisvc/test_llm_inference_service.py(1 hunks)test/e2e/pytest.ini(1 hunks)test/scripts/gh-actions/run-e2e-tests.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file `test/overlays/llm-istio-experimental/istio_base.yaml` is considered external and irrelevant for review in this repository.
Learnt from: hdefazio
PR: opendatahub-io/kserve#658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
.flake8 (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
test/e2e/pytest.ini (3)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
test/e2e/llmisvc/pytest.ini (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Makefile (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
test/e2e/llmisvc/README.md (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
.github/actions/kserve-dep-setup/action.yml (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
pkg/controller/llmisvc/DEV.md (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
test/e2e/llmisvc/test_configs.py (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
.github/workflows/e2e-test-llmisvc.yml (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
test/e2e/llmisvc/test_llm_inference_service.py (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
🧬 Code Graph Analysis (2)
test/e2e/llmisvc/test_configs.py (3)
python/kserve/kserve/exceptions.py (1)
ApiException(98-123)python/kserve/kserve/api/kserve_client.py (2)
KServeClient(31-834)get(166-223)test/e2e/llmisvc/conftest.py (1)
config_names(19-20)
test/e2e/llmisvc/test_llm_inference_service.py (5)
python/kserve/kserve/api/kserve_client.py (2)
KServeClient(31-834)get(166-223)test/e2e/llmisvc/test_configs.py (2)
generate_test_id(62-70)llm_config_factory(74-126)test/e2e/llmisvc/conftest.py (1)
config_names(19-20)python/kserve/kserve/utils/utils.py (1)
get_isvc_namespace(49-50)python/kserve/kserve/exceptions.py (1)
ApiException(98-123)
🪛 YAMLlint (1.37.1)
.github/actions/minikube-setup/action.yml
[error] 49-49: trailing spaces
(trailing-spaces)
.github/workflows/e2e-test-llmisvc.yml
[error] 69-69: trailing spaces
(trailing-spaces)
🪛 Ruff (0.12.2)
test/e2e/llmisvc/test_configs.py
123-126: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
test/e2e/llmisvc/test_llm_inference_service.py
27-27: .test_configs.llm_config_factory imported but unused
Remove unused import: .test_configs.llm_config_factory
(F401)
44-44: Redefinition of unused llm_config_factory from line 27
(F811)
🪛 actionlint (1.7.7)
.github/workflows/e2e-test-llmisvc.yml
120-120: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:21: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:5:13: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-llmisvc
🔇 Additional comments (6)
.flake8 (1)
19-19: llmisvc exclude pattern already covers nested files
The current exclude entrytest/e2e/llmisvc/**.pywill match all
.pyfiles undertest/e2e/llmisvc/at any depth, so switching totest/e2e/llmisvc/**/*.pyis redundant. No change needed.Likely an incorrect or invalid review comment.
.github/actions/minikube-setup/action.yml (1)
17-20: Validate upstream support foraddonskey
medyagh/setup-minikubeexpects anaddonsinput only on versions ≥1.20.0. If runners pull an older cached action revision, the workflow will fail early. Pin to a commit SHA that is known to support this field or add a pre-flight version check.test/scripts/gh-actions/run-e2e-tests.sh (1)
41-42: Helpful echo – nice touch
Echoing the full pytest command greatly aids debugging when tests hang in CI logs.test/e2e/llmisvc/test_configs.py (1)
24-59: Well-structured test configurations!The modular configuration design allows flexible composition of workload, model, and router specifications for comprehensive testing scenarios.
test/e2e/llmisvc/test_llm_inference_service.py (1)
247-290: Excellent diagnostic collection implementation!The function provides comprehensive debugging information including the service state and recent events, which will be invaluable for troubleshooting test failures.
pkg/controller/llmisvc/DEV.md (1)
15-15: Background helper binary may not be on PATHAfter
go install, the binary lands in$(go env GOPATH)/bin, which is not always on$PATH. Either export that directory or invoke it explicitly:-cloud-provider-kind > /dev/null 2>&1 & +$(go env GOPATH)/bin/cloud-provider-kind > /dev/null 2>&1 &
| else | ||
| echo "Selected network layer ${{ inputs.network-layer }}" | ||
| ./test/scripts/gh-actions/setup-deps.sh ${{ inputs.deployment-mode }} "${{ inputs.network-layer }}" "${{ inputs.enable-keda }}" | ||
| ./test/scripts/gh-actions/setup-deps.sh ${{ inputs.deployment-mode }} "${{ inputs.network-layer }}" "${{ inputs.enable-keda }}" "${{ inputs.enable-lws }}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Arg list length changed – confirm script signature
setup-deps.sh is now invoked with four args. Ensure the shell script was updated accordingly; otherwise the fourth parameter will be silently ignored or shift positional parameters, breaking existing logic.
🏁 Script executed:
#!/bin/bash
# List the first lines of the helper script to verify it expects 4 args
head -n 20 test/scripts/gh-actions/setup-deps.shLength of output: 828
Update setup-deps.sh to handle four parameters
The action is now invoking the script with four args, but the script’s header and parameter parsing only expect two. This will misalign $1/$2 and drop $3/$4.
Please update:
- test/scripts/gh-actions/setup-deps.sh
- Extend the usage banner to include the new flags
- Add parsing logic for
$3(enable-keda) and$4(enable-lws)
Example diff to apply at the top of setup-deps.sh:
- # Usage: setup-deps.sh $DEPLOYMENT_MODE $NETWORK_LAYER
+ # Usage: setup-deps.sh $DEPLOYMENT_MODE $NETWORK_LAYER $ENABLE_KEDA $ENABLE_LWSAnd then consume the extra params in the script, e.g.:
DEPLOYMENT_MODE=$1
NETWORK_LAYER=$2
ENABLE_KEDA=$3
ENABLE_LWS=$4🤖 Prompt for AI Agents
In .github/actions/kserve-dep-setup/action.yml at line 33, the script
setup-deps.sh is called with four parameters, but the script currently only
expects two. To fix this, update test/scripts/gh-actions/setup-deps.sh by
extending its usage banner to document all four parameters and modify the
parameter parsing logic to assign $3 to ENABLE_KEDA and $4 to ENABLE_LWS
variables, ensuring all four inputs are correctly handled within the script.
| run: | | ||
| sudo mkdir -p ${DOCKER_IMAGES_PATH} | ||
| sudo chown -R $USER ${DOCKER_IMAGES_PATH} | ||
| ./test/scripts/gh-actions/build-images.sh | ||
| docker image ls | ||
| sudo ls -lh ${DOCKER_IMAGES_PATH} |
There was a problem hiding this comment.
Fix shell variable quoting issues.
The shell script execution has unquoted variables that could cause word splitting issues.
- sudo mkdir -p ${DOCKER_IMAGES_PATH}
- sudo chown -R $USER ${DOCKER_IMAGES_PATH}
+ sudo mkdir -p "${DOCKER_IMAGES_PATH}"
+ sudo chown -R "$USER" "${DOCKER_IMAGES_PATH}"
./test/scripts/gh-actions/build-images.sh
docker image ls
- sudo ls -lh ${DOCKER_IMAGES_PATH}
+ sudo ls -lh "${DOCKER_IMAGES_PATH}"📝 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.
| run: | | |
| sudo mkdir -p ${DOCKER_IMAGES_PATH} | |
| sudo chown -R $USER ${DOCKER_IMAGES_PATH} | |
| ./test/scripts/gh-actions/build-images.sh | |
| docker image ls | |
| sudo ls -lh ${DOCKER_IMAGES_PATH} | |
| run: | | |
| sudo mkdir -p "${DOCKER_IMAGES_PATH}" | |
| sudo chown -R "$USER" "${DOCKER_IMAGES_PATH}" | |
| ./test/scripts/gh-actions/build-images.sh | |
| docker image ls | |
| sudo ls -lh "${DOCKER_IMAGES_PATH}" |
🧰 Tools
🪛 actionlint (1.7.7)
120-120: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:21: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:5:13: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
In .github/workflows/e2e-test-llmisvc.yml around lines 120 to 125, the shell
variables like ${DOCKER_IMAGES_PATH} and $USER are unquoted, which can lead to
word splitting or globbing issues if the variables contain spaces or special
characters. Fix this by adding double quotes around all variable references in
the shell commands, for example, change ${DOCKER_IMAGES_PATH} to
"${DOCKER_IMAGES_PATH}" and $USER to "$USER" to ensure safe and correct variable
expansion.
| minikube start --cpus='12' --memory='16G' --kubernetes-version=v1.33.1 | ||
| minikube addons enable metallb | ||
|
|
||
| # You need to configure metallb with an IP range. This depends on the minikube network. | ||
| # You can find your current minikube ip with: | ||
| # $ minikube ip | ||
| # 192.168.39.118 | ||
| # | ||
| # With the previous sample output, you would configure metallb with a range not including | ||
| # the minikube IP (change only the last entry). E.g: | ||
| minikube addons configure metallb | ||
| # Minikube will ask two prompts. Notice the configured range 192.168.39.200-192.168.39.235 is | ||
| # not including minikube IP: | ||
| # -- Enter Load Balancer Start IP: 192.168.39.200 | ||
| # -- Enter Load Balancer End IP: 192.168.39.235 | ||
| IP=$(minikube ip) | ||
| PREFIX=${IP%.*} | ||
| START=${PREFIX}.200 | ||
| END=${PREFIX}.235 | ||
|
|
||
| kubectl apply -f - <<EOF | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| namespace: metallb-system | ||
| name: config | ||
| data: | ||
| config: | | ||
| address-pools: | ||
| - name: default | ||
| protocol: layer2 | ||
| addresses: | ||
| - ${START}-${END} | ||
| EOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coding Kubernetes v1.33.1 risks future breakage
- 1.33.1 does not exist today; minikube will fail as soon as a new minor is released.
- MetalLB address range derivation silently assumes
/24.
Consider parameterising the version and adding error handling:
-minikube start --cpus='12' --memory='16G' --kubernetes-version=v1.33.1
+MINIKUBE_K8S_VERSION=${MINIKUBE_K8S_VERSION:-v1.32.4}
+minikube start --cpus=12 --memory=16G --kubernetes-version "${MINIKUBE_K8S_VERSION}"Document the /24 assumption or compute the CIDR with ipcalc for clarity.
📝 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.
| minikube start --cpus='12' --memory='16G' --kubernetes-version=v1.33.1 | |
| minikube addons enable metallb | |
| # You need to configure metallb with an IP range. This depends on the minikube network. | |
| # You can find your current minikube ip with: | |
| # $ minikube ip | |
| # 192.168.39.118 | |
| # | |
| # With the previous sample output, you would configure metallb with a range not including | |
| # the minikube IP (change only the last entry). E.g: | |
| minikube addons configure metallb | |
| # Minikube will ask two prompts. Notice the configured range 192.168.39.200-192.168.39.235 is | |
| # not including minikube IP: | |
| # -- Enter Load Balancer Start IP: 192.168.39.200 | |
| # -- Enter Load Balancer End IP: 192.168.39.235 | |
| IP=$(minikube ip) | |
| PREFIX=${IP%.*} | |
| START=${PREFIX}.200 | |
| END=${PREFIX}.235 | |
| kubectl apply -f - <<EOF | |
| apiVersion: v1 | |
| kind: ConfigMap | |
| metadata: | |
| namespace: metallb-system | |
| name: config | |
| data: | |
| config: | | |
| address-pools: | |
| - name: default | |
| protocol: layer2 | |
| addresses: | |
| - ${START}-${END} | |
| EOF | |
| MINIKUBE_K8S_VERSION=${MINIKUBE_K8S_VERSION:-v1.32.4} | |
| minikube start --cpus=12 --memory=16G --kubernetes-version "${MINIKUBE_K8S_VERSION}" | |
| minikube addons enable metallb | |
| IP=$(minikube ip) | |
| PREFIX=${IP%.*} | |
| START=${PREFIX}.200 | |
| END=${PREFIX}.235 | |
| kubectl apply -f - <<EOF | |
| apiVersion: v1 | |
| kind: ConfigMap | |
| metadata: | |
| namespace: metallb-system | |
| name: config | |
| data: | |
| config: | | |
| address-pools: | |
| - name: default | |
| protocol: layer2 | |
| addresses: | |
| - ${START}-${END} | |
| EOF |
🤖 Prompt for AI Agents
In pkg/controller/llmisvc/DEV.md between lines 21 and 42, the Kubernetes version
is hard-coded to v1.33.1, which does not exist and will cause minikube to fail;
also, the MetalLB address range derivation assumes a /24 subnet without
validation. To fix this, parameterize the Kubernetes version to allow easy
updates, add error handling to catch failures when starting minikube, and either
document the /24 subnet assumption clearly or replace the address range
calculation with a method that computes the CIDR dynamically using a tool like
ipcalc.
There was a problem hiding this comment.
@bartoszmajsak I understand this is example documentation. However, even examples benefit from being functional since users often copy-paste them directly. The v1.33.1 version will cause immediate minikube startup failures, which could frustrate developers trying to follow the guide.
That said, if you prefer to keep it as-is for the example, that's your call. The main functional concern is just that users won't be able to successfully run the minikube command as written.
🧠 Learnings used
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file `test/overlays/llm-istio-experimental/istio_base.yaml` is considered external and irrelevant for review in this repository.
Learnt from: hdefazio
PR: opendatahub-io/kserve#658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
There was a problem hiding this comment.
Show me how to fix MetalLB
There was a problem hiding this comment.
@bartoszmajsak Here are a few approaches to fix the MetalLB configuration issue:
Option 1: Simple fix with clear documentation
# Document the /24 assumption and add basic validation
IP=$(minikube ip)
echo "Minikube IP: $IP"
# This assumes a /24 network (255.255.255.0).
# If your minikube uses a different subnet, adjust accordingly.
PREFIX=${IP%.*}
START=${PREFIX}.200
END=${PREFIX}.235
echo "MetalLB range: $START-$END"
kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
namespace: metallb-system
name: config
data:
config: |
address-pools:
- name: default
protocol: layer2
addresses:
- ${START}-${END}
EOFOption 2: Robust solution using network calculation
IP=$(minikube ip)
echo "Minikube IP: $IP"
# Get the network interface and calculate proper range
NETWORK_INFO=$(ip route | grep "$(echo $IP | cut -d. -f1-3)" | head -1)
CIDR=$(echo $NETWORK_INFO | awk '{print $1}')
# Extract network and calculate range (assuming we want last 36 IPs)
NETWORK=$(echo $CIDR | cut -d/ -f1 | cut -d. -f1-3)
START="${NETWORK}.200"
END="${NETWORK}.235"
echo "Detected network: $CIDR"
echo "MetalLB range: $START-$END"
kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
namespace: metallb-system
name: config
data:
config: |
address-pools:
- name: default
protocol: layer2
addresses:
- ${START}-${END}
EOFOption 3: Using ipcalc for proper CIDR handling
# Requires ipcalc to be installed: apt-get install ipcalc
IP=$(minikube ip)
echo "Minikube IP: $IP"
# Get network info from minikube
SUBNET=$(docker network inspect minikube | jq -r '.[0].IPAM.Config[0].Subnet' 2>/dev/null || echo "192.168.49.0/24")
NETWORK=$(echo $SUBNET | cut -d/ -f1 | cut -d. -f1-3)
START="${NETWORK}.200"
END="${NETWORK}.235"
echo "Detected subnet: $SUBNET"
echo "MetalLB range: $START-$END"
kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
namespace: metallb-system
name: config
data:
config: |
address-pools:
- name: default
protocol: layer2
addresses:
- ${START}-${END}
EOFRecommendation: Option 1 is probably sufficient for documentation since it's simple and works for the vast majority of default minikube setups. The key improvement is adding the comment that explains the /24 assumption.
🧠 Learnings used
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file `test/overlays/llm-istio-experimental/istio_base.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in `test/overlays/llm-istio-experimental/` directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: brettmthompson
PR: opendatahub-io/kserve#709
File: pkg/controller/v1alpha1/utils/utils.go:118-128
Timestamp: 2025-06-30T22:30:38.045Z
Learning: In pkg/controller/v1alpha1/utils/utils.go, the condition `minReplicas == nil && constants.DefaultMinReplicas == 0` in ValidateInitialScaleAnnotation function is intentionally kept even though currently unreachable (DefaultMinReplicas = 1) to handle potential future changes to DefaultMinReplicas value, implementing defensive programming to limit future code changes.
Learnt from: hdefazio
PR: opendatahub-io/kserve#658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
test/scripts/gh-actions/run-e2e-tests.sh (1)
31-34: Default value contains stray quotes – breaks string comparisons
NETWORK_LAYER="${3:-'istio'}"sets the default to the literal string'istio'(including quotes).
The later comparison toistio-ingressand the pytest arg will never match as intended.-NETWORK_LAYER="${3:-'istio'}" +NETWORK_LAYER="${3:-istio}"
♻️ Duplicate comments (1)
test/e2e/llmisvc/test_llm_inference_service.py (1)
83-141: Consider moving helper methods to a dedicated test_utils packageThese CRUD operations could be reused across different test modules.
Would you like me to help create a test_utils package to organize these reusable helper functions?
🧹 Nitpick comments (8)
.github/actions/minikube-setup/action.yml (1)
49-49: Trailing spaces break yamllint & can block workflow runsLine 49 has trailing whitespace. Remove it to keep the action YAML-lint-clean.
- +test/scripts/gh-actions/run-e2e-tests.sh (1)
41-42: Minor: log statement duplicates command ‑ consider using$@styleNot critical, but you could simply
echo "pytest $PYTEST_OPTS"to keep the command string single-sourced.test/e2e/llmisvc/conftest.py (2)
18-21: Consider giving the fixture a broader scope or documenting intent
config_namesis instantiated for every single test invocation.
If the underlying object is immutable you can lower test-startup overhead by scoping it tosession:-@pytest.fixture +@pytest.fixture(scope="session") def config_names(request):At minimum, add a short docstring so future contributors understand why the default scope is required.
23-26: Registering marker in code and ini is redundantThe same
llminferenceservicemarker string is also present inpytest.ini.
Defining it in one place (preferably the ini) is sufficient and avoids silent drift.test/e2e/llmisvc/pytest.ini (1)
14-14: Trailing whitespace – minor but noisy in diffsThere is a single trailing space after
auto.
While the directory is Flake8-excluded, removing it avoids future “noise only” changes.-asyncio_mode = auto␠ +asyncio_mode = autotest/e2e/llmisvc/README.md (1)
14-16: Shell quoting in example could confuse users
pytest -m "llminferenceservice and cpu"will not match the marker set via@pytest.mark.llminferenceservice(type="cpu").
Correct invocation is:pytest -m "llminferenceservice and type_cpu"(or use
-kpattern matching). Consider revising the README snippets.test/e2e/llmisvc/test_configs.py (1)
121-127: Consider using contextlib.suppress for cleaner exception handlingThe cleanup logic can be simplified using
contextlib.suppress.+from contextlib import suppress + # teardown: best‑effort cleanup for name, namespace in created: - try: + with suppress(Exception): delete_llmisvc_config(client, name, namespace) - except Exception: - pass.github/workflows/e2e-test-llmisvc.yml (1)
69-69: Remove trailing spacesThere are trailing spaces at the end of this line.
- go-version-file: go.mod - + go-version-file: go.mod
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.flake8(1 hunks).github/actions/kserve-dep-setup/action.yml(2 hunks).github/actions/minikube-setup/action.yml(2 hunks).github/workflows/e2e-test-llmisvc.yml(1 hunks)Makefile(1 hunks)pkg/controller/llmisvc/DEV.md(2 hunks)test/e2e/llmisvc/README.md(1 hunks)test/e2e/llmisvc/conftest.py(1 hunks)test/e2e/llmisvc/pytest.ini(1 hunks)test/e2e/llmisvc/test_configs.py(1 hunks)test/e2e/llmisvc/test_llm_inference_service.py(1 hunks)test/e2e/pytest.ini(1 hunks)test/scripts/gh-actions/run-e2e-tests.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file `test/overlays/llm-istio-experimental/istio_base.yaml` is considered external and irrelevant for review in this repository.
Learnt from: hdefazio
PR: opendatahub-io/kserve#658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in `test/overlays/llm-istio-experimental/` directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
test/e2e/llmisvc/README.md (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
Makefile (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
test/e2e/llmisvc/pytest.ini (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
.github/actions/kserve-dep-setup/action.yml (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
test/e2e/pytest.ini (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
pkg/controller/llmisvc/DEV.md (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
.flake8 (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
.github/workflows/e2e-test-llmisvc.yml (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
test/e2e/llmisvc/test_configs.py (2)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: israel-hdez
PR: #738
File: pkg/apis/serving/v1beta1/configmap.go:0-0
Timestamp: 2025-07-17T16:19:36.771Z
Learning: In pkg/apis/serving/v1beta1/configmap.go, the GetStorageInitializerConfigs function was moved from another package to prevent circular imports, which explains why it may contain existing patterns rather than being newly written code.
test/e2e/llmisvc/test_llm_inference_service.py (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
🪛 YAMLlint (1.37.1)
.github/actions/minikube-setup/action.yml
[error] 49-49: trailing spaces
(trailing-spaces)
.github/workflows/e2e-test-llmisvc.yml
[error] 69-69: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/e2e-test-llmisvc.yml
120-120: shellcheck reported issue in this script: SC2086:info:1:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:15: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:2:21: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:5:13: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Ruff (0.12.2)
test/e2e/llmisvc/test_configs.py
123-126: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
test/e2e/llmisvc/test_llm_inference_service.py
27-27: .test_configs.llm_config_factory imported but unused
Remove unused import: .test_configs.llm_config_factory
(F401)
44-44: Redefinition of unused llm_config_factory from line 27
(F811)
🔇 Additional comments (13)
test/e2e/pytest.ini (1)
20-20: Marker addition looks goodNew marker is correctly declared; no issues.
.github/actions/kserve-dep-setup/action.yml (2)
16-19: Nice addition – new input is well-documented
33-34: No action needed: downstream script already supports four parameters
Thesetup-deps.shscript defines and defaultsENABLE_LWS="${4:-'false'}", so it safely consumes the fourth argument. Passing${{ inputs.enable-lws }}will not break the action.test/e2e/llmisvc/test_configs.py (5)
1-23: LGTM!The license header and imports are appropriate for the test configuration module.
24-59: LGTM!The test configurations cover different scenarios effectively - workload, model, and router configurations.
62-71: LGTM!The test ID generation logic is clear and produces readable identifiers.
129-149: LGTM!The create function properly handles API calls with clear error messages.
151-185: LGTM!The delete and get functions are implemented consistently with proper error handling.
.github/workflows/e2e-test-llmisvc.yml (2)
1-13: LGTM!The workflow triggers and paths configuration are appropriate for E2E testing.
14-53: LGTM!The environment variables are well-organized with consistent naming conventions.
test/e2e/llmisvc/test_llm_inference_service.py (3)
34-81: LGTM!The test function is well-structured with proper parametrization, error handling, and resource cleanup.
143-225: LGTM!The model readiness check and wait utility are well-implemented with proper timeout handling.
227-290: LGTM!The utility functions are well-designed, especially the diagnostic collection which will be valuable for debugging test failures.
| - name: Configure MetalLB for Minikube | ||
| if: ${{ contains(inputs.addons, 'metallb') }} | ||
| shell: bash | ||
| run: | | ||
| IP=$(minikube ip) | ||
| PREFIX=${IP%.*} | ||
| START=${PREFIX}.200 | ||
| END=${PREFIX}.235 | ||
|
|
||
| kubectl apply -f - <<EOF | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| namespace: metallb-system | ||
| name: config | ||
| data: | ||
| config: | | ||
| address-pools: | ||
| - name: default | ||
| protocol: layer2 | ||
| addresses: | ||
| - ${START}-${END} | ||
| EOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race condition: MetalLB CRs applied before namespace exists
metallb-system namespace is created asynchronously by minikube addons.
If the namespace isn’t ready when this step runs, the kubectl apply will fail and the whole job stops.
Minimal fix – wait for the namespace:
- IP=$(minikube ip)
+ # Wait until the metallb-system namespace appears (max 60s)
+ for i in {1..12}; do
+ kubectl get ns metallb-system && break || sleep 5
+ done
+
+ IP=$(minikube ip)📝 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: Configure MetalLB for Minikube | |
| if: ${{ contains(inputs.addons, 'metallb') }} | |
| shell: bash | |
| run: | | |
| IP=$(minikube ip) | |
| PREFIX=${IP%.*} | |
| START=${PREFIX}.200 | |
| END=${PREFIX}.235 | |
| kubectl apply -f - <<EOF | |
| apiVersion: v1 | |
| kind: ConfigMap | |
| metadata: | |
| namespace: metallb-system | |
| name: config | |
| data: | |
| config: | | |
| address-pools: | |
| - name: default | |
| protocol: layer2 | |
| addresses: | |
| - ${START}-${END} | |
| EOF | |
| - name: Configure MetalLB for Minikube | |
| if: ${{ contains(inputs.addons, 'metallb') }} | |
| shell: bash | |
| run: | | |
| # Wait until the metallb-system namespace appears (max 60s) | |
| for i in {1..12}; do | |
| kubectl get ns metallb-system && break || sleep 5 | |
| done | |
| IP=$(minikube ip) | |
| PREFIX=${IP%.*} | |
| START=${PREFIX}.200 | |
| END=${PREFIX}.235 | |
| kubectl apply -f - <<EOF | |
| apiVersion: v1 | |
| kind: ConfigMap | |
| metadata: | |
| namespace: metallb-system | |
| name: config | |
| data: | |
| config: | | |
| address-pools: | |
| - name: default | |
| protocol: layer2 | |
| addresses: | |
| - ${START}-${END} | |
| EOF |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 49-49: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/actions/minikube-setup/action.yml between lines 41 and 63, the
MetalLB configuration is applied before ensuring the 'metallb-system' namespace
exists, causing a race condition. Fix this by adding a wait loop or command
before the kubectl apply step to check for the existence of the 'metallb-system'
namespace and only proceed once it is confirmed to exist, preventing the apply
command from failing.
| minikube start --cpus='12' --memory='16G' --kubernetes-version=v1.33.1 | ||
| minikube addons enable metallb |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Kubernetes 1.33 does not exist yet
minikube start --kubernetes-version=v1.33.1 will fail for anyone following the guide today.
Use a released version (e.g. v1.30.x) or reference latest to avoid hard breakage.
🤖 Prompt for AI Agents
In pkg/controller/llmisvc/DEV.md around lines 21 to 22, the Kubernetes version
specified as v1.33.1 in the minikube start command does not exist yet and will
cause the command to fail. Update the Kubernetes version to a currently released
version such as v1.30.x or use 'latest' to ensure the command works for users
following the guide.
|
/approve |
@bartoszmajsak cancellation message is below |
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
kustomize manifests are significantly different making it impossible to re-use upstream e2e github action without major restructuring Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
with default exclusion for secrets Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
bartoszmajsak
force-pushed
the
llm-d/feature/basic-e2e-test
branch
from
bf97f64 to
5049f6a
Compare
|
New changes are detected. LGTM label has been removed. |
bartoszmajsak
force-pushed
the
llm-d/feature/basic-e2e-test
branch
from
5049f6a to
7e35fc7
Compare
Previous design of test fixtures did not work for xdict when multilple worker nodes were execution the same parametrized test due to limitation of session-scope fixtures in such a setup: pytest-dev/pytest-xdist#271 This was leading to race conditions and flaky tests when shared LLMInferenceServiceConfigs were removed prematurely, leading to failures of subsequent tests. This approach creates per-test clones of base_refs, making them localized and avoids introducing weird workarounds that author spent too time much on. Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
bartoszmajsak
force-pushed
the
llm-d/feature/basic-e2e-test
branch
from
7e35fc7 to
cc45a46
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/actions/minikube-setup/action.yml (1)
41-63: Race condition: MetalLB CRs applied before namespace existsThe MetalLB configuration step applies ConfigMap to
metallb-systemnamespace without ensuring the namespace exists first. Theminikube addons enable metallbcreates the namespace asynchronously, which can cause this step to fail.Add a wait condition before applying the ConfigMap:
- name: Configure MetalLB for Minikube if: ${{ contains(inputs.addons, 'metallb') }} shell: bash run: | + # Wait for metallb-system namespace to be created + for i in {1..12}; do + kubectl get ns metallb-system && break || sleep 5 + done + IP=$(minikube ip) PREFIX=${IP%.*} START=${PREFIX}.200 END=${PREFIX}.235 - + kubectl apply -f - <<EOFAlso fix the trailing spaces on line 49.
.github/workflows/e2e-test-llmisvc.yml (1)
123-128: Fix shell variable quoting issuesThe shell variables are unquoted, which can cause word splitting issues if paths contain spaces or special characters.
run: | - sudo mkdir -p ${DOCKER_IMAGES_PATH} - sudo chown -R $USER ${DOCKER_IMAGES_PATH} + sudo mkdir -p "${DOCKER_IMAGES_PATH}" + sudo chown -R "$USER" "${DOCKER_IMAGES_PATH}" ./test/scripts/gh-actions/build-images.sh docker image ls - sudo ls -lh ${DOCKER_IMAGES_PATH} + sudo ls -lh "${DOCKER_IMAGES_PATH}"
🧹 Nitpick comments (3)
test/e2e/llmisvc/diagnostic.py (1)
19-19: Remove unused importsSeveral KServe imports are not used in this module.
-from kserve import KServeClient, V1alpha1LLMInferenceService, constants.github/workflows/e2e-test-llmisvc.yml (1)
70-70: Fix trailing spacesStatic analysis detected trailing spaces that should be cleaned up.
go-version-file: go.mod - +test/e2e/llmisvc/fixtures.py (1)
20-20: Remove unused importThe
ApiExceptionimport is not used in this module sinceclient.rest.ApiExceptionis used directly.-from kubernetes.client.rest import ApiException
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.flake8(1 hunks).github/actions/kserve-dep-setup/action.yml(2 hunks).github/actions/minikube-setup/action.yml(2 hunks).github/workflows/e2e-test-llmisvc.yml(1 hunks)Makefile(1 hunks)pkg/controller/llmisvc/DEV.md(2 hunks)test/e2e/llmisvc/README.md(1 hunks)test/e2e/llmisvc/conftest.py(1 hunks)test/e2e/llmisvc/diagnostic.py(1 hunks)test/e2e/llmisvc/fixtures.py(1 hunks)test/e2e/llmisvc/logging.py(1 hunks)test/e2e/llmisvc/test_llm_inference_service.py(1 hunks)test/e2e/pytest.ini(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/e2e/llmisvc/README.md
- .github/actions/kserve-dep-setup/action.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/pytest.ini
- .flake8
- Makefile
- pkg/controller/llmisvc/DEV.md
- test/e2e/llmisvc/conftest.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file `test/overlays/llm-istio-experimental/istiod.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file `test/overlays/llm-istio-experimental/istio_base.yaml` is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: opendatahub-io/kserve#729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in `test/overlays/llm-istio-experimental/` directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
test/e2e/llmisvc/fixtures.py (1)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
test/e2e/llmisvc/test_llm_inference_service.py (5)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: hdefazio
PR: #658
File: pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:1873-1886
Timestamp: 2025-06-12T22:10:15.375Z
Learning: If a review comment is raised on code that was not introduced or changed by the current PR, and the user points this out, acknowledge it and clarify that no action is required from the PR author.
.github/workflows/e2e-test-llmisvc.yml (4)
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2559-2566
Timestamp: 2025-07-10T14:58:38.489Z
Learning: In the kserve repository, files in test/overlays/llm-istio-experimental/ directory are considered external files by the maintainers and should not be flagged for YAML formatting issues or other code quality concerns.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:46-65
Timestamp: 2025-07-10T14:58:54.997Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istiod.yaml:2025-2049
Timestamp: 2025-07-10T14:58:43.762Z
Learning: The file test/overlays/llm-istio-experimental/istiod.yaml is considered external and irrelevant for review in this repository.
Learnt from: bartoszmajsak
PR: #729
File: test/overlays/llm-istio-experimental/istio_base.yaml:6854-6869
Timestamp: 2025-07-10T14:58:14.161Z
Learning: The file test/overlays/llm-istio-experimental/istio_base.yaml is considered external and irrelevant for review in this repository.
🧬 Code Graph Analysis (1)
test/e2e/llmisvc/test_llm_inference_service.py (6)
python/kserve/kserve/api/kserve_client.py (2)
KServeClient(31-834)get(166-223)python/kserve/kserve/models/v1alpha1_llm_inference_service.py (1)
V1alpha1LLMInferenceService(35-242)test/e2e/llmisvc/diagnostic.py (2)
print_all_events_table(22-58)kinds_matching_by_labels(61-98)test/e2e/llmisvc/fixtures.py (2)
generate_test_id(180-182)test_case(90-142)test/e2e/llmisvc/logging.py (1)
log_execution(26-51)python/kserve/kserve/exceptions.py (1)
ApiException(98-123)
🪛 YAMLlint (1.37.1)
.github/actions/minikube-setup/action.yml
[error] 49-49: trailing spaces
(trailing-spaces)
.github/workflows/e2e-test-llmisvc.yml
[error] 70-70: trailing spaces
(trailing-spaces)
🪛 Flake8 (7.2.0)
test/e2e/llmisvc/diagnostic.py
[error] 19-19: 'kserve.KServeClient' imported but unused
(F401)
[error] 19-19: 'kserve.V1alpha1LLMInferenceService' imported but unused
(F401)
[error] 19-19: 'kserve.constants' imported but unused
(F401)
test/e2e/llmisvc/fixtures.py
[error] 20-20: 'kubernetes.client.rest.ApiException' imported but unused
(F401)
test/e2e/llmisvc/test_llm_inference_service.py
[error] 28-28: '.fixtures.test_case' imported but unused
(F401)
[error] 91-91: redefinition of unused 'test_case' from line 28
(F811)
[error] 115-115: 'kserve.utils.utils' imported but unused
(F401)
[error] 178-178: redefinition of unused 'test_case' from line 28
(F811)
🪛 Ruff (0.12.2)
test/e2e/llmisvc/diagnostic.py
19-19: kserve.KServeClient imported but unused
Remove unused import
(F401)
19-19: kserve.V1alpha1LLMInferenceService imported but unused
Remove unused import
(F401)
19-19: kserve.constants imported but unused
Remove unused import
(F401)
61-61: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
test/e2e/llmisvc/fixtures.py
20-20: kubernetes.client.rest.ApiException imported but unused
Remove unused import: kubernetes.client.rest.ApiException
(F401)
test/e2e/llmisvc/test_llm_inference_service.py
91-91: Redefinition of unused test_case from line 31
(F811)
115-115: kserve.utils.utils imported but unused
Remove unused import: kserve.utils.utils
(F401)
178-178: Redefinition of unused test_case from line 31
(F811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: precommit-check
- GitHub Check: test
- GitHub Check: Build
- GitHub Check: build (3.11)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: test
🔇 Additional comments (14)
.github/actions/minikube-setup/action.yml (1)
17-20: LGTM: Addons parameter additionThe new
addonsinput parameter is well-defined and properly integrated with the underlying Minikube setup action.test/e2e/llmisvc/logging.py (1)
26-51: LGTM: Well-implemented logging decoratorThe
log_executiondecorator is cleanly implemented with proper use offunctools.wraps, exception handling, and ISO 8601 timestamps. The millisecond precision duration logging will be valuable for E2E test diagnostics.test/e2e/llmisvc/diagnostic.py (2)
22-58: LGTM: Useful diagnostic functionThe
print_all_events_tablefunction is well-implemented for debugging Kubernetes events with proper error handling and formatted output.
74-98: LGTM: Dynamic resource discovery implementationThe function efficiently discovers and queries Kubernetes resources matching labels, with appropriate error handling for individual resource types.
.github/workflows/e2e-test-llmisvc.yml (2)
55-56: Jobs are disabled for developmentBoth test jobs are currently disabled with
if: false. This is appropriate for development but should be enabled before merging to production.Are these jobs ready to be enabled, or should they remain disabled for this PR?
76-79: LGTM: MetalLB addon integrationThe integration with the updated Minikube setup action using the
metallbaddon is clean and appropriate for the E2E test environment requirements.test/e2e/llmisvc/fixtures.py (3)
89-143: LGTM: Well-designed test fixtureThe
test_casefixture is excellently designed with:
- Proper validation of base references
- Unique resource name generation with hashing
- Clean resource lifecycle management
- Comprehensive error handling and cleanup
This provides a solid foundation for parameterized E2E tests.
155-171: LGTM: Kubernetes-safe name generationThe function properly handles Kubernetes naming constraints with length limits, character replacement, and hash-based uniqueness. The implementation is robust and follows K8s naming best practices.
185-234: LGTM: Robust CRUD operationsThe create/update function handles both resource creation and updates with proper 404 error handling. The metadata preservation during updates is particularly well-implemented.
test/e2e/llmisvc/test_llm_inference_service.py (5)
35-43: LGTM! Well-structured constants and assertion helper.The plural constant follows Kubernetes naming conventions, and the reusable assertion function provides clear error messages.
45-58: Well-designed test configuration dataclass.Good use of
__test__ = Falseto prevent pytest collection, sensible defaults, and clear documentation of factory-provided fields.
60-111: Excellent test structure with comprehensive error handling.The test follows pytest best practices with proper parametrization, cleanup, and diagnostic collection. The use of markers for test categorization is well done.
175-270: Robust implementation of service readiness checking.Excellent error handling, flexible URL extraction from multiple status fields, and a reusable generic wait function. The nested function with
nonlocalis an appropriate pattern for this use case.
272-298: Comprehensive diagnostic collection for debugging.Excellent implementation that collects the LLMInferenceService status, events, and all related resources using appropriate label selectors. This will be invaluable for debugging test failures.
Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
openshift-merge-bot
Bot
merged commit 6743f90
into
opendatahub-io:release-v0.15
github-project-automation
Bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
5 participants
Enables end-to-end (aka fvt) pytests from LLMInferenceService. This change includes:
models by simply combining LLMInferenceServiceConfig refs
metallbaddon for minikubeImportant
See test/e2e/llmisvc/README.md for test design ideas.
Refer to pkg/controller/llmisvc/DEV.md for updated description how to run the tests.
Fixes RHOAIENG-30183
Supersedes #736
Summary by CodeRabbit
New Features
Documentation
Tests
Chores