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

Added emptyDirs for provisioning directories #574

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Oct 20, 2021

Description

Added emptyDirs to provisioning directories.

Relevant issues/tickets

#501

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

@NissesSenap
Copy link
Collaborator

Is there a risk that these folders grow in size? My guess is no but I don't know.
Recently I have hit some issues with emptyDir:s growing by allot, like 100Gb ^^
This created issues with evictions on other pods due to missing disk for them when they wanted to use like 5Mb or something.

Internally we have started to apply the following to our emptyDir configs.

      emptyDir:
        sizeLimit: 10Mi

Another option could of course be to tell the users that thinks it's a problem they can always use:
https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#setting-requests-and-limits-for-local-ephemeral-storage

spec.containers[].resources.limits.ephemeral-storage
spec.containers[].resources.requests.ephemeral-storage

Yeah that is probably better because then we don't have to think about how much grafana might use these emptyDir.

@Bobbins228
Copy link
Contributor Author

@NissesSenap Are suggesting that we go with telling the users that they can use those ephemeral storage limits or somehow add these limits to the emptyDirs through the operator?

@NissesSenap
Copy link
Collaborator

Sorry for the confusion @Bobbins228, I did a few to many things at the same time yesterday so my entry wasn't that great.
I thought a bit more about it and I do think spec.containers[].resources.limits.ephemeral-storage would be the way to go but it shouldn't be included in this PR.
Sadly we don't support limits on ephemeral-storage currently but I will create a seperate ticket for it.

Overall I think this PR looks good, I will take a second check later today.

Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

Ran the code locally and can't see any of the errors at startup any more.
LGTM
Thanks @Bobbins228

@NissesSenap NissesSenap merged commit 0361195 into grafana:master Oct 21, 2021
@Bobbins228 Bobbins228 deleted the fix-missing-dirs branch October 21, 2021 19:26
@mcwienczek
Copy link

I think this functionality is missing in version 5 of the operator. Should I create a regression bug for this?

@NissesSenap
Copy link
Collaborator

Sounds reasonable. We will check it out

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

3 participants