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

dlb: add how to use #33818

Closed
wants to merge 16 commits into from
Closed

dlb: add how to use #33818

wants to merge 16 commits into from

Conversation

daixiang0
Copy link
Member

Add doc about how to use.

Commit Message:
Additional Description:
Risk Level: low
Testing: N/A
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax self-assigned this Apr 26, 2024
@phlax
Copy link
Member

phlax commented Apr 26, 2024

/docs

Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/33818/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #33818 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Member

phlax commented Apr 30, 2024

/retest

@adisuissa
Copy link
Contributor

/wait on CI

@repokitteh-read-only repokitteh-read-only bot added waiting deps Approval required for changes to Envoy's external dependencies and removed waiting labels Apr 30, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #33818 was synchronize by daixiang0.

see: more, trace.

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Seems that some of the modifications are not related to the PR.
/wait on cleanup

@daixiang0
Copy link
Member Author

@adisuissa sorry for mistake :(

@phlax
Copy link
Member

phlax commented May 10, 2024

still failing format and publishing ci

/wait

@daixiang0
Copy link
Member Author

Force push to fix DCO.

@daixiang0 daixiang0 requested a review from adisuissa May 15, 2024 02:12
@daixiang0
Copy link
Member Author

@phlax please help review :)

@phlax
Copy link
Member

phlax commented May 15, 2024

/docs

Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/33818/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #33818 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Member

phlax commented May 15, 2024

@daixiang0 there are a lot of format issues - see https://storage.googleapis.com/envoy-pr/c0fc41e/docs/configuration/other_features/dlb.html

could you resolve these first and then i can review

@phlax
Copy link
Member

phlax commented May 17, 2024

/wait

@daixiang0
Copy link
Member Author

@phlax please check the latest, CI is happy.

@phlax
Copy link
Member

phlax commented May 17, 2024

@daixiang0 please look at the rendered docs - they are obviously not correct

@tyxia
Copy link
Member

tyxia commented Jun 22, 2024

/wait-any

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

@daixiang0 im not convinced this is showing anything additional to the install docs

ideally we want to show how the connection is being balanced in some way

ci/verify_examples.sh Outdated Show resolved Hide resolved
examples/BUILD Show resolved Hide resolved

The number and PCIE address of DLB devices vary from CPU to CPU.

Also check your kernel version, 5.15+ is good.
Copy link
Member

Choose a reason for hiding this comment

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

this would also require that any relevant kernel modules have been compiled in

surely there is a more reliable way of checking whether a platform has DLB support - esp given the above deviced id is not reliable

Copy link
Member Author

Choose a reason for hiding this comment

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

The kernel module is installed by hand, if devices exist, means the platform supports DLB.

examples/dlb/example.rst Outdated Show resolved Hide resolved

.. code-block:: console

$ docker run -d -p 12000:80 nginx
Copy link
Member

Choose a reason for hiding this comment

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

the sandboxes use compose to start and connect services - this should not be any different

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that, but compose is not good at using hardware device in the container, also map the hardware introduce extra knowledge and difficulty for beginners, so here I use docker directly for all actions.

examples/dlb/example.rst Outdated Show resolved Hide resolved
@tyxia
Copy link
Member

tyxia commented Jun 23, 2024

/wait-any

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

htuch commented Jul 3, 2024

@phlax friendly ping.

@phlax
Copy link
Member

phlax commented Jul 3, 2024

as said previously

im not convinced this is showing anything additional to the install docs

this cant be tested as it has some very specific platform requirements - i think we can make an exception in this case - but i only think that is justifiable if it brings something extra to the table - eg clear steps to determine platform support and some step showing if/how it works

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

@phlax hi, I make support check more clear, and add some output to show how DLB works, hope you like it!

@KBaichoo
Copy link
Contributor

I think this is waiting for your input @phlax

@KBaichoo KBaichoo added the wait:review Waiting for (further) review label Jul 18, 2024
@yanavlasov
Copy link
Contributor

@daixiang0 I think the issue is that platform support is not clear at all. I've tried to look for any sort of pointers as to what CPUs or chipsets support DLB but could not find anything. Is it something that is only available to Intel devs? If not is it possible to link any documentation that tells which hardware platforms can support this.

This technology is interesting, but if nobody can try it with the commercially available hardware it will make this example unused.

@yanavlasov
Copy link
Contributor

/wait-any

This sandbox provides an example about how to enable DLB connection balanace.

.. note::
Please run below command to check your CPU supports DLB:
Copy link
Member Author

Choose a reason for hiding this comment

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

@yanavlasov I put command here to help users detect its CPU can support DLB.

@daixiang0
Copy link
Member Author

@daixiang0 I think the issue is that platform support is not clear at all. I've tried to look for any sort of pointers as to what CPUs or chipsets support DLB but could not find anything. Is it something that is only available to Intel devs? If not is it possible to link any documentation that tells which hardware platforms can support this.

This technology is interesting, but if nobody can try it with the commercially available hardware it will make this example unused.

DLB supports start from 4th Gen Intel Xeon, refer to this doc.

I only put detect command in the doc since there are many codename for CPU and users maybe not clear about the exact codename of their own CPUs.

I will add above doc link if you want.

@soulxu
Copy link
Member

soulxu commented Jul 29, 2024

gentle ping @phlax @yanavlasov, it seems @daixiang0 already updated, and waiting for next around review.

@soulxu
Copy link
Member

soulxu commented Jul 29, 2024

/wait:review

@RyanTheOptimist
Copy link
Contributor

@phlax PTAL

@phlax
Copy link
Member

phlax commented Aug 9, 2024

@daixiang0 the examples have moved now to https://github.com/envoyproxy/examples

could you reraise this there, or alternatively if lmk im happy to take it over and raise myself

@daixiang0
Copy link
Member Author

@phlax sure, I have moved it as envoyproxy/examples#67

@adisuissa
Copy link
Contributor

@phlax PTAL.
@daixiang0 please merge main again.

@phlax
Copy link
Member

phlax commented Aug 15, 2024

this can be closed now - there is a follow up here (envoyproxy/examples#67)

@phlax phlax closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait:review Waiting for (further) review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants