Skip to content

async client: add hash_policy option#8206

Merged
lizan merged 7 commits intoenvoyproxy:masterfrom
kyessenov:async_client_hashing
Sep 26, 2019
Merged

async client: add hash_policy option#8206
lizan merged 7 commits intoenvoyproxy:masterfrom
kyessenov:async_client_hashing

Conversation

@kyessenov
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov commented Sep 11, 2019

Signed-off-by: Kuat Yessenov kuat@google.com

Description:
Add hash policy to the async client. This is needed to support consistent hash load balancing for external requests originating from the proxy (e.g. telemetry, ext authz, WASM, etc).
PR moves hash policy implementation to http namespace with no functional changes.

/cc @mandarjog

Risk Level: low
Testing: unit test
Docs Changes: none
Release Notes: none
Fixes #4899

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@junr03 junr03 requested a review from goaway September 11, 2019 17:07
@kyessenov
Copy link
Copy Markdown
Contributor Author

We are also interested in providing a default hash policy for the internal client that is based off proxy node metadata. That should provide session affinity to the external system back-end with consistent hash load balancing.

@kyessenov
Copy link
Copy Markdown
Contributor Author

Friendly ping.

@kyessenov
Copy link
Copy Markdown
Contributor Author

Ping?

@junr03
Copy link
Copy Markdown
Member

junr03 commented Sep 17, 2019

Whoops sorry, this one fell through the cracks last week. I'll review by tomorrow.

@mattklein123
Copy link
Copy Markdown
Member

@junr03 @goaway please review. Thank you.

Copy link
Copy Markdown
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

One small comment. Otherwise, lgtm. Thanks for this change!

return *this;
}
RequestOptions& setHashPolicy(
const Protobuf::RepeatedPtrField<envoy::api::v2::route::RouteAction::HashPolicy>& v) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this can be StreamOptions::setHashPolicy(v); like above, right?

Copy link
Copy Markdown
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 needs to downcast to request options, so requires an override in a subclass. It's a config options class AFAICT.

}
RequestOptions& setHashPolicy(
const Protobuf::RepeatedPtrField<envoy::api::v2::route::RouteAction::HashPolicy>& v) {
hash_policy = v;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
hash_policy = v;
StreamOptions::setHashPolicy(v);

Sorry about the confusion above. I meant to tag this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. Yes, updated, thanks.

@kyessenov
Copy link
Copy Markdown
Contributor Author

The failing test looks like a random chance (birthday paradox?)
Can someone retrigger it?

@kyessenov
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8206 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #8206 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Copy Markdown
Contributor Author

This is finally de-flaked!
@junr03 do you mind taking a look?

@lizan lizan self-assigned this Sep 24, 2019
@lizan
Copy link
Copy Markdown
Member

lizan commented Sep 24, 2019

LGTM, can you merge master to pick latest CI change?

@kyessenov
Copy link
Copy Markdown
Contributor Author

@lizan Ping?

@lizan
Copy link
Copy Markdown
Member

lizan commented Sep 26, 2019

@kyessenov can you merge master?

@kyessenov
Copy link
Copy Markdown
Contributor Author

@lizan Done master merge.

@lizan lizan merged commit 4b2dd0c into envoyproxy:master Sep 26, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
Description:
Add hash policy to the async client. This is needed to support consistent hash load balancing for external requests originating from the proxy (e.g. telemetry, ext authz, WASM, etc).
PR moves hash policy implementation to http namespace with no functional changes.

Risk Level: low
Testing: unit test
Docs Changes: none
Release Notes: none
Fixes envoyproxy#4899

Signed-off-by: Kuat Yessenov <kuat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AsyncClient returned by httpAsyncClientForCluster() does not follow lb_policy defined for cluster

5 participants