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

Removed memory_ballast and making sure soft memory is set #4404

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

samiura
Copy link

@samiura samiura commented Mar 7, 2024

Description: Removed memory_ballast and making sure soft memory is set

Link to Splunk idea: <Link to Splunk idea, see https://ideas.splunk.com>

Testing:

Documentation:

Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Do you want to do removal of the ballast extension from the configs in this PR or separately?

tests/helm/helm_test.go Outdated Show resolved Hide resolved
@samiura samiura force-pushed the test-soft-limit branch 3 times, most recently from 34a0c02 to 0da9267 Compare March 8, 2024 23:30
@samiura
Copy link
Author

samiura commented Mar 8, 2024

@dmitryax Removed ballast_extension from configs.

@samiura samiura force-pushed the test-soft-limit branch 2 times, most recently from 1c833e0 to b45d867 Compare March 9, 2024 00:36
@samiura samiura marked this pull request as ready for review March 9, 2024 00:41
@samiura samiura requested review from a team as code owners March 9, 2024 00:41
@jinja2
Copy link
Contributor

jinja2 commented Mar 9, 2024

Please add a changelog entry.

We are planning to update the default configs here in a separate PR? We'll also need to address ansible, chef, etc. deployments after this is available. They seem to set env var for ballast size.

@dmitryax
Copy link
Contributor

dmitryax commented Mar 9, 2024

As @jinja2 mentioned, we need to remove the memory_balast from all the configs like https://github.com/signalfx/splunk-otel-collector/blob/main/cmd/otelcol/config/collector/agent_config.yaml. It should be done along with this change, probably the same PR. Please make sure SPLUNK_BALLAST_SIZE_MIB env var isn't referenced anymore across the repo.

@samiura
Copy link
Author

samiura commented Mar 11, 2024

@jinja2 and @dmitryax , I will remove all the memory_ballast from configs. i was not sure if that was part of the scope of this PR.

@samiura samiura closed this Mar 12, 2024
@samiura samiura deleted the test-soft-limit branch March 12, 2024 17:48
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2024
@samiura samiura restored the test-soft-limit branch March 12, 2024 17:55
@samiura samiura reopened this Mar 12, 2024
@samiura samiura force-pushed the test-soft-limit branch 2 times, most recently from 139cc13 to d10b2d9 Compare March 12, 2024 19:35
@samiura samiura closed this Mar 12, 2024
@samiura samiura reopened this Mar 12, 2024
@samiura samiura requested a review from dmitryax March 22, 2024 00:01
deployments/salt/templates/agent_config.yaml Outdated Show resolved Hide resolved
deployments/salt/templates/agent_config.yaml Outdated Show resolved Hide resolved
internal/configconverter/remove_memory_ballast_key.go Outdated Show resolved Hide resolved
internal/settings/settings.go Outdated Show resolved Hide resolved
internal/settings/settings.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@samiura samiura force-pushed the test-soft-limit branch 2 times, most recently from e272e34 to 77a535e Compare March 22, 2024 21:56
@samiura samiura requested review from dmitryax and jinja2 March 22, 2024 21:59
@samiura samiura force-pushed the test-soft-limit branch 4 times, most recently from e87a1a7 to 3ed2fe7 Compare March 25, 2024 15:53
Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Please rebase

CHANGELOG.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Contributor

@samiura are the failing integration tests related to the change?

@samiura
Copy link
Author

samiura commented Mar 25, 2024

@dmitryax No, if you look at the history, they have been failing ever since start of the last week. I have manually deployed this branch on my machine, kind cluster and observed the memory/cpu usage, I do not see any issues thus far.

@samiura
Copy link
Author

samiura commented Mar 25, 2024

@dmitryax As we speak this is the state of integration test in the main branch.

https://github.com/signalfx/splunk-otel-collector/actions/runs/8425269973/job/23071206244?pr=4413#step:7:7

@dmitryax
Copy link
Contributor

LGTM. @samiura please proceed with the follow-up cleanup as we agreed

@dmitryax dmitryax merged commit 397a374 into signalfx:main Mar 25, 2024
119 of 121 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants