-
Notifications
You must be signed in to change notification settings - Fork 70
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
Run kpack-image-builder
, job-task-runner
and statefulset-runner
as standalone Deployments
#3522
base: main
Are you sure you want to change the base?
Run kpack-image-builder
, job-task-runner
and statefulset-runner
as standalone Deployments
#3522
Conversation
4732231
to
7e4ba83
Compare
7e4ba83
to
3f36248
Compare
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.
Overall, looks good, minor comments.
I played a bit with the change:
- All tests pass
- Manual testing looks good
- Upgrade from 0.13 seems seamless, helm takes care of the new deployments nicely
- Kind installer works
I was trying to reason about resource requirements and how they are affected by splitting controllers into multiple deployments:
Monolit controllers deployments
- Requests
cpu: 50m
memory: 100Mi - Limits
cpu: 1000m
memory: 1Gi
Multiple controllers deployments
Controllers:
- Requests
cpu: 50m
memory: 100Mi - Limits
cpu: 1000m
memory: 1Gi
Statefulset runner:
- Requests
cpu: 50m
memory: 100Mi - Limits
cpu: 1000m
memory: 1Gi
Kpack builder:
- Requests
cpu: 50m
memory: 100Mi - Limits
cpu: 1000m
memory: 1Gi
Task runner:
- Requests
cpu: 50m
memory: 100Mi - Limits
cpu: 1000m
memory: 1Gi
Total:
- Requests
cpu: 200m
memory: 400Mi - Limits
cpu: 4000m
memory: 4Gi
What I see is that with the default request/limit values, the multiple deployment model requires 4 times more resources in comparisson with the monolit deployment model. I wonder whether we could come up with reasonable defaults that total up to similar numbers
57242b8
to
4b1c144
Compare
@pbusko any thoughts on the comment by @danail-branekov above with regards to limits and requests? |
4b1c144
to
7bd9984
Compare
On the other hand we're talking about 200m CPU and 400mb requests in total, which should be doable on pretty much every workstation/VM (unless it's running on a toaster). Limits themselves should not harm, since they do not allocate anything. |
Is there a related GitHub Issue?
Fixes #3519
Fixes #3495
What is this change about?
Implementation for #3519 (and a fix for #3495)
Does this PR introduce a breaking change?
no
Acceptance Steps
Tag your pair, your PM, and/or team