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

update automatically settting gomemlimit #1188

Merged
merged 14 commits into from
Oct 30, 2024
Merged

Conversation

mattdurham
Copy link
Collaborator

PR Description

Graduate automatically setting gomemlimit to GA. It has been in public preview with no problems for two releases. Manually setting the value is something we tell users to do often so this mimics that if they have a limit set, ie most commonly through kubernetes.

  • CHANGELOG.md updated

@mattdurham mattdurham marked this pull request as ready for review July 3, 2024 13:20
@mattdurham mattdurham marked this pull request as draft July 3, 2024 13:28
@mattdurham
Copy link
Collaborator Author

Adding some documentation somewhere on this.

@@ -164,6 +164,12 @@ original configuration.
Include `--config.extra-args` to pass additional command line flags from the original format to the converter.
Refer to [alloy convert][] for more details on how `extra-args` work.

## Automatic setting of GOMEMLIMIT
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@clayton-cornell I have no idea where to put this doc. I could put it in the environment variable section we have going since it relates closely to that. Maybe around the kubernetes sections since that is where it will be used 90% of the time.

Copy link
Contributor

@clayton-cornell clayton-cornell Jul 3, 2024

Choose a reason for hiding this comment

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

I would lean towards adding it to the Env Var topic and linking to the env var topic from wherever in the Kubernetes topics.

This would also push things to get the first draft of the env var topic published (and we can expand it as needed vs trying to capture additional vars).

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the Env Var topic is live, should the GOMEMLIMIT info be shifted over?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I havent forgotten this, on my todo list after I get the patch release out.

@@ -164,6 +164,12 @@ original configuration.
Include `--config.extra-args` to pass additional command line flags from the original format to the converter.
Refer to [alloy convert][] for more details on how `extra-args` work.

## Automatic setting of GOMEMLIMIT

The `GOMEMLIMIT` variable will be automatically set if {{< param "PRODUCT_NAME" >}} can determine the appropriate value. If the `GOMEMLIMIT` value is set before running {{< param "PRODUCT_NAME" >}} then that setting will be honored. The value will be set to 90% of the cgroup value set.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it set to when the appropriate value cannot be determined? if there is no cgroup? I imagine it's then not set, but just maybe good idea to doc that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is correct, I can explicitly call that out.

@@ -197,9 +197,7 @@ func (fr *alloyRun) Run(configPath string) error {

// Set the memory limit, this will honor GOMEMLIMIT if set
// If there is a cgroup will follow that
if fr.minStability.Permits(featuregate.StabilityPublicPreview) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to remove the Permits function? 😂
It could be useful though, just that I don't think it's used now so YAGNI.

@@ -35,4 +36,12 @@ Argument | Description | D
---------------------------|-------------------------------------------------|--------
`http://proxy.example.com` | The hostname or IP address of the proxy server. |

### GOMEMLIMIT

Normally the [Go runtime][runtime] will release memory back to the Operating System when requested. In certain environments this may cause issues such as Out of Memory errors. The `GOMEMLIMIT` variable is a numeric value in bytes with an optional unit suffix. The supported suffixes include B, KiB, MiB, GiB, and TiB. This should not be treated has a hard limit. The process can use more memory if that memory is required. A rough number is to set `GOMEMLIMIT` to 90% of the maximum amount of memory usage. For example if you wanted to keep memory usage below `10GiB` then use `GOMEMLIMIT=8GiB`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@clayton-cornell I added some doc here but its rough, would love additional eyes.

@mattdurham mattdurham marked this pull request as ready for review July 15, 2024 19:15
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

First pass at adjusting the text used to describe this env var and how its used.

docs/sources/reference/cli/environment-variables.md Outdated Show resolved Hide resolved
docs/sources/reference/cli/environment-variables.md Outdated Show resolved Hide resolved
Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@clayton-cornell
Copy link
Contributor

@mattdurham this PR is getting stale. Do we want to move this info into the Env Vars topic? Or something else?

@mattdurham
Copy link
Collaborator Author

Oh jeez, we should. I will get this updated and ready tomorrow.

@mattdurham mattdurham requested a review from a team as a code owner October 17, 2024 18:41
@mattdurham
Copy link
Collaborator Author

Updated from main. Fixed a logical typo I had in the doc.

@clayton-cornell
Copy link
Contributor

@mattdurham Is this one ready to merge?

@mattdurham mattdurham merged commit 282005f into main Oct 30, 2024
17 of 18 checks passed
@mattdurham mattdurham deleted the change_gomemlimit_stability branch October 30, 2024 12:32
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.

4 participants