add resource monitor framework by eziskind · Pull Request #3848 · envoyproxy/envoy

eziskind · 2018-07-12T14:45:39Z

Add an extensible resource monitor framework for monitoring resource "pressures" (usage/limit). This will be used by the overload manager to implement downstream circuit breaking (issue #373 - see design doc linked from there).

Risk Level: low (not yet used in envoy main)

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

to be used by the overload manager (issue envoyproxy#373) Signed-off-by: Elisha Ziskind <eziskind@google.com>

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

htuch

This looks really solid, LGTM modulo comments.

htuch · 2018-07-12T22:37:34Z

api/envoy/config/resource_monitor/fixed_heap/v2alpha/fixed_heap.proto

+option go_package = "v2alpha";
+
+message FixedHeapConfig {
+  // Limit of the Envoy process heap size. This is used to calculate heap memory pressure which


Wondering if there is a more push-button way to approach this, by having Envoy infer maximum memory available in the container? We have had some recent discussions with OpenCensus on how to do memory utilization determination and the main takeaway is its hard to do cross platform in a consistent way. OTOH, freeing users from having to make this determination seems a net win if there's some reasonable approaches (e.g. can tcmalloc tell us anything?).

Unfortunately, I couldn't find a way to dynamically get the heap limit from tcmalloc. I expect most users will want to define their own memory monitor which knows how to query the envoy container for this info and perhaps use it in combination with this or on its own.

FWIW I agree with @eziskind on this that it will be easier to have the operator provide the limit. How that limit is obtained/enforced is going to be system specific.

htuch · 2018-07-12T22:40:38Z

include/envoy/server/resource_monitor.h

+
+  // Callback for handling updated usage information for this resource.
+  // Exactly one of 'usage' or 'error' will be non-null.
+  typedef std::function<void(const ResourceUsage* usage, const EnvoyException* error)> UpdateCb;


I think it would make sense to register both an onSuccess and onFailure callback, following the same patterns as the async clients, e.g. https://github.com/envoyproxy/envoy/blob/master/include/envoy/http/async_client.h.

htuch · 2018-07-13T14:31:21Z

include/envoy/server/resource_monitor_config.h

+   * @return Event::Dispatcher& the main thread's dispatcher. This dispatcher should be used
+   *         for all singleton processing.
+   */
+  virtual Event::Dispatcher& dispatcher() PURE;


I would just inline this and skip the context unless you anticipate this growing significantly.

htuch · 2018-07-13T14:31:43Z

include/envoy/server/resource_monitor_config.h

+                                                   ResourceMonitorFactoryContext& context) PURE;
+
+  /**
+   * @return std::string the identifgitying name for a particular implementation of a resource


Nit: typo identifgitying

htuch · 2018-07-13T14:40:38Z

source/extensions/resource_monitors/fixed_heap/config.cc

+    const Protobuf::Message& config,
+    Server::Configuration::ResourceMonitorFactoryContext& /*unused_context*/) {
+  return std::make_unique<FixedHeapMonitor>(
+      MessageUtil::downcastAndValidate<


To avoid the cast/validation boilerplate, some other extension mechanisms have a templated adapter to the extension interface. Search for "FromProtoTyped" to see examples of this. Not necessarily something needed in this PR, but could be good to add a TODO at least.

htuch · 2018-07-13T14:41:14Z

source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc

+}
+
+void FixedHeapMonitor::updateResourceUsage(const Server::ResourceMonitor::UpdateCb& completionCb) {
+  size_t physical = Memory::Stats::totalCurrentlyReserved();


Nit: make these const size_t.

htuch · 2018-07-13T14:44:33Z

test/extensions/resource_monitors/fixed_heap/fixed_heap_monitor_test.cc

+TEST(FixedHeapMonitorTest, SaneUsage) {
+  envoy::config::resource_monitor::fixed_heap::v2alpha::FixedHeapConfig config;
+  config.set_max_heap_size_bytes(std::numeric_limits<uint64_t>::max());
+  std::unique_ptr<FixedHeapMonitor> monitor(new FixedHeapMonitor(config));


Could FixedHeapMonitor receive its memory query functions as a mockable object in its constructor? This would allow you to validate the actual math in the utilization function as well with mockable inputs.

lizan

You will need add a field in bootstrap.proto to actually include resource monitors in config.

lizan · 2018-07-14T00:29:41Z

source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc

+void FixedHeapMonitor::updateResourceUsage(const Server::ResourceMonitor::UpdateCb& completionCb) {
+  size_t physical = Memory::Stats::totalCurrentlyReserved();
+  size_t unmapped = Memory::Stats::totalPageHeapUnmapped();
+  size_t used = std::max<size_t>(physical - unmapped, 0);


size_t is unsigned, so I think this max does nothing?

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

htuch

LGTM modulo nits. @mattklein123 @lizan can you take another pass?

htuch · 2018-07-17T14:28:49Z

include/envoy/server/BUILD

+    name = "resource_monitor_interface",
+    hdrs = ["resource_monitor.h"],
+    deps = [
+        "//source/common/protobuf",


I think this dependency isn't needed (based on header analysis).

htuch · 2018-07-17T15:15:40Z

source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.cc

+void FixedHeapMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) {
+  const size_t physical = stats_->reservedHeapBytes();
+  const size_t unmapped = stats_->unmappedHeapBytes();
+  const size_t used = physical - unmapped;


Maybe ASSERT(physical >= unmapped); to sanity check here.

htuch · 2018-07-17T15:18:37Z

test/extensions/resource_monitors/fixed_heap/BUILD

+
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_mock",


htuch · 2018-07-17T15:21:12Z

source/extensions/resource_monitors/fixed_heap/fixed_heap_monitor.h

+  virtual ~MemoryStatsReader() {}
+
+  // Memory reserved for the process by the heap.
+  virtual uint64_t reservedHeapBytes();


The general style in Envoy for this is to declare pure interfaces for everything and then specialize as real vs. mock. I think what you have here is fine though, so no objections, just sharing an idiom that seems fairly pervasive.

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

mattklein123

Looks great from a high level skim. Few small comments. Very excited to see this being added!

mattklein123 · 2018-07-18T16:25:36Z

api/envoy/config/resource_monitor/fixed_heap/v2alpha/fixed_heap.proto

+option go_package = "v2alpha";
+
+message FixedHeapConfig {
+  // Limit of the Envoy process heap size. This is used to calculate heap memory pressure which


FWIW I agree with @eziskind on this that it will be easier to have the operator provide the limit. How that limit is obtained/enforced is going to be system specific.

mattklein123 · 2018-07-18T16:26:40Z

include/envoy/server/resource_monitor.h

+     * Called when the request for updated resource usage succeeds.
+     * @param usage the updated resource usage
+     */
+    virtual void onSuccess(const ResourceUsage& usage) PURE;


FWIW, given the size of ResourceUsage I would probably pass by value everywhere, unless you see it getting larger in the future?

We may want to add the actual usage and limit values - useful at least for reporting as stats. So it may grow a little, but probably not much. In any event, I'd expect resource usages won't be polled so frequently that passing by reference or value will make any noticeable difference.

mattklein123 · 2018-07-18T16:28:35Z

include/envoy/server/resource_monitor_config.h

+   * Create a particular resource monitor implementation.
+   * @param config const ProtoBuf::Message& supplies the config for the resource monitor
+   *        implementation.
+   * @param dispatcher Event::Dispatcher& the main thread's dispatcher. This is used by


Optional comment: I would expect that in the future we might need to add other things here beyond the dispatcher, such as cluster manager, TLS, etc. It might be better to just create some type of Factory interface like we do for the other types of plugins? That way it's easier to add additional members to pass in the future without having to change the interface everywhere.

Ok, adding back the ResourceMonitorFactoryContext I had originally... :)

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

eziskind added 2 commits July 12, 2018 10:33

add resource monitor framework

96cdb76

to be used by the overload manager (issue envoyproxy#373) Signed-off-by: Elisha Ziskind <eziskind@google.com>

Fixed asan/tsan unit test

f773b5f

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

htuch assigned mattklein123 and htuch Jul 12, 2018

htuch requested a review from mattklein123 July 12, 2018 16:58

htuch suggested changes Jul 13, 2018

View reviewed changes

lizan reviewed Jul 14, 2018

View reviewed changes

Address review feedback for resource monitor

e5e55d2

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

htuch reviewed Jul 17, 2018

View reviewed changes

Address some more review feedback

88e18af

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

mattklein123 reviewed Jul 18, 2018

View reviewed changes

eziskind added 2 commits July 18, 2018 14:29

Add back ResourceMonitorFactoryContext

bb6c083

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

clang-format fixes

167f077

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

htuch approved these changes Jul 18, 2018

View reviewed changes

mattklein123 approved these changes Jul 18, 2018

View reviewed changes

htuch merged commit 0e71582 into envoyproxy:master Jul 19, 2018

eziskind deleted the overload branch July 20, 2018 13:22

Conversation

eziskind commented Jul 12, 2018

Uh oh!

htuch 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!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants