-
Notifications
You must be signed in to change notification settings - Fork 385
Support default sidecar proxy resources #470
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.
This looks good! I've tested it out, and everything looked good to me. Going back to my comment in the hashicorp/consul-k8s#267 about checking if limit >= resource, I don't think we need to do it for annotations. I was able to see the error message in the status of my deployment. But I still think it'd be useful to check that for defaults in the connect inject command.
I'm curious though about this choice of defaults. Are you concerned that making our recommended defaults actual defaults will be a breaking change? Would it make sense to only set requests but not limits to eliminate the concern around running up against limits?
28a1935
to
f0ce5e1
Compare
I made this decision (not setting defaults) to make the first-run experience better. Needing 100Mi/100m per pod just for the sidecar could add up quickly on a small cluster where someone is testing things out. Since all of our defaults are catered towards the first-run experience and we expect production users to customize settings themselves this decision felt inline with the rest of the chart defaults. But I'm not super set on this, there are good arguments both sides. |
310924d
to
cc13b6e
Compare
2a3a4a0
to
97d94c6
Compare
cc13b6e
to
19d5921
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.
This looks good! Keeping defaults as null makes sense for now.
1143c7e
to
7627d3c
Compare
19d5921
to
818b55e
Compare
NOTE: Waiting for consul-k8s build. |
Require #499 |
Companion to hashicorp/consul-k8s#267
Allow setting default sidecar proxy resources. The defaults are set to
null
so that by default the envoy proxy doesn't run up against any limits.To test, use