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 1 commit
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
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 ProtobufWkt::Struct&) override {
Copy link
Member

Choose a reason for hiding this comment

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

@htuch I think I was too late to point out this in #4385, we should avoid leaking Struct to extension interfaces? This is similar to #3155, and given we are already facing some config performance issue so it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Let's continue this discussion in #4475. We do want to move in this direction, but we want to maintain a consistent and unsurprising API surface for extensions.

Copy link
Member

Choose a reason for hiding this comment

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

@snowp is this the right PR to followup on #4475 tactical changes or should we review as is and you'll do that in a followup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to do in a follow up, that way I can do both extensions at the same time without cluttering this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, considering #4385 has still not been reviewed (and this introduces the other Struct usage) I think I'll just do it in each PR separately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it's more elaborate than I thought - I'll update this PR to use MessagePtr instead of Struct and then follow up with another PR setting up a base factory to convert that into a config proto. This PR isn't relying on parsing a config proto, so I think it can be merged without having all that set up

Sorry for being so indecisive!

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