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.

Sign up for GitHub

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

Jump to bottom

router: implement RetryPriority #4437

Merged
merged 7 commits into from
Sep 23, 2018
Merged

router: implement RetryPriority #4437

merged 7 commits into from
Sep 23, 2018

Conversation

snowp
Copy link
Contributor

@snowp snowp commented Sep 17, 2018

This wires up the necessary logic to allow registering a
RetryPriorityFactory that can be used to impact which priority is
selected during host selection for retry attempts.

Signed-off-by: Snow Pettersen snowp@squareup.com

Description:
Risk Level: Low, new optional feautre
Testing: Integration test
Docs Changes: n/a
Release Notes: n/a
Part of #3958

@snowp
router: implement RetryPriority
b9efb44 
This wires up the necessary logic to allow registering a
RetryPriorityFactory that can be used to impact which priority is
selected during host selection for retry attempts.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Merge remote-tracking branch 'origin/master' into priority-impl
a11c8da 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Copy link
Contributor Author

snowp commented Sep 17, 2018

For some reason the linker is failing in CI:

ERROR: /build/tmp/_bazel_bazel/400406edc57d332f0b9b805d2b8e33a1/external/envoy/test/mocks/event/BUILD:11:1: Linking of rule '@envoy//test/mocks/event:event_mocks' failed (Exit 1): ar failed: error executing command 
  (cd /build/tmp/_bazel_bazel/400406edc57d332f0b9b805d2b8e33a1/execroot/envoy && \
  exec env - \
    HOME=/tmp/fake_home \
    PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
    PWD=/proc/self/cwd \
    PYTHONUSERBASE=/tmp/fake_home \
  /usr/bin/ar @bazel-out/k8-dbg/bin/external/envoy/test/mocks/event/libevent_mocks.lo-2.params)

/usr/bin/ar: bazel-out/k8-dbg/bin/external/envoy/test/mocks/event/libevent_mocks.lo: File format not recognized```

@htuch htuch requested a review from dio September 17, 2018 18:20
@snowp
Kick CI
49e5025 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Merge remote-tracking branch 'origin/master' into priority-impl
1573192 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
fix bad merge
ec7003a 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Copy link
Contributor Author

snowp commented Sep 19, 2018

@dio Mind giving this a look?

@snowp
use Message instead of Struct
5a40bc1 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@snowp
Copy link
Contributor Author

snowp commented Sep 21, 2018

Updated this to not use a Struct as per #4475.

@dio @htuch @zuercher Would love to get this reviewed to unblock further work

@dio
Copy link
Member

dio commented Sep 21, 2018

@snowp sorry for the late response here. Will take a look today.

dio
dio suggested changes Sep 21, 2018
View reviewed changes
Copy link
Member

@dio dio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good and it seems this is really useful.

A doc question and a nit.

test/integration/http_integration.cc
envoy::api::v2::route::RouteAction::NOT_FOUND,
envoy::api::v2::route::VirtualHost::NONE, retry_policy);

// Use load assignments instead of static hosts. Necessary in order to use priorities.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a note regarding this somewhere in the doc? I think it will be useful for the operator to be aware of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really just a consequence of the API, so I don't think it's documented anywhere explicitly. Looks like load_assignment deprecates the static host list (https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cds.proto#envoy-api-field-cluster-load-assignment), so I don't think there is much to be gained by documenting the the difference.

Afaik it's just because priorities were introduced with EDS, and the recently added load_assignment brings those features back into the cluster itself.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for the clarification.

include/envoy/upstream/retry.h Outdated
@@ -105,7 +105,10 @@ class RetryPriorityFactory {
public:
virtual ~RetryPriorityFactory() {}

virtual void createRetryPriority(RetryPriorityFactoryCallbacks& callbacks) PURE;
virtual void createRetryPriority(RetryPriorityFactoryCallbacks& callbacks,
const ProtobufWkt::Message& config) PURE;
Copy link
Member

@dio dio Sep 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Tiny nit: Protobuf::Message& instead of ProtobufWkt::Message&.

@snowp
use Protobuf instead of ProtobufWkt
d41ad40 
Signed-off-by: Snow Pettersen <snowp@squareup.com>
dio
dio approved these changes Sep 22, 2018
View reviewed changes
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @snowp

@htuch htuch self-assigned this Sep 23, 2018
@htuch htuch merged commit e8da6f3 into envoyproxy:master Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch approved these changes

@dio dio dio approved these changes

Assignees

@dio dio

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@snowp @dio @htuch