Repository navigation
Keep received exec exit status when the connection fails on close - #4967
Conversation
The API server can close the exec WebSocket right after sending the exit status. ExecProcess then closes its side, writing the close frame fails (e.g. "Connection or outbound has been closed"), and failure() overwrote the already received exit status with -1975219, so a command that finished was reported as a failure. Only set -1975219 when no exit status was received.
|
|
|
Welcome @sahilkhatri09! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, sahilkhatri09 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
9872bad
into
kubernetes-client:master
What type of PR is this?
/kind bug
What this PR does / why we need it:
The API server can close the exec WebSocket immediately after sending the exit status on the status channel (stream 3). When the status arrives,
ExecProcesscloses its own side, and writing that close frame can then fail withjava.net.SocketException: Connection or outbound has been closed(thrown fromWebSocketWriter.writeClose).failure()then unconditionally overwrote the already received exit status with-1975219. As a result, a command that finished, usually with exit code 0, was intermittently reported as failed. Callers can't tell this apart from a real mid-command disconnect, so they have to re-run commands that may not be idempotent.This change only sets
-1975219when no exit status was received (statusCode < 0). A failure before the status arrives behaves exactly as before. TheonUnhandledErrorcallback is still invoked in both cases.Which issue(s) this PR fixes:
Fixes #1891 (also reported in #1346, #3385, #4812)
Tests:
Added
ExecTest.unhandledErrorAfterExitStatusKeepsExitCode, parameterized for exit codes 0 and 126. It delivers a status frame and then callsfailure(). It fails without this change (exit value-1975219) and passes with it.ExecTestpasses (13 tests), andspotless:checkpasses.Does this PR introduce a user-facing change?