chore: simplifies llm-d setup script #882
Conversation
This PR introduces consistent flags (`--with-kserve` and `--with-kuadrant`) to control which infra pieces are installed. Previously they were carrying opposite meanings. The version check function is also renamed to `is_newer_version` to better capture desired behaviour. In addition all scripts are made executable for consistency. Signed-off-by: Bartosz Majsak <[email protected]>
|
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 WalkthroughThe E2E orchestration was updated to drop the PARALLELISM argument, adjust deployment profile handling, and switch LLM setup to a new script. A new llm-d setup script was added, the legacy llm setup script was removed, and a version utility function was renamed. Minor status messages were updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Job
participant Run as run-e2e-tests.sh
participant Setup as setup-e2e-tests.sh
participant LLM as setup-llm-d.sh
participant Ver as version.sh
participant Infra as Infra Deploy Scripts
CI->>Run: Invoke with MARKERS, DEPLOYMENT_PROFILE
Run->>Run: Log "⏳ Installing on the cluster"
Run->>Setup: setup-e2e-tests.sh MARKERS DEPLOYMENT_PROFILE
alt DEPLOYMENT_PROFILE == "llm-d"
Setup->>LLM: --with-kserve=false
LLM->>Ver: get_openshift_server_version + is_version_newer
Ver-->>LLM: Version OK/Fail
alt Version OK
LLM->>Infra: Deploy cert-manager, LWS
opt WITH_KSERVE=true
LLM->>Infra: Apply CRDs, deploy KServe, wait readiness
end
LLM->>Infra: Deploy gateway ingress
opt WITH_KUADRANT=true
LLM->>Infra: Deploy Kuadrant
end
else Version too old
LLM-->>Setup: Exit 1
end
else Other profiles
Setup->>Setup: Handle according to profile
end
Run->>Run: Log "⏳ Running E2E tests: ${MARKERS}"
Run->>Run: Execute E2E tests (unchanged args)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@coderabbitai review |
✅ 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: 2
🧹 Nitpick comments (5)
test/scripts/openshift-ci/version.sh (2)
42-53: Name implies “strictly newer” but logic is “>=”; consider clarifying or keep as-is and adjust messages.The function returns success for version1 >= version2. The name “is_version_newer” suggests “>”. If keeping behavior, consider renaming to is_version_at_least, or at minimum ensure call sites/logs say “meets minimum” rather than “newer than.”
49-53: Make version comparison robust to longer/shorter segments and rc tags.The awk padding handles only three numeric segments. Optional: switch to version sort for broader compatibility.
Apply this diff:
-is_version_newer() { - local version1="$1" - local version2="$2" - - local v1=$(echo "$version1" | awk -F. '{printf "%d%03d%03d", $1, $2, $3}') - local v2=$(echo "$version2" | awk -F. '{printf "%d%03d%03d", $1, $2, $3}') - - [ "$v1" -ge "$v2" ] -} +is_version_newer() { + local v1="$1" v2="$2" + [[ "$(printf '%s\n%s\n' "$v2" "$v1" | sort -V | tail -1)" == "$v1" ]] +}test/scripts/openshift-ci/run-e2e-tests.sh (1)
64-64: Minor: consistent, clearer status output.The hourglass status line is fine. Consider quoting $PROJECT_ROOT in pushd/popd elsewhere for safety if paths ever include spaces.
test/scripts/openshift-ci/setup-llm-d.sh (2)
43-48: Accept more boolean forms and normalize.Make CLI friendlier by accepting yes/no/1/0 (case-insensitive).
Apply this diff:
-parse_bool() { - case "$1" in - true|false) printf %s "$1" ;; - *) echo "Error: expected true|false, got '$1'" >&2; exit 1 ;; - esac -} +parse_bool() { + local v="${1,,}" + case "$v" in + true|yes|1) printf %s "true" ;; + false|no|0) printf %s "false" ;; + *) echo "Error: expected true|false (also accepts yes/no/1/0), got '$1'" >&2; exit 1 ;; + esac +}
66-73: Tweak wording to match >= behavior.is_version_newer returns success for “>=”. Adjusting the message avoids confusion when equal to the minimum.
Apply this diff:
-if is_version_newer "${server_version}" "${min_version}"; then - echo "✅ OpenShift version (${server_version}) is newer than ${min_version} - installing components..." +if is_version_newer "${server_version}" "${min_version}"; then + echo "✅ OpenShift version (${server_version}) meets minimum ${min_version} - installing components..." else - echo "❌ OpenShift version (${server_version}) older than ${min_version}. This is not supported environment." + echo "❌ OpenShift version (${server_version}) is older than ${min_version}. This environment is not supported." exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/scripts/openshift-ci/run-e2e-tests.sh(1 hunks)test/scripts/openshift-ci/setup-e2e-tests.sh(2 hunks)test/scripts/openshift-ci/setup-llm-d.sh(1 hunks)test/scripts/openshift-ci/setup-llm.sh(0 hunks)test/scripts/openshift-ci/version.sh(1 hunks)
💤 Files with no reviewable changes (1)
- test/scripts/openshift-ci/setup-llm.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
test/scripts/openshift-ci/setup-llm-d.sh
[warning] 20-20: PROJECT_ROOT appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (4)
test/scripts/openshift-ci/run-e2e-tests.sh (1)
54-57: Signature alignment LGTM.Passing only MARKERS and DEPLOYMENT_PROFILE to setup-e2e-tests.sh matches its updated interface.
test/scripts/openshift-ci/setup-e2e-tests.sh (2)
30-31: Argument reshuffle looks correct.DEPLOYMENT_PROFILE now at position 2 and validated. Callers updated accordingly.
91-92: Double-check intent: KServe disabled in llm-d script, but installed here unconditionally.You invoke setup-llm-d.sh with --with-kserve=false, yet later install KServe overlays regardless. If the goal is to centralize KServe install here, that’s fine—just confirming this is intentional to avoid confusing flags.
test/scripts/openshift-ci/setup-llm-d.sh (1)
17-21: SC2034: PROJECT_ROOT was unused; above refactor uses it.No further action needed once repo-relative paths are anchored with PROJECT_ROOT.
| --with-kserve[=true|false] Enable/disable KServe installation (default: ✅ true) | ||
| --with-kuadrant[=true|false] Enable/disable Kuadrant installation (default: ❌ false) | ||
| -h, --help Show this help message |
There was a problem hiding this comment.
By default, kserve will be installed so why we need to Enable the option?
There was a problem hiding this comment.
Kuadrant has the same behavior, but we need an option that does the reverse.
There was a problem hiding this comment.
I wanted to have consistent flags to avoid confusion. Before it was --skip-kserve --deploy-kuadrant. Maybe having this flags aligned makes it more natural? WDYT?
There was a problem hiding this comment.
I think we should set Kuadrant’s default value to true, with the option to disable it using --skip-kserve and --skip-kuadrant. While Kuadrant may not be necessary for LLMD development installations, having it enabled by default would be more useful for other scenarios.
There was a problem hiding this comment.
This is the original behavior brought in #876. I "refactored" the flags logic, but kept it.
echo "Default behavior:"
echo " • KServe deployment: enabled"
echo " • Kuadrant deployment: disabled"
Are you suggesting to keep them both enabled instead?
There was a problem hiding this comment.
Yes, exactly. My suggestion is to have both enabled by default, and let users disable them with --skip-kserve and --skip-kuadrant if needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Bartosz Majsak <[email protected]>
Signed-off-by: Bartosz Majsak <[email protected]>
|
@bartoszmajsak: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
This PR introduces consistent flags (
--with-kserveand--with-kuadrant) to control which infra pieces are installed. Previously they were carrying opposite meanings.The version check function is also renamed to
is_newer_versionto better capture desired behaviour.In addition all scripts are made executable for consistency.
Summary by CodeRabbit