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

remove the forwarding feature #4876

Merged
merged 12 commits into from
May 2, 2023
Merged

remove the forwarding feature #4876

merged 12 commits into from
May 2, 2023

Conversation

replay
Copy link
Contributor

@replay replay commented Apr 28, 2023

We don't need the distributor's forwarding feature anymore, this removes it.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay marked this pull request as ready for review April 28, 2023 21:36
@replay replay requested review from a team as code owners April 28, 2023 21:36
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this. We should mention it in the changelog too.

@dimitarvdimitrov
Copy link
Contributor

This wasn't part of the experimental or deprecated features listed in about-versioning.md. Should we wait two minor releases before removing it?

@pstibrany
Copy link
Member

This wasn't part of the experimental or deprecated features listed in about-versioning.md. Should we wait two minor releases before removing it?

I don't have a strong opinion on this, but this feature was always meant for our internal usage only, and we never documented how it works or what expectations are around it. Original forwarding feature is not even mentioned in the changelog, although improvements to it and bugfixes are.

@56quarters
Copy link
Contributor

This wasn't part of the experimental or deprecated features listed in about-versioning.md.

All the fields in the forwarding block are marked experimental. I think not adding it to the list of experimental features was an oversight and as Peter mentioned, it's esoteric and undocumented.

@dimitarvdimitrov
Copy link
Contributor

In that case mentioning the removal in the changelog might make sense. There have been some issues related to forwarding and I assume some users are relying it. Some paper trail in the changelog is better than having to dig through pull requests.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks Mauro!

Few things I've noticed:

  • The forwarding feature was mentioned few times in the CHANGELOG in the past. Should we add a CHANGELOG entry to mention this experimental feature has been removed?
  • Shouldn't we cleanup ForwardingRules, ForwardingEndpoint and ForwardingDropOlderThan in the limits config too?

replay added 2 commits May 2, 2023 11:14
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay
Copy link
Contributor Author

replay commented May 2, 2023

Thx for all the feedback.

I added a CHANGELOG entry, as requested by multiple people.
I was a bit unsure whether removing a feature should fall into the CHANGE or FEATURE category, but based on the description in the contribution guidelines it seemed to me that CHANGE is more appropriate for the removal of a feature:

[CHANGE]
The CHANGE scope denotes a change that changes the expected behavior of the project while not adding new functionality or fixing an underling issue. This commonly occurs when renaming things to make them more consistent or to accommodate updated versions of vendored dependencies.

[FEATURE]
The FEATURE scope denotes a change that adds new functionality to the project/service.

@replay
Copy link
Contributor Author

replay commented May 2, 2023

Shouldn't we cleanup ForwardingRules, ForwardingEndpoint and ForwardingDropOlderThan in the limits config too?

Thx Marco, I missed that, cleaning it up.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay
Copy link
Contributor Author

replay commented May 2, 2023

Shouldn't we cleanup ForwardingRules, ForwardingEndpoint and ForwardingDropOlderThan in the limits config too?

The type validation.ForwardingRules is used in mimirtool config convert because it targets the version 2.6 which supports this config. I don't think we should update mimirtool config convert to target the config of the current main branch, because it might change again before the next release.
Considering that the distributor forwarding feature didn't exist in Cortex I think it is fine to modify the mimirtool config convert command to not support the forwarding settings anymore, even if 2.6 should support it.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay
Copy link
Contributor Author

replay commented May 2, 2023

I think I have addressed all the feedback, PTAL

@replay
Copy link
Contributor Author

replay commented May 2, 2023

Other changes which I'd like to make are blocked by this.
Since I addressed all the feedback and I already have one approval, I'll go ahead and merge it.
If there is something else that I should change then please let me know and I'll open another PR to fix it.

@replay replay merged commit ce87556 into main May 2, 2023
@replay replay deleted the remove_forwarding_feature branch May 2, 2023 21:37
@QuentinBisson
Copy link
Contributor

Hello, I was atually thinking of using this feature to send some of the data send to one of my mimir instance to another mimir but as this is removed this is not the way to go :D

How would you configure it then as mimir does not support sending data via remote write? Use envoy mirroring as described https://grafana.com/docs/mimir/latest/configure/mirror-requests-to-a-second-cluster/ ? Or did I miss that mimir can be configured to send data to another location usign remote write?

@replay
Copy link
Contributor Author

replay commented Jun 2, 2023

How would you configure it then as mimir does not support sending data via remote write? Use envoy mirroring as described https://grafana.com/docs/mimir/latest/configure/mirror-requests-to-a-second-cluster/ ?

The documentation also mentions that the requests to the secondary cluster are "fire-and-forget", you have no guarantee that they succeeded, so if you're planning to mirror a production workload I don't think this is recommendable.

Or did I miss that mimir can be configured to send data to another location usign remote write?

Mimir has not functionality to remote write to a second location, I'd recommend configuring the remote_write client to write to two separate endpoints instead. That way you can be sure that failed requests will be retried in each of the two cluster separately.

@QuentinBisson
Copy link
Contributor

@replay thank you for the feedback. I wanted to avoid having to pass the credentials to the customer's clusters to avoid secret from being accessible by cluster users but I guess there is no way other than having 2 remote_write configurations, thank you

@replay
Copy link
Contributor Author

replay commented Jun 2, 2023

I wanted to avoid having to pass the credentials to the customer's clusters to avoid secret from being accessible by cluster users

If the problem is the credentials, you could always set up a reverse proxy which has endpoints for both of your clusters and which sets the authentication header when connecting to the Mimir clusters. The remote_write clients which connect to the reverse proxy could use different credentials to auth themselves to the reverse proxy. I don't have a recommendation which reverse proxy to use, i think that most likely Nginx should be able to do that, but probably there are many others too.

@QuentinBisson
Copy link
Contributor

That is a really good point, thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants