config: finish warming only after named response

[ramaraochavali]

Description: This PR changes the way cluster warming works and finishes warming only after a named response i.e a ClusterLoadAssginment response for the cluster being warmed. This happens consistently both in init/updated warming cases.
Risk Level: Medium
Testing: Added automated tests to test the new behaviour.
Docs Changes: N/A
Release Notes: Updated
Fixes #5168

[ramaraochavali]

Document where this approach was discussed https://docs.google.com/document/d/1Ca4YX9XNnV9rjTR7U0_xyCxUDKfoSun8pWSUNO9N-tY/edit?ts=5c79affc

[ramaraochavali]

@mattklein123 @htuch @snowp implemented as discussed in the doc. PTAL.

Regarding doc updates, I think the cluster warming section here https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/cluster_manager.html?highlight=cluster%20warming#cluster-warming correctly represents the expected behaviour.

I will update xDS protocol doc (including guidance on renaming) in a follow-up PR.

[ramaraochavali]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6151 (comment) was created by @ramaraochavali.

see: more, trace.

[htuch]

Looks good code wise. A few things to consider:

1. Should we config guard? It does change behavior.
2. Can you add references to Empty EDS prevents LDS and RDS requests #4485 and the rollback to your commit message, so we have a commit trail.
3. We need to get @rfaulk looped in on this, to make sure his concerns are addressed. He is a Googler, so I will reach out to him internally and bring this to his attention.

[ramaraochavali]

Should we config guard? It does change behavior.

My personal opinion - I think we should not because this is the desired behavior we want and

• The warming documentation linked above states clearly the expected behavior of cluster warming
• This fix does not invalidate/contradict with any of the existing docs though it is a change in behavior. We have not documented any where that warming would be complete with a ClusterLoadAssignment response that does not reference a cluster.

But open to it if others feel it is best to add the config.

Can you add references to #4485 and the rollback to your commit message, so we have a commit trail.

Sorry this is not clear to me. Do you want me to push a empty commit with references to this?

We need to get @rfaulk looped in on this, to make sure his concerns are addressed. He is a Googler, so I will reach out to him internally and bring this to his attention.

Thank you. That makes sense.

[ramaraochavali]

nm on 2, added references.

[htuch]

I chatted with @rfaulk and we're good to go with this PR, we'll make the required changes internally to deal with this. @costinm FYI, this may have Pilot side implications, unclear.

I don't feel that strongly about config guarding, I'll let @alyssawilk who is our new config guard police provide thoughts.

[ramaraochavali]

@htuch Thank you.

[htuch]

Approved, contingent on approval from another @envoyproxy/senior-maintainers, since this is technically a behavioral change and might warrant guarding.

[mattklein123]

I think I'm OK without a config guard here. IMO the previous behavior was super confusing and could be considered a bug.

Thanks @ramaraochavali for working through this!