🌱 update kueue admission check solution with kueue addon v0.1.4 release#1165
Conversation
WalkthroughShifts Kueue multi-cluster setup from static MultiKueue manifests to an OCM-driven AdmissionCheck flow; rewrites the README as an integration guide with scenario-driven examples and verification steps; updates demo YAMLs to reference AdmissionCheck; and removes the local environment bootstrap script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
solutions/kueue-admission-check/multikueue-setup-demo1.yaml (1)
46-47: Nit: capitalize resource properly in commentPrefer “AdmissionCheck controller” over “admissioncheck controller” for consistency with Kueue docs.
-# OCM implements an admissioncheck controller to automate the MultiKueue setup process. +# OCM implements an AdmissionCheck controller to automate the MultiKueue setup process.solutions/kueue-admission-check/README.md (8)
5-11: Tighten terminology (kubeconfig, AdmissionCheck) and minor copyeditsSmall wording tweaks for accuracy and consistency.
-This solution demonstrates the integration of ... automating the generation of MultiKueue KubeConfigs, centralizing queue resource management from a single hub, and enhancing multicluster scheduling... +This solution demonstrates the integration of ... automating the generation of MultiKueue kubeconfig Secrets, centralizing queue resource management from a single hub, and enhancing multi‑cluster scheduling... -- **Enhanced Multicluster Scheduling**: Integrates with OCM's placement with MultiKueue by implementing an admission check controller, generates MultiKueueConfig dynamically based on OCM placement decisions, and supports advanced placement strategies +- **Enhanced Multi‑cluster Scheduling**: Integrates OCM Placement with MultiKueue via an AdmissionCheck controller, generates MultiKueueConfig dynamically based on Placement decisions, and supports advanced placement strategies
65-76: Clarify resource names and kindsUse exact kinds (MultiKueueCluster) and Secret type wording to reduce ambiguity.
-- On hub cluster, check `multikueuecluster` and `kubeconfig` secrets for the managed cluster created. +- On the hub cluster, check MultiKueueCluster and kubeconfig Secrets created for each managed cluster.
86-86: Doc correctness: why local-cluster is not connectedGood callout. Consider linking directly to the specific KEP section for “manager cannot submit to itself” if available.
96-101: Grammar tweakMinor readability improvement.
-- With the `multikueueconfig` auto created, we can easily set up MultiKueue environment. +- With the `multikueueconfig` auto‑created, you can easily set up the MultiKueue environment.
167-172: Names in sample output don’t match manifestsManifests use cluster-queue and user-queue; sample shows cluster-queue-demo1 and user-queue-demo1. Align for consistency or note that names may vary.
241-241: Grammar tweakSmall fix.
-If success, there should be "status": "True" and reasons like "Active" or "Ready" presented in the conditions. +If successful, conditions should show `"status": "True"` with reasons like `"Active"` or `"Ready"`.
309-325: Make jq selection resilient (avoid hard-coded index)Indexing
.status.scores[5]is brittle. Filter by score name to work across environments.kubectl get addonplacementscore -A -ojson \ | jq -r '.items[] | .metadata.name as $n | (.status.scores[] | select(.name=="gpuClusterAvailable")) as $s | "\($n)\n\($s)"'
365-370: Output snippet includes an inline comment inside JSONInline comments are not valid JSON; consider moving the note below the block to prevent copy/paste confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
solutions/kueue-admission-check/README.md(12 hunks)solutions/kueue-admission-check/multikueue-setup-demo1.yaml(2 hunks)solutions/kueue-admission-check/multikueue-setup-demo2.yaml(1 hunks)solutions/kueue-admission-check/setup-env.sh(0 hunks)
💤 Files with no reviewable changes (1)
- solutions/kueue-admission-check/setup-env.sh
🧰 Additional context used
🪛 LanguageTool
solutions/kueue-admission-check/README.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...CM) installed with the following addons: - [Cluster Permission Addon](https://github...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...ng addons: - Cluster Permission Addon - [Managed Service Account Addon](https://g...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...sion) - Managed Service Account Addon - [Cluster Proxy Addon](https://github.com/...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...serviceaccount) - Cluster Proxy Addon (Optional) Enables hub-to-spoke connect...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...ke connectivity for enhanced networking. - Kueue installed: - Hub Cluster with [K...
(QB_NEW_EN)
[grammar] ~22-~22: Ensure spelling is correct
Context: ...stallation/) pre-installed, or let this addon install Kueue via [operator](https://gi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~96-~96: There might be a mistake here.
Context: ...config` auto created, we can easily set up MultiKueue environment. ```bash kubect...
(QB_NEW_EN)
[grammar] ~306-~306: Ensure spelling is correct
Context: ...ores change. Here in this environment, cluster1 has no GPUs, while cluster2 and cluster...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~306-~306: Ensure spelling is correct
Context: ...nvironment, cluster1 has no GPUs, while cluster2 and cluster3 each have 3 GPUs. Check `A...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~306-~306: Ensure spelling is correct
Context: ...luster1 has no GPUs, while cluster2 and cluster3 each have 3 GPUs. Check `AddonPlacement...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~306-~306: Ensure spelling is correct
Context: ...vailable receiving higher scores. Here, cluster1, which has no GPUs, should have a score...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (5)
solutions/kueue-admission-check/multikueue-setup-demo2.yaml (1)
46-58: Add namespace to Placement reference in AdmissionCheck parametersPlacement is namespaced (kueue-system in your README). Add namespace to avoid ambiguity.
spec: controllerName: open-cluster-management.io/placement parameters: apiGroup: cluster.open-cluster-management.io kind: Placement - name: multikueue-config-demo2 + name: multikueue-config-demo2 + namespace: kueue-systemsolutions/kueue-admission-check/multikueue-setup-demo1.yaml (3)
23-26: LGTM: ClusterQueue references both AdmissionChecksBoth multikueue-demo1 and multikueue-config-demo1 are defined below and correctly listed here.
42-45: Confirm default MultiKueueConfig nameAC multikueue-demo1 points to MultiKueueConfig name "default". Ensure the OCM-driven generator actually emits that name in your environment (README shows "default"). If not, align the name here or in the generator.
53-57: Add namespace to Placement reference in AdmissionCheck parametersPlacement is namespaced; include namespace to avoid implicit defaults.
spec: controllerName: open-cluster-management.io/placement parameters: apiGroup: cluster.open-cluster-management.io kind: Placement - name: default + name: default + namespace: kueue-systemsolutions/kueue-admission-check/README.md (1)
329-354: Placement spec looks good; ensure controller interprets prioritizer as expectedOCM Placement v1beta1 supports predicates + Exact prioritizer. Verify that the addOn score name matches the addon’s advertised ScoreDefinition.
| curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/main/kueue-addon/build/setup-env.sh | bash | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Pin the quick-setup script to kueue-addon v0.1.4
Avoid using “main” in docs; pin to the released tag referenced by this PR objective.
-curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/main/kueue-addon/build/setup-env.sh | bash
+curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/v0.1.4/kueue-addon/build/setup-env.sh | bashPin quick-setup script to kueue-addon v0.1.2
File: solutions/kueue-admission-check/README.md
-curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/main/kueue-addon/build/setup-env.sh | bash
+curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/kueue-addon/v0.1.2/kueue-addon/build/setup-env.sh | bash📝 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.
| curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/main/kueue-addon/build/setup-env.sh | bash | |
| ``` | |
| curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/kueue-addon/v0.1.2/kueue-addon/build/setup-env.sh | bash |
🤖 Prompt for AI Agents
In solutions/kueue-admission-check/README.md around lines 29-30, the quick-setup
curl currently pulls the script from the repository's main branch; update the
URL to pin the script to the kueue-addon v0.1.2 tag (use the raw URL that
includes /v0.1.2/ in the path) so the setup is deterministic and won't break
with future changes.
Signed-off-by: Qing Hao <qhao@redhat.com>
haoqing0110
force-pushed
the
update-doc
branch
from
f01087f to
da3ad15
Compare
|
/unhold |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
solutions/kueue-admission-check/README.md (2)
12-12: Pin docs link to the released addon tag (v0.1.4).Avoid “main” to keep docs stable with this PR’s target version.
-**For comprehensive design documentation and technical workflows, refer to the [kueue-addon](https://github.com/open-cluster-management-io/addon-contrib/blob/main/kueue-addon/README.md).** +**For comprehensive design documentation and technical workflows, refer to the [kueue-addon](https://github.com/open-cluster-management-io/addon-contrib/blob/v0.1.4/kueue-addon/README.md).**
29-30: Pin quick-setup script to kueue-addon v0.1.4.Deterministic bootstrap; avoids breakage from future changes on main.
-curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/main/kueue-addon/build/setup-env.sh | bash +curl -sL https://raw.githubusercontent.com/open-cluster-management-io/addon-contrib/v0.1.4/kueue-addon/build/setup-env.sh | bash#!/bin/bash # Verify no remaining "addon-contrib/main/kueue-addon" references rg -n "addon-contrib/main/kueue-addon"
🧹 Nitpick comments (3)
solutions/kueue-admission-check/multikueue-setup-demo1.yaml (2)
46-47: Typo/casing: “Admissioncheck” → “AdmissionCheck”.Fix the comment for consistency.
-# OCM implements an Admissioncheck controller to automate the MultiKueue setup process. +# OCM implements an AdmissionCheck controller to automate the MultiKueue setup process.
53-57: Placement reference likely needs a namespace.
Placementis namespaced. Add the namespace (typicallykueue-system) to avoid lookup ambiguity at runtime.spec: controllerName: open-cluster-management.io/placement parameters: apiGroup: cluster.open-cluster-management.io kind: Placement - name: default + namespace: kueue-system + name: defaultsolutions/kueue-admission-check/README.md (1)
96-101: Precondition note: ensure Placement/default exists in kueue-system.Scenario 1 relies on it for dynamic MultiKueueConfig generation; add a quick check or provide a minimal manifest.
-```bash -kubectl apply -f ./multikueue-setup-demo1.yaml -``` +```bash +# Ensure the default Placement exists in kueue-system (required by the OCM AdmissionCheck). +kubectl get placement -n kueue-system default || echo "Missing: create a Placement named 'default' in kueue-system." +kubectl apply -f ./multikueue-setup-demo1.yaml +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
solutions/kueue-admission-check/README.md(12 hunks)solutions/kueue-admission-check/multikueue-setup-demo1.yaml(2 hunks)solutions/kueue-admission-check/multikueue-setup-demo2.yaml(1 hunks)solutions/kueue-admission-check/setup-env.sh(0 hunks)
💤 Files with no reviewable changes (1)
- solutions/kueue-admission-check/setup-env.sh
✅ Files skipped from review due to trivial changes (1)
- solutions/kueue-admission-check/multikueue-setup-demo2.yaml
🔇 Additional comments (3)
solutions/kueue-admission-check/multikueue-setup-demo1.yaml (2)
25-25: Confirm need and ordering of both AdmissionChecks on the ClusterQueue.If
multikueue-config-demo1only materializes MultiKueueConfig andmultikueue-demo1performs the admission, having both is correct. Order shouldn’t matter, but please confirm controller behavior with mixed ACs.
40-45: LGTM — MultiKueue AdmissionCheck points to generated MultiKueueConfigdefault; verify resource exists after deployFile: solutions/kueue-admission-check/multikueue-setup-demo1.yaml (lines 40-45)
Unable to run the check here (kubectl not available). Run after deploy:
kubectl get multikueueconfig default -o yaml >/dev/nullsolutions/kueue-admission-check/README.md (1)
329-354: LGTM: Prioritizer policy for AddonPlacementScore — verify addon score exists locallyConfig is correct for selecting the highest GPU score; verification couldn't run here (kubectl not found). Run:
kubectl get addonplacementscore -A -ojson | jq -r '.items[].status.scores[].name' | sort -u
Confirm the output includes "gpuClusterAvailable" and that the score name matches the installed addon.
|
/assign @qiujian16 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, qiujian16 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-merge-bot
bot
merged commit cbbb8f1
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Documentation
Chores