-
Notifications
You must be signed in to change notification settings - Fork 40.9k
Clear pod.Status.NominatedNodeName when pod is bound #132443
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
base: master
Are you sure you want to change the base?
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Changelog suggestion -When a pod is successfully bound to a node, the kube-apiserver now clears the pod's NominatedNodeName field to prevent stale information from affecting external scheduling components.
+Whenever a pod is successfully bound to a node, the kube-apiserver now clears the pod's `nominatedNodeName` field. This prevents stale information from affecting external scheduling components. |
c2ade7a
to
58b4179
Compare
@imikushin Thanks for your review. Updated. |
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.
We (i.e., including me in other PRs) forgot feature-gating the logic!!! 😅
58b4179
to
0d030b4
Compare
0d030b4
to
5ae0d94
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sanposhiho, utam0k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3106b92
to
395d168
Compare
ff52974
to
603b4f9
Compare
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.
still
/lgtm
from sig-scheduling
LGTM label has been added. Git tree hash: d73c51eeab270fa51f422436dc477f9cf8f222c2
|
@@ -973,6 +973,12 @@ | |||
lockToDefault: false | |||
preRelease: Beta | |||
version: "1.30" | |||
- name: NominatedNodeNameForExpectation | |||
versionedSpecs: | |||
- default: false |
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 think it should be enabled by default?
pkg/features/kube_features.go
Outdated
@@ -1379,6 +1389,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate | |||
{Version: version.MustParse("1.30"), Default: true, PreRelease: featuregate.Beta}, | |||
}, | |||
|
|||
NominatedNodeNameForExpectation: { | |||
{Version: version.MustParse("1.34"), Default: false, PreRelease: featuregate.Beta}, |
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.
{Version: version.MustParse("1.34"), Default: false, PreRelease: featuregate.Beta}, | |
{Version: version.MustParse("1.34"), Default: true, PreRelease: featuregate.Beta}, |
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.
Hi, @dom4ha 👋 Thanks for your review.
I believe that the default is set to true when approaching GA.
cc: @sanposhiho
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.
During beta, the feature gate should be enabled by default, so the @dom4ha's comment is valid
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'm adding a feature gate for the first time and don't quite understand the process. Thanks for letting me know 🙏
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'm adding a feature gate for the first time and don't quite understand the process. Thanks for letting me know 🙏
AFAIK there is no rule regarding the default value in beta, but it's described in the KEP.
Signed-off-by: utam0k <[email protected]>
603b4f9
to
8192a1c
Compare
@utam0k: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/lgtm |
LGTM label has been added. Git tree hash: 19e0bf7e631766fe1c7768ff76f1ff54037ea53f
|
@@ -256,6 +256,11 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI | |||
Type: api.PodScheduled, | |||
Status: api.ConditionTrue, | |||
}) | |||
// Clear nomination hint to prevent stale information affecting external components. |
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.
does anything prevent setting this again via another status update after the pod is bound?
@@ -256,6 +256,11 @@ func (r *BindingREST) setPodNodeAndMetadata(ctx context.Context, podUID types.UI | |||
Type: api.PodScheduled, | |||
Status: api.ConditionTrue, | |||
}) | |||
// Clear nomination hint to prevent stale information affecting external components. | |||
if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.NominatedNodeNameForExpectation) { |
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 having read the KEP before reviewing this, this is a surprising behavior to associate with a feature named "NominatedNodeNameForExpectation" ... clearing the NominatedNode field after binding (and not letting it be set on an already-bound pod) makes a lot of sense... is there a clearer name we could use for the gate of this behavior change?
What type of PR is this?
/kind feature
/sig scheduling
/cc sanposhiho
What this PR does / why we need it:
This PR implements the kube-apiserver side of KEP-5278 by clearing the NominatedNodeName field when a pod is successfully bound to a node through the binding API.
Previously, only the scheduler cleared NominatedNodeName, but with KEP-5278, external components like Cluster Autoscaler and Karpenter can also set this field. To prevent stale information from affecting scheduling decisions, the apiserver now clears this field upon successful binding, regardless of which component performed the scheduling.
Which issue(s) this PR is related to:
Fixes #132385
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: