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

[tempo-distributed] Added resource limit and request samples for all components #3452

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

xogoodnow
Copy link
Contributor

Added commented sample for request and limit for components
Having the comments is quite convenient.

@xogoodnow
Copy link
Contributor Author

Hi,
For consistency, shouldn't the resource section of Minio, Admin-api and enterprise gateway be commented out like the other components?

@xogoodnow xogoodnow changed the title Added resource limit and request samples for all components [tempo-distributed] Added resource limit and request samples for all components Nov 28, 2024
@xogoodnow xogoodnow requested a review from a team as a code owner November 28, 2024 10:49
@Sheikh-Abubaker Sheikh-Abubaker added the enhancement New feature or request label Dec 7, 2024
@Starefossen
Copy link
Contributor

Please don't add CPU limit as it is impossible to remove the CPU limit entirely then.

@xogoodnow
Copy link
Contributor Author

Hi @Starefossen,

If you are referring to the enterprise section, for the sake of consistency, since this was the only section on which resource request was set by default I did not comment out the limits as well. Now I have commented the section (since the default value itself was very low as well).
Since they are just comments, I do not see any issues. If you meant something else please let me know.

Cheers

Signed-off-by: xogoodnow <[email protected]>
@Starefossen
Copy link
Contributor

Yes, the comments are no problem. It was just regarding any default values. Sorry for the confusion.

@xogoodnow
Copy link
Contributor Author

Yes, the comments are no problem. It was just regarding any default values. Sorry for the confusion.

Thank you, NP.

Signed-off-by: xogoodnow <[email protected]>
Signed-off-by: xogoodnow <[email protected]>
Copy link
Contributor

@zalegrala zalegrala left a comment

Choose a reason for hiding this comment

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

Thanks of for the PR. I think the comments are good, but not sure about the default values on the two components which add them.

charts/tempo-distributed/values.yaml Show resolved Hide resolved
@@ -1747,6 +1807,9 @@ minio:
persistence:
size: 5Gi
resources:
limits:
cpu: 100m
memory: 128Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect this to be too small to be useful, but I don't have an example to check at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems more reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about commenting out the resource section for Minio as well?!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I agree. I think folks can place their own limits here. Scale will matter, so choosing sane defaults is a bit difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly.

@xogoodnow
Copy link
Contributor Author

@zalegrala, If there is anything that I should do please let me know.

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

Successfully merging this pull request may close these issues.

4 participants