[v14] Prevent exiting a session prior to output being consumed #45373
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rosstimothy
added
the
no-changelog
strideynet
approved these changes
Aug 12, 2024
zmb3
approved these changes
Aug 12, 2024
|
Tests are failing due to a missing backport of #41434 to branch/v14 |
rosstimothy
force-pushed
the
bot/backport-45223-branch/v14
branch
4 times, most recently
from
81337e3 to
df83a95
Compare
rosstimothy
force-pushed
the
bot/backport-45223-branch/v14
branch
2 times, most recently
from
3b5a72c to
4add1aa
Compare
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
* fix(srv): SSH remote sessions resources not being closed correctly * refactor(srv): code review suggestions * test(srv): move t.Helper to the correct function * chore(srv): typo * chore(srv): typo
rosstimothy
force-pushed
the
bot/backport-45223-branch/v14
branch
from
b97f10c to
c8a19c0
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 28, 2024
* Prevent exiting a session prior to output being consumed #17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped. * Fix SSH sessions recorded on proxy not being fully closed (#41434) * fix(srv): SSH remote sessions resources not being closed correctly * refactor(srv): code review suggestions * test(srv): move t.Helper to the correct function * chore(srv): typo * chore(srv): typo --------- Co-authored-by: Gabriel Corado <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport #45223 and #41434 to branch/v14