Skip to content

fix(controller): fall back to default scheduleTimeoutSeconds when unset - #4106

Closed
DsThakurRawat wants to merge 1 commit into
kubeflow:masterfrom
DsThakurRawat:fix/coscheduling-default-timeout
Closed

DsThakurRawat wants to merge 1 commit into
kubeflow:masterfrom
DsThakurRawat:fix/coscheduling-default-timeout

Conversation

@DsThakurRawat

Copy link
Copy Markdown
Contributor

What this PR does

In the Coscheduling plugin Build method (pkg/runtime/framework/plugins/coscheduling/coscheduling.go), ScheduleTimeoutSeconds was unconditionally dereferenced.

Because scheduleTimeoutSeconds is an optional field in CoschedulingPodGroupPolicySource (+optional, +kubebuilder:default=60), any runtime configured with podGroupPolicy.coscheduling: {} (or created programmatically without ScheduleTimeoutSeconds set) causes the controller worker to panic with a nil pointer dereference during reconciliation.

This PR:

  • Safely dereferences ScheduleTimeoutSeconds using ptr.Deref with a default of 60 seconds (defaultScheduleTimeoutSeconds).
  • Adds a unit test case in coscheduling_test.go verifying that building a PodGroup when ScheduleTimeoutSeconds is unset successfully assigns the default 60-second scheduling timeout without panicking.

Fixes #4085

How has this been tested?

  • go test -v ./pkg/runtime/framework/plugins/coscheduling/... passing.
  • go test ./pkg/runtime/framework/... passing.
  • make fmt vet verify-boilerplate passing.

…nset

In the Coscheduling plugin Build method, ScheduleTimeoutSeconds was
unconditionally dereferenced. Because scheduleTimeoutSeconds is an optional field
in CoschedulingPodGroupPolicySource with +kubebuilder:default=60, configuring
coscheduling without specifying scheduleTimeoutSeconds caused a nil pointer
dereference panic during reconciliation.

Safely dereference ScheduleTimeoutSeconds using ptr.Deref with the documented
default of 60 seconds, and add unit test coverage for the unset case.

Fixes kubeflow#4085

Assisted-by: Claude Code

Signed-off-by: Divyansh Rawat <divyanshrawatofficial@gmail.com>
@google-oss-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@github-actions

Copy link
Copy Markdown
Contributor

🎉 Welcome to the Kubeflow Trainer! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

  • If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
  • Our team will review your PR soon! cc @kubeflow/kubeflow-trainer-team

Join the community:

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

@DsThakurRawat DsThakurRawat changed the title fix(coscheduling): fall back to default scheduleTimeoutSeconds when unset fix(controller): fall back to default scheduleTimeoutSeconds when unset Sep 24, 2026
@DsThakurRawat

Copy link
Copy Markdown
Contributor Author

hi can any one take a look at this

@abhishekKokadwar

abhishekKokadwar commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Thanks @DsThakurRawat. Flagging that this overlaps with #4086, which @yush-1018 opened on 21 September against the same issue, #4085: same constant, same ptr.Deref in Build, same test shape. I put my review comments on #4086 since it was opened first, but they apply equally here.

One thing yours does nicely is build CoschedulingPodGroupPolicySource{} empty rather than setting the field to nil, which is closer to what a stored object looks like.

@DsThakurRawat

Copy link
Copy Markdown
Contributor Author

Thanks @abhishekKokadwar for the review and notice. Since #4086 was opened first and already adopted the empty struct test shape, closing this in favor of #4086 to keep the review stream consolidated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG] Potential nil pointer dereference on ScheduleTimeoutSeconds in Coscheduling plugin Build

2 participants