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 multiple init pods to scheduler perf test cases #89272
Add multiple init pods to scheduler perf test cases #89272
Conversation
/assign @ingvagabund |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor 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 |
/test pull-kubernetes-e2e-gce |
/priority important-longterm |
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.
Few nits though looks good in overall. Can you also split the PR into two commits? First one where the functions return error and the second one that adds multiple init pods. So it's easier to follow the changes in the future.
for { | ||
scheduled, err := getScheduledPods(podInformer) | ||
if err != nil { | ||
klog.Fatalf("%v", err) | ||
return nil |
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.
return err
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.
Oops. Fixed.
|
||
testCases := make([]testCase, 0) | ||
for _, s := range simpleTests { | ||
testCase := s.Template | ||
for _, p := range s.Params { | ||
testCase.Nodes.Num = p.NumNodes | ||
testCase.InitPods.Num = p.NumInitPods | ||
testCase.InitPods = append([]podCase(nil), testCase.InitPods...) |
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.
Why is this change necessary?
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.
Otherwise the slice is shared and a second iteration modifies both test cases.
18a3727
to
c881580
Compare
That would require to rewrite a lot of the PR. I don't think the "return error" changes are significant. Is that fine with you? |
/retest |
/priority important-longterm |
It's not a must though it will help to make the git history more friendly. Please, reconsider. |
The test tool doesn't work properly with klog.Fatal Signed-off-by: Aldo Culquicondor <acondor@google.com>
Add test case with several init pods with affinity or antiaffinity. Signed-off-by: Aldo Culquicondor <acondor@google.com>
c881580
to
5adc4c4
Compare
Done. Thanks for reminding me to be mindful of traceability. |
/retest |
ping @ingvagabund |
Sorry, I got distracted, to many tabs open. /lgtm |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allowing different types of init pods we can represent more realistic test cases for benchmarks and profiling.
I'm adding a test case where there are several init pods with affinity or antiaffinity, and we measure basic pods.
I'm also surfacing suppressed errors and replacing
klog.Fatalf
withb.Fatalf
. The previous approach wasn't playing well with the test tool.Sample run: https://gist.github.com/alculquicondor/5d98a2f6534a8fd6dca8564c3d75b196
Does this PR introduce a user-facing change?: