Add additional resource settings for Connect and gateways #556
Conversation
adilyse
added
area/connect
adilyse
requested review from
a team,
lkysow and
thisisnotashwin
and removed request for
a team
There was a problem hiding this comment.
This is most excellent @adilyse !! Can't wait to see this in the open :)
| @@ -138,6 +138,38 @@ spec: | |||
| {{- if not (kindIs "invalid" $resources.requests.cpu) }} | |||
| -default-sidecar-proxy-cpu-request={{ $resources.requests.cpu }} \ | |||
| {{- end }} | |||
|
|
|||
| {{- if .Values.connectInject.initContainer }} | |||
There was a problem hiding this comment.
Do we need to support
connectInject:
initContainer: null
? (I'm assuming that's what this is for).
I think it would be fine to support
connectInject:
initContainer:
resources: null
and that's the pattern we follow for the sidecarProxy.resources.
I have the same question for the other sections.
There was a problem hiding this comment.
We give a nil pointer exception if someone removes or comments out the entire initContainer section in the values file. This check handles that case. If we aren't worried about that scenario, the extra layer of checks can be removed.
There was a problem hiding this comment.
This Helm doc is related.
There was a problem hiding this comment.
I don't think we need to handle that case since I think it would be unlikely a user does that and since I haven't seen other locations where we're doing it. But I don't feel too strongly about it.
There was a problem hiding this comment.
Since it provides some extra protection, I'm going to leave it for now.
| yq 'any(contains("-lifecycle-sidecar-cpu-limit=200m"))' | tee /dev/stderr) | ||
| [ "${actual}" = "true" ] | ||
| } | ||
|
|
||
| #-------------------------------------------------------------------- | ||
| # sidecarProxy.resources |
There was a problem hiding this comment.
Should we add tests for:
- can set
initContainer: null,lifecycleSidecarContainer: null(if we keep that functionality) - can set
resources: null - can set each setting to
nulland the flag won't be set - can set explicitly to 0 and the flag will be set
There was a problem hiding this comment.
I was hesitant to add too many tests before getting feedback. I've add those suggested except the first, pending a decision on that functionality.
adilyse
force-pushed
the
refactor_container_resources
branch
from
cf1f3f9 to
747f284
Compare
… resources to consul-k8s
Init container resource settings are now split into each individual section. Lifecycle sidecar settings remain a global setting.
adilyse
force-pushed
the
refactor_container_resources
branch
from
747f284 to
30be79c
Compare
Updates #532.
Adds resource setting configuration values for lifecycle sidecar and init containers for Connect and gateways.