one cluster should have only one transport to reduce the number of TCP connections #5615

lcw2 · 2024-09-27T09:37:36Z

What type of PR is this?

What this PR does / why we need it:
It can solve the problem about too many tcp connection between aggregated apiserver and apiserver of member's cluster
Which issue(s) this PR fixes:
Fixes #5574

Special notes for your reviewer:

test.sh

KUBECONFIG="/root/.kube/karmada.config"
SLEEP_INTERVAL=0.1
MAX_JOBS=100
function run_karmadactl() {
  for ((i = 1; i <= 50; i++)); do
    karmadactl  --kubeconfig "$KUBECONFIG" get node --operation-scope=members
    sleep "$SLEEP_INTERVAL"
  done
}
for ((i = 1; i <= MAX_JOBS; i++)); do
  run_karmadactl &
done
wait

result.sh

#!/bin/bash

while true
do
  tcp_count=$(netstat -anp | grep 6443| wc -l)
  sleep 1
  echo "$(date '+%Y-%m-%d %H:%M:%S') - Current total TCP connections: $((tcp_count))"
done

fix before

fix after

karmada-bot · 2024-09-27T09:37:46Z

Welcome @lcw2! It looks like this is your first PR to karmada-io/karmada 🎉

codecov-commenter · 2024-09-27T13:51:09Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.96552% with 9 lines in your changes missing coverage. Please review.

Project coverage is 38.28%. Comparing base (58612d3) to head (884052c).
Report is 76 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/util/proxy/proxy.go	68.96%	5 Missing and 4 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5615      +/-   ##
==========================================
+ Coverage   35.20%   38.28%   +3.07%     
==========================================
  Files         645      649       +4     
  Lines       44869    45160     +291     
==========================================
+ Hits        15795    17288    +1493     
+ Misses      27844    26559    -1285     
- Partials     1230     1313      +83

Flag	Coverage Δ
unittests	`38.28% <68.96%> (+3.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chaunceyjiang · 2024-09-29T01:56:20Z

pkg/util/proxy/proxy.go

+		clusterEndpoint := clusterEndpointInfo{
+			Transport: proxyTransport,
+		}
+		clusterEndpointInfoStore.Store(cluster.UID, clusterEndpointInfo{


Just a question: When will the clusterEndpointInfo be removed from the clusterEndpointInfoStore?

whitewindmills · 2024-09-29T06:32:07Z

go.mod

@@ -174,7 +175,6 @@ require (
 	golang.org/x/exp v0.0.0-20231226003508-02704c960a9b // indirect
 	golang.org/x/mod v0.17.0 // indirect
 	golang.org/x/oauth2 v0.18.0 // indirect
-	golang.org/x/sync v0.7.0 // indirect


it seems that the go.mod file has not changed substantially. you need to restore it.

difference in indirect

I don't think it's necessary.

lcw2 · 2024-10-10T06:17:49Z

two questions:

when ProxyURL and ProxyHeader of cluster changed， how to handle transport ?
store ProxyURL and ProxyHeader in clusterEndpointInfo, when a request comes in, compare the current values with the stored ones. If those fields have changed, create a new transport ? or other idea?
when cluster is deleted， should the transport of the cluster also be deleted？
If deletion is required, can this be archieved by periodically list cluster? 30s?
any suggestion? @whitewindmills @chaunceyjiang @chaunceyjiang

whitewindmills · 2024-10-11T06:27:50Z

I just feel like this approach seems too complicated and easy to go wrong. can we consider using all key fields(such as APIEndpoint, ProxyURL ...) to generate a hash value as the clusterEndpointInfoStore key? and we might do not need to remove it when cluster is deleted.

karmada-bot · 2024-10-13T18:57:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chaunceyjiang 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:

OWNERS

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

…P connections Signed-off-by: mengying <[email protected]>

XiShanYongYe-Chang · 2024-10-15T02:54:29Z

/cc @whitewindmills @chaunceyjiang @yanfeng1992
Hi, guys, can you help take a review again?

whitewindmills · 2024-10-17T07:44:10Z

pkg/util/proxy/proxy.go

+	for _, key := range proxyHeaderKeys {
+		usedFields = append(usedFields, key, cluster.Spec.ProxyHeader[key])
+	}
+	usedFields = append(usedFields, string(cluster.UID), cluster.Spec.ProxyURL, cluster.Spec.APIEndpoint)


is UID used by createProxyTransport?

UID changed means the cluster rejoined, should create a new transport.

whitewindmills

have you tested it yet? I thought of a scenario: when the cluster goes offline, the connection will be automatically disconnected after a period of time. when the cluster comes back online, can it work fine?

yanfeng1992 · 2024-10-17T08:05:41Z

pkg/util/proxy/proxy.go

+		if value, ok := clusterEndpointInfoStore.Load(clusterHash); ok {
+			return value, nil
+		}
+		proxyTransport, err := createProxyTransport(cluster, tlsConfig)


What happens if tlsConfig changes?

It will not work fine, cannot request successfully. But I think no need to consider the scenario abourt secrets changes.

lcw2 · 2024-10-17T08:08:40Z

have you tested it yet? I thought of a scenario: when the cluster goes offline, the connection will be automatically disconnected after a period of time. when the cluster comes back online, can it work fine?

I stop the apiserver two minutes ,and restart the apiserver, karmadactl get nodes work fine.

whitewindmills · 2024-10-17T09:16:59Z

I'm not sure if you built the cache before stoping apiserver. maybe we haven't considered it comprehensively enough. can we talk about it in the next community meeting?

lcw2 · 2024-10-17T10:15:32Z

I'm not sure if you built the cache before stoping apiserver. maybe we haven't considered it comprehensively enough. can we talk about it in the next community meeting?

OK

karmada-bot requested review from chaunceyjiang and XiShanYongYe-Chang September 27, 2024 09:37

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 27, 2024

lcw2 force-pushed the transport branch 2 times, most recently from 9289780 to ee350be Compare September 27, 2024 09:46

karmada-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2024

lcw2 force-pushed the transport branch 2 times, most recently from 5b909be to 423abb3 Compare September 27, 2024 09:55

lcw2 changed the title ~~one cluster have only one transport~~ one cluster should have only one transport to reduce the number of TCP connections Sep 27, 2024

lcw2 force-pushed the transport branch from 423abb3 to 9364386 Compare September 27, 2024 10:07

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 27, 2024

lcw2 force-pushed the transport branch from 9364386 to cb1f3e3 Compare September 27, 2024 13:38

lcw2 force-pushed the transport branch from cb1f3e3 to c675e3f Compare September 27, 2024 17:28

chaunceyjiang reviewed Sep 29, 2024

View reviewed changes

whitewindmills reviewed Sep 29, 2024

View reviewed changes

lcw2 force-pushed the transport branch 3 times, most recently from 60a61a6 to 63a7ba2 Compare October 13, 2024 18:57

lcw2 force-pushed the transport branch from 63a7ba2 to ce9803f Compare October 14, 2024 08:42

one cluster should have only one transport to reduce the number of TC…

884052c

…P connections Signed-off-by: mengying <[email protected]>

lcw2 force-pushed the transport branch from ce9803f to 884052c Compare October 14, 2024 09:41

karmada-bot requested a review from chaunceyjiang October 15, 2024 02:54

karmada-bot requested review from whitewindmills and yanfeng1992 October 15, 2024 02:54

whitewindmills reviewed Oct 17, 2024

View reviewed changes

yanfeng1992 reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

one cluster should have only one transport to reduce the number of TCP connections #5615

one cluster should have only one transport to reduce the number of TCP connections #5615

lcw2 commented Sep 27, 2024 •

edited

Loading

karmada-bot commented Sep 27, 2024

codecov-commenter commented Sep 27, 2024 •

edited

Loading

chaunceyjiang Sep 29, 2024

whitewindmills Sep 29, 2024

whitewindmills Sep 29, 2024

lcw2 Oct 13, 2024

whitewindmills Oct 17, 2024

lcw2 commented Oct 10, 2024

whitewindmills commented Oct 11, 2024

karmada-bot commented Oct 13, 2024

XiShanYongYe-Chang commented Oct 15, 2024

whitewindmills Oct 17, 2024

lcw2 Oct 17, 2024

whitewindmills left a comment

yanfeng1992 Oct 17, 2024

lcw2 Oct 17, 2024

lcw2 commented Oct 17, 2024 •

edited

Loading

whitewindmills commented Oct 17, 2024

lcw2 commented Oct 17, 2024

one cluster should have only one transport to reduce the number of TCP connections #5615

Are you sure you want to change the base?

one cluster should have only one transport to reduce the number of TCP connections #5615

Conversation

lcw2 commented Sep 27, 2024 • edited Loading

karmada-bot commented Sep 27, 2024

codecov-commenter commented Sep 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcw2 commented Oct 10, 2024

whitewindmills commented Oct 11, 2024

karmada-bot commented Oct 13, 2024

XiShanYongYe-Chang commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitewindmills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcw2 commented Oct 17, 2024 • edited Loading

whitewindmills commented Oct 17, 2024

lcw2 commented Oct 17, 2024

lcw2 commented Sep 27, 2024 •

edited

Loading

codecov-commenter commented Sep 27, 2024 •

edited

Loading

lcw2 commented Oct 17, 2024 •

edited

Loading