fix: revert signal pass through handling (#96)

* Revert "fix: add a 1m timeout to signal shutdown (#92)" This reverts commit 0d37a62. * Revert "fix: pass through signals to inner container (#83)" This reverts commit c07d2c2.
coder · Aug 22, 2024 · f609e64 · f609e64
1 parent 9b6f446
commit f609e64
Show file tree

Hide file tree

Showing 11 changed files with 44 additions and 322 deletions.
diff --git a/README.md b/README.md
@@ -27,7 +27,6 @@ The environment variables can be used to configure various aspects of the inner
 | `CODER_CPUS`                   | Dictates the number of CPUs to allocate the inner container. It is recommended to set this using the Kubernetes [Downward API](https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-container-fields-as-values-for-environment-variables).                                                                                                                                                                                                                                | false    |
 | `CODER_MEMORY`                 | Dictates the max memory (in bytes) to allocate the inner container. It is recommended to set this using the Kubernetes [Downward API](https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-container-fields-as-values-for-environment-variables).                                                                                                                                                                                                                         | false    |
 | `CODER_DISABLE_IDMAPPED_MOUNT` | Disables idmapped mounts in sysbox. For more information, see the [Sysbox Documentation](https://github.com/nestybox/sysbox/blob/master/docs/user-guide/configuration.md#disabling-id-mapped-mounts-on-sysbox).                                                                                                                                                                                                                                                                                                                | false    |
-| `CODER_SHUTDOWN_TIMEOUT`       | Configure a custom shutdown timeout to wait for the boostrap command to exit. Defaults to 1 minute.                                                                                                                                                                                                                                                                                                                                                                                                                            | false    |
 
 ## Coder Template
 

diff --git a/cli/clitest/cli.go b/cli/clitest/cli.go
@@ -56,7 +56,7 @@ func New(t *testing.T, cmd string, args ...string) (context.Context, *cobra.Comm
 		ctx    = ctx(t, fs, execer, mnt, client)
 	)
 
-	root := cli.Root(nil)
+	root := cli.Root()
 	// This is the one thing that isn't really mocked for the tests.
 	// I cringe at the thought of introducing yet another mock so
 	// let's avoid it for now.

diff --git a/cli/docker.go b/cli/docker.go
@@ -7,13 +7,11 @@ import (
 	"io"
 	"net/url"
 	"os"
-	"os/exec"
 	"path"
 	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
-	"time"
 
 	dockertypes "github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
@@ -40,10 +38,6 @@ const (
 	// EnvBoxContainerName is the name of the inner user container.
 	EnvBoxPullImageSecretEnvVar = "CODER_IMAGE_PULL_SECRET" //nolint:gosec
 	EnvBoxContainerName         = "CODER_CVM_CONTAINER_NAME"
-	// We define a custom exit code to distinguish from the generic '1' when envbox exits due to a shutdown timeout.
-	// Docker claims exit codes 125-127 so we start at 150 to
-	// ensure we don't collide.
-	ExitCodeShutdownTimeout = 150
 )
 
 const (
@@ -82,9 +76,8 @@ const (
 	// with UID/GID 1000 will be mapped to `UserNamespaceOffset` + 1000
 	// on the host. Changing this value will result in improper mappings
 	// on existing containers.
-	UserNamespaceOffset    = 100000
-	devDir                 = "/dev"
-	defaultShutdownTimeout = time.Minute
+	UserNamespaceOffset = 100000
+	devDir              = "/dev"
 )
 
 var (
@@ -108,7 +101,6 @@ var (
 	EnvDockerConfig         = "CODER_DOCKER_CONFIG"
 	EnvDebug                = "CODER_DEBUG"
 	EnvDisableIDMappedMount = "CODER_DISABLE_IDMAPPED_MOUNT"
-	EnvShutdownTimeout      = "CODER_SHUTDOWN_TIMEOUT"
 )
 
 var envboxPrivateMounts = map[string]struct{}{
@@ -146,15 +138,14 @@ type flags struct {
 	cpus                 int
 	memory               int
 	disableIDMappedMount bool
-	shutdownTimeout      time.Duration
 
 	// Test flags.
 	noStartupLogs bool
 	debug         bool
 	ethlink       string
 }
 
-func dockerCmd(ch chan func() error) *cobra.Command {
+func dockerCmd() *cobra.Command {
 	var flags flags
 
 	cmd := &cobra.Command{
@@ -295,7 +286,7 @@ func dockerCmd(ch chan func() error) *cobra.Command {
 				return xerrors.Errorf("wait for dockerd: %w", err)
 			}
 
-			err = runDockerCVM(ctx, log, client, blog, ch, flags)
+			err = runDockerCVM(ctx, log, client, blog, flags)
 			if err != nil {
 				// It's possible we failed because we ran out of disk while
 				// pulling the image. We should restart the daemon and use
@@ -324,7 +315,7 @@ func dockerCmd(ch chan func() error) *cobra.Command {
 					}()
 
 					log.Debug(ctx, "reattempting container creation")
-					err = runDockerCVM(ctx, log, client, blog, ch, flags)
+					err = runDockerCVM(ctx, log, client, blog, flags)
 				}
 				if err != nil {
 					blog.Errorf("Failed to run envbox: %v", err)
@@ -358,7 +349,6 @@ func dockerCmd(ch chan func() error) *cobra.Command {
 	cliflag.IntVarP(cmd.Flags(), &flags.cpus, "cpus", "", EnvCPUs, 0, "Number of CPUs to allocate inner container. e.g. 2")
 	cliflag.IntVarP(cmd.Flags(), &flags.memory, "memory", "", EnvMemory, 0, "Max memory to allocate to the inner container in bytes.")
 	cliflag.BoolVarP(cmd.Flags(), &flags.disableIDMappedMount, "disable-idmapped-mount", "", EnvDisableIDMappedMount, false, "Disable idmapped mounts in sysbox. Note that you may need an alternative (e.g. shiftfs).")
-	cliflag.DurationVarP(cmd.Flags(), &flags.shutdownTimeout, "shutdown-timeout", "", EnvShutdownTimeout, defaultShutdownTimeout, "Duration after which envbox will be forcefully terminated.")
 
 	// Test flags.
 	cliflag.BoolVarP(cmd.Flags(), &flags.noStartupLogs, "no-startup-log", "", "", false, "Do not log startup logs. Useful for testing.")
@@ -368,7 +358,7 @@ func dockerCmd(ch chan func() error) *cobra.Command {
 	return cmd
 }
 
-func runDockerCVM(ctx context.Context, log slog.Logger, client dockerutil.DockerClient, blog buildlog.Logger, shutdownCh chan func() error, flags flags) error {
+func runDockerCVM(ctx context.Context, log slog.Logger, client dockerutil.DockerClient, blog buildlog.Logger, flags flags) error {
 	fs := xunix.GetFS(ctx)
 
 	// Set our OOM score to something really unfavorable to avoid getting killed
@@ -688,87 +678,36 @@ func runDockerCVM(ctx context.Context, log slog.Logger, client dockerutil.Docker
 	}
 
 	blog.Info("Envbox startup complete!")
-	if flags.boostrapScript == "" {
-		return nil
-	}
-
-	bootstrapExec, err := client.ContainerExecCreate(ctx, containerID, dockertypes.ExecConfig{
-		User:         imgMeta.UID,
-		Cmd:          []string{"/bin/sh", "-s"},
-		Env:          []string{fmt.Sprintf("BINARY_DIR=%s", bootDir)},
-		AttachStdin:  true,
-		AttachStdout: true,
-		AttachStderr: true,
-		Detach:       true,
-	})
-	if err != nil {
-		return xerrors.Errorf("create exec: %w", err)
-	}
-
-	resp, err := client.ContainerExecAttach(ctx, bootstrapExec.ID, dockertypes.ExecStartCheck{})
-	if err != nil {
-		return xerrors.Errorf("attach exec: %w", err)
-	}
 
-	_, err = io.Copy(resp.Conn, strings.NewReader(flags.boostrapScript))
-	if err != nil {
-		return xerrors.Errorf("copy stdin: %w", err)
-	}
-	err = resp.CloseWrite()
-	if err != nil {
-		return xerrors.Errorf("close write: %w", err)
-	}
-
-	go func() {
-		defer resp.Close()
-		rd := io.LimitReader(resp.Reader, 1<<10)
-		_, err := io.Copy(blog, rd)
-		if err != nil {
-			log.Error(ctx, "copy bootstrap output", slog.Error(err))
-		}
-	}()
-
-	// We can't just call ExecInspect because there's a race where the cmd
-	// hasn't been assigned a PID yet.
-	bootstrapPID, err := dockerutil.GetExecPID(ctx, client, bootstrapExec.ID)
+	// The bootstrap script doesn't return since it execs the agent
+	// meaning that it can get pretty noisy if we were to log by default.
+	// In order to allow users to discern issues getting the bootstrap script
+	// to complete successfully we pipe the output to stdout if
+	// CODER_DEBUG=true.
+	debugWriter := io.Discard
+	if flags.debug {
+		debugWriter = os.Stdout
+	}
+	// Bootstrap the container if a script has been provided.
+	blog.Infof("Bootstrapping workspace...")
+	err = dockerutil.BootstrapContainer(ctx, client, dockerutil.BootstrapConfig{
+		ContainerID: containerID,
+		User:        imgMeta.UID,
+		Script:      flags.boostrapScript,
+		// We set this because the default behavior is to download the agent
+		// to /tmp/coder.XXXX. This causes a race to happen where we finish
+		// downloading the binary but before we can execute systemd remounts
+		// /tmp.
+		Env:       []string{fmt.Sprintf("BINARY_DIR=%s", bootDir)},
+		StdOutErr: debugWriter,
+	})
 	if err != nil {
-		return xerrors.Errorf("exec inspect: %w", err)
+		return xerrors.Errorf("boostrap container: %w", err)
 	}
 
-	shutdownCh <- killBootstrapCmd(ctx, log, bootstrapPID, bootstrapExec.ID, client, flags.shutdownTimeout)
-
 	return nil
 }
 
-// KillBootstrapCmd is the command we run when we receive a signal
-// to kill the envbox container.
-func killBootstrapCmd(ctx context.Context, log slog.Logger, pid int, execID string, client dockerutil.DockerClient, timeout time.Duration) func() error {
-	return func() error {
-		log.Debug(ctx, "killing container",
-			slog.F("bootstrap_pid", pid),
-			slog.F("timeout", timeout.String()),
-		)
-
-		ctx, cancel := context.WithTimeout(ctx, timeout)
-		defer cancel()
-		// The PID returned is the PID _outside_ the container...
-		//nolint:gosec
-		out, err := exec.CommandContext(ctx, "kill", "-TERM", strconv.Itoa(pid)).CombinedOutput()
-		if err != nil {
-			return xerrors.Errorf("kill bootstrap process (%s): %w", out, err)
-		}
-
-		log.Debug(ctx, "sent kill signal waiting for process to exit")
-		err = dockerutil.WaitForExit(ctx, client, execID)
-		if err != nil {
-			return xerrors.Errorf("wait for exit: %w", err)
-		}
-
-		log.Debug(ctx, "bootstrap process successfully exited")
-		return nil
-	}
-}
-
 //nolint:revive
 func dockerdArgs(link, cidr string, isNoSpace bool) ([]string, error) {
 	// We need to adjust the MTU for the host otherwise packets will fail delivery.

diff --git a/cli/root.go b/cli/root.go
@@ -4,7 +4,7 @@ import (
 	"github.com/spf13/cobra"
 )
 
-func Root(ch chan func() error) *cobra.Command {
+func Root() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:           "envbox",
 		SilenceErrors: true,
@@ -15,6 +15,6 @@ func Root(ch chan func() error) *cobra.Command {
 		},
 	}
 
-	cmd.AddCommand(dockerCmd(ch))
+	cmd.AddCommand(dockerCmd())
 	return cmd
 }
diff --git a/cmd/envbox/main.go b/cmd/envbox/main.go
@@ -1,51 +1,20 @@
 package main
 
 import (
-	"context"
 	"fmt"
 	"os"
-	"os/signal"
 	"runtime"
-	"syscall"
 
-	"golang.org/x/xerrors"
-
-	"cdr.dev/slog"
-	"cdr.dev/slog/sloggers/slogjson"
 	"github.com/coder/envbox/cli"
 )
 
 func main() {
-	ch := make(chan func() error, 1)
-	sigs := make(chan os.Signal, 1)
-	signal.Notify(sigs, syscall.SIGTERM, syscall.SIGINT, syscall.SIGWINCH)
-	go func() {
-		ctx := context.Background()
-		log := slog.Make(slogjson.Sink(os.Stderr))
-		log.Info(ctx, "waiting for signal")
-		<-sigs
-		log.Info(ctx, "got signal")
-		select {
-		case fn := <-ch:
-			log.Info(ctx, "running shutdown function")
-			err := fn()
-			if err != nil {
-				log.Error(ctx, "shutdown function failed", slog.Error(err))
-				if xerrors.Is(err, context.DeadlineExceeded) {
-					os.Exit(cli.ExitCodeShutdownTimeout)
-				}
-				os.Exit(1)
-			}
-		default:
-			log.Info(ctx, "no shutdown function")
-		}
-		log.Info(ctx, "exiting")
-		os.Exit(0)
-	}()
-	_, err := cli.Root(ch).ExecuteC()
+	_, err := cli.Root().ExecuteC()
 	if err != nil {
 		_, _ = fmt.Fprintln(os.Stderr, err.Error())
 		os.Exit(1)
 	}
+
+	// We exit the main thread while keepin all the other procs goin strong.
 	runtime.Goexit()
 }
diff --git a/dockerutil/container.go b/dockerutil/container.go
@@ -113,7 +113,7 @@ func BootstrapContainer(ctx context.Context, client DockerClient, conf Bootstrap
 
 	var err error
 	for r, n := retry.New(time.Second, time.Second*2), 0; r.Wait(ctx) && n < 10; n++ {
-		var out io.Reader
+		var out []byte
 		out, err = ExecContainer(ctx, client, ExecConfig{
 			ContainerID: conf.ContainerID,
 			User:        conf.User,
@@ -122,16 +122,9 @@ func BootstrapContainer(ctx context.Context, client DockerClient, conf Bootstrap
 			Stdin:       strings.NewReader(conf.Script),
 			Env:         conf.Env,
 			StdOutErr:   conf.StdOutErr,
-			Detach:      conf.Detach,
 		})
 		if err != nil {
-			output, rerr := io.ReadAll(out)
-			if rerr != nil {
-				err = xerrors.Errorf("read all: %w", err)
-				continue
-			}
-
-			err = xerrors.Errorf("boostrap container (%s): %w", output, err)
+			err = xerrors.Errorf("boostrap container (%s): %w", out, err)
 			continue
 		}
 		break

diff --git a/dockerutil/dockerfake/client.go b/dockerutil/dockerfake/client.go
@@ -162,9 +162,7 @@ func (m MockClient) ContainerExecCreate(ctx context.Context, name string, config
 
 func (m MockClient) ContainerExecInspect(ctx context.Context, id string) (dockertypes.ContainerExecInspect, error) {
 	if m.ContainerExecInspectFn == nil {
-		return dockertypes.ContainerExecInspect{
-			Pid: 1,
-		}, nil
+		return dockertypes.ContainerExecInspect{}, nil
 	}
 
 	return m.ContainerExecInspectFn(ctx, id)