Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
90f775a
Add interface for ScaledRangeTimerManager
akonradi Oct 7, 2020
0dd57aa
Convert config literals to YAML
akonradi Oct 7, 2020
151f1ca
Add reduce timeouts overload action
akonradi Oct 6, 2020
c5c2be9
Enable scaling HTTP connection idle timeout
akonradi Oct 7, 2020
26a4b2d
Fix formatting
akonradi Oct 12, 2020
f95bd82
Add interface for ScaledRangeTimerManager
akonradi Oct 7, 2020
effc203
Create TimerPtr from ScaledRangeTimerManager
akonradi Oct 12, 2020
324198d
Address review feedback
akonradi Oct 13, 2020
b8e5e1e
Fix build on Windows
akonradi Oct 14, 2020
dc82f73
Add comments
akonradi Oct 14, 2020
b3f1c99
Merge remote-tracking branch 'upstream/master' into scaled-timer-inte…
akonradi Oct 15, 2020
794e9e1
Merge remote-tracking branch 'upstream/master' into scaled-timer-inte…
akonradi Oct 16, 2020
5808eed
Fix scaled timer min > max behavior
akonradi Oct 19, 2020
e29329b
Fix formatting
akonradi Oct 19, 2020
20c8af4
Merge branch 'scaled-timer-interface' into overload-scaled-timeouts
akonradi Oct 20, 2020
08f1331
Address feedback
akonradi Oct 20, 2020
226c183
Add missed generated api change
akonradi Oct 20, 2020
4b00ebe
Remove RangeTimer interface
akonradi Oct 20, 2020
72916ca
Merge branch 'scaled-timer-interface' into overload-scaled-timeouts
akonradi Oct 20, 2020
99fb591
Merge remote-tracking branch 'upstream/master' into overload-scaled-t…
akonradi Oct 21, 2020
f7736e7
Add documentation for new action
akonradi Oct 21, 2020
8fe5101
Add release notes
akonradi Oct 22, 2020
bbd65fd
Mock behavior to fix tests
akonradi Oct 22, 2020
6bbcd2a
Fix constructor argument name
akonradi Oct 23, 2020
43f0673
Fix coverage for new method never called
akonradi Oct 23, 2020
94821c0
Merge remote-tracking branch 'upstream/master' into overload-scaled-t…
akonradi Oct 23, 2020
28500e1
Add example for overload timeout reduction config
akonradi Oct 27, 2020
7825f39
Add link to example
akonradi Oct 27, 2020
2e2d5e0
Merge branch 'master' into overload-scaled-timeouts
akonradi Oct 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/config/overload/v3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ licenses(["notice"]) # Apache 2
api_proto_package(
deps = [
"//envoy/config/overload/v2alpha:pkg",
"//envoy/type/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
34 changes: 34 additions & 0 deletions api/envoy/config/overload/v3/overload.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ syntax = "proto3";

package envoy.config.overload.v3;

import "envoy/type/v3/percent.proto";

import "google/protobuf/any.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/struct.proto";
Expand Down Expand Up @@ -80,6 +82,35 @@ message Trigger {
}
}

// Typed configuration for the "envoy.overload_actions.reduce_timeouts" action.
message ScaleTimersOverloadActionConfig {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no mentions of this config corresponding to the "envoy.overload_actions.reduce_timeouts" action.

Should envoy.overload_actions.reduce_timeouts be mentioned in this config message comment?
Should there be some doc updates that mention how to configure this action?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

enum TimerType {
UNSPECIFIED = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason this is specified to force the user to explicitly configure the timeout they want? I think that's OK but can you add a small comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, done. I would have liked to just start at 1 but the compiler (or maybe lint checker?) complains.


// Adjusts the idle timer for downstream HTTP connections that takes effect when there are no active streams.
// This affects the value of :ref:`RouteAction.idle_timeout <envoy_api_field_config.route.v3.RouteAction.idle_timeout>`.
HTTP_DOWNSTREAM_CONNECTION_IDLE = 1;
}

message ScaleTimer {
// The type of timer this minimum applies to.
TimerType timer = 1 [(validate.rules).enum = {defined_only: true not_in: 0}];

oneof overload_adjust {
option (validate.required) = true;

// Sets the minimum duration as an absolute value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading the docs, it's not immediately clear to me how min_timeout interacts with the scaling. Can you flesh this out a bit more to better describe how everything fits together in terms of scaling? An example would probably help.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elaborated in the docs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks the example is very helpful. Can you potentially ref link to the docs somehow from here just so that people can go back and forth if they are confused what this does?

/wait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, done.

google.protobuf.Duration min_timeout = 2;

// Sets the minimum duration as a percentage of the maximum value.
type.v3.Percent min_scale = 3;
Comment on lines +108 to +109

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this might be a way to shoot yourself in the foot as it's not as intuitive and easy to compare as absolute time -- unless you end up doing the calculation yourself in which case maybe just use the min_timeout to set the absolute duration.

If this gets set too low, I could see the aggressive timeouts causing cascading failures i.e. user requests times out so they end up refreshing the page.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, though a lot of the overload manger actions let you shoot yourself in the foot. Having a scale factor allows this to apply meaningfully to timers that can have different values across listeners.

}
}

// A set of timer scaling rules to be applied.
repeated ScaleTimer timer_scale_factors = 1 [(validate.rules).repeated = {min_items: 1}];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add release notes describing this new feature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


message OverloadAction {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.overload.v2alpha.OverloadAction";
Expand All @@ -89,6 +120,9 @@ message OverloadAction {
// DNS to ensure uniqueness.
string name = 1 [(validate.rules).string = {min_len: 1}];

// Configuration for the action being instantiated.
google.protobuf.Any typed_config = 3;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Consider ordering by field id, since there's no obvious reason to put typed_config next to the name parameter.

// A set of triggers for this action. The state of the action is the maximum
// state of all triggers, which can be scaling between 0 and 1 or saturated. Listeners
// are notified when the overload action changes state.
Expand Down
4 changes: 4 additions & 0 deletions api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,10 @@ message RouteAction {
// fires, the stream is terminated with a 408 Request Timeout error code if no
// upstream response header has been received, otherwise a stream reset
// occurs.
//
// If the overload action "envoy.overload_actions.reduce_timeouts" is configured, this timeout

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you ref link "overload action" to the overload manager docs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, done.

// is scaled according to the value for
// :ref:`HTTP_DOWNSTREAM_CONNECTION_IDLE <envoy_api_enum_value_config.overload.v3.ScaleTimersOverloadActionConfig.TimerType.HTTP_DOWNSTREAM_CONNECTION_IDLE>`.
google.protobuf.Duration idle_timeout = 24;

// Indicates that the route has a retry policy. Note that if this is set,
Expand Down
4 changes: 4 additions & 0 deletions api/envoy/config/route/v4alpha/route_components.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions generated_api_shadow/envoy/config/overload/v3/BUILD

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions generated_api_shadow/envoy/config/overload/v3/overload.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions include/envoy/event/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "scaled_range_timer_manager_interface",
hdrs = ["scaled_range_timer_manager.h"],
deps = [
":range_timer_interface",
":timer_interface",
],
)

envoy_cc_library(
name = "schedulable_cb_interface",
hdrs = ["schedulable_cb.h"],
Expand Down
99 changes: 99 additions & 0 deletions include/envoy/event/scaled_range_timer_manager.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
#pragma once

#include "envoy/common/pure.h"
#include "envoy/event/range_timer.h"
#include "envoy/event/timer.h"

#include "absl/types/variant.h"

namespace Envoy {
namespace Event {

/**
* Describes a minimum timer value that is equal to a scale factor applied to the maximum.
*/
struct ScaledMinimum {
explicit ScaledMinimum(double scale_factor) : scale_factor_(scale_factor) {}
const double scale_factor_;
};

/**
* Describes a minimum timer value that is an absolute duration.
*/
struct AbsoluteMinimum {
explicit AbsoluteMinimum(std::chrono::milliseconds value) : value_(value) {}
const std::chrono::milliseconds value_;
};

/**
* Class that describes how to compute a minimum timeout given a maximum timeout value. It wraps
* ScaledMinimum and AbsoluteMinimum and provides a single computeMinimum() method.
*/
class ScaledTimerMinimum : private absl::variant<ScaledMinimum, AbsoluteMinimum> {
public:
// Use base class constructor.
using absl::variant<ScaledMinimum, AbsoluteMinimum>::variant;

// Computes the minimum value for a given maximum timeout. If this object was constructed with a
// - ScaledMinimum value:
// the return value is the scale factor applied to the provided maximum.
// - AbsoluteMinimum:
// the return value is that minimum, and the provided maximum is ignored.
std::chrono::milliseconds computeMinimum(std::chrono::milliseconds maximum) const {
struct Visitor {
explicit Visitor(std::chrono::milliseconds value) : value_(value) {}
std::chrono::milliseconds operator()(ScaledMinimum scale_factor) {
return std::chrono::duration_cast<std::chrono::milliseconds>(scale_factor.scale_factor_ *
value_);
}
std::chrono::milliseconds operator()(AbsoluteMinimum absolute_value) {
return absolute_value.value_;
}
const std::chrono::milliseconds value_;
};
return absl::visit<Visitor, const absl::variant<ScaledMinimum, AbsoluteMinimum>&>(
Visitor(maximum), *this);
}
};

/**
* Class for creating RangeTimer objects that can be adjusted towards either the minimum or maximum
* of their range by the owner of the manager object. Users of this class can call createTimer() to
* receive a new RangeTimer object that they can then enable or disable at will (but only on the
* same dispatcher), and setScaleFactor() to change the scaling factor. Updates to the current
* scale factor are applied to all timers, including those created in the past.
*/
class ScaledRangeTimerManager {
public:
virtual ~ScaledRangeTimerManager() = default;

/**
* Creates a new range timer backed by the manager. The returned timer will be subject to the
* current and future scale factor values set on the manager. All returned timers must be deleted
* before the manager.
*/
virtual RangeTimerPtr createRangeTimer(TimerCb callback) PURE;

/**
* Creates a new range timer backed by the manager that mimics the Timer interface. Calling
* enableTimer on the returned object sets the maximum duration, while the first argument here
* controls the minimum. Passing a value of ScaleFactor(x) sets the min to (x * max) when the
* timer is enabled, while AbsoluteValue(y) sets the min to the duration y.
*/
virtual TimerPtr createTimer(ScaledTimerMinimum minimum, TimerCb callback) PURE;

/**
* Sets the scale factor for all timers that have been or will be created through this manager.
* The value must be between 0.0 and 1.0, inclusive. The scale factor affects the amount of time
* timers spend in their target range. The RangeTimers returned by createTimer will fire after
* (min + (max - min) * scale_factor). This means that a scale factor of 0 causes timers to fire
* immediately at the min duration, a factor of 0.5 causes firing halfway between min and max, and
* a factor of 1 causes firing at max.
*/
virtual void setScaleFactor(double scale_factor) PURE;
};

using ScaledRangeTimerManagerPtr = std::unique_ptr<ScaledRangeTimerManager>;

} // namespace Event
} // namespace Envoy
1 change: 1 addition & 0 deletions include/envoy/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ envoy_cc_library(
name = "overload_manager_interface",
hdrs = ["overload_manager.h"],
deps = [
"//include/envoy/event:timer_interface",
"//include/envoy/thread_local:thread_local_interface",
"//source/common/singleton:const_singleton",
],
Expand Down
16 changes: 16 additions & 0 deletions include/envoy/server/overload_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <string>

#include "envoy/common/pure.h"
#include "envoy/event/timer.h"
#include "envoy/thread_local/thread_local.h"

#include "common/common/macros.h"
Expand Down Expand Up @@ -39,13 +40,25 @@ class OverloadActionState {
*/
using OverloadActionCb = std::function<void(OverloadActionState)>;

enum class OverloadTimerType {
// Timers created with this type will never be scaled. This should only be used for testing.
UnscaledRealTimerForTest,
// The amount of time an HTTP connection to a downstream client can remain idle (no streams). This
// corresponds to the HTTP_DOWNSTREAM_CONNECTION_IDLE TimerType in overload.proto.
HttpDownstreamIdleConnectionTimeout,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a reference in this comment to the real parameter associated with this enum value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

};

/**
* Thread-local copy of the state of each configured overload action.
*/
class ThreadLocalOverloadState : public ThreadLocal::ThreadLocalObject {
public:
// Get a thread-local reference to the value for the given action key.
virtual const OverloadActionState& getState(const std::string& action) PURE;

// Get a scaled timer whose minimum corresponds to the configured value for the given timer type.
virtual Event::TimerPtr createScaledTimer(OverloadTimerType timer_type,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Event::TimerCb callback) PURE;
};

/**
Expand All @@ -68,6 +81,9 @@ class OverloadActionNameValues {

// Overload action to try to shrink the heap by releasing free memory.
const std::string ShrinkHeap = "envoy.overload_actions.shrink_heap";

// Overload action to reduce some subset of configured timeouts.
const std::string ReduceTimeouts = "envoy.overload_actions.reduce_timeouts";
};

using OverloadActionNames = ConstSingleton<OverloadActionNameValues>;
Expand Down
7 changes: 4 additions & 3 deletions source/common/event/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,13 @@ envoy_cc_library(
)

envoy_cc_library(
name = "scaled_range_timer_manager",
srcs = ["scaled_range_timer_manager.cc"],
hdrs = ["scaled_range_timer_manager.h"],
name = "scaled_range_timer_manager_lib",
srcs = ["scaled_range_timer_manager_impl.cc"],
hdrs = ["scaled_range_timer_manager_impl.h"],
deps = [
"//include/envoy/event:dispatcher_interface",
"//include/envoy/event:range_timer_interface",
"//include/envoy/event:scaled_range_timer_manager_interface",
"//source/common/common:scope_tracker",
],
)
Loading