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

Size limits for Empty Dirs #632

Merged
merged 9 commits into from
Jul 7, 2022
Merged

Size limits for Empty Dirs #632

merged 9 commits into from
Jul 7, 2022

Conversation

chaitanya14
Copy link
Contributor

@chaitanya14 chaitanya14 commented Jul 6, 2022

What this PR does / why we need it:

Empty directory without size limit is consuming all the ephemeral local storage resources. Need to have a capability to assign limits on it.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@chaitanya14 chaitanya14 requested a review from a team as a code owner July 6, 2022 18:52
@CLAassistant
Copy link

CLAassistant commented Jul 6, 2022

CLA assistant check
All committers have signed the CLA.

@rainest
Copy link
Contributor

rainest commented Jul 6, 2022

What were you seeing these growing to in practice? What were the largest files in the mounts?

I'd prefer to make these static limits rather than configurable if possible, since they should be small and consistent enough in size that users shouldn't need to worry about configuring them. I don't know offhand what'd consume much space in those locations other than logs (which we want to divert to stdout and stderr)--I'm checking with the gateway engineers, but info from your instance would be helpful.

If we do end up using configurable values, they need to be in the default values.yaml and in documentation, else you'll hit the error that's currently failing in CI:

Error:  templates/: template: kong/templates/deployment.yaml:288:10: executing "kong/templates/deployment.yaml" at <include "kong.volumes" .>: error calling include: template: kong/templates/_helpers.tpl:414:25: executing "kong.volumes" at <.Values.deployment.prefixDir.sizeLimit>: nil pointer evaluating interface {}.sizeLimit

@chaitanya14
Copy link
Contributor Author

chaitanya14 commented Jul 7, 2022

What were you seeing these growing to in practice? What were the largest files in the mounts?

I'd prefer to make these static limits rather than configurable if possible, since they should be small and consistent enough in size that users shouldn't need to worry about configuring them. I don't know offhand what'd consume much space in those locations other than logs (which we want to divert to stdout and stderr)--I'm checking with the gateway engineers, but info from your instance would be helpful.

If we do end up using configurable values, they need to be in the default values.yaml and in documentation, else you'll hit the error that's currently failing in CI:

Error:  templates/: template: kong/templates/deployment.yaml:288:10: executing "kong/templates/deployment.yaml" at <include "kong.volumes" .>: error calling include: template: kong/templates/_helpers.tpl:414:25: executing "kong.volumes" at <.Values.deployment.prefixDir.sizeLimit>: nil pointer evaluating interface {}.sizeLimit

I don't have certain numbers on limits but we are facing disk pressure issues on AKS nodes. These directories may or may not be directly effecting our issue but as a best practice it is better to evict the pods if empty dirs are using more ephemeral storage than defined. This will improve things in kubernetes cluster .

@chaitanya14
Copy link
Contributor Author

What were you seeing these growing to in practice? What were the largest files in the mounts?
I'd prefer to make these static limits rather than configurable if possible, since they should be small and consistent enough in size that users shouldn't need to worry about configuring them. I don't know offhand what'd consume much space in those locations other than logs (which we want to divert to stdout and stderr)--I'm checking with the gateway engineers, but info from your instance would be helpful.
If we do end up using configurable values, they need to be in the default values.yaml and in documentation, else you'll hit the error that's currently failing in CI:

Error:  templates/: template: kong/templates/deployment.yaml:288:10: executing "kong/templates/deployment.yaml" at <include "kong.volumes" .>: error calling include: template: kong/templates/_helpers.tpl:414:25: executing "kong.volumes" at <.Values.deployment.prefixDir.sizeLimit>: nil pointer evaluating interface {}.sizeLimit

I don't have certain numbers on limits but we are facing disk pressure issues on AKS nodes. These directories may or may not be directly effecting our issue but as a best practice it is better to evict the pods if empty dirs are using more ephemeral storage than defined. This will improve things in kubernetes cluster .

Also, we have a data plane where we have used 256Mi size of empty dirs that never caused an issue since 4 months. So IMO default to 256Mi is more meaningful.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

We'll want to merge #634 first. Several of those were wrong and would eventually fill the prefix on a long-lived Pod.

There's one unbounded consumer of /tmp/ space whose actual needs will vary depending on use characteristics, so we do indeed need to make these configurable. NGINX will buffer large client bodies to disk, and we set no default upper bound on their size. An instance with a large number of concurrent large bodies (e.g. something that handles video uploads) is expected to consume a bunch of /tmp/ space even though most instances won't, so IMO we should give that one more breathing room.

charts/kong/values.yaml Show resolved Hide resolved
chaitanya14 and others added 2 commits July 7, 2022 13:15
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
charts/kong/values.yaml Outdated Show resolved Hide resolved
chaitanya14 and others added 2 commits July 7, 2022 14:02
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
@chaitanya14 chaitanya14 requested a review from rainest July 7, 2022 20:08
@rainest rainest enabled auto-merge (squash) July 7, 2022 21:05
@rainest rainest merged commit 532c1d0 into Kong:main Jul 7, 2022
@chaitanya14
Copy link
Contributor Author

@rainest When will this be released ?

@rainest
Copy link
Contributor

rainest commented Jul 7, 2022

Probably end of this week or start of next; there are a few other things I'd like to work in if possible.

@@ -2,6 +2,10 @@

## Unreleased

###
Copy link
Member

Choose a reason for hiding this comment

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

We missed a section name in here I suppose

sgrzemski pushed a commit to sgrzemski/charts that referenced this pull request Jul 11, 2022
Add .deployment.prefixDir.sizeLimit and
.Values.deployment.tmpDir.sizeLimit, which control the maximum size of
the prefix and /tmp/ emptyDirs.

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
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.

None yet

4 participants