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

Support setting ballast size in percentage of total Mem in ballast extension #3456

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

mxiamxia
Copy link
Member

Descritpion:
Enhance the current ballast extension which is to set memory ballast for the collector process. And we'll remove --mem-ballast-size-mib support from command line later.

The changes,
Added new configuration size_in_percentage which can dynamically set ballast size based on the total memory of the running environments. (hosts, Docker, K8s, etc)

Sample Config,

extensions:
  memory_ballast:
    size_in_percentage: 20

How ballast size is calculated by percentage setup
When size_in_percentage is enabled with the value(1-100), the ballast_size will be calculated by
size_in_percentage * totalMemory. The totalMemory will be calculated for hosts and containers(in docker, k8s, etc) by the following steps,

  1. Look up Memory Cgroup subsystem where the collector is running on(it could be container, VM or bare metal host), check if there is total memory limitation has been set for the collector process.
    The total assigned memory for the process is in memory.limit_in_bytes file under cgroup (eg, /sys/fs/cgroup/memory/memory.limit_in_bytes).

  2. If memory.limit_in_bytes has a positive value other than 9223372036854771712. The ballest_size
    will be calculated by memory.limit_in_bytes * size_in_percentage. Else If memory.limit_in_bytes value is 9223372036854771712(0x7FFFFFFFFFFFF000), it indicates there is no memory limit has been set for the collector process in cgroup. Then the totalMemoery will be determined in the next step

  3. if there is no memory limit set in cgroup for the collector process or container where the collector is running. The total memory will be calculated by github.com/shirou/gopsutil/mem which is supported in multiple OS systems.

Link to tracking Issue:
#2516

Test:
Tested locally on EC2, Docker and K8S for ballast_size_in_percentage configuration

@mxiamxia mxiamxia requested review from a team and owais June 16, 2021 21:18
@alolita
Copy link
Member

alolita commented Jun 17, 2021

@mxiamxia can you fix these failing tests. Thx.

@punya
Copy link
Member

punya commented Jun 18, 2021

@Aneurysm9 @anuraaga could you please take a look?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Cool feature

extension/ballastextension/memory_ballast.go Show resolved Hide resolved
internal/iruntime/total_memory_linux.go Outdated Show resolved Hide resolved
extension/ballastextension/memory_ballast.go Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/memory_ballast.go Show resolved Hide resolved
@alolita
Copy link
Member

alolita commented Jun 23, 2021

@anuraaga @Aneurysm9 can you please re-review? The tests are now passing.

extension/ballastextension/README.md Outdated Show resolved Hide resolved
internal/iruntime/total_memory_other.go Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/README.md Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
extension/ballastextension/README.md Outdated Show resolved Hide resolved
@mxiamxia
Copy link
Member Author

Hi @anuraaga @bogdandrutu @tigrannajaryan @rakyll , PTAL! Thanks.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

@tigrannajaryan @rakyll PTAL, will merge tomorrow otherwise

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, except I have just found we also have a built-in ballast implementation in

func (col *Collector) createMemoryBallast() ([]byte, uint64) {

I am confused. Why do we have 2 implementations? Are they related or competing?

@mxiamxia
Copy link
Member Author

mxiamxia commented Jun 28, 2021

opentelemetry-collector/service/collector.go

This old one is taking the ballast size input from the command line. I will send another PR to deprecate the old ballast memory creation implementation.

@alolita alolita added the ready-to-merge Code review completed; ready to merge by maintainers label Jun 29, 2021
@alolita
Copy link
Member

alolita commented Jun 29, 2021

@bogdandrutu this PR is ready to be merged. :-)

@bogdandrutu bogdandrutu merged commit cbd822e into open-telemetry:main Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:service ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants