extenstions: retry predicate - omit_canary_hosts #7230

sriduth · 2019-06-11T07:05:58Z

Description: Created a new retry predicate that will reject canary hosts when selecting a host to retry a failure.
Risk Level: Low
Testing: Unit Tests added, manual testing done (1 canary host, 1 non canary host)
Docs Changes: Added brief description in docs/root/intro/arch_overview/http_connection_management.rst
Release Notes: N/A
[Optional Fixes #Issue] Fixes #7203
[Optional Deprecated:]

snowp

Thanks for working on this! This seems like exactly what I was thinking, just have some comments about style/conventions.

You'll also want to fix DCO (https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco) and format the files using tools/check_format.py fix.

snowp · 2019-06-11T15:25:15Z

docs/root/intro/arch_overview/http_connection_management.rst

@@ -60,6 +60,9 @@ can be used to modify this behavior, and they fall into two categories:
  * *envoy.retry_host_predicates.previous_hosts*: This will keep track of previously attempted hosts, and rejects
    hosts that have already been attempted.

+  * *community.retry_host_predicates.omit_canary_hosts*: This will ensure that a retry will always be done on


Since this extension lives in the envoy repo we'll want to prefix the name with envoy like all the other extensions

snowp · 2019-06-11T15:26:19Z

source/extensions/retry/host/omit_canary_hosts/config.h

+  std::string name() override { return RetryHostPredicateValues::get().OmitCanaryHostsPredicate; }
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};


Use std::make_unique instead of explicit new

Sure! I got started on this PR by copying the sources of the previous_hosts extensions, can I make this change there also?

Sure, I wouldn't mind the cleanup

snowp · 2019-06-11T15:26:42Z

source/extensions/retry/host/omit_canary_hosts/omit_canary_hosts.h

+namespace Envoy {
+  class OmitCanaryHostsRetryPredicate : public Upstream::RetryHostPredicate {
+  public:
+    OmitCanaryHostsRetryPredicate() {}


this does nothing, you can just remove it

…nvoyproxy#7203) In case a user wants to deploy a new change, this can help in cases where any failure in the canary downstream is retried in a stable downstream. Signed-off-by: Sriduth Jayhari <[email protected]>

…anary_hosts` retry predicate in docs/root/intro/arch_overview/http_connection_management.rst Signed-off-by: Sriduth Jayhari <[email protected]>

source/extensions/retry/host/well_known_names.h

source/extensions/retry/host/previous_hosts/config.h

* Format code * Remove empty OmitCanaryHostsRetryPredicate constructor * Cleanup: Use std::unique_ptr to make empty proto config for all host based predicates * Fix extenstion name in unit test and well_known_names.h Signed-off-by: Sriduth Jayhari <[email protected]>

snowp

Nice this looks pretty good. Just a few minor comments. Could you add a release note for this as well?

source/extensions/retry/host/omit_canary_hosts/config.h

docs/root/intro/arch_overview/http_connection_management.rst

* Added release note * Improve documentation * return unique_ptr of Envoy::ProtobufWkt::Empty instead of wrapping with a message ptr in createEmptyProto (also added for previous hosts predicate) Signed-off-by: Sriduth Jayhari <[email protected]>

snowp · 2019-06-18T14:30:57Z

This looks good to me. Could you merge master and see if CI is okay? You can also move this out of Draft mode at this point.

snowp · 2019-06-18T19:55:35Z

Seems like there's a CI failure

/wait

sriduth · 2019-06-19T02:16:28Z

@snowp , running tools/check_format.py fix has touched a lot of other files as well. I have clang-format version 7.0.1 (tags/RELEASE_701/final) and buildifier from eb1a85ca787f0f5f94ba66f41ee66fdfd4c49b70 (tag: 0.26.0). Am I doing something wrong?

Should I revert the last 2 commits (format fix and master merge)?

snowp · 2019-06-19T16:27:04Z

Yeah please avoid changing files not related to this change. I tend to run the format script just on the files I've changed in a PR instead of on the whole repo to avoid this kinda stuff from happening.

Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth · 2019-06-19T17:18:47Z

I should have checked the help section of the check_format.py script - my bad!

I've run the following and no diff is produced:

tools/check_format.py fix source/extensions/retry/host/omit_canary_hosts
tools/check_format.py fix source/extensions/retry/host
tools/check_format.py fix test/extensions/retry/host
tools/check_format.py fix test/extensions/retry/host/omit_canary_hosts

git diff master --name-only is now:

docs/root/intro/arch_overview/http/http_connection_management.rst
docs/root/intro/version_history.rst
source/extensions/extensions_build_config.bzl
source/extensions/retry/host/omit_canary_hosts/BUILD
source/extensions/retry/host/omit_canary_hosts/config.cc
source/extensions/retry/host/omit_canary_hosts/config.h
source/extensions/retry/host/omit_canary_hosts/omit_canary_hosts.h
source/extensions/retry/host/previous_hosts/config.h
source/extensions/retry/host/well_known_names.h
test/extensions/retry/host/omit_canary_hosts/BUILD
test/extensions/retry/host/omit_canary_hosts/config_test.cc

sriduth · 2019-06-20T12:28:36Z

Should I add this to the CODEOWNERS file?

ERROR: New directory extensions/retry/host/omit_canary_hosts appears to not have owners in CODEOWNERS
ERROR: check format failed. run 'tools/check_format.py fix'

snowp · 2019-06-20T16:10:18Z

Yeah, please add yourself and me as the code owners.

Signed-off-by: Sriduth Jayhari <[email protected]>

snowp

Thanks this is looking really good, just a couple nits.

docs/root/intro/arch_overview/http/http_connection_management.rst

docs/root/intro/version_history.rst

* Remove unrequired empty line in documentation for the predicate in http_connection_management.rst * Improve wording and formatting that describes change in version_history.rst Signed-off-by: Sriduth Jayhari <[email protected]>

snowp

LGTM, thanks!

…oyproxy#7230 Signed-off-by: Sriduth Jayhari <[email protected]>

… (#7394) Signed-off-by: Sriduth Jayhari <[email protected]>

mcasper · 2019-07-01T20:17:44Z

It looks like this feature was added to the version 1.10 release notes, was that a mistake?

sriduth · 2019-07-02T04:05:34Z

@mcasper you are right! i created the feature branch off the v.1.10.0 commit. This is bad - I'll open a PR to fix this right away.

…r v1.11 version history. Signed-off-by: Sriduth Jayhari <[email protected]>

…rsion (#7444) history. Signed-off-by: Sriduth Jayhari <[email protected]>

mattklein123 assigned snowp Jun 11, 2019

snowp suggested changes Jun 11, 2019

View reviewed changes

sriduth added 2 commits June 13, 2019 10:48

Updated documentation for the `community.retry_host_predicates.omit_c…

3f6ac41

…anary_hosts` retry predicate in docs/root/intro/arch_overview/http_connection_management.rst Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth commented Jun 13, 2019

View reviewed changes

source/extensions/retry/host/well_known_names.h Outdated Show resolved Hide resolved

sriduth force-pushed the feature/retry-on-non-canary-host branch from e6e7f4e to 06e27c4 Compare June 13, 2019 06:45

sriduth commented Jun 13, 2019

View reviewed changes

source/extensions/retry/host/previous_hosts/config.h Outdated Show resolved Hide resolved

Changes:

266f00f

* Format code * Remove empty OmitCanaryHostsRetryPredicate constructor * Cleanup: Use std::unique_ptr to make empty proto config for all host based predicates * Fix extenstion name in unit test and well_known_names.h Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth force-pushed the feature/retry-on-non-canary-host branch from 06e27c4 to 266f00f Compare June 13, 2019 06:55

snowp suggested changes Jun 13, 2019

View reviewed changes

source/extensions/retry/host/omit_canary_hosts/config.h Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/http_connection_management.rst Outdated Show resolved Hide resolved

PR review changes:

2ba538c

* Added release note * Improve documentation * return unique_ptr of Envoy::ProtobufWkt::Empty instead of wrapping with a message ptr in createEmptyProto (also added for previous hosts predicate) Signed-off-by: Sriduth Jayhari <[email protected]>

snowp added the waiting label Jun 18, 2019

repokitteh-read-only bot removed the waiting label Jun 18, 2019

sriduth marked this pull request as ready for review June 18, 2019 16:47

repokitteh-read-only bot added the waiting label Jun 18, 2019

sriduth requested a review from lizan as a code owner June 19, 2019 01:56

repokitteh-read-only bot removed the waiting label Jun 19, 2019

sriduth force-pushed the feature/retry-on-non-canary-host branch from 2ba14cd to 3d1bc6b Compare June 19, 2019 01:58

Merge branch 'master' into feature/retry-on-non-canary-host

93e440e

Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth force-pushed the feature/retry-on-non-canary-host branch from 3d1bc6b to 93e440e Compare June 19, 2019 17:00

CODEOWNERS: add @snowp and @sriduth

e5a8530

Signed-off-by: Sriduth Jayhari <[email protected]>

snowp suggested changes Jun 20, 2019

View reviewed changes

docs/root/intro/arch_overview/http/http_connection_management.rst Show resolved Hide resolved

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

Docs Changes:

21d3b9b

* Remove unrequired empty line in documentation for the predicate in http_connection_management.rst * Improve wording and formatting that describes change in version_history.rst Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth force-pushed the feature/retry-on-non-canary-host branch from 1b31992 to 21d3b9b Compare June 20, 2019 19:44

snowp approved these changes Jun 20, 2019

View reviewed changes

snowp merged commit 2e03ef2 into envoyproxy:master Jun 20, 2019

sriduth added a commit to sriduth/envoy that referenced this pull request Jun 25, 2019

docs: fix typo in http_connection_management.rst introduced in PR env…

9ea9d97

…oyproxy#7230 Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth mentioned this pull request Jun 25, 2019

docs: fix typo in http_connection_management.rst introduced in PR #7230 #7394

Merged

mattklein123 pushed a commit that referenced this pull request Jun 25, 2019

docs: fix typo in http_connection_management.rst introduced in PR #7230…

e868a35

… (#7394) Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth added a commit to sriduth/envoy that referenced this pull request Jul 2, 2019

docs: Add changelog for retry predicate added in envoyproxy#7230 unde…

7bd4185

…r v1.11 version history. Signed-off-by: Sriduth Jayhari <[email protected]>

sriduth mentioned this pull request Jul 2, 2019

docs: Fix changelog entry for #7230 #7444

Merged

mattklein123 pushed a commit that referenced this pull request Jul 2, 2019

docs: Add changelog for retry predicate added in #7230 under v1.11 ve…

c9a2c32

…rsion (#7444) history. Signed-off-by: Sriduth Jayhari <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extenstions: retry predicate - omit_canary_hosts #7230

extenstions: retry predicate - omit_canary_hosts #7230

sriduth commented Jun 11, 2019

snowp left a comment

snowp Jun 11, 2019

snowp Jun 11, 2019

sriduth Jun 12, 2019

snowp Jun 12, 2019

snowp Jun 11, 2019

snowp left a comment

snowp commented Jun 18, 2019

snowp commented Jun 18, 2019

sriduth commented Jun 19, 2019

snowp commented Jun 19, 2019

sriduth commented Jun 19, 2019

sriduth commented Jun 20, 2019

snowp commented Jun 20, 2019

snowp left a comment

snowp left a comment

mcasper commented Jul 1, 2019

sriduth commented Jul 2, 2019

extenstions: retry predicate - omit_canary_hosts #7230

extenstions: retry predicate - omit_canary_hosts #7230

Conversation

sriduth commented Jun 11, 2019

snowp left a comment

Choose a reason for hiding this comment

snowp Jun 11, 2019

Choose a reason for hiding this comment

snowp Jun 11, 2019

Choose a reason for hiding this comment

sriduth Jun 12, 2019

Choose a reason for hiding this comment

snowp Jun 12, 2019

Choose a reason for hiding this comment

snowp Jun 11, 2019

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

snowp commented Jun 18, 2019

snowp commented Jun 18, 2019

sriduth commented Jun 19, 2019

snowp commented Jun 19, 2019

sriduth commented Jun 19, 2019

sriduth commented Jun 20, 2019

snowp commented Jun 20, 2019

snowp left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

mcasper commented Jul 1, 2019

sriduth commented Jul 2, 2019