Add overload manager to bootstrap config #4038
Conversation
…dd documentation Signed-off-by: Elisha Ziskind <eziskind@google.com>
Signed-off-by: Elisha Ziskind <eziskind@google.com>
Signed-off-by: Elisha Ziskind <eziskind@google.com>
| // resource monitor type. | ||
| // resource monitor type. The built-in resource monitors are: | ||
| // | ||
| // clang-format off |
There was a problem hiding this comment.
I don't think this should be here :)
There was a problem hiding this comment.
Without this clang-format suppression, it complains that the line is too long but fix-format splits the line like this:
// * :ref:envoy.resource_monitors.fixed_heap // <envoy_api_msg_config.resource_monitor.fixed_heap.v2alpha.FixedHeapConfig>
which breaks documentation:
/source/generated/rst/api-v2/config/overload/v2alpha/overload.proto.rst:32:Inline interpreted text or phrase reference start-string without end-string.
There was a problem hiding this comment.
I'm confused how clang-format is running on RST or proto; I think we run our fix-format script on the protos, but I don't think Clang has any proto knowledge to contribute..
There was a problem hiding this comment.
We run clang-format on protos. It has a proto mode.
There was a problem hiding this comment.
Fixed - realized I just needed to add a couple spaces to the ref comment so it could be parsed correctly by the docs generator.
| string name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
|
||
| // Configuration for the resource monitor being instantiated. | ||
| google.protobuf.Struct config = 2; | ||
| } | ||
|
|
||
| // Convenience protobuf for resource monitors that do not require any configuration. |
There was a problem hiding this comment.
Do we need this? Can't we just use google.protobuf.Empty?
There was a problem hiding this comment.
Yup, added a helper EmptyConfigFactoryBase class for resources monitors with no configs that uses google.protobuf.Empty.
| // The fixed heap resource monitor reports the Envoy process memory pressure, computed as a | ||
| // fraction of currently reserved heap memory divided by a statically configured maximum | ||
| // specified in the FixedHeapConfig. | ||
|
|
| Overload manager | ||
| ================ | ||
|
|
||
| The overload manager is configured in the Boostrap |
There was a problem hiding this comment.
Maybe link "overload manager" back to the architecture overview intro.
source/server/server.cc
Outdated
| // Initialize the overload manager early so other modules can register for actions. | ||
| overload_manager_.reset(new OverloadManagerImpl( | ||
| dispatcher(), stats(), | ||
| bootstrap_.has_overload_manager() ? bootstrap_.overload_manager() |
There was a problem hiding this comment.
Nit: you don't need the conditional here; if you write bootstrap_.overload_manager(), it will give you the empty proto if it hasn't been defined in the config.
source/server/server.cc
Outdated
| @@ -403,6 +409,8 @@ RunHelper::RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm | |||
| } | |||
|
|
|||
| void InstanceImpl::run() { | |||
| overload_manager_->start(); | |||
There was a problem hiding this comment.
If there's an easy way to validate this in the server tests with the mocked overload manager, that could be nice to check.
There was a problem hiding this comment.
Ok, I moved this call to the RunHelper class so it can be tested.
Signed-off-by: Elisha Ziskind <eziskind@google.com>
| /** | ||
| * Factory for resource monitors that have empty configuration blocks. | ||
| */ | ||
| class EmptyConfigFactoryBase : public Server::Configuration::ResourceMonitorFactory { |
There was a problem hiding this comment.
What's the difference between this and FactoryBase<Envoy::ProtobufWkt::Empty>?
There was a problem hiding this comment.
Envoy::ProtobufWkt::Empty doesn't have a "Validate" method so the call to MessageUtil::downcastAndValidate here (https://github.com/envoyproxy/envoy/blob/master/source/extensions/resource_monitors/common/factory_base.h#L19) doesn't compile.
There was a problem hiding this comment.
fwiw, there is an http filter config factory that does something similar - https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/common/empty_http_filter_config.h
Initialize on startup and add documentation (issue #373)
Risk Level: low
Testing: unit tests
Docs Changes: add docs for overload manager
Signed-off-by: Elisha Ziskind eziskind@google.com