Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Overload] Active downstream connections resource monitor #19186

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ extensions/filters/common/original_src @snowp @klarose
/*/extensions/resource_monitors/injected_resource @eziskind @htuch
/*/extensions/resource_monitors/common @eziskind @htuch
/*/extensions/resource_monitors/fixed_heap @eziskind @htuch
/*/extensions/resource_monitors/downstream_connections @antoniovicente @mattklein123 @nezdolik
/*/extensions/retry/priority @snowp @alyssawilk
/*/extensions/retry/priority/previous_priorities @snowp @alyssawilk
/*/extensions/retry/host @snowp @alyssawilk
Expand Down
1 change: 1 addition & 0 deletions api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ proto_library(
"//envoy/extensions/rate_limit_descriptors/expr/v3:pkg",
"//envoy/extensions/rbac/matchers/upstream_ip_port/v3:pkg",
"//envoy/extensions/request_id/uuid/v3:pkg",
"//envoy/extensions/resource_monitors/downstream_connections/v3:pkg",
"//envoy/extensions/resource_monitors/fixed_heap/v3:pkg",
"//envoy/extensions/resource_monitors/injected_resource/v3:pkg",
"//envoy/extensions/retry/host/omit_canary_hosts/v3:pkg",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# DO NOT EDIT. This file is generated by tools/proto_format/proto_sync.py.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package(
deps = ["@com_github_cncf_udpa//udpa/annotations:pkg"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
syntax = "proto3";

package envoy.extensions.resource_monitors.downstream_connections.v3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like is not possible to specify any other version for extension than v3 according to api style guide for extensions. I've marked entire proto file for this extension as work in progress and hid it from the docs (until the third PR lands in that would make use of this new monitor in tcp listener)


import "udpa/annotations/status.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.extensions.resource_monitors.downstream_connections.v3";
option java_outer_classname = "DownstreamConnectionsProto";
option java_multiple_files = true;
option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/resource_monitors/downstream_connections/v3;downstream_connectionsv3";
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: Downstream connections]
// [#extension: envoy.resource_monitors.downstream_connections]

// The downstream connections resource monitor tracks the global number of open downstream connections.
// [#not-implemented-hide:]
message DownstreamConnectionsConfig {
// Maximum threshold for global open active downstream connections, defaults to 0.
// If monitor is configured via Overload manager api and has no value set, Envoy will reject all incoming connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically with the gt annotation that you added Envoy will reject the configuration as your test https://github.com/envoyproxy/envoy/pull/19186/files#diff-1fdfe762c6263d74ffc5365cd93a44ff14c64adac898bbf1e2de53a3a91e000bR61 shows.

Is this expected to be >= 0 or > 0?

"If monitor is configured via Overload manager api and has no value set, Envoy will reject all incoming connections."

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is up for discussion. Having it '>0' would require users to explicitly configure threshold for monitor (and fail/error on startup otherwise if threshold is not configured). While '>=0' can be more tricky for users, if they forget to configure the threshold, monitor will use default value 0 and reject all incoming connections. First option (require explicit value > 0) is cleaner in my opinion, although monitor will not be able to reject all incoming connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

int64 max_active_downstream_connections = 1 [(validate.rules).int64 = {gt: 0}];
}
1 change: 1 addition & 0 deletions api/versioning/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ proto_library(
"//envoy/extensions/rate_limit_descriptors/expr/v3:pkg",
"//envoy/extensions/rbac/matchers/upstream_ip_port/v3:pkg",
"//envoy/extensions/request_id/uuid/v3:pkg",
"//envoy/extensions/resource_monitors/downstream_connections/v3:pkg",
"//envoy/extensions/resource_monitors/fixed_heap/v3:pkg",
"//envoy/extensions/resource_monitors/injected_resource/v3:pkg",
"//envoy/extensions/retry/host/omit_canary_hosts/v3:pkg",
Expand Down
1 change: 1 addition & 0 deletions source/extensions/extensions_build_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ EXTENSIONS = {

"envoy.resource_monitors.fixed_heap": "//source/extensions/resource_monitors/fixed_heap:config",
"envoy.resource_monitors.injected_resource": "//source/extensions/resource_monitors/injected_resource:config",
"envoy.resource_monitors.downstream_connections": "//source/extensions/resource_monitors/downstream_connections:config",

#
# Stat sinks
Expand Down
5 changes: 5 additions & 0 deletions source/extensions/extensions_metadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,11 @@ envoy.request_id.uuid:
- envoy.request_id
security_posture: robust_to_untrusted_downstream_and_upstream
status: stable
envoy.resource_monitors.downstream_connections:
categories:
- envoy.resource_monitors
security_posture: data_plane_agnostic
status: alpha
envoy.resource_monitors.fixed_heap:
categories:
- envoy.resource_monitors
Expand Down
29 changes: 29 additions & 0 deletions source/extensions/resource_monitors/common/factory_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,35 @@ class FactoryBase : public Server::Configuration::ResourceMonitorFactory {
const std::string name_;
};

template <class ConfigProto>
class ProactiveFactoryBase : public Server::Configuration::ProactiveResourceMonitorFactory {
public:
Server::ProactiveResourceMonitorPtr createProactiveResourceMonitor(
const Protobuf::Message& config,
Server::Configuration::ResourceMonitorFactoryContext& context) override {
return createProactiveResourceMonitorFromProtoTyped(
MessageUtil::downcastAndValidate<const ConfigProto&>(config,
context.messageValidationVisitor()),
context);
}

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return std::make_unique<ConfigProto>();
}

std::string name() const override { return name_; }

protected:
ProactiveFactoryBase(const std::string& name) : name_(name) {}

private:
virtual Server::ProactiveResourceMonitorPtr createProactiveResourceMonitorFromProtoTyped(
const ConfigProto& config,
Server::Configuration::ResourceMonitorFactoryContext& context) PURE;

const std::string name_;
};

} // namespace Common
} // namespace ResourceMonitors
} // namespace Extensions
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_extension",
"envoy_cc_library",
"envoy_extension_package",
)

licenses(["notice"]) # Apache 2

envoy_extension_package()

envoy_cc_library(
name = "downstream_connections_monitor",
srcs = ["downstream_connections_monitor.cc"],
hdrs = ["downstream_connections_monitor.h"],
deps = [
"//envoy/server:proactive_resource_monitor_interface",
"//envoy/server:resource_monitor_config_interface",
"//source/common/common:assert_lib",
"@envoy_api//envoy/extensions/resource_monitors/downstream_connections/v3:pkg_cc_proto",
],
)

envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
deps = [
":downstream_connections_monitor",
"//envoy/registry",
"//source/common/common:assert_lib",
"//source/extensions/resource_monitors/common:factory_base_lib",
"@envoy_api//envoy/extensions/resource_monitors/downstream_connections/v3:pkg_cc_proto",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include "source/extensions/resource_monitors/downstream_connections/config.h"

#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.h"
#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.validate.h"
#include "envoy/registry/registry.h"

#include "source/common/protobuf/utility.h"
#include "source/extensions/resource_monitors/downstream_connections/downstream_connections_monitor.h"

namespace Envoy {
namespace Extensions {
namespace ResourceMonitors {
namespace DownstreamConnections {

Server::ProactiveResourceMonitorPtr
ActiveDownstreamConnectionsMonitorFactory::createProactiveResourceMonitorFromProtoTyped(
const envoy::extensions::resource_monitors::downstream_connections::v3::
DownstreamConnectionsConfig& config,
Server::Configuration::ResourceMonitorFactoryContext& /*unused_context*/) {
return std::make_unique<ActiveDownstreamConnectionsResourceMonitor>(config);
}

/**
* Static registration for the downstream connections resource monitor factory. @see
* RegistryFactory.
*/
REGISTER_FACTORY(ActiveDownstreamConnectionsMonitorFactory,
Server::Configuration::ProactiveResourceMonitorFactory);

} // namespace DownstreamConnections
} // namespace ResourceMonitors
} // namespace Extensions
} // namespace Envoy
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#pragma once

#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.h"
#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.validate.h"
#include "envoy/server/resource_monitor_config.h"

#include "source/extensions/resource_monitors/common/factory_base.h"

namespace Envoy {
namespace Extensions {
namespace ResourceMonitors {
namespace DownstreamConnections {

class ActiveDownstreamConnectionsMonitorFactory
: public Common::ProactiveFactoryBase<
envoy::extensions::resource_monitors::downstream_connections::v3::
DownstreamConnectionsConfig> {
public:
ActiveDownstreamConnectionsMonitorFactory()
: ProactiveFactoryBase("envoy.resource_monitors.downstream_connections") {}

private:
Server::ProactiveResourceMonitorPtr createProactiveResourceMonitorFromProtoTyped(
const envoy::extensions::resource_monitors::downstream_connections::v3::
DownstreamConnectionsConfig& config,
Server::Configuration::ResourceMonitorFactoryContext& context) override;
};

} // namespace DownstreamConnections
} // namespace ResourceMonitors
} // namespace Extensions
} // namespace Envoy
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "source/extensions/resource_monitors/downstream_connections/downstream_connections_monitor.h"

#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.h"

#include "source/common/common/assert.h"

namespace Envoy {
namespace Extensions {
namespace ResourceMonitors {
namespace DownstreamConnections {

ActiveDownstreamConnectionsResourceMonitor::ActiveDownstreamConnectionsResourceMonitor(
const envoy::extensions::resource_monitors::downstream_connections::v3::
DownstreamConnectionsConfig& config)
: max_(config.max_active_downstream_connections()), current_(0){};

bool ActiveDownstreamConnectionsResourceMonitor::tryAllocateResource(int64_t increment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In real use cases can tryAllocateResources and tryDeallocateResources be called concurrently?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, they can be called for example from multiple worker threads

Copy link
Contributor

Choose a reason for hiding this comment

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

Do think using RAII for the resource allocated / decremented is probably the way to go to stop incorrect from being possible and leaking resources.

For example if max = 15, and originally we have current=10 with some bad calls and threads swapping I think we can up with strange results:

  1. Call to tryAlloc(10) that stops at line 20 (doesn't yet read current for the decrement)
  2. Call to deAlloc(20) => current is still 20, so we decrement and store 0 into current
  3. The first call reads current (now 0) and decrements 10, giving us current or -10 :(

Copy link
Member Author

@nezdolik nezdolik Feb 18, 2022

Choose a reason for hiding this comment

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

Realised that RAII/Memento approach will not work with cross thread visibility. Various threads will be accessing tryAlloc/deAlloc resource via their thread local overload state object. The point of using atomic counter was to bypass periodic slow flushes/updates in OM (where thread local overload state is periodically updated for all threads) and instead perform faster checks from any thread relying on atomic guarantees for cross thread visibility of latest counter value.
@KBaichoo wdyt?

int64_t new_val = (current_ += increment);
if (new_val > max_ || new_val < 0) {
current_ -= increment;
return false;
}
return true;
}

bool ActiveDownstreamConnectionsResourceMonitor::tryDeallocateResource(int64_t decrement) {
RELEASE_ASSERT(decrement <= current_,
"Cannot deallocate resource, current resource usage is lower than decrement");
int64_t new_val = (current_ -= decrement);
if (new_val < 0) {
current_ += decrement;
return false;
}
return true;
}

int64_t ActiveDownstreamConnectionsResourceMonitor::currentResourceUsage() const {
return current_.load();
}
int64_t ActiveDownstreamConnectionsResourceMonitor::maxResourceUsage() const { return max_; };

} // namespace DownstreamConnections
} // namespace ResourceMonitors
} // namespace Extensions
} // namespace Envoy
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#pragma once

#include "envoy/extensions/resource_monitors/downstream_connections/v3/downstream_connections.pb.h"
#include "envoy/server/proactive_resource_monitor.h"

#include "source/common/common/assert.h"

namespace Envoy {
namespace Extensions {
namespace ResourceMonitors {
namespace DownstreamConnections {

class ActiveDownstreamConnectionsResourceMonitor : public Server::ProactiveResourceMonitor {
public:
ActiveDownstreamConnectionsResourceMonitor(
const envoy::extensions::resource_monitors::downstream_connections::v3::
DownstreamConnectionsConfig& config);

bool tryAllocateResource(int64_t increment) override;

bool tryDeallocateResource(int64_t decrement) override;

int64_t currentResourceUsage() const override;
int64_t maxResourceUsage() const override;

protected:
int64_t max_;
std::atomic<int64_t> current_;
};

} // namespace DownstreamConnections
} // namespace ResourceMonitors
} // namespace Extensions
} // namespace Envoy
38 changes: 38 additions & 0 deletions test/extensions/resource_monitors/downstream_connections/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_package",
)
load(
"//test/extensions:extensions_build_system.bzl",
"envoy_extension_cc_test",
)

licenses(["notice"]) # Apache 2

envoy_package()

envoy_extension_cc_test(
name = "downstream_connections_monitor_test",
srcs = ["downstream_connections_monitor_test.cc"],
extension_names = ["envoy.resource_monitors.downstream_connections"],
external_deps = ["abseil_optional"],
deps = [
"//source/extensions/resource_monitors/downstream_connections:downstream_connections_monitor",
"@envoy_api//envoy/extensions/resource_monitors/downstream_connections/v3:pkg_cc_proto",
],
)

envoy_extension_cc_test(
name = "config_test",
srcs = ["config_test.cc"],
extension_names = ["envoy.resource_monitors.downstream_connections"],
deps = [
"//envoy/registry",
"//source/extensions/resource_monitors/downstream_connections:config",
"//source/server:resource_monitor_config_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/server:options_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/extensions/resource_monitors/downstream_connections/v3:pkg_cc_proto",
],
)
Loading