Skip to content

server: replace cmux with native HTTP/2 multiplexing#21801

Open
kairosci wants to merge 1 commit into
etcd-io:mainfrom
kairosci:replace-cmux-with-native-http2
Open

server: replace cmux with native HTTP/2 multiplexing#21801
kairosci wants to merge 1 commit into
etcd-io:mainfrom
kairosci:replace-cmux-with-native-http2

Conversation

@kairosci
Copy link
Copy Markdown
Contributor

@kairosci kairosci commented May 23, 2026

Remove the unmaintained cmux dependency in favor of h2c for
cleartext gRPC and TLS ALPN for encrypted connections.
Fixes #18032

@k8s-ci-robot
Copy link
Copy Markdown

Hi @kairosci. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch from 7bfd949 to 9314781 Compare May 23, 2026 20:48
@kairosci
Copy link
Copy Markdown
Contributor Author

/assign @ahrtr
/cc @ahrtr

@k8s-ci-robot k8s-ci-robot requested a review from ahrtr May 23, 2026 20:51
@serathius
Copy link
Copy Markdown
Member

/ok-to-test

@serathius
Copy link
Copy Markdown
Member

serathius commented May 24, 2026

cc @aojea is the idea sound at all?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2026

Codecov Report

❌ Patch coverage is 23.12500% with 123 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.97%. Comparing base (dde11c0) to head (476fc51).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
server/embed/serve.go 25.78% 92 Missing and 3 partials ⚠️
server/etcdmain/grpc_proxy.go 0.00% 22 Missing ⚠️
server/embed/apply/buffered_conn.go 0.00% 6 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
server/embed/etcd.go 74.08% <100.00%> (-2.09%) ⬇️
server/embed/apply/buffered_conn.go 0.00% <0.00%> (ø)
server/etcdmain/grpc_proxy.go 14.13% <0.00%> (+0.03%) ⬆️
server/embed/serve.go 47.04% <25.78%> (-12.35%) ⬇️

... and 29 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #21801      +/-   ##
==========================================
- Coverage   70.08%   69.97%   -0.11%     
==========================================
  Files         426      427       +1     
  Lines       35245    35337      +92     
==========================================
+ Hits        24701    24727      +26     
- Misses       9157     9212      +55     
- Partials     1387     1398      +11     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde11c0...476fc51. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kairosci kairosci marked this pull request as draft May 24, 2026 09:53
@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch from 9314781 to 0a4cbb8 Compare May 24, 2026 10:05
@k8s-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kairosci
Once this PR has been reviewed and has the lgtm label, please ask for approval from ahrtr. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch from 0a4cbb8 to cffdd5b Compare May 24, 2026 10:15
@kairosci kairosci marked this pull request as ready for review May 24, 2026 10:15
@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch 2 times, most recently from 27d8df0 to f93eb11 Compare May 24, 2026 10:25
@kairosci
Copy link
Copy Markdown
Contributor Author

/retest

@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch from f93eb11 to 87b6ccf Compare May 24, 2026 12:38
@kairosci kairosci marked this pull request as draft May 24, 2026 12:42
Signed-off-by: Alessio Attilio <attilio.alessio@protonmail.com>
@kairosci kairosci force-pushed the replace-cmux-with-native-http2 branch from 87b6ccf to 476fc51 Compare May 24, 2026 14:50
@kairosci kairosci marked this pull request as ready for review May 24, 2026 20:57
@k8s-ci-robot
Copy link
Copy Markdown

@kairosci: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-verify 476fc51 link true /test pull-etcd-verify
pull-etcd-e2e-arm64 476fc51 link true /test pull-etcd-e2e-arm64
pull-etcd-e2e-amd64 476fc51 link true /test pull-etcd-e2e-amd64
pull-etcd-e2e-386 476fc51 link true /test pull-etcd-e2e-386

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@serathius
Copy link
Copy Markdown
Member

serathius commented May 24, 2026

@kairosci previously this task was assumed to be impossible due to how HTTP/2 works. But it's yours AI tokens.

@aojea
Copy link
Copy Markdown
Contributor

aojea commented May 25, 2026

cc @aojea is the idea sound at all?

after a quick look this PR is implementing the same logic as cmux, a read ahead buffer and a dispatcher ... I do not see the benefit of absorbing the problem in tree, the cmux package is pretty stable and battle tested ... unless there is a very big benefit I advice to be conservative, touching in this area can open a lot of subtle bugs and regressions

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented May 25, 2026

@kairosci thank you for the PR.

I suggest to look at this change after we release etcd v3.7, as we don't want to introduce any new feature or major change to etcd at current stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Evaluate & potentially replace https://github.com/soheilhy/cmux

5 participants