Fix default memberlist configuration value for RetransmitMult.

[stevesg]

What this PR does:
If configuration is not explicitly given for RetransmitMult (via
-memberlist.retransmit_factor), then it is intended to be picked up
from DefaultLANConfig. However, though the correct value was being
used to configure memberlist itself, zero would be passed into the
TransmitLimitedQueue used for broadcasting ring updates. This
essentially means that ring updates are only ever gossiped once.

Which issue(s) this PR fixes:
Fixes #4010 (unconfirmed)

Checklist

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

[pracucci]

Good job! This PR definitely needs a CHANGELOG entry 😉

[stevesg]

Looking into unit test failure...

[pstibrany]

Nice find!

[pracucci]

Good job! I left a couple of nits in the integration test. Failing unit tests looks flaky tests unrelated from this PR changes.

[bboreham]

I was confused that we document the default -memberlist.retransmit_factor as zero, when you say your expectation was that it will be 4. It seems that KV.buildMemberlistConfig() does the same thing with a range of arguments.

Could we have KVConfig.RegisterFlags() call memberlist.DefaultLANConfig() and obtain the defaults that way?

(this could be a separate issue/PR)

[stevesg]

That's a good idea - makes the defaults more obvious and may have avoided this bug. I'll try it as a separate PR.

Edit: Thinking about this, it might be a breaking change, setting a configuration explicitly to "0" will have different behavior. Will think on it.

[pstibrany]

I was confused that we document the default -memberlist.retransmit_factor as zero, when you say your expectation was that it will be 4. It seems that KV.buildMemberlistConfig() does the same thing with a range of arguments.

Could we have KVConfig.RegisterFlags() call memberlist.DefaultLANConfig() and obtain the defaults that way?

This can introduce unintended change of default values over time. (Our current approach can as well, but it’s more explicit about what’s going on). That said, I think it would be helpful.

[stevesg]

Hold off merging this - the unit test failure appears to be a race condition in the unit test: the test will signal to shut down the memberlist service, then it will try and grab the state from the clients. Sometimes the memberlist service will shutdown before the test was able to grab the state.

[pstibrany]

Hold off merging this

You can mark PR as draft, that prevents merging.

[stevesg]

Fixed now, didn't seem worth pulling out into a separate PR.