server: add support for range trigger overload actions #12352

akonradi · 2020-07-29T15:28:56Z

Commit Message:
Change the Overload Manager API to extend the overload action state with non-binary values. This will allow future overload actions to take effect in response to increasing load, instead of to the existing inactive/active binary values. This PR also adds a range field to the overload trigger config, though no actions currently trigger on the 'scaling' state.

Additional Description:
The existing behavior is preserved by replacing all places where OverloadActionState::Active was being used with OverloadActionState::saturated(). This is a minimal re-hashing of #11697 for ##11427.

Risk Level: medium
Testing: ran unit and integration tests
Docs Changes: none
Release Notes: add "scaling" trigger for OverloadManager actions

Extend the existing overload action states with a 'scaling' state with a value in the range [0, 1]. The scaling state is not used yet, but will be used to support actions that can take partial effect even if resource pressure is below saturation. Signed-off-by: Alex Konradi <akonradi@google.com>

Add a Range trigger that will produce overload action states that are in between the inactive and saturated states. This will enable the creation of actions that take more effect as load increases. Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only · 2020-07-29T15:29:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12352 was opened by akonradi.

see: more, trace.

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-07-29T15:58:15Z

/cc @eziskind @antoniovicente

source/server/overload_manager_impl.cc

Rename the gauge to match the documentation Signed-off-by: Alex Konradi <akonradi@google.com>

Both stats are present for all actions. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

source/server/overload_manager_impl.cc

test/server/overload_manager_impl_test.cc

Correct test for stat name change in prior commit Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

eziskind · 2020-07-31T18:39:33Z

docs/root/intro/arch_overview/operations/overload_manager.rst

+  saturated, the resource pressure is at or above the critical point; drastic action should be taken
+  scaling,   the resource pressure is below the critical point; action may be taken


Should we also have an "inactive" state so we can distinguish when envoy is not at all in overload so no action will be taken vs "scaling" when envoy is in partial overload so some scaled action will be taken? You refer to such an "inactive" state in the OverloadAction proto below.

I removed it from code after talking to @antoniovicente. I think the expectation is that actions that respond to active/inactive can just check for "active or not" and actions that look at the scaling value can check for "value() == 0"

Ok, then you should probably remove the reference to the "inactive" state in the proto comments.

One last argument for the "inactive" state - it might not be so useful in the code that implements the overload actions but it would be useful in logging and monitoring to distinguish between "inactive" and "scaling". During normal operation I think we'd expect all overload actions to be in "inactive" state and it would be useful to know when we transition out of that state.

Yeah I keep going back and forth. It feels awkward in the documentation to say 0 is still scaling. WDYT @eziskind of doing this in a follow-on PR?

Sure, a follow-on PR is fine with me.

eziskind

LGTM!

antoniovicente

Looking pretty good. Some nits below.

antoniovicente · 2020-07-31T18:01:39Z

docs/root/intro/arch_overview/operations/overload_manager.rst

+  :widths: 1, 2
+
+  saturated, the resource pressure is at or above the critical point; drastic action should be taken
+  scaling,   the resource pressure is below the critical point; action may be taken


this description does not mention the role that the min threshold plays. I'm having some trouble with terminology here, I keep thinking about the linear region of amplifier circuits.

include/envoy/server/overload_manager.h

source/server/overload_manager_impl.cc

antoniovicente · 2020-07-31T19:03:58Z

source/server/overload_manager_impl.cc

+                    if (old_state.isSaturated() != state.isSaturated()) {
+                      ENVOY_LOG(info, "Overload action {} became {}", action,
+                                (state.isSaturated() ? "saturated" : "scaling"));
+                    }


is some logging appropriate for scaling state transitions?

Possibly LOG_EVERY_N_SEC when scaling, and/or LOG when entering and exiting scaling mode.

Probably a good idea. Does Envoy have a LOG_EVERY_N_SEC macro?

Doesn't seem like it. At least not yet.

This should also be at debug level I think.

test/server/overload_manager_impl_test.cc

Signed-off-by: Alex Konradi <akonradi@google.com>

…ined-states Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

Explicitly document the meaning of the scaling and saturated ranges in the overload manager overview, and list the two types of triggers available and the meaning of each. Signed-off-by: Alex Konradi <akonradi@google.com>

source/server/overload_manager_impl.cc

antoniovicente · 2020-08-05T16:12:43Z

source/server/overload_manager_impl.cc

-                        is_active ? OverloadActionState::Active : OverloadActionState::Inactive;
-                    ENVOY_LOG(info, "Overload action {} became {}", action,
-                              is_active ? "active" : "inactive");
+                    const auto state = action_it->second.getState();


Is additional work needed to optimize update propagation? It seems that each action is propagated independently to workers. updateResourcePressure returns true a lot more often than before. Is there some potential for aggregation?

How often does updateResourcePressure run anyway? Should the code below wait for propagation to complete before returning as a way to ensure that at most 1 update is in progress?

Yea, I think it should be straightforward to aggregate a bunch of <action, state> pairs in this loop and then send them to workers with a single call to runOnAllThreads. Maybe best to do this in a follow-on PR?

Re: how often this is called - our best practices doc for use as an edge proxy has an example config where we set this to 250ms (https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge)

Yeah I think there are a couple optimizations we can apply here. Right now we post an event to each worker for each resource update, but we can do a better job of coalescing those. updateResourcePressure returns true more often only if the proxy is configured with a range trigger, which none should be yet.

I'm going to address this an Elisha's other comment about the 'inactive' state in a follow-on PR.

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-08-06T14:58:26Z

@snowp Can I get a pass on this from you or another maintainer? Also, who should I talk to for an API review?

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-08-10T13:37:59Z

@mattklein123 @snowp this should be ready for another round

mattklein123

API/docs LGTM with one small doc request. Very nice! @antoniovicente any further code comments? I think @snowp is out this week so happy to merge if you are satisfied otherwise.

/wait

mattklein123 · 2020-08-10T20:26:50Z

docs/root/configuration/operations/overload_manager/overload_manager.rst

+  * - :ref:`threshold <envoy_v3_api_msg_config.overload.v3.ThresholdTrigger>`
+    - Sets the action state to 1 (= *saturated*) when the resource pressure is above a threshold, and to 0 otherwise.
+  * - :ref:`scaled <envoy_v3_api_msg_config.overload.v3.ScaledTrigger>`
+    - Sets the action state to 0 when the resource pressure is below the min threshold,


replace min/max with the new names and for bonus points ref link them

Gotta get the bonus points. Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123

Thanks!

htuch

Looks great, just a few minor comments.
/wait

htuch · 2020-08-11T12:08:42Z

source/server/overload_manager_impl.cc

+        saturated_threshold_(config.saturation_threshold()),
+        state_(OverloadActionState::inactive()) {
+    if (scaling_threshold_ >= saturated_threshold_) {
+      throw EnvoyException("min_value must be less than max_value");


Nit: can you adjust this exception message to match the field names? I.e. scaling/saturated_threshold.

htuch · 2020-08-11T12:09:42Z

include/envoy/server/overload_manager.h

+  bool isSaturated() const { return action_value_ == 1; }
+
+private:
+  float action_value_;


No, because then OverloadActionState isn't assignable.

htuch · 2020-08-11T12:13:16Z

source/server/overload_manager_impl.cc

+                    if (old_state.isSaturated() != state.isSaturated()) {
+                      ENVOY_LOG(info, "Overload action {} became {}", action,
+                                (state.isSaturated() ? "saturated" : "scaling"));
+                    }


This should also be at debug level I think.

htuch · 2020-08-11T12:14:36Z

test/server/overload_manager_impl_test.cc

+                          "min_value must be less than max_value.*");
+}
+
+TEST_F(OverloadManagerImplTest, ScaledTriggerMaxEqualsMin) {


Can you add one liner // above these tests explaining the intuition of what is being tested?

repokitteh-read-only · 2020-08-11T12:16:36Z

🙀 Error while processing event:

evaluation error
error: function _github_call error: DELETE https://api.github.com/repos/envoyproxy/envoy/issues/12352/labels/api: 404 Label does not exist []:
 Traceback (most recent call last):
  bootstrap:143: in <toplevel>
  bootstrap:137: in _main
  bootstrap:64: in _call
  bootstrap:15: in _call1
  github.com/envoyproxy/envoy/ci/repokitteh/modules/ownerscheck.star:209: in _pr_review
  github.com/envoyproxy/envoy/ci/repokitteh/modules/ownerscheck.star:146: in _reconcile
  github:240: in issue_unlabel
  github:134: in call
Error: function _github_call error: DELETE https://api.github.com/repos/envoyproxy/envoy/issues/12352/labels/api: 404 Label does not exist []

🐱

Caused by: a #12352 (review) was submitted by @htuch.

see: more, trace.

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 · 2020-08-12T00:20:53Z

LGTM assigning over to @htuch to verify his comments and merge. Thank you!

htuch

LGTM, thanks!

akonradi added 2 commits July 29, 2020 10:39

Add Range trigger to Overload Manager

864ab5e

Add a Range trigger that will produce overload action states that are in between the inactive and saturated states. This will enable the creation of actions that take more effect as load increases. Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot added the api label Jul 29, 2020

akonradi mentioned this pull request Jul 29, 2020

[http] Adaptive timeouts for idle HTTP client connections waiting for a request #11427

Closed

Add documentation for action states

accf2e3

Signed-off-by: Alex Konradi <akonradi@google.com>

eziskind reviewed Jul 29, 2020

View reviewed changes

source/server/overload_manager_impl.cc Outdated Show resolved Hide resolved

alyssawilk assigned antoniovicente and snowp Jul 30, 2020

akonradi added 3 commits July 30, 2020 10:43

Fix scaling -> scale_value gauge for actions

981ff10

Rename the gauge to match the documentation Signed-off-by: Alex Konradi <akonradi@google.com>

Address feedback on docs

ecad37b

Both stats are present for all actions. Signed-off-by: Alex Konradi <akonradi@google.com>

Run fix_format

d1f482c

Signed-off-by: Alex Konradi <akonradi@google.com>

eziskind reviewed Jul 30, 2020

View reviewed changes

source/server/overload_manager_impl.cc Outdated Show resolved Hide resolved

eziskind reviewed Jul 30, 2020

View reviewed changes

test/server/overload_manager_impl_test.cc Outdated Show resolved Hide resolved

akonradi added 2 commits July 31, 2020 14:21

Fix overload manager impl test

8dd5584

Correct test for stat name change in prior commit Signed-off-by: Alex Konradi <akonradi@google.com>

fix formatting

622d9da

Signed-off-by: Alex Konradi <akonradi@google.com>

eziskind reviewed Jul 31, 2020

View reviewed changes

eziskind previously approved these changes Jul 31, 2020

View reviewed changes

antoniovicente reviewed Jul 31, 2020

View reviewed changes

akonradi added 2 commits August 3, 2020 10:06

Address some review feedback

bf77119

Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into overload-fine-gra…

77c2187

…ined-states Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed eziskind’s stale review via 77c2187 August 3, 2020 14:06

akonradi mentioned this pull request Aug 4, 2020

event: scaled range bucket timer #12414

Closed

akonradi added 2 commits August 5, 2020 10:29

Restore missing build rule dependency

4f9426e

Signed-off-by: Alex Konradi <akonradi@google.com>

Make action state documentation less opaque

6d6fcf9

Explicitly document the meaning of the scaling and saturated ranges in the overload manager overview, and list the two types of triggers available and the meaning of each. Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Aug 5, 2020

View reviewed changes

Use insert_or_assign instead of spelling out

ad0b0ab

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via ad0b0ab August 5, 2020 17:49

akonradi dismissed antoniovicente’s stale review via 7a4578c August 6, 2020 17:55

repokitteh-read-only bot removed the waiting label Aug 6, 2020

akonradi added 2 commits August 7, 2020 10:54

Fix compilation errors

4de8f83

Signed-off-by: Alex Konradi <akonradi@google.com>

Fix more name errors

264a753

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 requested changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 10, 2020

Fix min/max refs in docs

d0acf25

Gotta get the bonus points. Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot removed the waiting label Aug 10, 2020

mattklein123 previously approved these changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 10, 2020

htuch suggested changes Aug 11, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 11, 2020

Address feedback

111f31d

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed mattklein123’s stale review via 111f31d August 11, 2020 13:54

repokitteh-read-only bot added api and removed waiting labels Aug 11, 2020

mattklein123 assigned htuch and unassigned snowp, mattklein123 and antoniovicente Aug 12, 2020

htuch approved these changes Aug 12, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 12, 2020

htuch merged commit 28df333 into envoyproxy:master Aug 12, 2020

akonradi deleted the overload-fine-grained-states branch August 12, 2020 17:16

akonradi mentioned this pull request Aug 14, 2020

server: consolidate overload manager cross-thread updates #12649

Merged

akonradi mentioned this pull request Dec 8, 2020

Rate limit: add soft limiting capability to local rate limit filter #14268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add support for range trigger overload actions #12352

server: add support for range trigger overload actions #12352

akonradi commented Jul 29, 2020 •

edited

Loading

repokitteh-read-only bot commented Jul 29, 2020

akonradi commented Jul 29, 2020

eziskind Jul 31, 2020

akonradi Jul 31, 2020

eziskind Jul 31, 2020

eziskind Aug 5, 2020

akonradi Aug 5, 2020

eziskind Aug 5, 2020

eziskind left a comment

antoniovicente left a comment

antoniovicente Jul 31, 2020

antoniovicente Jul 31, 2020

akonradi Aug 3, 2020

antoniovicente Aug 5, 2020

htuch Aug 11, 2020

antoniovicente Aug 5, 2020

eziskind Aug 5, 2020

akonradi Aug 5, 2020

akonradi Aug 5, 2020

akonradi commented Aug 6, 2020

akonradi commented Aug 10, 2020

mattklein123 left a comment

mattklein123 Aug 10, 2020

mattklein123 left a comment

htuch left a comment

htuch Aug 11, 2020

htuch Aug 11, 2020

akonradi Aug 11, 2020

htuch Aug 11, 2020

htuch Aug 11, 2020

repokitteh-read-only bot commented Aug 11, 2020

mattklein123 commented Aug 12, 2020

htuch left a comment

		saturated, the resource pressure is at or above the critical point; drastic action should be taken
		scaling, the resource pressure is below the critical point; action may be taken

server: add support for range trigger overload actions #12352

server: add support for range trigger overload actions #12352

Conversation

akonradi commented Jul 29, 2020 • edited Loading

repokitteh-read-only bot commented Jul 29, 2020

akonradi commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eziskind left a comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akonradi commented Aug 6, 2020

akonradi commented Aug 10, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Aug 11, 2020

mattklein123 commented Aug 12, 2020

htuch left a comment

Choose a reason for hiding this comment

akonradi commented Jul 29, 2020 •

edited

Loading