Allow init and lifecycle containers resource limits to be configured #298
Conversation
adilyse
added
type/enhancement
adilyse
requested review from
a team,
lkysow and
ishustava
and removed request for
a team
There was a problem hiding this comment.
This looks good! I've made a couple of edits and refactoring suggestions.
Deploy the connect injector using the new flags and verify that any injected pods include the provided values.
Should we wait for the consul-helm changes to deploy it or make those changes ourselves in order to test it? Or maybe use hashicorp/consul-helm#532?
CHANGELOG.md
Outdated
| @@ -1,5 +1,9 @@ | |||
| ## UNRELEASED | |||
|
|
|||
| IMPROVEMENTS: | |||
|
|
|||
| * Connect: Add resource request and limit flags for the injected init container and lifecycle sidecar. | |||
There was a problem hiding this comment.
| * Connect: Add resource request and limit flags for the injected init container and lifecycle sidecar. | |
| * Connect: Add resource request and limit flags for the injected init and lifecycle sidecar containers [[GH-298](https://github.com/hashicorp/consul-k8s/pull/298)]. |
There was a problem hiding this comment.
Should we also mention what their defaults are and why?
There was a problem hiding this comment.
Updated. No, I don't think listing the new flags is necessary here. The flags are clearly laid out in the PR and descriptions available through the command's help text.
The pull request didn't exist so I couldn't link it when I wrote this up. It's pretty circular to expect it as part of the PR submission.
|
As I was testing this out, I had a question about how would one disable/remove resources for init and lifecycle containers. I think in a Helm/yaml world you would just set it to an empty hash, but how would we translate that to consul-k8s flags? I couldn't find a way to set these flags in a way that would essentially remove them. I could be missing something. I'll address the rest of the comments and finish my testing tomorrow. |
There was a problem hiding this comment.
I built a docker image and tested it out and it all works!
I looked over the refactoring suggestions again and I think they make sense to tackle in this PR. I briefly looked at the code changes to make them happen and they didn't seem too onerous so I think we should get them in while we're here.
|
I've finished my testing; everything looked great! I've resolved the issue from my comment above with @lkysow 's suggestion to set flags to I don't have any other feedback, other than 👍 to Luke's refactoring comments that if we think this is a good refactor, we should address it here. |
CHANGELOG.md
Outdated
| @@ -1,5 +1,9 @@ | |||
| ## UNRELEASED | |||
|
|
|||
| IMPROVEMENTS: | |||
|
|
|||
| * Connect: Add resource request and limit flags for the injected init container and lifecycle sidecar. | |||
There was a problem hiding this comment.
Updated. No, I don't think listing the new flags is necessary here. The flags are clearly laid out in the PR and descriptions available through the command's help text.
The pull request didn't exist so I couldn't link it when I wrote this up. It's pretty circular to expect it as part of the PR submission.
…nit container Adds default flag values so this isn't a breaking change. Moves the flag parsing and validation into a separate function which includes the translation into a ResourceRequirement.
adilyse
force-pushed
the
make_resource_mandatory_for_sidecar_and_init
branch
from
fe070f1
to
9a57a42
Compare
Allow setting hostPath for client data directory
Updates the work done in #289.
Changes proposed in this PR:
How I've tested this PR:
Added and updated tests for the functionality. Changed default values to unique values and deployed to a cluster, verifying that they were applied and in the correct place.
How I expect reviewers to test this PR:
Verify the tests function as expected and include all the relevant cases. Deploy the connect injector using the new flags and verify that any injected pods include the provided values.
Checklist: