Init / system-logger containers are not defining the resources to use. #184
Conversation
rcosnita
requested review from
bradfordcp,
jimdickinson,
johntrimble,
respringer and
sandoichi
as code owners
rcosnita
force-pushed
the
master
branch
6 times, most recently
from
8f88d07 to
241da6d
Compare
|
Thank you for the PR! :) Can you add something similar for the reaper sidecar? Also, are you OK with signing a DataStax CLA? We're getting the website for it updated next week, but we can probably merge this in advance if you're up for it. |
|
@jimdickinson I will update the reaper sidecar by the end of tomorrow. Regarding the CLA, I've just signed it. |
rcosnita
force-pushed
the
master
branch
2 times, most recently
from
b7ee9e2 to
2b9871b
Compare
operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
Outdated
Show resolved
Hide resolved
operator/pkg/apis/cassandra/v1beta1/cassandradatacenter_types.go
Outdated
Show resolved
Hide resolved
| // Provides reasonable defaults for the configuration container. | ||
| DefaultsConfigInitContainer = buildResourceRequirements(1000, 256) | ||
|
|
||
| DefaultsReaperContainer = buildResourceRequirements(2000, 512) |
There was a problem hiding this comment.
@jsanda what do you think is reasonable here?
There was a problem hiding this comment.
Sorry for the delayed response. I will try to have some numbers for you today.
There was a problem hiding this comment.
Should we leave the values as is and tune them later? @jsanda / @jimdickinson what do you think?
|
@rcosnita I left some feedback that was a little nitpicky, but I don't think we can merge it as is without the couple small changes. Thank you for the solid effort here! Which CLA did you sign? |
|
@jimdickinson Thank you for the review. I will address by the end of tomorrow. Regarding the CLA, here is a quick screenshot:
|
Co-authored-by: Jim Dickinson <jimdickinson>
Closes #183
|
@jimdickinson Thank you for the review. I merged all the changes in the PR. |
|
Just as final info here is a valid configuration now supported with the code from the pr: systemLoggerResources:
requests:
memory: 1Gi
cpu: 1
limits:
memory: 1Gi
cpu: 1
configBuilderResources:
requests:
memory: 1Gi
cpu: 1
limits:
memory: 1Gi
cpu: 1
reaper:
enabled: true
resources:
requests:
memory: 1Gi
cpu: 1
limits:
memory: 1Gi
cpu: 1 |
|
Looks great! I think I'll add on whatever we need to make these marked as optional on the CRD. |
This PR adds supports for fine grained control over the quotas of each default container currently defined by the cass-operator:
Before this PR, only cassandra container had quota configured. This PR makes the configuration of additional containers explicitly visible in the CRD definition: