New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[KEP-3521] Part 1: New Pod API .spec.schedulingGates #113274
Conversation
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
f3b90e4
to
d14305f
Compare
cc @ahg-g There are 2 PRs upcoming: scheduler implementation and integration/e2e tests. |
@@ -3017,6 +3030,13 @@ type PodOS struct { | |||
Name OSName | |||
} | |||
|
|||
// PodSchedulingGate is associated to a Pod to guard its scheduling. | |||
type PodSchedulingGate struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I asked for it for future proofing.
@@ -81,6 +81,9 @@ const ( | |||
|
|||
// nodeLabelRole specifies the role of a node | |||
nodeLabelRole = "kubernetes.io/role" | |||
|
|||
// schedulingPaused indicates a Pod is gated by one or more scheduling readiness gates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use blocked instead of pause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Or SchedulingGated (it should be rare enough that calling it gated will help people find the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to schedulingGated
which is more close to the new field name.
// If the Pod carries {type:PodScheduled, reason:WaitingForGates}, set reason to SchedulingPaused. | ||
for _, condition := range pod.Status.Conditions { | ||
if condition.Type == api.PodScheduled && condition.Reason == api.PodReasonWaitingForGates { | ||
reason = schedulingPaused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the same reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's b/c this "reason" is used in the output of k get pod
, and thusPodReasonWaitingForGates
which holds the literal "WaitingForGates" loses the context - a word describing that it's scheduling gated would be more self-descriptive.
/assign @smarterclayton |
/label api-review |
@@ -221,6 +223,10 @@ func (r *BindingREST) setPodHostAndAnnotations(ctx context.Context, podUID types | |||
if pod.Spec.NodeName != "" { | |||
return nil, fmt.Errorf("pod %v is already assigned to node %q", pod.Name, pod.Spec.NodeName) | |||
} | |||
// Reject binding to a scheduling un-ready Pod. | |||
if utilfeature.DefaultFeatureGate.Enabled(features.PodSchedulingReadiness) && len(pod.Spec.SchedulingGates) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be in validation - a regular user can patch a pod and set spec.nodeName without binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, update's validation has already been covered. Here it's an additional validation for pod creation - prevent user creating a pod directly with nodeName and non-empty scheduling gates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here pod.Spec.NodeName
won't be empty, we returned in the above. I think we should move this ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite. L223~L225 is checking whether it's updating nodeName X to Y.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prevent user creating a pod directly with nodeName and non-empty scheduling gates
I see, misunderstood the words here. Great catch.
Thanks. Tracked in #113608 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei, smarterclayton 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 |
- New API field .spec.schedulingGates - Validation and drop disabled fields - Disallow binding a Pod carrying non-nil schedulingGates - Disallow creating a Pod with non-nil nodeName and non-nil schedulingGates - Adds a {type:PodScheduled, reason:WaitingForGates} condition if necessary - New literal SchedulingGated in the STATUS column of `k get pod`
/lgtm |
/hold |
/unhold |
/hold cancel |
A bit late to the game, but this looks great - thx! |
/remove-sig api-machinery |
/triage accepted |
@Huang-Wei @smarterclayton there is a typo the PR comment that made it into the CHANGELOG. From the user-facing change section (that is scraped by the changelog generator), |
What type of PR is this?
/kind feature
/sig scheduling
What this PR does / why we need it:
This is the first part of implementing KEP 3521, including:
{type:PodScheduled, reason:SchedulingGated}
condition upon pod creationSchedulingGated
in the STATUS column ofk get pod
Which issue(s) this PR fixes:
Part of #113269
Special notes for your reviewer:
Get best review UX commit by commit.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: