Fix broadcast of large messages

[marianafranco]

What this PR does:
Reverting grafana/dskit#85 as pods are failing to unmarshal large messages causing failed to unmarshal received KV Pair errors. This can be verified by increasing the number of cortex instances on the TestSingleBinaryWithMemberlistScaling integration test.

I tried to update cortex to include grafana/memberlist#1 but the problem persisted.

This also seems to be related to pods getting OOM killed: #4668

Which issue(s) this PR fixes:
Fixes #4668 (still need to be confirmed).

Checklist

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

[marianafranco]

@pracucci Could you take a look on this one since you worked on the change that is going to be reverted? There is probably some work that need to be done on the KeyValuePair.Unmarshal() method before this can be enabled again.

[alvinlin123]

According to https://github.com/grafana/dskit/blob/01ce9286d7d52c7ddcfd0a63ef42d240ea4454d2/go.mod#L43

I think we also need to add similar replace directive to Cortex's go.mod, so Cortex would have this change: hashicorp/memberlist@8d2a27a

I'm trying to apply hashicorp/memberlist@8d2a27a in my local repo and run TestSingleBinaryWithMemberlistScaling integration test to see if I see the failed to unmarshal received KV Pair errors.

[alvinlin123]

After applying hashicorp/memberlist@8d2a27a and running TestSingleBinaryWithMemberlistScaling integ test, I am still seeing failed to unmarshal received KV Pair

We shall discuss path forward.

[pracucci]

I agree the current code in Cortex is not working. There are two ways you can handle it:

1. Revert the change (as done in this PR)
2. Replace memberlist implementation with github.com/grafana/memberlist

After applying hashicorp/memberlist@8d2a27a and running TestSingleBinaryWithMemberlistScaling integ test, I am still seeing failed to unmarshal received KV Pair

That change is not enough. In github.com/grafana/memberlist we did more changes to properly fix it. You can diff github.com/grafana/memberlist with github.com/hashicorp/memberlist to see the actual differences.

[marianafranco]

@pracucci I will try to replace the memberlist implementation with github.com/grafana/memberlist, but I was thinking... if these large messages are causing pods that don't have this memberlist change to crash, should we also add this behind some feature flag so it can be enabled in two steps? Otherwise, this will probably cause problems during the cortex upgrade.

Btw, we have not see any OOM since we deployed this change on our load test environment, but we will keep it running for a couple of days more before we can confirm that this fixes #4668

[pracucci]

should we also add this behind some feature flag so it can be enabled in two steps?

That's a valid concern. I think we should add it behind feature flag to allow for a smoother migration.

Btw, we have not see any OOM since we deployed this change on our load test environment, but we will keep it running for a couple of days more before we can confirm that this fixes #4668

We've experienced the same in the past when testing memberlist. It's caused by decoding of corrupted packets. It makes sense that this PR would fix it (or switching memberlist to github.com/grafana/memberlist).

[alvinlin123]

Thanks @pracucci After trying with replace github.com/hashicorp/memberlist v0.2.4 => github.com/grafana/memberlist v0.2.5-0.20211201083710-c7bc8e9df94b The integration test does not print the unmarshal error anymore. Whether or not OOM issue persists is another story :)

For release 1.12 I am thinking to just replace the hashcorp memberlist with grafana memberlist, because there was point in time where large message works, and we should keep it like that for release 1.12, I am less inclined to revert large message support for release 1.12. With that said, I am open to other thoughts.

[pracucci]

I'm fine to move to github.com/grafana/memberlist: our OOM issues have been solved using it.

[alvinlin123]

We should release this as part of 1.12, so can you kindly:

• Make this PR's target to be the release-1.12 branch instead of master; We will merge from release branch to master later on.
• Put this line under the 1.12.0 change log.

Thank you so much for working on this feature flag,

[alvinlin123]

Do we need to briefly mention why one might not want to enable this "feature"?

[marianafranco]

@alvinlin123 @pracucci I updated this PR to replace the memberlist implementation with github.com/grafana/memberlist and also added a new option to allow for smoother migrations.

[pracucci]

Side note: I opened a PR to upstream the fix hashicorp/memberlist#260.

[pracucci]

should we also add this behind some feature flag so it can be enabled in two steps?

That's a valid concern. I think we should add it behind feature flag to allow for a smoother migration.

I think I said something wrong.

When I replied a couple of days ago I didn't remember the exact memberlist fix (I remember I fixed, but not the details). Today I opened a PR to upstream that fix, so I went through the code changes once again. The fix is only on the sender side, so I don't think we need any CLI flag to conditionally enable it.

[marianafranco]

The fix is only on the sender side, so I don't think we need any CLI flag to conditionally enable it.

Great! I created a new PR targeting the release-1.12 branch that only replaces the memberlist implementation: #4671