Add overload manager for Envoy by eziskind · Pull Request #3954 · envoyproxy/envoy

eziskind · 2018-07-25T19:17:16Z

Add an overload manager module to protect an Envoy instance from resource starvation.
For issue #373 - see design doc linked from there

Risk level: low (not linked into envoy main yet)

Signed-off-by: Elisha Ziskind eziskind@google.com

Signed-off-by: Elisha Ziskind <eziskind@google.com>

zuercher

This looks like it's headed in the right direction. I put down some initial review comments for you to look at in addition to the failing test.

zuercher · 2018-07-25T22:33:21Z

source/server/overload_manager_impl.cc

+public:
+  ThresholdTriggerImpl(const envoy::config::overload::v2alpha::ThresholdTrigger& config)
+      : threshold_(config.value()) {
+    RELEASE_ASSERT(threshold_ >= 0, "");


Configuration validation should normally throw EnvoyException rather than crashing the server with RELEASE_ASSERT.

This one in particular, however, you could implement with the PGV (see https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/percent.proto#L13 for an example). I think you'll need to invoke MessageUtil::validate on the OverloadManager config object somewhere for this to work. That may come for free later when OverloadManager is added to the config hierarchy.

zuercher · 2018-07-25T22:35:33Z

source/server/overload_manager_impl.cc

+      trigger = std::make_unique<ThresholdTriggerImpl>(trigger_config.threshold());
+      break;
+    case envoy::config::overload::v2alpha::Trigger::TRIGGER_ONEOF_NOT_SET:
+      RELEASE_ASSERT(false, "missing trigger");


style nit: I think we prefer

default: NOT_REACHED_GCOVR_EXCL_LINE;

zuercher · 2018-07-25T22:36:17Z

source/server/overload_manager_impl.cc

+    }
+
+    const auto result = triggers_.insert(std::make_pair(trigger_config.name(), std::move(trigger)));
+    RELEASE_ASSERT(result.second, "duplicate trigger");


As mentioned above, use an EnvoyException here. Likewise in the RELEASE_ASSERT statements below, when used for config validation.

zuercher · 2018-07-25T22:41:09Z

source/server/overload_manager_impl.h

+  OverloadManagerImpl(Event::Dispatcher& dispatcher,
+                      const envoy::config::overload::v2alpha::OverloadManager& config);
+
+  void start() override;


Style nit: use // Server::OverloadManager to indicate the interface you're implementing here (and group the interface method overrides without blank lines)

zuercher · 2018-07-25T22:41:27Z

source/server/overload_manager_impl.h

+    Resource(const std::string& name, ResourceMonitorPtr monitor, OverloadManagerImpl& manager)
+        : name_(name), monitor_(std::move(monitor)), manager_(manager), pending_update_(false) {}
+
+    void onSuccess(const ResourceUsage& usage) override;


// ResourceMonitor::Callbacks

zuercher · 2018-07-25T23:03:03Z

source/server/overload_manager_impl.cc

+void OverloadManagerImpl::updateResourcePressure(const std::string& resource, double pressure) {
+  auto action_range = resource_to_actions_.equal_range(resource);
+  typedef std::unordered_multimap<std::string, std::string> ActionMap;
+  typedef std::unordered_multimap<std::string, ActionCallback> CallbackMap;


Consider putting these typedefs in the class definition.

zuercher · 2018-07-25T23:03:32Z

source/server/overload_manager_impl.cc

+  typedef std::unordered_multimap<std::string, ActionCallback> CallbackMap;
+  std::for_each(action_range.first, action_range.second, [&](ActionMap::value_type& entry) {
+    const std::string& action = entry.second;
+    auto action_it = actions_.find(entry.second);


nit: Use action instead of entry.second.

zuercher · 2018-07-25T23:05:26Z

api/envoy/config/overload/v2alpha/overload.proto

+
+message Trigger {
+  // The name of the resource this is a trigger for.
+  string name = 1;


Should this have a validation rule to prevent empty strings?

zuercher · 2018-07-25T23:06:18Z

api/envoy/config/overload/v2alpha/overload.proto

+message ThresholdTrigger {
+  // If the resource metric (ie. pressure) is greater than or equal to this value, the trigger
+  // will fire.
+  double value = 1;


Related to a previous comment, if this has a limited range, it should be documented/validated here.

zuercher · 2018-07-25T23:07:04Z

include/envoy/server/overload_manager.h

+   * Register a callback to be invoked when the specified action changes state (ie. becomes
+   * activated or inactivated). The callback is posted with the given dispatcher.
+   * Must only be called during Envoy initialization when still in single-threaded mode.
+   */


style nit: Please add param tags for the parameters.

Signed-off-by: Elisha Ziskind <eziskind@google.com>

The percent protobuf allows values 0..100, while we just want a fraction from 0 to 1. Added PGV validation rule. Signed-off-by: Elisha Ziskind <eziskind@google.com>

eziskind · 2018-07-26T21:01:20Z

Thanks for the review - all feedback should be addressed now.

zuercher

One more comment from me.

Probably @mattklein123 or @htuch should have a look now.

zuercher · 2018-07-27T16:50:09Z

source/server/overload_manager_impl.cc

+  const bool active = isActive();
+
+  auto it = triggers_.find(name);
+  RELEASE_ASSERT(it != triggers_.end(), "");


These look like program invariants to me, in which case they should probably stay plain ASSERTs. (per https://github.com/envoyproxy/envoy/blob/master/STYLE.md#error-handling).

I think this is true for most if not all the RELEASE_ASSERTs you have.

Signed-off-by: Elisha Ziskind <eziskind@google.com>

mattklein123

Looks great. Excited to see this land. Some questions and small comments.

mattklein123 · 2018-07-30T18:06:28Z

api/envoy/config/overload/v2alpha/overload.proto

@@ -0,0 +1,63 @@
+syntax = "proto3";


Can you make sure this shows up in the docs? I don't think it will without including this new package, but I'm not sure. Also, I think it would be worthwhile to add a new short arch overview section on this functionality and briefly describe the built-in capabilities. https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/arch_overview. Perhaps a new section on "Overload manager?"

Also we might also want some docs here with more details that finally link into the v2 docs: https://www.envoyproxy.io/docs/envoy/latest/configuration/configuration. Unclear if that is worth it or not if you have arch overview, so we can see how it goes?

Alternatively, if you want to do this in a follow up when you actually implement the listener actions, that's fine also. Can we check pending items like docs/stats/etc. in a tracking issue?

I'm planning on adding docs in an upcoming PR when I hook the overload manager config into the bootstrap proto. We can use the original issue (#373) to track this unless you prefer a separate one.

mattklein123 · 2018-07-30T18:06:46Z

api/envoy/config/overload/v2alpha/overload.proto

+
+import "validate/validate.proto";
+
+// The Overload Manager provides an extensible framework to protect Envoy instances


Will need proto title for docs.

Will do in a separate docs PR.

mattklein123 · 2018-07-30T18:07:09Z

api/envoy/config/overload/v2alpha/overload.proto

+
+message ResourceMonitor {
+  // The name of the resource monitor to instantiate. Must match a registered
+  // resource monitor type.


Can we list out the built-in ones? See other similar examples in docs for logging, filters, etc.

ditto - deferring to a separate docs PR

mattklein123 · 2018-07-30T18:07:36Z

api/envoy/config/overload/v2alpha/overload.proto

+message OverloadAction {
+  // The name of the overload action. This is just a well-known string that listeners can
+  // use for registering callbacks. Custom overload actions should be named using reverse
+  // DNS to ensure uniqueness.


Any built-in ones?

None yet. I'll be adding some in a future PR and will update the docs here as part of that.

mattklein123 · 2018-07-30T18:08:50Z

include/envoy/server/overload_manager.h

+   *        changes state
+   */
+  virtual void registerForAction(const std::string& action, Event::Dispatcher& dispatcher,
+                                 std::function<void(bool)> callback) PURE;


instead of bool can we create an enum with clear state names? In addition to docs this might be useful in the future if we need more states.

mattklein123 · 2018-07-30T18:10:34Z

source/server/overload_manager_impl.h

+    virtual ~Trigger() {}
+
+    // Updates the current value of the metric and returns whether the trigger has changed state.
+    virtual bool updateValue(double value) = 0;


nit: s/= 0/PURE. Same elsewhere.

mattklein123 · 2018-07-30T18:13:59Z

source/server/overload_manager_impl.h

+namespace Envoy {
+namespace Server {
+
+class OverloadActionImpl {


nit: Since this doesn't actually implement any interface I would call this OverloadAction

Signed-off-by: Elisha Ziskind <eziskind@google.com>

eziskind · 2018-07-31T01:01:11Z

tsan failure is unrelated to my change:
@envoy//test/extensions/filters/network/thrift_proxy:filter_integration_test TIMEOUT in 315.0s

mattklein123 · 2018-07-31T02:59:51Z

@eziskind I kicked the test. Will take another pass tomorrow.

mattklein123

Looks great, happy to see this land.

Add overload manager for Envoy

865b341

Signed-off-by: Elisha Ziskind <eziskind@google.com>

zuercher reviewed Jul 25, 2018

View reviewed changes

eziskind added 2 commits July 26, 2018 14:22

Address review feedback and fix tests

08c270d

Signed-off-by: Elisha Ziskind <eziskind@google.com>

Change ThresholdTrigger back to use a double value rather than percent

5ce627f

The percent protobuf allows values 0..100, while we just want a fraction from 0 to 1. Added PGV validation rule. Signed-off-by: Elisha Ziskind <eziskind@google.com>

zuercher reviewed Jul 27, 2018

View reviewed changes

Use ASSERT instead of RELEASE_ASSERT

6f47b21

Signed-off-by: Elisha Ziskind <eziskind@google.com>

mattklein123 assigned zuercher and mattklein123 Jul 30, 2018

mattklein123 reviewed Jul 30, 2018

View reviewed changes

eziskind added 2 commits July 30, 2018 15:28

Add OverloadActionState enum to pass to listener callbacks

179698e

Signed-off-by: Elisha Ziskind <eziskind@google.com>

Fix lambda capture for overload action callbacks

423c88d

Signed-off-by: Elisha Ziskind <eziskind@google.com>

mattklein123 approved these changes Jul 31, 2018

View reviewed changes

mattklein123 merged commit 83b9e2d into envoyproxy:master Jul 31, 2018

eziskind deleted the overload2 branch July 31, 2018 16:57

rgs1 mentioned this pull request Aug 1, 2018

Add option for merging cluster updates #3941

Merged


		import "validate/validate.proto";

		// The Overload Manager provides an extensible framework to protect Envoy instances

Conversation

eziskind commented Jul 25, 2018

Uh oh!

zuercher left a comment

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!

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!

eziskind commented Jul 26, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

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!

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!

eziskind commented Jul 31, 2018

Uh oh!

mattklein123 commented Jul 31, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants