Skip to content

enable DLB connection balance extension#4320

Merged
istio-testing merged 1 commit intoistio:masterfrom
daixiang0:dlb
Jan 6, 2023
Merged

enable DLB connection balance extension#4320
istio-testing merged 1 commit intoistio:masterfrom
daixiang0:dlb

Conversation

@daixiang0
Copy link
Copy Markdown
Member

Signed-off-by: Loong loong.dai@intel.com

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 January 6, 2023 03:11
@istio-policy-bot
Copy link
Copy Markdown

😊 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
Copy Markdown
Member Author

wasm test failed

/retest

@daixiang0
Copy link
Copy Markdown
Member Author

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

@zirain
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

/retest

Signed-off-by: Loong <loong.dai@intel.com>
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

@kyessenov Thanks, I will do it:)

@ramaraochavali
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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