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.

Sign up for GitHub

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

Jump to bottom

Fix memory limit calculation for deployments with 20Gi+ of total memory #558

Merged
merged 1 commit into from
Jul 22, 2021
Merged

Fix memory limit calculation for deployments with 20Gi+ of total memory #558

merged 1 commit into from
Jul 22, 2021

Conversation

dmitryax
Copy link
Contributor

@dmitryax dmitryax commented Jul 16, 2021 •
edited
Loading

Bug: If SPLUNK_MEMORY_TOTAL_MIB set to more than 20480, the calculated memory limit drops from 90% to 2048 and collector is failing to start with

main.go:283: Memory limit (2048) is less than 2x ballast (6761). Increase memory limit or decrease ballast size.

This change fixes the bug and removes the unnecessary complication for default memory limit calculation. If user wants to adjust memory allocation for a 20Gb+ deployment they can always use SPLUNK_MEMORY_LIMIT_MIB env var.

@dmitryax dmitryax requested review from a team as code owners July 16, 2021 05:50
@dmitryax dmitryax changed the title Fix memory limit calculation for memory 20Gi+ deployment Fix memory limit calculation for deployments with 20Gi+ of total memory Jul 16, 2021
@dmitryax
Fix memory limit calculation for memory 20Gi+ deployment
cc0c763 
Bug: If `SPLUNK_MEMORY_TOTAL_MIB` set to more than 20480, the calculated memory limit drops from 90% to 2048 and collector is failing to start with 
```
main.go:283: Memory limit (2048) is less than 2x ballast (6761). Increase memory limit or decrease ballast size.
```

This change fixes the bug and removes the unnecessary complication for default memory limit calculation. If user wants to adjust memory allocation for a 20Gb+ deployment they can always use SPLUNK_MEMORY_LIMIT_MIB env var.
@dmitryax dmitryax force-pushed the fix-max-memory-calculation branch from c1ac50c to cc0c763 Compare July 16, 2021 16:47
@flands
Copy link
Contributor

flands commented Jul 18, 2021

@dmitryax I cannot reproduce this -- can you please provide the exact command and version to see the described behavior?

SPLUNK_MEMORY_TOTAL_MIB=20480 SPLUNK_ACCESS_TOKEN=12345 SPLUNK_API_URL=https://api.us0.signalfx.com SPLUNK_INGEST_URL=https://ingest.us0.signalfx.com SPLUNK_TRACE_URL=https://ingest.us0.signalfx.com/v2/trace SPLUNK_HEC_URL=https://ingest.us0.signalfx.com/v1/log SPLUNK_HEC_TOKEN=12345 SPLUNK_DEBUG_CONFIG_SERVER_PORT=55556 ./otelcol_darwin_amd64 --config ../cmd/otelcol/config/collector/agent_config.yaml

Using the above -- which I would assume would repro given the description -- I am able to start the collector and navigating to http://localhost:55556/debug/configz/effective shows a properly configured processor. I agree the logic is complicated but this comes directly from the memory processor documentation.

@dmitryax
Copy link
Contributor Author

@flands 20480 may be right on the edge. Please try SPLUNK_MEMORY_TOTAL_MIB=20490

flands
flands reviewed Jul 20, 2021
View reviewed changes
Copy link
Contributor

@flands flands left a comment

Choose a reason for hiding this comment

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

OK, confirmed. So keeping logic can just update line 297 otherwise we can remove logic. Only concern is that as memory increases more and more memory will be "wasted". I am good either way -- note other YAML config will need comments updated.

cmd/otelcol/main.go
// The memory limit should be set to defaultMemoryLimitPercentage of total memory
// while reserving a maximum of defaultMemoryLimitMaxMiB of memory.
if (memTotalSizeMiB - memLimit) > defaultMemoryLimitMaxMiB {
memLimit = defaultMemoryLimitMaxMiB
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping logic you can just change this to:

memLimit = (memTotalSizeMiB - defaultMemoryLimitMaxMiB)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we discussed that option with @pjanotti and agreed on removing the logic.

@dmitryax
Copy link
Contributor Author

dmitryax commented Jul 20, 2021 •
edited
Loading

I am good either way -- note other YAML config will need comments updated.

@flands I think I already updated all the configurations. Please let me know if you see any of them missed

@dmitryax dmitryax merged commit d24ed00 into main Jul 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-max-memory-calculation branch July 22, 2021 20:49
@dmitryax dmitryax mentioned this pull request Aug 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flands flands flands left review comments

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@pjanotti pjanotti pjanotti approved these changes

@rmfitzpatrick rmfitzpatrick rmfitzpatrick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmitryax @flands @bogdandrutu @pjanotti @rmfitzpatrick