lifecycle-sidecar and connect inject init container have mandatory resource requirements #289
Conversation
kschoche
added
type/bug
| LifecycleSidecarCPULimit resource.Quantity | ||
| LifecycleSidecarCPURequest resource.Quantity | ||
| LifecycleSidecarMemoryLimit resource.Quantity | ||
| LifecycleSidecarMemoryRequest resource.Quantity |
There was a problem hiding this comment.
As discussed, in the refactor I think we could have
LifecycleSidecarResources corev1.ResourceRequirements
There was a problem hiding this comment.
Kyle, great work on all these changes; thanks for doing this work!
I've left a couple of comments inline.
An additional thought I had is that this will be a breaking change since we're making new flags required. Now to run this command I need to provide all the newly added resource flags, and I didn't need to before. I know some folks are running sync stand-alone, but I'm not sure if we have any users that are running inject-connect command as stand-alone. Would it make sense to add defaults to these flags to minimize the impact?
| "request must be <= limit: -default-sidecar-proxy-cpu-request value of %q is greater than the -default-sidecar-proxy-cpu-limit value of %q", | ||
| c.flagDefaultSidecarProxyCPURequest, c.flagDefaultSidecarProxyCPULimit)) |
There was a problem hiding this comment.
I think we need to add a similar validation for other resource flags too.
Kyle and I talked about this offline about how it's safe to require the consul-k8s version to match up with consul-helm but we should probably get on the same page about this as a team. In this case I realize it's a moot point because the Helm chart supports setting In terms of defaults though, we agreed we wanted to put all defaults in the Helm chart so if the flag isn't passed in then I think the default needs to be 0 or not set. |
In some environments we are seeing (#283) OOM killer being triggered during the init container for connect inject, as well as high cpu usage inside the lifecycle sidecar (hashicorp/consul-helm#515).
This PR resolves #283 and (hashicorp/consul-helm#529) by making the memory limits configurable by consul-helm, passed in as options in the ingress/mesh/terminating gateways and connect-inject deployments.
Resources for the connect inject init copy container and lifecycle sidecar are passed as flags into the consul-k8s binary now for :
Cpu Limit, Cpu Request, Memory Limit, Memory Request.These are mandatory arguments passed in from consul-helm and defined as defaults which are set high enough to resolve the above issues.This PR also makes the values configurable from the end user instead of compiled in, which will simplify maintenance and usability going forward.