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
Graduate PodTopologySpread to Beta #88105
Conversation
037c364
to
3c460a8
Compare
Ready for review. |
I didn't review yet, but this should have a release note. |
// force it to update | ||
nodeCopy.ResourceVersion = "0" | ||
ginkgo.By(fmt.Sprintf("Apply 10 fake resource to node %v.", node.Name)) | ||
nodeCopy.Status.Capacity[fakeRes] = resource.MustParse("10") |
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.
is this reliable? will kubelet erase this since no such resource actually exist?
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.
will kubelet erase this since no such resource actually exist?
For extended resources, no. We used to exercise this pattern for another test here:
kubernetes/test/e2e/scheduling/preemption.go
Line 280 in 2565140
var _ = SIGDescribe("PreemptionExecutionPath", func() { |
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.
Since we are still not GA, I propose we up the weight of the PodTopologySpread score plugin. PodTopologySpread expresses explicit user intent that I think should have higher weight than scoring generic functions (like NodeResources or ImageLocality).
} | ||
|
||
// Make the nodes have balanced cpu,mem usage. | ||
err := createBalancedPodForNodes(f, cs, ns, nodes, podRequestedResource, 0.5) |
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 know we need this so that the "node resource" plugin doesn't influence the score, but this is going to cause flakes because a pod created by a previous test may get deleted after this function is called, and so breaking the "balanced" assumption. I don't have a better suggestion how to fix this though.
Giving PodTopologySpread a higher priority should make this less important.
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.
Exactly like what I mentioned in #88174 (comment). And this is why I hesitated to add this score test in e2e - you can't adjust the default weight (which is 1) for a given e2e env.
Please update the issue title, something like: "Graduate PodTopologySpread to Beta". Otherwise looks good to me, let's get the features files approved. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, Huang-Wei 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 |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind feature
/sig scheduling
What this PR does / why we need it:
This PR includes the necessary change to enable featuregate
EvenPodsSpread
to be beta, by default. Additionally, e2e tests covering basic happy paths have been added.kube_features.go
Which issue(s) this PR fixes:
Fixes #88168
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: