One-shot commands (pebble exec) #61
Conversation
This uses the github.com/pkg/term/termios package (instead of cgo)
(this is kind of like "connect to operation websocket" in LXD)
Websockets haven't been reworked as per spec yet
Non-terminal mode is now broken
| // You should have received a copy of the GNU General Public License | ||
| // along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package client |
There was a problem hiding this comment.
This test file looks very lightweight considering everything we have under client/exec.go. Looks like there's a lot of logic uncovered. I think we've been pretty good about keeping things tested so far, and it would be nice to continue that trend. We already have fake web servers in our suite, for example, to make things a bit easier.
See client_test.go and changes_test.go for some ideas of things we've done before, which could perhaps be used as inspiration.
The idea I'd try to keep in mind is this: Pebble is a young and active project, which means people will come in and hack this logic. How can we make it likely that they'll find out if they're breaking something important?
There was a problem hiding this comment.
Agreed. The client is implicitly tested by the API/daemon tests, but there's no isolated client tests, which isn't great. I'm half way through adding this now (it required a small bit of refactoring to use interfaces for the websockets), but I'm going to do it as a fast-follow PR.
cmd/pebble/cmd_exec.go
Outdated
| } else if cmd.NoTerminal { | ||
| useTerminal = false | ||
| } else { | ||
| useTerminal = stdinIsTerminal && stdoutIsTerminal |
There was a problem hiding this comment.
It's not urgent indeed, and I'd also be glad to have this merged sooner rather than later. At the same time, let's please fix that as a follow up indeed, instead of leaving it behind in a backlog. This is a common enough operation that makes me surprised LXD hasn't noticed it before. My theory is the same we discussed in our last call: LXD most likely uses this API to get a full shell into the container much more than for one-off commands as we'll see with Pebble.
Let's please change that logic before merging, though, so that indeed this is a visible bug to be fixed rather than stdin controlling the use of the terminal.
|
I'm going to merge this, yay! The two items for fast-follow are: 1) fixing stdin when use-terminal = true, and 2) adding isolated client tests. Adding client tests is in progress, but I'll do in a fast-follow PR. |
More specifically, these PRs: * canonical/pebble#59 Add GPLv3 license (to match source header and snapd) * canonical/pebble#64 Avoid use of multipart.Part.FileName() to fix files API on Go 1.17 * canonical/pebble#63 Add wait-change API endpoint and client function * canonical/pebble#65 Make Pebble "go vet" clean * canonical/pebble#66 Remove a bunch of snapd references * canonical/pebble#67 Add API section to README; add HACKING.md; remove Makefile * canonical/pebble#60 Add Replan operation to restart changed services. * canonical/pebble#61 One-shot commands (pebble exec)
#13381 Includes these Pebble PRs: * canonical/pebble#59 * canonical/pebble#64 * canonical/pebble#63 * canonical/pebble#65 * canonical/pebble#66 * canonical/pebble#67 * canonical/pebble#60 * canonical/pebble#61
This implements the Python Operator Framework side of the one-shot commands
spec ("Pebble exec"). The Pebble side is implemented in
canonical/pebble#61.
Per discussion at canonical#61 (comment), we wanted to make the default use-terminal mode based only on stdout, but that's different to what both "lxc exec" and "ssh" do, so seems wise to copy them. Additionally, it's problematic to implement, because when allocating a PTY on the remote, you get a "master" file descriptor that handles both stdin and stdout. You have to call close() on the master FD to close stdin so that a command that reads stdin (eg: "cat") stops, but you're closing the entire master FD so that also closes stdout, causing the POLLNVAL errors we were seeing on the stdout side, as well as the command output sometimes being cut short. We probably could address that by having a separate pipe for stdin and specifying that in the API, or allocating two PTYs on the remote, or similar, but it's complex and I don't think deviating from ssh or "lxc exec" is a good idea. In practice you very rarely want interactivity if you're piping stdin, which is I guess why ssh (and lxc exec) do it the way they do. When you try to pipe stdin and specify -t, here is what "lxc exec" and ssh do: * lxc exec: "echo foo | lxc exec <instance> -t -- cat" ... this fails with the error we were seeing here, exit code 129 and intermittent output for the same reason. * ssh: "echo foo | ssh giftyweddings.com -t -- cat" ... because it's not going to work anyway, ssh explicitly prevents this, uses non-terminal mode anyway, and prints the following message on stderr: $ echo foo | ssh giftyweddings.com -t -- cat Pseudo-terminal will not be allocated because stdin is not a terminal. foo I think the stderr message is a good idea, because it tells you you've done something wrong, so I've copied that here in "pebble exec". This reverts commit d7ca528 plus adding the stderr msg if -t is specified and stdin is not a terminal.
This implements the Pebble/Go side of the one-shot commands spec for command execution using websocket communication, including the Go client and
pebble execCLI.Other notes for the code reviewer:
*websocket.Conn(which makes things kinda hard to test right now).ptyutilorwsutilpackages were pulled basically verbatim from LXD, so I'm not going to add tests for those. The LXD team will be extracting those to a shared library, at which point we cango getthem instead of vendoring.Manual tests:
Fixes #37.