-
Notifications
You must be signed in to change notification settings - Fork 321
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
lifecycle-sidecar and connect inject init container have mandatory resource requirements #289
Conversation
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.
LGTM!! Good job bud!!
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.
Looking at this now I think it definitely does need a refactor as we discussed. The actual code flow is great. I don't think it would complicate things too much to include the refactor here as the tests would stay the same.
LifecycleSidecarCPULimit resource.Quantity | ||
LifecycleSidecarCPURequest resource.Quantity | ||
LifecycleSidecarMemoryLimit resource.Quantity | ||
LifecycleSidecarMemoryRequest resource.Quantity |
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.
As discussed, in the refactor I think we could have
LifecycleSidecarResources corev1.ResourceRequirements
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.