-
Notifications
You must be signed in to change notification settings - Fork 41.2k
scheduler: stop clearing NominatedNodeName on all cases #132439
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
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. |
/test pull-kubernetes-unit |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
/test pull-kubernetes-e2e-kind |
ad9c039
to
9b2783b
Compare
Is this relevant to #132443? |
Changelog suggestion -The scheduler no longer clears the NominatedNodeName field for pods. External components (like Cluster Autoscaler and Karpenter) are responsible for managing this field when needed.
+The scheduler no longer clears the `nominatedNodeName` field for Pods. External components (such as Cluster Autoscaler and Karpenter) are responsible for managing this field when needed. However, see #132443 (comment) We should align the two changelog entries. |
Yes, it is. I've updated the release note. |
/retest-required |
@macsko @dom4ha @sanposhiho PTAL 🙏 |
name: "pod with existing nominated node name clears NNN when feature gate is disabled", | ||
sendPod: func() *v1.Pod { | ||
p := podWithID("foo", "") | ||
p.Status.NominatedNodeName = "existing-node" |
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.
Can't we add the logic with NominatedNodeName to the test itself?
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.
@macsko Thank you for your review. I want to make sure I understand your suggestion correctly.
Currently, the test logic already handles NominatedNodeName dynamically for all test cases (lines
992-993):
if !nominatedNodeNameForExpectationEnabled && expectedNominatingInfo == nil {
expectedNominatingInfo = &framework.NominatingInfo{NominatingMode: framework.ModeOverride,
NominatedNodeName: ""}
}
Are you suggesting that:
- We should remove the dedicated test case I added and rely on the existing test logic?
- Or should we modify the existing test cases to include NominatedNodeName scenarios?
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 meant to run even this dedicated test case with the feature enabled and disabled. And, check everything required in the testing code.
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.
Now I think this test case can even be dropped. It's outcome is equivalent to the schedule pod failed
test case, as the NominatedNodeName
on sendPod
is ignored in these tests anyway.
b5d0d32
to
bb3dece
Compare
/retest-required |
Signed-off-by: utam0k <k0ma@utam0k.jp>
/test pull-kubernetes-unit |
PR needs rebase. 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. |
/milestone v1.34 |
/milestone v1.34 |
} | ||
if nominatedNodeNameForExpectationEnabled { | ||
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NominatedNodeNameForExpectation, nominatedNodeNameForExpectationEnabled) |
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.
} | |
if nominatedNodeNameForExpectationEnabled { | |
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NominatedNodeNameForExpectation, nominatedNodeNameForExpectationEnabled) | |
} else { | |
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.NominatedNodeNameForExpectation, nominatedNodeNameForExpectationEnabled) |
expectedNominatingInfo = &framework.NominatingInfo{NominatingMode: framework.ModeOverride, NominatedNodeName: ""} | ||
} | ||
if diff := cmp.Diff(expectedNominatingInfo, gotNominatingInfo); diff != "" { | ||
t.Errorf("Unexpected nominatingInfo diff (-want +got):\n%s", diff) |
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.
t.Errorf("Unexpected nominatingInfo diff (-want +got):\n%s", diff) | |
t.Errorf("Unexpected nominatingInfo (-want,+got):\n%s", diff) |
name: "pod with existing nominated node name clears NNN when feature gate is disabled", | ||
sendPod: func() *v1.Pod { | ||
p := podWithID("foo", "") | ||
p.Status.NominatedNodeName = "existing-node" |
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.
Now I think this test case can even be dropped. It's outcome is equivalent to the schedule pod failed
test case, as the NominatedNodeName
on sendPod
is ignored in these tests anyway.
|
||
for _, p := range pods { | ||
if p.Status.NominatedNodeName != "" { | ||
podsOnceNominated = append(podsOnceNominated, p.Name) |
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 this slice even populate at any point? p.Status.NominatedNodeName
is likely always empty, as we don't overwrite these particular objects with the new value anywhere (not calling informers). Isn't it enough to check the "medium" pod only, similarly to the previous mechanism (in if !tt.enableNominatedNodeNameForExpectation
)?
@@ -1735,6 +1767,23 @@ func TestNominatedNodeCleanUp(t *testing.T) { | |||
} | |||
} | |||
|
|||
if tt.enableNominatedNodeNameForExpectation { |
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 code could be moved at the bottom of the testing code (where if !tt.enableNominatedNodeNameForExpectation
resides). But, we have to make sure this check won't be ran too early, i.e. before the pod is actually re-processed.
/close |
@macsko: Closed this PR. In response to this:
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. |
What type of PR is this?
/kind feature
/sig scheduling
/cc sanposhiho
What this PR does / why we need it:
ref: #132384
Which issue(s) this PR is related to:
Fixes #132384
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: