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

e2e test reverse proxy unused functions cleanup #18640

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

henrybear327
Copy link
Contributor

@henrybear327 henrybear327 commented Sep 25, 2024

Part of the patches to fix #17737.

This PR aims to simplify the review load of #18641, by separating all the code cleanup parts first.

During the development of #17938, we agreed that during the transition to an L7 forward proxy, unused features and features targeting the L4 reverse proxy will be dropped.

The functions dropped are

  • PauseAccept
  • DelayAccept
  • LatencyAccept
  • PauseTX and PauseRX

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henrybear327
Once this PR has been reviewed and has the lgtm label, please assign wenjiaswe for approval. For more information see the Kubernetes Code Review Process.

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

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

@henrybear327
Copy link
Contributor Author

/cc @serathius

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.76%. Comparing base (c86c93c) to head (345bdd4).

Current head 345bdd4 differs from pull request most recent head 5fb0352

Please upload reports for the commit 5fb0352 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
Files with missing lines Coverage Δ
pkg/proxy/server.go 62.13% <ø> (+1.18%) ⬆️

... and 19 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18640      +/-   ##
==========================================
- Coverage   68.77%   68.76%   -0.01%     
==========================================
  Files         420      420              
  Lines       35535    35395     -140     
==========================================
- Hits        24438    24341      -97     
+ Misses       9668     9618      -50     
- Partials     1429     1436       +7     

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 c86c93c...5fb0352. Read the comment docs.

@henrybear327
Copy link
Contributor Author

/retest

@henrybear327
Copy link
Contributor Author

/cc @fuweid
/cc @ahrtr

@henrybear327
Copy link
Contributor Author

/retest

Part of the patches to fix etcd-io#17737

During the development of etcd-io#17938,
we agreed that during the transition to L7 forward proxy, unused
features and features targeting L4 reverse proxy will be dropped.

This feature falls under the unused feature.

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Part of the patches to fix etcd-io#17737

During the development of etcd-io#17938,
we agreed that during the transition to L7 forward proxy, unused
features and features targeting L4 reverse proxy will be dropped.

This feature falls under the unused feature.

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Part of the patches to fix etcd-io#17737

During the development of etcd-io#17938,
we agreed that during the transition to L7 forward proxy, unused
features and features targeting L4 reverse proxy will be dropped.

This feature falls under the unused feature. Also, the initial
implementation has a bug: if connections are not created continuously,
the latency accept will not work. Consider the following case:
a) set latency accept
b) put latency accept into effect
c) latency accept will start idling the goroutine
d) block-wait at accept() - waiting for new connections
e) new connection comes in - establish it
f) go to c -> as we can see, if the request come every x seconds, where
x is larger than the latency accept time we set, we can see that the
latency accept has no effect.

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Part of the patches to fix etcd-io#17737

During the development of etcd-io#17938,
we agreed that during the transition to L7 forward proxy, unused
features and features targeting L4 reverse proxy will be dropped.

This feature falls under the unused feature.

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@henrybear327
Copy link
Contributor Author

/retest

1 similar comment
@henrybear327
Copy link
Contributor Author

/retest

@serathius
Copy link
Member

As mentioned in above comment, instead of gutting L4 proxy to make it L7 proxy can you instead fork it?

@henrybear327
Copy link
Contributor Author

As mentioned in above comment, instead of gutting L4 proxy to make it L7 proxy can you instead fork it?

May I clarify what you mean by "fork it"? Do you mean that we maintain both L4 and L7 proxies in the codebase? :)

@ahrtr
Copy link
Member

ahrtr commented Sep 27, 2024

Thanks @henrybear327 for the cleanup. We created so many unused API/methods!

The only concern is that they are exported API/Methods, it means that they may be used by other applications (which is unlikely in practice?).

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.

Blackhole failpoint in the proxy does not block all updates
5 participants