Skip to content

fix(chart): keep scheduler livenessProbe inside the kubeScheduler container guard - #2977

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
magic-peach:fix-scheduler-liveness-probe-indent
Sep 8, 2026
Merged

fix(chart): keep scheduler livenessProbe inside the kubeScheduler container guard#2977
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
magic-peach:fix-scheduler-liveness-probe-indent

Conversation

@magic-peach

@magic-peach magic-peach commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
The scheduler deployment template renders the livenessProbe block after the {{- end }} that closes the kubeScheduler.enabled guard, so it's indented as if still part of the kube-scheduler container but has no container list item above it when kubeScheduler.enabled is false. With scheduler.livenessProbe=true and scheduler.kubeScheduler.enabled=false, helm template fails with a YAML parse error instead of rendering a valid manifest. This moves the block back inside the guard so the probe only renders when the container it targets exists.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Verified with helm template hami charts/hami --set scheduler.livenessProbe=true --set scheduler.kubeScheduler.enabled=false before and after: before, the command fails with yaml: line 44: did not find expected key; after, it renders successfully. Also verified with both flags true that the probe still nests correctly under the kube-scheduler container, and helm lint passes. No open issue or PR found covering this.

Does this PR introduce a user-facing change?:
Fixes a chart render failure when scheduler.kubeScheduler.enabled=false and scheduler.livenessProbe=true are combined; no behavior change at default values.


This PR was written primarily by Claude Code, an AI assistant, under my direction and review. I verified the fix by reproducing the render failure and confirming it is resolved, and reviewed the diff before submitting.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected scheduler deployment configuration so kube-scheduler health checks are applied only when kube-scheduler is enabled.
    • Prevented health-check settings from being incorrectly assigned to the vGPU scheduler extender when kube-scheduler is disabled.
    • Improved reliability for deployments using the alternate scheduler configuration.

…tainer guard

The livenessProbe block for the kube-scheduler container rendered
after that container's closing end, so with kubeScheduler.enabled
false and livenessProbe true it landed as a bare mapping key under
containers, which breaks helm template with a YAML parse error.
Move it back inside the guard.

Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
@hami-robot hami-robot Bot added the kind/bug Something isn't working label Sep 7, 2026
@coderabbitai

coderabbitai Bot commented Sep 7, 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: Team

Run ID: 3c4bcd78-eab7-4cf5-b0e1-60eb22e0a166

📥 Commits

Reviewing files that changed from the base of the PR and between 1092ba2 and fc222cc.

📒 Files selected for processing (1)
  • charts/hami/templates/scheduler/deployment.yaml

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The deployment template now renders the kube-scheduler livenessProbe only when scheduler.kubeScheduler.enabled is true. The probe no longer attaches to vgpu-scheduler-extender when kube-scheduler is disabled.

Changes

Scheduler deployment rendering

Layer / File(s) Summary
Conditional kube-scheduler probe rendering
charts/hami/templates/scheduler/deployment.yaml
The conditional block now includes the kube-scheduler livenessProbe, preventing the probe from leaking to the vgpu-scheduler-extender container.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to fc222

The chart now renders the kube-scheduler probe only with its container, preventing invalid or misattached probe configuration for disabled kube-scheduler deployments. No current merge-blocking risk remains.

Poem

A rabbit checked the scheduler nest
The probe now stays where it belongs best
No extender wears its health check
The template keeps the containers in check
Hop, hop—the chart is correct and precise

🚥 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 and concisely describes the main change: keeping the scheduler livenessProbe inside the kubeScheduler container guard.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.

@maishivamhoo123

Copy link
Copy Markdown
Member

/lgtm

@FouoF

FouoF commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@hami-robot hami-robot Bot added the approved label Sep 8, 2026
@hami-robot

hami-robot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FouoF, magic-peach

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

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 71.24% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 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.

@hami-robot
hami-robot Bot merged commit 4434305 into Project-HAMi:master Sep 8, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved kind/bug Something isn't working lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants