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

Buffer Factory: Configuration of the minimum account size to track. #17562

Merged
merged 11 commits into from
Aug 12, 2021
23 changes: 23 additions & 0 deletions api/envoy/config/overload/v3/overload.proto
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,26 @@ message OverloadAction {
google.protobuf.Any typed_config = 3;
}

// Configuration for which accounts the WatermarkBuffer Factories should
// track.
message BufferFactoryConfig {
// The minimum power of two at which Envoy starts tracking an account.
//
// Envoy has 8 power of two buckets starting with the provided exponent below.
// Concretely the 1st bucket contains accounts for streams that use
// [2^minimum_account_to_track_power_of_two,
// 2^(minimum_account_to_track_power_of_two + 1)) bytes.
// With the 8th bucket tracking accounts
// >= 128 * 2^minimum_account_to_track_power_of_two.
//
// The maximum value is 56, since we're using uint64_t for bytes counting,
// and that's the last value that would use the 8 buckets. In practice,
// we don't expect the proxy to be holding 2^56 bytes.
//
// If omitted, Envoy should not do any tracking.
uint32 minimum_account_to_track_power_of_two = 1 [(validate.rules).uint32 = {lte: 56 gte: 10}];
}

message OverloadManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.overload.v2alpha.OverloadManager";
Expand All @@ -153,4 +173,7 @@ message OverloadManager {

// The set of overload actions.
repeated OverloadAction actions = 3;

// Configuration for buffer factory.
BufferFactoryConfig buffer_factory_config = 4;
}
1 change: 1 addition & 0 deletions envoy/api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ envoy_cc_library(
"//envoy/filesystem:filesystem_interface",
"//envoy/server:process_context_interface",
"//envoy/thread:thread_interface",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
],
)

Expand Down
6 changes: 6 additions & 0 deletions envoy/api/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "envoy/common/random_generator.h"
#include "envoy/common/time.h"
#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/event/dispatcher.h"
#include "envoy/event/scaled_range_timer_manager.h"
#include "envoy/filesystem/filesystem.h"
Expand Down Expand Up @@ -83,6 +84,11 @@ class Api {
* @return an optional reference to the ProcessContext
*/
virtual ProcessContextOptRef processContext() PURE;

/**
* @return the bootstrap Envoy started with.
*/
virtual const envoy::config::bootstrap::v3::Bootstrap& bootstrap() const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think dispatchers are always allocated through the Api interface. In this case you do not need this method. Just save the bootstrap in the ApiImpl and then pass buffer config to the dispatcher constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dispatchers are created outside of the Api interface in certain places.

Given that the Api has the information, and is already plumbed through the 3 ctors of the dispatcher, avoiding telescoping ctor by having the object with the information expose it seemed like the way to go.

};

using ApiPtr = std::unique_ptr<Api>;
Expand Down
23 changes: 23 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.

1 change: 1 addition & 0 deletions source/common/api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ envoy_cc_library(
"//source/common/common:thread_lib",
"//source/common/event:dispatcher_lib",
"//source/common/network:socket_lib",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
],
)

Expand Down
8 changes: 6 additions & 2 deletions source/common/api/api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <chrono>
#include <string>

#include "envoy/config/bootstrap/v3/bootstrap.pb.h"

#include "source/common/common/thread.h"
#include "source/common/event/dispatcher_impl.h"

Expand All @@ -11,10 +13,12 @@ namespace Api {

Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store,
Event::TimeSystem& time_system, Filesystem::Instance& file_system,
Random::RandomGenerator& random_generator, const ProcessContextOptRef& process_context,
Random::RandomGenerator& random_generator,
const envoy::config::bootstrap::v3::Bootstrap& bootstrap,
const ProcessContextOptRef& process_context,
Buffer::WatermarkFactorySharedPtr watermark_factory)
: thread_factory_(thread_factory), store_(store), time_system_(time_system),
file_system_(file_system), random_generator_(random_generator),
file_system_(file_system), random_generator_(random_generator), bootstrap_(bootstrap),
process_context_(process_context), watermark_factory_(std::move(watermark_factory)) {}

Event::DispatcherPtr Impl::allocateDispatcher(const std::string& name) {
Expand Down
4 changes: 4 additions & 0 deletions source/common/api/api_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <string>

#include "envoy/api/api.h"
#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/event/timer.h"
#include "envoy/filesystem/filesystem.h"
#include "envoy/network/socket.h"
Expand All @@ -19,6 +20,7 @@ class Impl : public Api {
public:
Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store, Event::TimeSystem& time_system,
Filesystem::Instance& file_system, Random::RandomGenerator& random_generator,
const envoy::config::bootstrap::v3::Bootstrap& bootstrap,
const ProcessContextOptRef& process_context = absl::nullopt,
Buffer::WatermarkFactorySharedPtr watermark_factory = nullptr);

Expand All @@ -34,6 +36,7 @@ class Impl : public Api {
TimeSource& timeSource() override { return time_system_; }
Stats::Scope& rootScope() override { return store_; }
Random::RandomGenerator& randomGenerator() override { return random_generator_; }
const envoy::config::bootstrap::v3::Bootstrap& bootstrap() const override { return bootstrap_; }
ProcessContextOptRef processContext() override { return process_context_; }

private:
Expand All @@ -42,6 +45,7 @@ class Impl : public Api {
Event::TimeSystem& time_system_;
Filesystem::Instance& file_system_;
Random::RandomGenerator& random_generator_;
const envoy::config::bootstrap::v3::Bootstrap& bootstrap_;
ProcessContextOptRef process_context_;
const Buffer::WatermarkFactorySharedPtr watermark_factory_;
};
Expand Down
1 change: 1 addition & 0 deletions source/common/buffer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ envoy_cc_library(
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/runtime:runtime_features_lib",
"@envoy_api//envoy/config/overload/v3:pkg_cc_proto",
],
)

Expand Down
15 changes: 13 additions & 2 deletions source/common/buffer/watermark_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@

namespace Envoy {
namespace Buffer {
namespace {
// Effectively disables tracking as this should zero out all reasonable account
// balances when shifted by this amount.
constexpr uint32_t kEffectivelyDisableTrackingBitshift = 63;
} // end namespace

void WatermarkBuffer::add(const void* data, uint64_t size) {
OwnedImpl::add(data, size);
Expand Down Expand Up @@ -174,6 +179,12 @@ void WatermarkBufferFactory::unregisterAccount(const BufferMemoryAccountSharedPt
}
}

WatermarkBufferFactory::WatermarkBufferFactory(
const envoy::config::overload::v3::BufferFactoryConfig& config)
: bitshift_(config.minimum_account_to_track_power_of_two()
? config.minimum_account_to_track_power_of_two() - 1
: kEffectivelyDisableTrackingBitshift) {}

WatermarkBufferFactory::~WatermarkBufferFactory() {
for (auto& account_set : size_class_account_sets_) {
ASSERT(account_set.empty(),
Expand All @@ -195,10 +206,10 @@ BufferMemoryAccountImpl::createAccount(WatermarkBufferFactory* factory,
}

int BufferMemoryAccountImpl::balanceToClassIndex() {
const uint64_t shifted_balance = buffer_memory_allocated_ >> 20; // shift by 1MB.
const uint64_t shifted_balance = buffer_memory_allocated_ >> factory_->bitshift();

if (shifted_balance == 0) {
return -1; // Not worth tracking anything < 1MB.
return -1; // Not worth tracking anything < configured minimum threshold
}

const int class_idx = absl::bit_width(shifted_balance) - 1;
Expand Down
8 changes: 8 additions & 0 deletions source/common/buffer/watermark_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "envoy/buffer/buffer.h"
#include "envoy/common/optref.h"
#include "envoy/config/overload/v3/overload.pb.h"

#include "source/common/buffer/buffer_impl.h"

Expand Down Expand Up @@ -182,6 +183,8 @@ class BufferMemoryAccountImpl : public BufferMemoryAccount {
*/
class WatermarkBufferFactory : public WatermarkFactory {
public:
WatermarkBufferFactory(const envoy::config::overload::v3::BufferFactoryConfig& config);

// Buffer::WatermarkFactory
~WatermarkBufferFactory() override;
InstancePtr createBuffer(std::function<void()> below_low_watermark,
Expand All @@ -198,6 +201,8 @@ class WatermarkBufferFactory : public WatermarkFactory {
void updateAccountClass(const BufferMemoryAccountSharedPtr& account, int current_class,
int new_class);

uint32_t bitshift() const { return bitshift_; }

// Unregister a buffer memory account.
virtual void unregisterAccount(const BufferMemoryAccountSharedPtr& account, int current_class);

Expand All @@ -206,6 +211,9 @@ class WatermarkBufferFactory : public WatermarkFactory {
using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>,
BufferMemoryAccountImpl::NUM_MEMORY_CLASSES_>;
MemoryClassesToAccountsSet size_class_account_sets_;
// How much to bit shift right balances to test whether the account should be
// tracked in *size_class_account_sets_*.
const uint32_t bitshift_;
};

} // namespace Buffer
Expand Down
1 change: 1 addition & 0 deletions source/common/event/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ envoy_cc_library(
"//source/common/network:dns_lib",
"//source/common/network:connection_lib",
"//source/common/network:listener_lib",
"@envoy_api//envoy/config/overload/v3:pkg_cc_proto",
] + select({
"//bazel:apple": ["//source/common/network:apple_dns_lib"],
"//conditions:default": [],
Expand Down
4 changes: 3 additions & 1 deletion source/common/event/dispatcher_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "envoy/api/api.h"
#include "envoy/common/scope_tracker.h"
#include "envoy/config/overload/v3/overload.pb.h"
#include "envoy/network/listen_socket.h"
#include "envoy/network/listener.h"

Expand Down Expand Up @@ -61,7 +62,8 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api,
: name_(name), api_(api),
buffer_factory_(watermark_factory != nullptr
? watermark_factory
: std::make_shared<Buffer::WatermarkBufferFactory>()),
: std::make_shared<Buffer::WatermarkBufferFactory>(
api.bootstrap().overload_manager().buffer_factory_config())),
scheduler_(time_system.createScheduler(base_scheduler_, base_scheduler_)),
thread_local_delete_cb_(
base_scheduler_.createSchedulableCallback([this]() -> void { runThreadLocalDelete(); })),
Expand Down
1 change: 1 addition & 0 deletions source/server/config_validation/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ envoy_cc_library(
"//envoy/api:api_interface",
"//envoy/filesystem:filesystem_interface",
"//source/common/api:api_lib",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
],
)

Expand Down
5 changes: 3 additions & 2 deletions source/server/config_validation/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ namespace Api {

ValidationImpl::ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store,
Event::TimeSystem& time_system, Filesystem::Instance& file_system,
Random::RandomGenerator& random_generator)
: Impl(thread_factory, stats_store, time_system, file_system, random_generator),
Random::RandomGenerator& random_generator,
const envoy::config::bootstrap::v3::Bootstrap& bootstrap)
: Impl(thread_factory, stats_store, time_system, file_system, random_generator, bootstrap),
time_system_(time_system) {}

Event::DispatcherPtr ValidationImpl::allocateDispatcher(const std::string& name) {
Expand Down
4 changes: 3 additions & 1 deletion source/server/config_validation/api.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/api/api.h"
#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/event/timer.h"
#include "envoy/filesystem/filesystem.h"

Expand All @@ -17,7 +18,8 @@ class ValidationImpl : public Impl {
public:
ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store,
Event::TimeSystem& time_system, Filesystem::Instance& file_system,
Random::RandomGenerator& random_generator);
Random::RandomGenerator& random_generator,
const envoy::config::bootstrap::v3::Bootstrap& bootstrap);

Event::DispatcherPtr allocateDispatcher(const std::string& name) override;
Event::DispatcherPtr allocateDispatcher(const std::string& name,
Expand Down
5 changes: 3 additions & 2 deletions source/server/config_validation/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ ValidationInstance::ValidationInstance(
: options_(options), validation_context_(options_.allowUnknownStaticFields(),
!options.rejectUnknownDynamicFields(),
!options.ignoreUnknownDynamicFields()),
stats_store_(store), api_(new Api::ValidationImpl(thread_factory, store, time_system,
file_system, random_generator_)),
stats_store_(store),
api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system,
random_generator_, bootstrap_)),
dispatcher_(api_->allocateDispatcher("main_thread")),
singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())),
access_log_manager_(options.fileFlushIntervalMsec(), *api_, *dispatcher_, access_log_lock,
Expand Down
3 changes: 2 additions & 1 deletion source/server/config_validation/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <iostream>

#include "envoy/config/bootstrap/v3/bootstrap.pb.h"
#include "envoy/config/core/v3/config_source.pb.h"
#include "envoy/config/listener/v3/listener.pb.h"
#include "envoy/config/listener/v3/listener_components.pb.h"
Expand Down Expand Up @@ -195,11 +196,11 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
ProtobufMessage::ProdValidationContextImpl validation_context_;
Stats::IsolatedStoreImpl& stats_store_;
ThreadLocal::InstanceImpl thread_local_;
envoy::config::bootstrap::v3::Bootstrap bootstrap_;
Api::ApiPtr api_;
Event::DispatcherPtr dispatcher_;
std::unique_ptr<Server::ValidationAdmin> admin_;
Singleton::ManagerPtr singleton_manager_;
envoy::config::bootstrap::v3::Bootstrap bootstrap_;
std::unique_ptr<Runtime::ScopedLoaderSingleton> runtime_singleton_;
Random::RandomGeneratorImpl random_generator_;
std::unique_ptr<Ssl::ContextManager> ssl_context_manager_;
Expand Down
8 changes: 4 additions & 4 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ InstanceImpl::InstanceImpl(
time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)),
original_start_time_(start_time_), stats_store_(store), thread_local_(tls),
random_generator_(std::move(random_generator)),
api_(new Api::Impl(thread_factory, store, time_system, file_system, *random_generator_,
process_context ? ProcessContextOptRef(std::ref(*process_context))
: absl::nullopt,
watermark_factory)),
api_(new Api::Impl(
thread_factory, store, time_system, file_system, *random_generator_, bootstrap_,
process_context ? ProcessContextOptRef(std::ref(*process_context)) : absl::nullopt,
watermark_factory)),
dispatcher_(api_->allocateDispatcher("main_thread")),
singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())),
handler_(new ConnectionHandlerImpl(*dispatcher_, absl::nullopt)),
Expand Down
2 changes: 1 addition & 1 deletion source/server/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
Assert::ActionRegistrationPtr envoy_bug_action_registration_;
ThreadLocal::Instance& thread_local_;
Random::RandomGeneratorPtr random_generator_;
envoy::config::bootstrap::v3::Bootstrap bootstrap_;
Api::ApiPtr api_;
Event::DispatcherPtr dispatcher_;
std::unique_ptr<AdminImpl> admin_;
Expand All @@ -367,7 +368,6 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
std::unique_ptr<Server::GuardDog> worker_guard_dog_;
bool terminated_;
std::unique_ptr<Logger::FileSinkDelegate> file_logger_;
envoy::config::bootstrap::v3::Bootstrap bootstrap_;
ConfigTracker::EntryOwnerPtr config_tracker_entry_;
SystemTime bootstrap_config_update_time_;
Grpc::AsyncClientManagerPtr async_client_manager_;
Expand Down
1 change: 1 addition & 0 deletions test/common/buffer/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ envoy_cc_test(
"//test/integration:tracked_watermark_buffer_lib",
"//test/mocks/buffer:buffer_mocks",
"//test/mocks/http:stream_reset_handler_mock",
"@envoy_api//envoy/config/overload/v3:pkg_cc_proto",
],
)

Expand Down
Loading