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

helm: add exporter resource entry to ceph cluster #12251

Merged
merged 1 commit into from
May 30, 2023

Conversation

galexrt
Copy link
Member

@galexrt galexrt commented May 18, 2023

Description of your changes:

This adds the new exporter: resource key to the Ceph cluster example yaml. It also set some sane resource requests and limits for it in the CephCluster section of the rook-ceph Helm chart.

Which issue is resolved by this Pull Request:
Closes #11914

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@galexrt
Copy link
Member Author

galexrt commented May 18, 2023

To ask @travisn question from the other PR here, @avanthakkar do you have an idea about the memory/cpu the exporter would be expected to consume?

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Besides getting an idea for the recommended limits, if we can also document this in the ceph-cluster-crd.md in the section "Cluster-wide Resources Configuration Settings".

@avanthakkar
Copy link
Member

To ask @travisn question from the other PR here, @avanthakkar do you have an idea about the memory/cpu the exporter would be expected to consume?

I don't have the exact data with me currently, but i think 1Gi is good to have, same as it's been in cephadm. And for cpu I'm not really sure, but will check, probably should be same as logcollector.

exporter:
limits:
cpu: "300m"
memory: "512Mi"
Copy link
Member

Choose a reason for hiding this comment

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

Per discussion in huddle, how much memory are you seeing used in practice? Even 512Mi seems like a high limit.

@@ -542,6 +543,7 @@ If a user configures a limit or request value that is too low, Rook will still r
* `crashcollector`: 60MB
* `mgr-sidecar`: 100MB limit, 40MB requests
* `prepareosd`: no limits (see the note)
* `exporter`: 512MB
Copy link
Member

Choose a reason for hiding this comment

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

This list is more of a recommended limit, rather than an upper limit. So based on your testing, hopefully we can lower this recommended limit maybe to 256 or even 128Mi

@galexrt galexrt force-pushed the pickup_pr_11914 branch 2 times, most recently from e25001c to 6c7a8f2 Compare May 25, 2023 10:03
@@ -540,6 +541,7 @@ If a user configures a limit or request value that is too low, Rook will still r
* `crashcollector`: 60MB
* `mgr-sidecar`: 100MB limit, 40MB requests
* `prepareosd`: no limits (see the note)
* `exporter`: 128MB
Copy link
Member

Choose a reason for hiding this comment

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

@avanthakkar Can you add your findings here for the memory/cpu requirements? In our offline conversation, sounds like it was much lower than this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@avanthakkar do you think 128MB is too much or okay for the exporter on each node?

This adds the new `exporter:` resource key to the Ceph cluster example
yaml. It also set some sane resource requests and limits for it in the
CephCluster section of the rook-ceph Helm chart.

Closes rook#11914

Signed-off-by: Alexander Trost <galexrt@googlemail.com>
@travisn travisn merged commit 3dc12c0 into rook:master May 30, 2023
44 checks passed
travisn added a commit that referenced this pull request May 30, 2023
helm: add exporter resource entry to ceph cluster (backport #12251)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants