cluster manager: make add/update and initial host set update atomic

[mattklein123]

Previously, we would do a separate TLS set to add/update the cluster,
and then another TLS operation to populate the initial host set. For
cluster updates for a server already in operation this can lead to a
gap of no hosts.

Fixes #13070

Risk Level: High
Testing: I didn't add any new tests, because it's unclear to me what tests to add.
I could have added a unit test to cover the race condition, but it would be broken
by the change and then deleted. Note that getting all of the existing tests to pass
was quite difficult, so I do feel there is good coverage here, but if anyone has good
ideas on how to better test this please let me know.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[mattklein123]

@snowp @lambdai could you take a first pass on this? This was much harder to fix than I thought. I tried to do the minimal change possible and left some TODOs for follow ups.

cc @rgs1 it would be great if you would be willing to smoke test this once we get past the initial round of reviews.

[rgs1]

happy to smoke test this when more eyes have made a pass.

[lambdai]

So the major change is never createOrUpdateCluster, but go with host update.

At high level I feel it's fine. The question is whether non-add-update should deserve a individual post.
My concern is that cluster update is not always along with member change and Envoy should propagate this cluster update ASAP instead of waiting for the next member change.

[mattklein123]

My concern is that cluster update is not always along with member change and Envoy should propagate this cluster update ASAP instead of waiting for the next member change.

Essentially, this is the fix. Now, we could track whether the cluster is new or updated, and only do this in the updating case, but I'm not sure the extra logic is worth it. In the bootstrap case, we do what you describe above, but as I mentioned in the TODOs I would actually like to remove it from the few paths that require it (e.g. route validation).

[snowp]

At a high level this makes perfect sense. Is there any way we could try racing requests and cluster updates and check for intermittent failures?

[mattklein123]

Is there any way we could try racing requests and cluster updates and check for intermittent failures?

You mean like in an integration test? This seems kind of fragile from a test perspective.

[snowp]

You mean like in an integration test? This seems kind of fragile from a test perspective.

Yeah that was my thinking, just to have something that could hit this in case of a regression. But yeah introducing a test that would likely just end up being flaky if something broke might be too painful to debug etc if something actually breaks, so it might not be a great idea.

[mattklein123]

@lambdai @snowp updated per comments.

@rgs1 I think this should be ready for a smoke test. Thank you!

[snowp]

Thanks, this LGTM. Definitely a high risk change that's hard to test, so would want to see some smoke testing results if possible before merging.

[mattklein123]

I'm going to work on the follow up clean up to this PR. @rgs1 depending on when you have time to smoke test we might want to just get them both in together.

[rgs1]

I'm going to work on the follow up clean up to this PR. @rgs1 depending on when you have time to smoke test we might want to just get them both in together.

Won't happen before Thu, I am afraid. I've been looking at the new TCP conn pool with Alyssa. So if you can have both by then, I'll test them together.

[rgs1]

I'm going to work on the follow up clean up to this PR. @rgs1 depending on when you have time to smoke test we might want to just get them both in together.

Won't happen before Thu, I am afraid. I've been looking at the new TCP conn pool with Alyssa. So if you can have both by then, I'll test them together.

Ok, giving this a try now.

[rgs1]

@mattklein123 so it's not breaking anything, but I don't have a repro handy.

I guess it would be something like:

a) push cluster update (e.g.: change HC settings)
b) see if membership count drops and/or 5xxs happen

?

[mattklein123]

@rgs1 the repro would be to do CDS updates under high traffic load. However, I think I'm OK with merging assuming nothing broke. :) So let's go from there. Thank you!