-
Notifications
You must be signed in to change notification settings - Fork 228
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
Increase reconcilers for Pod #1589
Increase reconcilers for Pod #1589
Conversation
[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 |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
b087c57
to
ec59bab
Compare
Change-Id: Ibee32beacf7c6e705a5f7d0ca69d5d6a4fa9944a
ec59bab
to
39337b2
Compare
The error happened during cleanup and it doesn't seem related to this change. But I opened #1591 to investigate. |
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.
The code changes LGTM, asked some questions to better understand the context.
@@ -13,10 +13,11 @@ leaderElection: | |||
controller: | |||
groupKindConcurrency: | |||
Job.batch: 5 | |||
Pod.: 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.
What is the default if not specified? Also, why only batch.Job and Pod frameworks are picked to have a custom value (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.
The default is one.
We could add all integrations, but this is just a default configuration that should work for most users.
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 see, and we guesstimate to need more for pods, because there will be many of such objects, because the worker is invoked per pod, not per the grouping object as for say "MPIJob", or "PyTorchJob"?
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.
Would that make sense it defaults according to the number of CPUs available / cgroups CPU quota?
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 number is a default, users will have to pick up what makes sense for them.
We just have to pick some numbers that work at a reasonable scale (~50 nodes).
I picked the number based on the default number of workers in kcm.
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.
sgtm, we have concurrent-job-syncs: 5
indeed. I'm wondering if we should set 5 also for other Job CRDs, but that would probably be for another PR.
/retest |
/lgtm |
LGTM label has been added. Git tree hash: 5db6052880697585b3553652b7c80e6e3fc14a24
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Increase the number of reconcilers for Pod in the default configuration and E2E tests.
Also doing the same for Workload object, which are expected to be a significant number of objects.
Which issue(s) this PR fixes:
Fixes #1448
Special notes for your reviewer:
Does this PR introduce a user-facing change?