Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support resource settings #267

Merged
merged 2 commits into from
Jun 3, 2020
Merged

Support resource settings #267

merged 2 commits into from
Jun 3, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented May 26, 2020 •
edited
Loading

  • Set resource settings for init and lifecycle containers. These are set by default and not overridable. They're set by default to avoid needing a separate flag to turn them on. This is safe because they're set to more than double observed resource requirements so users that don't need resource settings won't be affected. They're not exposed for overriding because they won't change based on use-case, i.e. a service with a lot of throughput may need to customize its envoy proxy cpu/memory however the init container and lifecycle sidecar will need the same cpu/memory as all the other connect pods since they're doing the same job.
  • Allow setting resource settings for envoy sidecar via default settings passed in through Helm chart values that will apply to all pods, overridable by annotations on injected pods.
  • Flags:
    • -default-sidecar-proxy-cpu-limit
    • -default-sidecar-proxy-cpu-request
    • -default-sidecar-proxy-memory-limit
    • -default-sidecar-proxy-memory-request
  • Annotations:
    • consul.hashicorp.com/sidecar-proxy-cpu-limit
    • consul.hashicorp.com/sidecar-proxy-cpu-request
    • consul.hashicorp.com/sidecar-proxy-memory-limit
    • consul.hashicorp.com/sidecar-proxy-memory-request

See hashicorp/consul-helm#470 for testing.

Refactors @wuvs work from #109.

@lkysow lkysow mentioned this pull request May 26, 2020
Merged
@lkysow lkysow marked this pull request as ready for review May 26, 2020 20:41
@lkysow lkysow requested a review from a team May 26, 2020 20:41
@ishustava ishustava self-assigned this May 28, 2020
ishustava
ishustava reviewed May 28, 2020
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luke, great work! It's very exciting to see this! This mostly looked good to me, and I left a couple of comments and suggestions.

One question I had is whether you think we need to validate that limit >= request for annotations and defaults. I think this is an invalid configuration and kubernetes will return an error. I'm thinking that at least in the case of defaults, it might be useful to get early feedback before you get to deploying services.

connect-inject/envoy_sidecar.go Outdated Show resolved Hide resolved
connect-inject/envoy_sidecar_test.go Outdated Show resolved Hide resolved
connect-inject/envoy_sidecar_test.go Show resolved Hide resolved
connect-inject/envoy_sidecar_test.go Show resolved Hide resolved
subcommand/inject-connect/command.go Outdated Show resolved Hide resolved
subcommand/inject-connect/command.go Outdated Show resolved Hide resolved
subcommand/inject-connect/command.go Outdated Show resolved Hide resolved
subcommand/inject-connect/command.go Outdated Show resolved Hide resolved
subcommand/inject-connect/command.go Show resolved Hide resolved
@lkysow lkysow requested a review from ishustava June 1, 2020 23:29
@lkysow
Copy link
Member Author

lkysow commented Jun 1, 2020

One question I had is whether you think we need to validate that limit >= request for annotations and defaults. I think this is an invalid configuration and kubernetes will return an error. I'm thinking that at least in the case of defaults, it might be useful to get early feedback before you get to deploying services.

Good call, added.

ishustava
ishustava approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates look great!

FYI I haven't re-tested everything with these updates on an actual cluster because I haven't found any issues the first time.

@lkysow
Refactor resource settings
d0a01a0 
- default resource settings for the envoy sidecar can be set via flags
- these can be overridden by annotations on the injected pods
- by default there are not resource settings
- add resource settings to init container and sidecar container. These
are always set so users who require resource settings don't need to turn
them on. They are set to more than double observed usage so they won't
affect any users. They are not customizable via annotation or flag because they
don't change based on use-case. They could be exposed later.
@lkysow lkysow merged commit 8267732 into master Jun 3, 2020
@lkysow lkysow deleted the resource-limit branch June 3, 2020 23:17
@lkysow lkysow mentioned this pull request Jun 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishustava ishustava ishustava approved these changes

Assignees

@ishustava ishustava

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lkysow @ishustava