- Notifications
You must be signed in to change notification settings - Fork914
feat(agent/reconnectingpty): allow selecting backend type#17011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -32,6 +32,8 @@ type Options struct { | ||
| Timeout time.Duration | ||
| // Metrics tracks various error counters. | ||
| Metrics *prometheus.CounterVec | ||
| // BackendType specifies the ReconnectingPTY backend to use. | ||
| BackendType string | ||
| } | ||
| // ReconnectingPTY is a pty that can be reconnected within a timeout and to | ||
| @@ -64,13 +66,20 @@ func New(ctx context.Context, logger slog.Logger, execer agentexec.Execer, cmd * | ||
| // runs) but in CI screen often incorrectly claims the session name does not | ||
| // exist even though screen -list shows it. For now, restrict screen to | ||
| // Linux. | ||
| autoBackendType := "buffered" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Suggestion: can we call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I'll do this as a follow-up refactor 👍 | ||
| if runtime.GOOS == "linux" { | ||
| _, err := exec.LookPath("screen") | ||
| if err == nil { | ||
| autoBackendType = "screen" | ||
| } | ||
| } | ||
| var backendType string | ||
| switch options.BackendType { | ||
| case "": | ||
| backendType = autoBackendType | ||
| default: | ||
| backendType = options.BackendType | ||
| } | ||
| logger.Info(ctx, "start reconnecting pty", slog.F("backend_type", backendType)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -4,7 +4,6 @@ import ( | ||
| "bufio" | ||
| "context" | ||
| "encoding/json" | ||
| "io" | ||
| "os" | ||
| "strings" | ||
| @@ -15,6 +14,7 @@ import ( | ||
| "golang.org/x/xerrors" | ||
| "github.com/coder/coder/v2/cli/cliui" | ||
| "github.com/coder/coder/v2/coderd/util/slice" | ||
| "github.com/coder/coder/v2/codersdk" | ||
| "github.com/coder/coder/v2/codersdk/workspacesdk" | ||
| "github.com/coder/coder/v2/pty" | ||
| @@ -96,6 +96,7 @@ func handleRPTY(inv *serpent.Invocation, client *codersdk.Client, args handleRPT | ||
| } else { | ||
| reconnectID = uuid.New() | ||
| } | ||
| ws, agt, err := getWorkspaceAndAgent(ctx, inv, client, true, args.NamedWorkspace) | ||
| if err != nil { | ||
| return err | ||
| @@ -118,14 +119,6 @@ func handleRPTY(inv *serpent.Invocation, client *codersdk.Client, args handleRPT | ||
| } | ||
| } | ||
| // Get the width and height of the terminal. | ||
| var termWidth, termHeight uint16 | ||
| stdoutFile, validOut := inv.Stdout.(*os.File) | ||
| @@ -149,6 +142,15 @@ func handleRPTY(inv *serpent.Invocation, client *codersdk.Client, args handleRPT | ||
| }() | ||
| } | ||
| // If a user does not specify a command, we'll assume they intend to open an | ||
| // interactive shell. | ||
| var backend string | ||
| if isOneShotCommand(args.Command) { | ||
| // If the user specified a command, we'll prefer to use the buffered method. | ||
| // The screen backend is not well suited for one-shot commands. | ||
| backend = "buffered" | ||
| } | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Suggestion: It'd be nice to see this as a CLI flag too, to ( Side-note: I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, I considered having it a CLI flag, but hesitated exposing this internal implementation detail. It does make sense, especially with your | ||
| conn, err := workspacesdk.New(client).AgentReconnectingPTY(ctx, workspacesdk.WorkspaceAgentReconnectingPTYOpts{ | ||
| AgentID: agt.ID, | ||
| Reconnect: reconnectID, | ||
| @@ -157,14 +159,13 @@ func handleRPTY(inv *serpent.Invocation, client *codersdk.Client, args handleRPT | ||
| ContainerUser: args.ContainerUser, | ||
| Width: termWidth, | ||
| Height: termHeight, | ||
| BackendType: backend, | ||
| }) | ||
| if err != nil { | ||
| return xerrors.Errorf("open reconnecting PTY: %w", err) | ||
| } | ||
| defer conn.Close() | ||
| closeUsage := client.UpdateWorkspaceUsageWithBodyContext(ctx, ws.ID, codersdk.PostWorkspaceUsageRequest{ | ||
| AgentID: agt.ID, | ||
| AppName: codersdk.UsageAppNameReconnectingPty, | ||
| @@ -210,7 +211,21 @@ func handleRPTY(inv *serpent.Invocation, client *codersdk.Client, args handleRPT | ||
| _, _ = io.Copy(inv.Stdout, conn) | ||
| cancel() | ||
| _ = conn.Close() | ||
| return nil | ||
| } | ||
| var knownShells = []string{"ash", "bash", "csh", "dash", "fish", "ksh", "powershell", "pwsh", "zsh"} | ||
| func isOneShotCommand(cmd []string) bool { | ||
| // If the command is empty, we'll assume the user wants to open a shell. | ||
| if len(cmd) == 0 { | ||
| return false | ||
| } | ||
| // If the command is a single word, and that word is a known shell, we'll | ||
| // assume the user wants to open a shell. | ||
| if len(cmd) == 1 && slice.Contains(knownShells, cmd[0]) { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This has some interesting repercussions when it's neither a shell and not a one-shot command. Imagine doing This is probably fine though, but I'm flagging this behavior as it's not ideal. At the very least worth a comment. (The flag would alleviate this as you can enforce the behavior then.) | ||
Uh oh!
There was an error while loading.Please reload this page.