ssh-agent: Increase timeout before we explicitly close connection #3631
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc 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 |
There are cases where remote will close connection by itself with a message make sure we give connection enough time instead of closing explictly early. Future improvement: Relay output and perform close instead of relying on `ServeAgent` to flush buffer by closing connection. [NO NEW TESTS NEEDED] Signed-off-by: Aditya Rajan <arajan@redhat.com>
flouthoc
force-pushed
the
ssh-agent-increase-timout
branch
from
52e3bc1
to
b74d71d
Compare
|
Patch verified here: #3587 (comment) |
|
@vrothberg @ashley-cui PTAL |
| go func() { | ||
| time.Sleep(500 * time.Millisecond) | ||
| time.Sleep(2000 * time.Millisecond) |
There was a problem hiding this comment.
I'm OK with the value, but just wonder if we could instead get this value from a config file.
There was a problem hiding this comment.
@TomSweeneyRedHat Sure we could do that but I am confused if this should be visible to end-user and anybody would ever want to configure this.
Ideally we should remove timeout and add buffer forwarding mechanism which should close connection but that seemed like a bigger change to me and would like to refactor bunch of things in sshagent after discussing with maintainers in a separate thread.
There was a problem hiding this comment.
No to config, since users will have no idea what to set it. None of us have any idea either, since it will probably vary under load.
I say increase the hack for now, and then consider fixing this long term in the future.
LGTM
There was a problem hiding this comment.
I concur with Dan. At least I would have absolutely no feeling for what a good value for that would be, and also be worried why I have to make that decision.
There was a problem hiding this comment.
FWIW, it would not be a config setting I'd heavily document. More of a tool for support if the issue popped up in the future, or we found out that 2,000 was too much and was causing some other unanticipated error. I think Dan and Valentin being unsure about a valid value is why I would like to make it configurable. But hopefully, this will fix this, and it will be a non-issue.
There was a problem hiding this comment.
I would say that if this happens again in the field, then we should put in the work to build a complete solution.
There are cases where remote will close connection by itself with a message
make sure we give connection enough time instead of closing explictly
early.
Future improvement: Relay output and perform close instead of relying on
ServeAgentto flushbuffer by closing connection.
Output after this:
[NO TESTS NEEDED]
There is no convenient way to test this in CI.
Closes: #3587