Add client_max_body_size to helm / nginxconf

[itspooya]

What this PR does

This pr adds ability to set client_max_body_size, when using nginx and mimirtool to migrate the prometheus data to mimir, sometimes sizes are more than default client_max_body_size and you need to set its value high

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

• [ ] Tests updated.
• [ ] Documentation added.
• [ ] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• [ ] about-versioning.md updated with experimental features.

[CLAassistant]

All committers have signed the CLA.

[dimitarvdimitrov]

is it not possible to configure this via gateway.nginx.config.serverSnippet or gateway.nginx.config.httpSnippet?

[itspooya]

This ultimately comes down to personal preference. Similar to other values, this one can be passed via httpsnippet/serversnippet or as a dedicated entry in values.yaml. While I think a dedicated value makes sense, I've tested and confirmed that using httpsnippet/serversnippet also works. If you're okay with the latter approach, feel free to close this MR. I understand that this decision is more about coding style and conventions rather than a purely technical consideration.

[dimitarvdimitrov]

I'm leaning towards not increasing the configuration surface of the chart - i.e. use httpsnippet/serversnippet instead of a dedicated field.

Having said that it might make sense to just change the default of client_max_body_size for the helm chart so others don't hit the same problem you did. Can you share some more details to see if it will help others as well?

[itspooya]

Hi, the issue I encountered was that the default client_max_body_size is too low, which causes errors when using mimirtool to upload metrics from Prometheus to Mimir. This is because the block size can exceed the default limit, especially when dealing with a large number of metrics.

I understand the concern about increasing the configuration surface of the chart. However, I think modifying the default client_max_body_size value might not be the best solution, as the ideal value can vary greatly depending on the specific infrastructure and stack.

That's why I proposed adding a configurable option for client_max_body_size. This would allow users to easily adjust the value according to their needs.

If we decide not to merge this PR, I think it would be helpful to mention this potential issue in the migration documentation, so that users are aware of the limitation and can take necessary steps to avoid it.

[ying-jeanne]

Hi, thanks for the PR, the use case seems valid, it probably worths to update the migration doc in the same PR for the problem/solution so that people can benefits from this in the future. also asked @dimitarvdimitrov for another look.

[itspooya]

Hi, I wanted to ask whether I should do anything else or not? @dimitarvdimitrov @ying-jeanne @56quarters

[dimitarvdimitrov]

I'm still not convinced we need this. I don't think it's considerably easier than modifying the serverSnippets

[mrclrchtr]

I would definitely need this too. I have just created the following issue: https://github.com/grafana/mimir/issues/8293

It took me some time to solve the problem. A note in the migration docs would have been fantastic.

[dimitarvdimitrov]

after some discussion with @mrclrchtr on https://github.com/grafana/mimir/issues/8293 I agree that this PR is better merged. The only things necessary before merging are:

1. switching the config from the deprecated nginx top-level block to the gateway nginx block here. This will supersede the nginx block in the future. (Update docs accordingly)
2. Add a changelog entry in the helm changelog

@itspooya if you still have the time, feel free to reopen and continue the work. Otherwise, other folks can open a new PR.

[itspooya]

Hi, I would update the MR as you mentioned today @dimitarvdimitrov

[itspooya]

I think it is ready now @dimitarvdimitrov may you please check it?

[mrclrchtr]

Thank you too!