Skip to content

Fix: adopt to gangScheduling feature gates on k8s v1.36+ - #2206

Merged
hami-robot[bot] merged 14 commits into
Project-HAMi:masterfrom
archlitchi:master
Jul 31, 2026
Merged

Fix: adopt to gangScheduling feature gates on k8s v1.36+#2206
hami-robot[bot] merged 14 commits into
Project-HAMi:masterfrom
archlitchi:master

Conversation

@archlitchi

@archlitchi archlitchi commented Jul 30, 2026

Copy link
Copy Markdown
Member

/kind bug

PR:#2066 only support coScheduling plugin, but doesn't support GangScheduling feature gate implemented in v1.36.

How to use:

  1. Enable
  --feature-gates=GenericWorkload=true,GangScheduling=true

on kube-apiserver, kube-scheduler, kube-controller-manager

  1. Then, for kube-apiserver, add the parameter below:
  --runtime-config=scheduling.k8s.io/v1alpha2=true

Example:

apiVersion: scheduling.k8s.io/v1alpha2
kind: PodGroup
metadata:
  name: test-fail-group
  namespace: default
spec:
  schedulingPolicy:
    gang:
      minCount: 2
---
apiVersion: v1
kind: Pod
metadata:
  name: e2e-test2-fail-pod1
  namespace: default
spec:
  schedulerName: hami-scheduler
  restartPolicy: Never
  schedulingGroup:
    podGroupName: test-fail-group
  containers:
  - name: c
    image: registry.k8s.io/pause:3.10.1
    resources:
      limits:
        hami-test.com/gpu: "1"
        hami-test.com/gpumem: "9000"
---
apiVersion: v1
kind: Pod
metadata:
  name: e2e-test2-fail-pod2
  namespace: default
spec:
  schedulerName: hami-scheduler
  restartPolicy: Never
  schedulingGroup:
    podGroupName: test-fail-group
  containers:
  - name: c
    image: registry.k8s.io/pause:3.10.1
    resources:
      limits:
        hami-test.com/gpu: "1"
        hami-test.com/gpumem: "9000"

Result:
cbbead1a-a0ea-4706-983c-40fef9597af6

I'll later add a user-guide to website

Summary by CodeRabbit

  • Bug Fixes
    • Node lock contention errors are now consistently identified as retryable when multiple workloads attempt to lock the same node.
    • Pod group membership detection now recognizes both scheduler labels and native scheduling group configuration.
  • Tests
    • Added coverage for concurrent node-lock attempts and pod group membership scenarios, including missing and empty configuration values.

archlitchi added 13 commits July 9, 2026 08:02
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
Signed-off-by: limengxuan <mengxuan.li@dynamia.ai>
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Jul 30, 2026
@hami-robot
hami-robot Bot requested review from FouoF and ouyangluwei163 July 30, 2026 10:19
@cursor

cursor Bot commented Jul 30, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8860c669-1d43-4c18-a784-1727747f43af

📥 Commits

Reviewing files that changed from the base of the PR and between 6977dfa and 8f48438.

📒 Files selected for processing (1)
  • pkg/util/nodelock/nodelock_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/util/nodelock/nodelock_test.go

📝 Walkthrough

Walkthrough

The change classifies duplicate node-lock errors with ErrNodeLockContention and adds concurrency coverage. It also expands IsPodGroupMember to recognize native scheduling groups and adds table-driven tests.

Changes

Node-lock contention classification

Layer / File(s) Summary
Retryable node-lock contention
pkg/util/nodelock/nodelock.go, pkg/util/nodelock/nodelock_test.go
SetNodeLock wraps duplicate-lock errors with ErrNodeLockContention. Tests cover concurrent lock attempts and preserve cleanup assertions.

PodGroup membership detection

Layer / File(s) Summary
Label and scheduling-group membership
pkg/util/util.go, pkg/util/util_test.go
IsPodGroupMember recognizes non-empty labels and native scheduling-group names. Tests cover nil, empty, label-based, native, and combined inputs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: dsfans2014, fouof

Poem

I hop through locks, one holds the key,
The other retries predictably.
Labels and groups now join the show,
With tests to prove the paths we know.
A tidy patch beneath the moon.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: support for Kubernetes v1.36+ GangScheduling feature gates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from DSFans2014 July 30, 2026 10:20
@archlitchi

Copy link
Copy Markdown
Member Author

CC @mesutoezdil @FouoF

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 63.00% <100.00%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/util/nodelock/nodelock.go 70.14% <100.00%> (+2.98%) ⬆️
pkg/util/util.go 71.59% <100.00%> (+3.11%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/util/nodelock/nodelock_test.go`:
- Around line 457-483: Update TestSetNodeLockRaceIsRetryable and its comments to
reflect that the test performs sequential SetNodeLock calls and only verifies
the already-locked path, or rewrite it to run concurrent LockNode callers so it
actually exercises the race and validates retryable contention classification.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 03ec1363-9ce4-428d-b33a-45c7eebefc27

📥 Commits

Reviewing files that changed from the base of the PR and between 05e6c80 and 6977dfa.

📒 Files selected for processing (4)
  • pkg/util/nodelock/nodelock.go
  • pkg/util/nodelock/nodelock_test.go
  • pkg/util/util.go
  • pkg/util/util_test.go

Comment thread pkg/util/nodelock/nodelock_test.go
Comment thread pkg/util/util.go
Comment thread pkg/util/util.go
Comment thread pkg/util/nodelock/nodelock_test.go Outdated
Signed-off-by: archlitchi <mengxuan.li@dynamia.ai>
@FouoF

FouoF commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@hami-robot hami-robot Bot added the lgtm label Jul 31, 2026
@hami-robot

hami-robot Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, FouoF

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot
hami-robot Bot merged commit 0f6570e into Project-HAMi:master Jul 31, 2026
17 checks passed
coldzerofear added a commit to coldzerofear/vgpu-manager that referenced this pull request Jul 31, 2026
PodGroup 是**命名空间作用域**对象 —— coscheduling 的 label、Volcano /
Koordinator / kube-batch 的注解、k8s 1.36 原生的 spec.schedulingGroup、以及
PodGroup ownerReference,全部在 Pod 自己的命名空间内解析。但此前所有"判断两个
Pod 是否同一个 gang"的地方都直接比裸名字,而 scheduler 的 Pod informer 是
集群级的,于是两个租户各自建一个叫 "training" 的 PodGroup 就会互相当成兄弟 Pod。
多租户集群里 gang 名往往来自工作负载名(Kubeflow PyTorchJob 等按 job 名建
PodGroup),同名概率并不低。

受影响的四处判定:

1. IndexerKeyPodGangName 索引(filter_predicate.go) —— 按裸名建索引,
   ListByIndexValue 会返回其它命名空间的同名 gang Pod。被 filter 的
   FindGangSiblingDomain 与 preempt 的跨 Pod 拓扑对齐消费。
2. GangAnchorComponent(device/types.go) —— 遍历 n.nodePods(含节点上所有
   命名空间)按裸名匹配兄弟,会把别的租户的 Pod 当锚点,把本 gang 锚到错误的
   NVLink 组件上; strict 模式下可能进一步导致节点被误拒。
3. isProtectedFromPreemption(preempt) —— "避免抢占兄弟 Pod"按裸名匹配,会
   误保护无关租户的 Pod,导致抢占凑不齐受害者、抢占方一直 Pending。
4. 抢占事件里列出的 pod group 名 —— 受害者可能跨命名空间,裸名有歧义。

修复: 新增 util.PodGangKey(pod) 返回 "<namespace>/<name>",上述四处判定与
req.GangName 的生产端统一改用它。util.PodHasGangName 保留原语义(返回用户书写
的原始名),仅用于展示和"是否属于某个 gang"的布尔判断 ——
sortVictimsByPreference 只取布尔值,不受影响。

测试: cross_pod_test 的 fixture 补上命名空间并新增回归用例(其它命名空间的同名
gang 不算兄弟); 新增 Test_PodGangKey 覆盖限定、跨命名空间不相等、非 gang、nil。

背景: 排查 Project-HAMi/HAMi#2206(适配 k8s 1.36 原生 gangScheduling)时顺带发现。
该 PR 的主体改动(识别 spec.schedulingGroup)本项目已支持且覆盖面更全,另一半
(nodelock 争用重试)本项目无对应机制。

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants