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.

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

Allow to skip resources on agent images through annotations #174

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

sarahhenkens
Copy link
Contributor

Currently, it's not possible to skip creating the resource requests and limits on the injected containers via the injector. At most, you can set them to empty strings which will emit a 0 as the limits.

This pull request ensures that resource configuration can be skipped if the user has disabled them in their annotations by setting their value to "" (empty string).

  • Specifying vault.hashicorp.com/agent-limits-cpu: "" will skip assigning the resource limit all together
  • Manually setting vault.hashicorp.com/agent-limits-cpu: "0" will still allow you to set it to 0
  • No changes to the default resources when they are omitted from the annonations

Current Behavior

The current behavior will emit a literal 0 for the limits and requests if setting them to "", which has some unexpected behavior with the scheduler and will cause the agents to get OOM killed.

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: ""
  vault.hashicorp.com/agent-requests-mem: ""

Will result in:

Init Containers:
  vault-agent-init:
    Restart Count:  0
    Limits:
      cpu:     0
      memory:  0
    Requests:
      cpu:     0

Updated behavior:

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: ""
  vault.hashicorp.com/agent-requests-mem: ""

Will result in no resources set all together. As its the default for pods in Kubernetes.

Specifying only CPU

When selecting only CPU limits, it is working as expected:

annotations:
  vault.hashicorp.com/agent-inject: "true"
  vault.hashicorp.com/role: your-vault-role
  vault.hashicorp.com/agent-limits-cpu: ""
  vault.hashicorp.com/agent-limits-mem: ""
  vault.hashicorp.com/agent-requests-cpu: "150m"
  vault.hashicorp.com/agent-requests-mem: "64Mi"

Will result in:

    Restart Count:  0
    Requests:
      cpu:     150m
      memory:  64Mi
    Environment:

Testing

  • Updated the unit tests to ensure tests pass and check for the new behavior
  • Confirmed that the build passes locally
  • Manually tested in my local kubernetes cluster

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 26, 2020

CLA assistant check
All committers have signed the CLA.

@sarahhenkens
Copy link
Contributor Author

@tvoran, @jasonodonnell, friendly bump up on this pending PR :) Would love your 👀 , feedback and thoughts 🙌

@jasonodonnell
Copy link
Contributor

Hi @sarahhenkens, I had taken a look at this but got distracted, my apologies! In general this looks fine but I'll need to do some functionality testing and a thorough code review.

I'm wondering what your use case is for disabling resource limits? Typically we don't see uncapped containers.

@sarahhenkens
Copy link
Contributor Author

I'm wondering what your use case is for disabling resource limits? Typically we don't see uncapped containers.

The current version of Kubernetes has some inefficient mechanisms in how it executes CPU throttling through cgroups. Because of this, I decided to not use limits at all in my setup and just use requests.

Kubernetes doesn't enforce users to set resource limits, so the use case is generally: Make it compatible with Kubernetes own capabilities without being overly opinionated on resource best practices.

The current version of vault-k8s supports disabling of resource configuration by passing in "" but it incorrectly sets it to 0 instead of omitting the resource definitions from the Yaml Resource config. This has some unexpected behavior with the scheduler and will cause the agents to get OOM killed.

Since this PR doesn't change the default behavior of vault-k8s, I see it more of an enhancement to give end-users of this library more choices by allowing them to do any of the following:

  • Don't set any overrides, use the default
  • Set explicit overrides
  • Set them to 0 if they really have a reason for it
  • Disable resources if they don't want vault-k8s to add resource limits to the sidecar init-container.

Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

Code looks good, tested fine locally and unit tests are passing. LGTM, thank you for the contribution!

@jasonodonnell jasonodonnell merged commit ce5b77d into hashicorp:master Sep 3, 2020
@sarahhenkens
Copy link
Contributor Author

Thank you for accepting the change! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants