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

extensions: add retry host predicate for attempted hosts #4452

Merged
merged 3 commits into from
Sep 23, 2018
Merged
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
2 changes: 1 addition & 1 deletion include/envoy/upstream/retry.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class RetryHostPredicateFactory {
virtual ~RetryHostPredicateFactory() {}

virtual void createHostPredicate(RetryHostPredicateFactoryCallbacks& callbacks,
const ProtobufWkt::Struct& config) PURE;
const Protobuf::Message& config) PURE;

/**
* @return name name of this factory.
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/extensions_build_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ EXTENSIONS = {
# TODO(lizan): switch to config target once a transport socket exists
"envoy.transport_sockets.alts": "//source/extensions/transport_sockets/alts:tsi_handshaker",
"envoy.transport_sockets.capture": "//source/extensions/transport_sockets/capture:config",

# Retry host predicates
"envoy.retry_host_predicates.other_hosts": "//source/extensions/retry/host/other_hosts:config",
}

WINDOWS_EXTENSIONS = {
Expand Down
17 changes: 17 additions & 0 deletions source/extensions/retry/host/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "well_known_names",
hdrs = ["well_known_names.h"],
deps = [
"//source/common/singleton:const_singleton",
],
)
29 changes: 29 additions & 0 deletions source/extensions/retry/host/other_hosts/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "other_hosts_predicate_lib",
hdrs = ["other_hosts.h"],
deps = [
"//include/envoy/upstream:retry_interface",
],
)

envoy_cc_library(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
deps = [
":other_hosts_predicate_lib",
"//include/envoy/registry",
"//include/envoy/upstream:retry_interface",
"//source/extensions/retry/host:well_known_names",
],
)
17 changes: 17 additions & 0 deletions source/extensions/retry/host/other_hosts/config.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#include "extensions/retry/host/other_hosts/config.h"

#include "envoy/registry/registry.h"
#include "envoy/upstream/retry.h"

namespace Envoy {
namespace Extensions {
namespace Retry {
namespace Host {

static Registry::RegisterFactory<OtherHostsRetryPredicateFactory,
Upstream::RetryHostPredicateFactory>
register_;
}
} // namespace Retry
} // namespace Extensions
} // namespace Envoy
26 changes: 26 additions & 0 deletions source/extensions/retry/host/other_hosts/config.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#pragma once

#include "envoy/upstream/retry.h"

#include "extensions/retry/host/other_hosts/other_hosts.h"
#include "extensions/retry/host/well_known_names.h"

namespace Envoy {
namespace Extensions {
namespace Retry {
namespace Host {

class OtherHostsRetryPredicateFactory : public Upstream::RetryHostPredicateFactory {
public:
void createHostPredicate(Upstream::RetryHostPredicateFactoryCallbacks& callbacks,
const Protobuf::Message&) override {
callbacks.addHostPredicate(std::make_shared<OtherHostsRetryPredicate>());
}

std::string name() override { return RetryHostPredicateValues::get().PreviousHostsPredicate; }
};

} // namespace Host
} // namespace Retry
} // namespace Extensions
} // namespace Envoy
19 changes: 19 additions & 0 deletions source/extensions/retry/host/other_hosts/other_hosts.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#pragma once

#include "envoy/upstream/retry.h"
#include "envoy/upstream/upstream.h"

namespace Envoy {
class OtherHostsRetryPredicate : public Upstream::RetryHostPredicate {
public:
bool shouldSelectAnotherHost(const Upstream::Host& candidate_host) override {
return attempted_hosts_.find(candidate_host.address()->asString()) != attempted_hosts_.end();
}
void onHostAttempted(Upstream::HostDescriptionConstSharedPtr attempted_host) override {
attempted_hosts_.insert(attempted_host->address()->asString());
}

private:
std::unordered_set<std::string> attempted_hosts_;
Copy link
Member

Choose a reason for hiding this comment

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

@snowp I'm not sure of my comment came through here or not: I suspect that a vector or fixed size array initialized to the number of max retries would end up being faster here and use less memory given the generally tiny number of allowed retries. Something to think about / TODO in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good idea. I think the only complication is having to plumb through the max retries count, but since other implementations might also find it useful for similar use cases it should probably be part of the factory interface.

};
} // namespace Envoy
24 changes: 24 additions & 0 deletions source/extensions/retry/host/well_known_names.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#pragma once

#include "common/singleton/const_singleton.h"

namespace Envoy {
namespace Extensions {
namespace Retry {
namespace Host {

/**
* Well-known retry host predicate names.
*/
class RetryHostPredicatesNameValues {
public:
// Previous host predicate. Rejects hosts that have already been tried.
const std::string PreviousHostsPredicate = "envoy.retry_host_predicates.previous_hosts";
};

typedef ConstSingleton<RetryHostPredicatesNameValues> RetryHostPredicateValues;

} // namespace Host
} // namespace Retry
} // namespace Extensions
} // namespace Envoy
22 changes: 22 additions & 0 deletions test/extensions/retry/host/other_hosts/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_package",
)
load(
"//test/extensions:extensions_build_system.bzl",
"envoy_extension_cc_test",
)

envoy_package()

envoy_extension_cc_test(
name = "config_test",
srcs = ["config_test.cc"],
extension_name = "envoy.retry_host_predicates.other_hosts",
deps = [
"//source/extensions/retry/host/other_hosts:config",
"//test/mocks/upstream:upstream_mocks",
],
)
65 changes: 65 additions & 0 deletions test/extensions/retry/host/other_hosts/config_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#include "envoy/registry/registry.h"
#include "envoy/upstream/retry.h"

#include "extensions/retry/host/other_hosts/config.h"
#include "extensions/retry/host/well_known_names.h"

#include "test/mocks/upstream/mocks.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"

using namespace testing;

namespace Envoy {
namespace Extensions {
namespace Retry {
namespace Host {

struct TestHostPredicateFactoryCallback : public Upstream::RetryHostPredicateFactoryCallbacks {

void addHostPredicate(Upstream::RetryHostPredicateSharedPtr host_predicate) {
host_predicate_ = host_predicate;
}

Upstream::RetryHostPredicateSharedPtr host_predicate_;
};

TEST(OtherHostsRetryPredicateConfigTest, PredicateTest) {
auto factory = Registry::FactoryRegistry<Upstream::RetryHostPredicateFactory>::getFactory(
RetryHostPredicateValues::get().PreviousHostsPredicate);

ASSERT_NE(nullptr, factory);

TestHostPredicateFactoryCallback callback;
ProtobufWkt::Struct config;
factory->createHostPredicate(callback, config);

auto predicate = callback.host_predicate_;

auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
auto host1_address = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1", 123);
ON_CALL(*host1, address()).WillByDefault(Return(host1_address));

auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
auto host2_address = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1", 456);
ON_CALL(*host2, address()).WillByDefault(Return(host2_address));

ASSERT_FALSE(predicate->shouldSelectAnotherHost(*host1));
ASSERT_FALSE(predicate->shouldSelectAnotherHost(*host2));

predicate->onHostAttempted(host1);

ASSERT_TRUE(predicate->shouldSelectAnotherHost(*host1));
ASSERT_FALSE(predicate->shouldSelectAnotherHost(*host2));

predicate->onHostAttempted(host2);

ASSERT_TRUE(predicate->shouldSelectAnotherHost(*host1));
ASSERT_TRUE(predicate->shouldSelectAnotherHost(*host2));
}

} // namespace Host
} // namespace Retry
} // namespace Extensions
} // namespace Envoy
2 changes: 1 addition & 1 deletion test/integration/test_host_predicate_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class TestHostPredicateFactory : public Upstream::RetryHostPredicateFactory {
std::string name() override { return "envoy.test_host_predicate"; }

void createHostPredicate(Upstream::RetryHostPredicateFactoryCallbacks& callbacks,
const ProtobufWkt::Struct&) override {
const Protobuf::Message&) override {
callbacks.addHostPredicate(std::make_shared<TestHostPredicate>());
}
};
Expand Down