Skip to content
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

Add NominatedNodeName field to PodStatus #58990

Merged
merged 2 commits into from
Feb 2, 2018

Conversation

bsalamat
Copy link
Member

@bsalamat bsalamat commented Jan 29, 2018

What this PR does / why we need it:
Today, Scheduler uses an annotation called "nominated-node-name" to mark a preemptor Pod. This annotation helps scheduler know about the Pods that are destined to run on the nodes so that the resources made available by preemption is not allocated to a different Pod. In a recent discussion with @bgrant0607, we learned that we should change the annotation to a field as this field can be used by multiple schedulers and other components that may make scheduling-related decisions (descheduler, auto-scaler, kube-arbitrator, ...).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

ref #57471

Special notes for your reviewer:

Release note:

Add "nominatedNodeName" field to PodStatus. This field is set when a pod preempts other pods on the node.

/sig scheduling

@bsalamat bsalamat added this to the v1.10 milestone Jan 29, 2018
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 29, 2018
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jan 29, 2018
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2018
@@ -3051,6 +3051,13 @@ type PodStatus struct {
// More info: https://git.k8s.io/community/contributors/design-proposals/node/resource-qos.md
// +optional
QOSClass PodQOSClass `json:"qosClass,omitempty" protobuf:"bytes,9,rep,name=qosClass"`
// NominatedNodeName is set when this pod preempts other pods on the node, but it cannot be
Copy link
Member

Choose a reason for hiding this comment

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

These comments are translated to user-facing documentation. Either don't mention the field name or use the json field name (nominatedNodeName).

Copy link
Member

Choose a reason for hiding this comment

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

Details to specify somewhere:

  1. Will this field always be set before nodeName is set?
  2. Will this field be unset when nodeName is set?
  3. Will this field be changed if nodeName is set to something different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the comment and moved the field up. Also added one more line to the comment to mention that NominatedNodeName and NodeName are not necessarily the same. More details about how this field is set is provided in preemption design doc.

@bgrant0607
Copy link
Member

Note: New fields shouldn't necessarily be put at the bottom. Please place the field near logically related fields, in order of importance / relevance.

Will the annotation still be understood/respected in the next release?

@bgrant0607 bgrant0607 self-assigned this Jan 30, 2018
@bgrant0607
Copy link
Member

Also, there should be a release note with the introduction of any new API field.

@bgrant0607
Copy link
Member

Release-note guidance: kubernetes/community#484

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 30, 2018
@bsalamat bsalamat force-pushed the nominated_node branch 2 times, most recently from ce24497 to 556a36b Compare January 30, 2018 20:55
@bgrant0607
Copy link
Member

Please write the release note in a form that would match what a user would see in the API.

// This field does not guarantee that the pod will be scheduled on this node. Scheduler may decide
// to place the pod elsewhere if other nodes become available sooner. Scheduler may also decide to
// give the resources on this node to a higher priority pod that is created after preemption.
// As a result, this field may be different than PodSpec.nodeName when the pod is
Copy link
Member

Choose a reason for hiding this comment

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

Why is it desirable to allow them to be different? Why not either set them to match or clear the nominatedNodeName once nodeName is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a couple of reasons:

  1. It is useful to understand/debug behavior of the cluster. When a pod causes preemption, but it is scheduled elsewhere, checking nominatedNodeName reveals that this is the pod that caused preemption on the node. Users who don't have access to scheduler logs can still see this field.
  2. Performance: setting the nominatedNodeName to match the nodeName or clearing it requires another API call.

@bsalamat
Copy link
Member Author

bsalamat commented Jan 31, 2018

Please write the release note in a form that would match what a user would see in the API.

Done.
Thanks for your review, @bgrant0607!

@bgrant0607
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, bsalamat

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants