fix(overlord): send EOF to master pty in interactive mode #466
Conversation
If the interactive mode is enforced, there are scenarios like: go run ./cmd/pebble exec -i -- tee < ./HACKING.md that would prompt the server to shutdown the execution earlier than the whole output from 'tee' will be read and sent to the client. Hence, instead of closing the master pty descriptor when there is no more input from the client, send Ctrl-D (EOF) to indicate to the process that there will be no more input.
dmitry-lyfar
changed the title
| // output to the client. Thus, closing the master descriptor | ||
| // here will terminate mirroring prematurely. Instead, we | ||
| // should send Ctrl-D to the fd to indicate the end of input. | ||
| master.Write([]byte{byte(unix.VEOF)}) |
There was a problem hiding this comment.
The fix makes sense, see e.g. https://stackoverflow.com/a/18366397/705086
I do wonder about some corner cases, e.g.:
pebble -i yada yada < somefilewhere input is not from a terminal, but the output is- all of stdin/stdout/stderr are redirected, and yet
-iis supplied
Please consider how to test these -- unit tests? functional? one-off manual tests? I'm not sure what would be more appropriate.
There was a problem hiding this comment.
Yeah, it's not ideal, but we currently don't have any automated tests for interactive mode. We have on our roadmap for @IronCore864 to write end-to-end tests for pebble run; maybe some pebble exec -i tests can be included in that work?
In any case, I've done some pretty extensive manual testing of all the various exec modes, and this fix seems to work nicely. I've also tested that the number of file descriptors isn't increasing (leaking) by watching /bin/sh -c 'ls /proc/{pebble_pid}/fd/ | wc -l', and it's stable (stays at 10).
Tiexin has also done manual testing on this PR.
|
This does seem like the right fix -- thanks very much, @dmitry-lyfar -- really appreciated! As mentioned above, both I and Tiexin have done fairly extensive manual testing on this PR, and we think it's good to go. So I'm going to merge this now. |
|
@benhoyt @IronCore864 @dimaqq amazing, thanks for looking into and testing this! |
#17991 Per discussion with @hpidcock, update Juju 3.6 to use the just-released Pebble v1.16.0 ([changelog](https://github.com/canonical/pebble/releases/tag/v1.16.0)). Summary of main changes: * Improvements to how services with dependencies are started and stopped (using "lanes"), so that independent services are started in parallel, and dependent services start up serially. Error handling is also improved. Fix in canonical/pebble#437. * A fix for a tricky bug in pebble exec, so it doesn't lose output in interactive mode. Fix in canonical/pebble#466! prdesc Reduce the size of exec tasks by not storing the execSetup (which includes all environment variables) in state. This speeds up (and shrinks the data) when serialising state to disk during state.Unlock. Fix is in canonical/pebble#478. prdesc Ensure Pebble doesn't hang (with no error message) when the state file directory is read-only or otherwise inaccessible. Fix in canonical/pebble#481. * Re-implement warnings using notices. This was always the intention since the notices feature was added (it was designed as a superset of warnings), but we'd never gotten to it. Lots of nice code deletion in canonical/pebble#486. ## QA steps Deploy a K8s charm, like `snappass-test`, and ensure `pebble version --client` on the workload reports v1.16.0.
If the interactive mode is enforced in
pebble exec, there are scenarios like:go run ./cmd/pebble exec -i -- tee < ./HACKING.mdthat would prompt the server to shutdown the execution earlier than the whole output from 'tee' will be read and sent to the client. Hence, instead of closing the master pty descriptor when there is no more input from the client, send
Ctrl-D (EOF) to indicate to the process that there will be no more input.
Fixes #306.