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
Fix inter-pod affinity scheduler benchmarks #86028
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g 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 |
0a39a2c
to
589d849
Compare
@@ -67,15 +70,24 @@ func BenchmarkScheduling(b *testing.B) { | |||
// PodAntiAffinity rules when the cluster has various quantities of nodes and | |||
// scheduled pods. | |||
func BenchmarkSchedulingPodAntiAffinity(b *testing.B) { | |||
tests := []struct{ nodes, existingPods, minPods int }{ | |||
{nodes: 500, existingPods: 100, minPods: 400}, | |||
{nodes: 5000, existingPods: 1000, minPods: 1000}, |
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 the pods has anti affinity to each other, the number of pods to schedule can't exceed the number of nodes (the topology used in the test)
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.
Ack. Can you add the comment in the 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.
done.
589d849
to
72f70dc
Compare
/cc @Huang-Wei |
/retest |
2 similar comments
/retest |
/retest |
@@ -67,15 +70,24 @@ func BenchmarkScheduling(b *testing.B) { | |||
// PodAntiAffinity rules when the cluster has various quantities of nodes and | |||
// scheduled pods. | |||
func BenchmarkSchedulingPodAntiAffinity(b *testing.B) { | |||
tests := []struct{ nodes, existingPods, minPods int }{ | |||
{nodes: 500, existingPods: 100, minPods: 400}, | |||
{nodes: 5000, existingPods: 1000, minPods: 1000}, |
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.
Ack. Can you add the comment in the code?
name := fmt.Sprintf("%vNodes/%vPods", test.nodes, test.existingPods) | ||
b.Run(name, func(b *testing.B) { | ||
benchmarkScheduling(test.nodes, test.existingPods, test.minPods, nodeStrategy, testStrategy, b) | ||
nodeStrategies := []testutils.CountToStrategy{{Count: test.nodes, Strategy: nodeStrategy}} |
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.
Should we add LabelHostName for this as well?
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.
no because we will end up having to schedule all the pods on one node, which does not really make sense unless we create tuples of pods with affinity to each other rather than having all pods having affinity to each other.
The test is still useful in that all the pods will still have affinity to each other at the zone level.
{nodes: 500, existingPods: 500, minPods: 1000}, | ||
{nodes: 5000, existingPods: 5000, minPods: 1000}, | ||
} | ||
testNamespace = "sched-test" |
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 we use a single namespace?
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 could, but some features work at the namespace level, so I find it useful to test that.
72f70dc
to
a051c59
Compare
/lgtm |
/hold |
/hold cancel |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Some affinity benchmarks use
v1.LabelHostname
as the topology in the affinity term, but this label is not set on the nodes, and so the benchmark was not quite accurate in what it evaluates.This PR fixes that by adding the
v1.LabelHostname
label to the nodes.Does this PR introduce a user-facing change?: