support override host status restriction for stateful session by wbpcode · Pull Request #19665 · envoyproxy/envoy

wbpcode · 2022-01-24T13:36:18Z

Commit Message: support override host status restriction for stateful session
Additional Description:

Stateful session will try to parse upstream address from downstream request directly and override the result of load balancing algorithm by the LoadBalancerContext::overrideHostToSelect API.

To avoid the load balancer selecting hosts that in unexpected statuses, specifying some expected statuses are necessary.

In the previous design, we will provide expected statuses of override host by the LoadBalancerContext::overrideHostToSelect API.

And in the PR #18207, after some discussion with @htuch, we found may be cluster-level config may be more reasonable design and implementation.
Ref some more details: #18207 (comment)

So this PR try to close previous discussion in the #18207:

Refactoring LoadBalancerContext::overrideHostToSelect API to remove expected statuses for the return value.
Add new common lb config override_host_status and related implementation.

Risk Level: Mid.
Testing: N/A.
Docs Changes: N/A.
Release Notes: N/A.
Platform Specific Features: N/A.

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only · 2022-01-24T13:36:26Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19665 was opened by wbpcode.

see: more, trace.

wbpcode · 2022-01-24T13:38:17Z

@htuch Could you please give a review to the new proto API when you have free time? 😄

wbpcode · 2022-01-24T13:40:24Z

And @snowp Could you please add some comments for these new code when you have some bandwidth? 🌷

wbpcode · 2022-01-24T13:43:33Z

cc @howardjohn

Signed-off-by: wbpcode <wbphub@live.com>

adisuissa

Thanks for working on this!
Looked mostly at the API, left a few comments.

adisuissa · 2022-01-26T14:03:23Z

api/envoy/config/core/v3/health_check.proto

  DEGRADED = 5;
 }

+message HealthStatusSet {


Do you expect this message to have more fields?
If not, what's the advantage over just specifying the repeated field directly (in your specific case renaming the field to override_host_status_set)?

This is because we need to distinguish between the case where no override_host_status is configured at all and the case where 0 items are configured. The former will provide a default value.

adisuissa · 2022-01-26T14:15:38Z

api/envoy/config/cluster/v3/cluster.proto

+    // Expected override host statuses. The result of load balancing is allowed to be overwritten
+    // only when the target host status is as expected when stateful session stickiness directly
+    // specifies a target host.
+    //
+    // If this is set and and has no any item then any host from stateful session stickiness will be
+    // ignored.
+    // If this is not set then healthy statuses (``UNKNOWN``, ``HEALTHY`` and ``DEGRADED`` for now)
+    // will be applied by default.


Is this field only relevant when the stateful_session extension is used?
If so, can you add this to the comment, and also add a reference to it?

Should we be referring to the specific extension here at all? Or should we instead have this talk about the host override feature in general? I can imagine other extensions wanting to use host overrides as well

For the moment, it does. I will add some more comments. Thanks.

One thing that I do not fully grok is what is expected if a config has this field set, but doesn't have the sticky_session extension configured.

Should we be referring to the specific extension here at all? Or should we instead have this talk about the host override feature in general? I can imagine other extensions wanting to use host overrides as well

~~@htuch~~ @snowp This configuration is designed for general host override. But LoadBalancerContext::overrideHostToSelect() is internaly C++ API which is annomymous for public users and the stateful session is the only related implemenatation for now. Although we will have more extensions in the future, now we still need a specific example to help users to understand this config.

The result of load balancing is allowed to be overwritten by the L4/L7 filters specified host. For example the `stateful session filter <........>` will specfic upstream host directly accoriding to the downstream request. But the overridden will only worked when the status of specficed host is as expected in the configuration.

how about some comments like this? cc @htuch cc @adisuissa

I'm a bit lost reading this. I think there is context assumed of the reader that isn't likely to be there in most cases. Can you explain this section as if the reader had never heard of this override feature, pointing them to the relevant docs with RST links and/or some examples?

The override host feature is currently only available through the L4/L7 extensions. Now stateful session is the only extension that is ready.

So my intention is to use the stateful session as an example to help the reader understand the functionality and the configuration.

I will think about how to make it clearer and easier to understand. 🤔

adisuissa · 2022-01-26T14:26:23Z

source/common/upstream/load_balancer_impl.cc

-    return status & HealthyStatus;
+uint32_t LoadBalancerContextBase::createOverrideHostStatus(
+    const envoy::config::cluster::v3::Cluster::CommonLbConfig& common_config) {
+  if (!common_config.has_override_host_status()) {


What happens if there is but it is empty?

Then override_host_status will be zero and all the override host from LoadBalancerContext::overrideHostToSelect() will be ignored and stateful session stickiness will be disabled for current cluster.

adisuissa · 2022-01-26T14:26:55Z

api/envoy/config/core/v3/health_check.proto


+message HealthStatusSet {
+  // An order-independent set of health status.
+  repeated HealthStatus statuses = 1;


Could this be an empty set? If not, then add a PGV constraint.

Yep, it can be an empty set.

Then override_host_status will be zero and all the override host from LoadBalancerContext::overrideHostToSelect() will be ignored and stateful session stickiness will be disabled for current cluster.

That sounds like the case when the override_host_status field is set which is technically different from being set with a zero list. Is there a distinction between override_host_status not being set and it being set with an empty list of statuses?

@snowp Yep. If override_host_status is set with an empty list of statuses then the override host will be ignored. And if override_host_status is not set completely, we will give some default statuses.

Signed-off-by: wbpcode <wbphub@live.com>

…ide_host_status_restriction

Signed-off-by: wbpcode <wbphub@live.com>

adisuissa

Minor comment about the api, but otherwise LGTM.

api/envoy/config/cluster/v3/cluster.proto

adisuissa · 2022-02-03T02:28:38Z

/wait

Signed-off-by: wbpcode <wbphub@live.com>

…ide_host_status_restriction

wbpcode · 2022-02-08T02:36:59Z

@adisuissa @htuch docs/API is updated. Thanks for all your patient reivews 😄. And sorry for the delayed replies and updates.

adisuissa

/lgtm api

wbpcode · 2022-02-08T07:20:53Z

hi, @snowp , the API is ready and could you give a review to the implementation when you have free time? Thanks. 🌷

htuch

/lgtm api

…ide_host_status_restriction

wbpcode · 2022-02-10T10:08:23Z

/retest

repokitteh-read-only · 2022-02-10T10:08:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19665 (comment) was created by @wbpcode.

see: more, trace.

KBaichoo · 2022-02-16T15:06:54Z

ping @snowp PTAL :)

htuch · 2022-02-20T19:30:49Z

I can provide senior maintainer coverage once @adisuissa has reviewed.

…ide_host_status_restriction

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-02-21T06:56:27Z

/retest

repokitteh-read-only · 2022-02-21T06:56:31Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19665 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2022-02-21T13:32:04Z

Hi, @adisuissa the review comments from @snowp are addressed. PTAL, thanks. 🌷

adisuissa

Thanks! Overall LGTM.
Left one minor comment.
/wait

adisuissa · 2022-02-22T17:49:10Z

source/common/upstream/load_balancer_impl.cc

+    default:
+      break;


I think this may hide a case where the server sends a config with some other health status.
Consider adding [(validate.rules).enum.defined_only = true] to the statuses, and maybe a emit a log here.

Signed-off-by: wbpcode <wbphub@live.com>

rojkov · 2022-02-28T14:03:29Z

@adisuissa gentle ping

adisuissa

/lgtm api
LGTM code changes.
@htuch for final review.
Also, not sure if this needs release notes.

htuch

Looks good, but a few suggestions.

source/common/upstream/load_balancer_impl.cc

Signed-off-by: wbpcode <wbphub@live.com>

htuch

LGTM modulo one last nit on type alias.

htuch · 2022-03-09T02:40:05Z

source/common/upstream/load_balancer_impl.cc

-    return status & DegradedStatus;
-  case Host::Health::Healthy:
-    return status & HealthyStatus;
+std::bitset<32> LoadBalancerContextBase::createOverrideHostStatus(


Nit: can you add a using type alias for this, e.g. using HostStatusSet = std::bitset<32>? search-replace this across the diff.

htuch · 2022-03-09T02:43:26Z

source/common/upstream/load_balancer_impl.h

+  //
+  // If multiple bit fields are set, it is acceptable as long as the status of override host is in
+  // any of these statuses.
+  const std::bitset<32> override_host_status_{};


FWIW it doesn't need to be 32-bit long, but it probably doesn't matter that much either.

Yes. Only 3 bits are used for now. But in the x64 matchine, the storage size of std::bitset<N> (N > 0) is at least 8 bytes. So I think setting N to 32 or 8 makes little difference.

std::cout << sizeof(std::bitset<8>) << std::endl; // 8 std::cout << sizeof(std::bitset<16>) << std::endl; // 8 std::cout << sizeof(std::bitset<32>) << std::endl; // 8 std::cout << sizeof(std::bitset<64>) << std::endl; // 8 std::cout << sizeof(std::bitset<128>) << std::endl; // 16

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-03-09T12:20:01Z

/retest

repokitteh-read-only · 2022-03-09T12:20:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19665 (comment) was created by @wbpcode.

see: more, trace.

htuch

LGTM, thanks!

@htuch

…roxy#19665) Stateful session will try to parse upstream address from downstream request directly and override the result of load balancing algorithm by the LoadBalancerContext::overrideHostToSelect API. To avoid the load balancer selecting hosts that in unexpected statuses, specifying some expected statuses are necessary. In the previous design, we will provide expected statuses of override host by the LoadBalancerContext::overrideHostToSelect API. And in the PR envoyproxy#18207, after some discussion with @htuch, we found may be cluster-level config may be more reasonable design and implementation. Ref some more details: envoyproxy#18207 (comment) So this PR try to close previous discussion in the envoyproxy#18207: Refactoring LoadBalancerContext::overrideHostToSelect API to remove expected statuses for the return value. Add new common lb config override_host_status and related implementation. Risk Level: Mid. Testing: N/A. Docs Changes: N/A. Release Notes: N/A. Platform Specific Features: N/A. @wbpcode Signed-off-by: wbpcode <wbphub@live.com> Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

support override host status restriction for stateful session

7285146

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot added the api label Jan 24, 2022

repokitteh-read-only bot assigned adisuissa Jan 24, 2022

fix test

67fe321

Signed-off-by: wbpcode <wbphub@live.com>

adisuissa reviewed Jan 26, 2022

View reviewed changes

wbpcode added 4 commits January 31, 2022 14:19

update docs

92cb9e3

Signed-off-by: wbpcode <wbphub@live.com>

add version history

8480754

Signed-off-by: wbpcode <wbphub@live.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into overr…

8253bc5

…ide_host_status_restriction

fix conflict

cd6b20d

Signed-off-by: wbpcode <wbphub@live.com>

adisuissa reviewed Feb 2, 2022

View reviewed changes

api/envoy/config/cluster/v3/cluster.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 3, 2022

wbpcode added 2 commits February 8, 2022 02:33

minor doc update

3ae80f8

Signed-off-by: wbpcode <wbphub@live.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into overr…

bd713a3

…ide_host_status_restriction

repokitteh-read-only bot removed the waiting label Feb 8, 2022

adisuissa reviewed Feb 8, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Feb 8, 2022

wbpcode requested a review from snowp February 8, 2022 07:19

htuch reviewed Feb 10, 2022

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into overr…

a4a9d0a

…ide_host_status_restriction

KBaichoo assigned snowp Feb 16, 2022

repokitteh-read-only bot added the waiting label Feb 19, 2022

wbpcode added 3 commits February 21, 2022 02:16

Merge branch 'main' of https://github.com/envoyproxy/envoy into overr…

31bc86b

…ide_host_status_restriction

fix format

95c182f

Signed-off-by: wbpcode <wbphub@live.com>

update comments

f5366ac

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot added api and removed waiting labels Feb 21, 2022

adisuissa requested changes Feb 22, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 22, 2022

minor updates

c0b6218

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot removed the waiting label Feb 23, 2022

adisuissa previously approved these changes Mar 1, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Mar 1, 2022

htuch suggested changes Mar 3, 2022

View reviewed changes

source/common/upstream/load_balancer_impl.cc Outdated Show resolved Hide resolved

source/common/upstream/load_balancer_impl.cc Outdated Show resolved Hide resolved

minor update

f4d16c6

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode dismissed adisuissa’s stale review via f4d16c6 March 3, 2022 13:12

adisuissa previously approved these changes Mar 5, 2022

View reviewed changes

replace uint flag with bitset

37f823f

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode dismissed adisuissa’s stale review via 37f823f March 7, 2022 09:41

htuch reviewed Mar 9, 2022

View reviewed changes

alias name for std::bitset<32>

a02a7fc

Signed-off-by: wbpcode <wbphub@live.com>

htuch approved these changes Mar 10, 2022

View reviewed changes

htuch merged commit 1575185 into envoyproxy:main Mar 10, 2022

Conversation

wbpcode commented Jan 24, 2022

Uh oh!

repokitteh-read-only bot commented Jan 24, 2022

Uh oh!

wbpcode commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jan 24, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa commented Feb 3, 2022

Uh oh!

wbpcode commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Feb 8, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Feb 10, 2022

Uh oh!

repokitteh-read-only bot commented Feb 10, 2022

Uh oh!

KBaichoo commented Feb 16, 2022

Uh oh!

htuch commented Feb 20, 2022

Uh oh!

wbpcode commented Feb 21, 2022

Uh oh!

repokitteh-read-only bot commented Feb 21, 2022

wbpcode commented Jan 24, 2022 •

edited

Loading

wbpcode commented Jan 24, 2022 •

edited

Loading

wbpcode Jan 26, 2022 •

edited

Loading

wbpcode Jan 26, 2022 •

edited

Loading

wbpcode Jan 26, 2022 •

edited

Loading

wbpcode commented Feb 8, 2022 •

edited

Loading

wbpcode Mar 9, 2022 •

edited

Loading