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

enable DLB connection balance extension #4320

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

daixiang0
Copy link
Member

Signed-off-by: Loong [email protected]

What this PR does / why we need it:

It is to enable DLB connection balance extension.

DLB support in Envoy is merged and available from envoyproxy/envoy#20268

Intel® Dynamic Load Balancer (Intel® DLB) is a hardware managed system of queues and arbiters connecting producers and consumers. It is a PCI device envisaged to live in the server CPU uncore and can interact with software running on cores, and potentially with other devices. More details please refer to https://www.intel.com/content/www/us/en/download/686372/intel-dynamic-load-balancer.html.

Special notes for your reviewer:

With EnvoyFilter, this feature can work, no more extra integration work in Istio side.

@daixiang0 daixiang0 requested a review from a team as a code owner January 6, 2023 03:11
@istio-policy-bot
Copy link

😊 Welcome @daixiang0! This is either your first contribution to the Istio proxy repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 6, 2023
@daixiang0
Copy link
Member Author

wasm test failed

/retest

@daixiang0
Copy link
Member Author

@istio/wg-policies-and-telemetry-maintainers please review.

@zirain
Copy link
Member

zirain commented Jan 6, 2023

@kfaseela have same request in istio/istio#42380.

I think istio need a clear rule about contrib filter import policy.

cc @howardjohn @lei-tang @kyessenov

@daixiang0
Copy link
Member Author

daixiang0 commented Jan 6, 2023

@kfaseela have same request in istio/istio#42380.

I think istio need a clear rule about contrib filter import policy.

cc @howardjohn @lei-tang @kyessenov

They are different cases: Postgres has been moved from core to contrib, but no move history for DLB.

Also, it is not a filter, actually, it accelerates connection balance inside Envoy, which means all key applications like ingress gateway can get benefit from it.

It's more like #4202.

@daixiang0
Copy link
Member Author

/retest

Signed-off-by: Loong <[email protected]>
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

seems reasonable, left to @kyessenov for another look.

@kyessenov
Copy link
Contributor

I don't think we can say "no" after accepting cryptomb and other contrib accelerators.

@kyessenov kyessenov self-requested a review January 6, 2023 17:23
@kyessenov
Copy link
Contributor

Per-usual: please work with Istio community to define a first-class API (not EnvoyFilter). Without that, users cannot assume this extension works for managed versions of Istio.

@istio-testing istio-testing merged commit 65a681d into istio:master Jan 6, 2023
@daixiang0 daixiang0 deleted the dlb branch January 9, 2023 01:06
@daixiang0
Copy link
Member Author

@kyessenov Thanks, I will do it:)

@ramaraochavali
Copy link

IMO, crytoMB is different. We have an API for it already. I certainly think we need a process around bringing Envoy contrib extensions to Istio Proxy. Otherwise it has potential to destabilize Istio proxy.
cc: @howardjohn @hzxuzhonghu

@kyessenov
Copy link
Contributor

@ramaraochavali That's quite surprising to have a de-facto stable API (Mesh) to depend on a contrib extension in Envoy (which is technically "experimental"). What matters is that they should be aligned: an experimental Istio API can depend on contrib Envoy extension, but a stable Istio API should not depend on contrib.

@ramaraochavali Does Istio have a way to define experimental APIs? I think almost all versions have been pretty much stuck under v1alpha1.

@ramaraochavali
Copy link

That's quite surprising to have a de-facto stable API (Mesh) to depend on a contrib extension in Envoy (which is technically "experimental"). What matters is that they should be aligned: an experimental Istio API can depend on contrib Envoy extension, but a stable Istio API should not depend on contrib.

I agree.

@kyessenov
Copy link
Contributor

Since you're a lead, do you mind raising the issue of "how do we define (reversible) experimental API changes" to ToC? If the answer is "use EnvoyFilter" then we need to revert the mesh config change for contrib extensions.

For postgres, I'm not even sure EnvoyFilter can handle generation of multiple filter chains for postgres ports.

@daixiang0
Copy link
Member Author

@kyessenov do you know which release would include this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants