Remove the proxy agent retry logic to simplify the code. #108
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
caesarxuchao
force-pushed
the
remove-retry
branch
from
69db895
to
b5963f6
Compare
caesarxuchao
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @anfernee |
|
lgtm too. not particularly familiar with the host counting logic. everything else looks good. |
caesarxuchao
force-pushed
the
remove-retry
branch
from
b5963f6
to
c153a1f
Compare
pkg/agent/client.go
Outdated
| stream agent.AgentService_ConnectClient | ||
| agentID string | ||
| serverID string // the id of the proxy server this client connects to. | ||
| serverCount int // the number of the proxy server instances. |
There was a problem hiding this comment.
Not sure we should keep serverCount here. It seems like there should only be one value for serverCount. Not sure what it means if we somehow get different clients with different values for serverCount. Seems better to just have serverCount in ClientSet.
There was a problem hiding this comment.
Done. Bubbled up the serverCount as a return value.
| a.recvLock.Lock() | ||
| defer a.recvLock.Unlock() | ||
|
|
||
| pkt, err := a.stream.Recv() |
There was a problem hiding this comment.
Not blocking for this PR but a little worried that Recv is blocking and may stop us shutting down properly.
There was a problem hiding this comment.
Good point. I did some research on this and it seems fine.
When we close the agent client, we close the underlying GRPC connection, which should close the stream and unblocks Recv() with an error, according to grpc/grpc-go#3230 (comment).
pkg/agent/client.go
Outdated
| return 0, err | ||
| } | ||
| scounts := md.Get(header.ServerCount) | ||
| if len(scounts) == 0 { |
There was a problem hiding this comment.
When is it ok to get more than 1 ServerCount? If you get more than 1, how do you know which one is the right value?
There was a problem hiding this comment.
Updated it to check the length is 1.
pkg/agent/clientset.go
Outdated
| @@ -175,13 +171,13 @@ func (cs *ClientSet) syncOnce() error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| cs.serverCount = c.stream.serverCount | |||
| if err := cs.AddClient(c.stream.serverID, c); err != nil { | |||
| cs.serverCount = c.serverCount | |||
There was a problem hiding this comment.
If cs.serverCount > 0 and cs.serverCount != c.serverCount, we should probably at least print a warning.
There was a problem hiding this comment.
Removed c.serverCount per your other comment :)
| if err != nil { | ||
| return err | ||
| } | ||
| cs.serverCount = c.stream.serverCount | ||
| if err := cs.AddClient(c.stream.serverID, c); err != nil { | ||
| cs.serverCount = serverCount |
There was a problem hiding this comment.
I think we need to log a warning if cs.serverCount !=0 and cs.serverCount != serverCount. Whether we are looking at a serverCount of zero creeping in, two proxy servers/apiservers with differing beliefs in the needed number of servers or a legitimate runtime change in the count, it will be hard to debug these scenarios without this information. In addition this should be a very rare event.
There was a problem hiding this comment.
Good call. Updated.
caesarxuchao
force-pushed
the
remove-retry
branch
from
6820797
to
7d3dd7f
Compare
k8s-ci-robot
added
the
lgtm
…modules/golang.org/x/sys-0.7.0 Build(deps): bump golang.org/x/sys from 0.6.0 to 0.7.0
Copied from #106 (comment):
"It seems that the reconnecting code is causing more and more maintenance burden....The advantage of the reconnecting is that it will retry the current packet so it surfaces less errors to the caller."
It's a nice-to-have, but we think the maintenance burden has overgrown the advantage, so we remove it.
In a follow-up PR, I'll add client-side metrics to record how many send/receive errors we got, so that we will know if the optimization is needed.