Skip to content

enable DLB connection balance extension - #4320

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

enable DLB connection balance extension#4320
istio-testing merged 1 commit into
istio: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

zirain commented Jan 6, 2023

Copy link
Copy Markdown
Member

@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

daixiang0 commented Jan 6, 2023

Copy link
Copy Markdown
Member Author

@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>

@zirain zirain left a comment

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.

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