Both Docker and Podman populate non-existent VOLUMES on first mention…

…; stop pre-creating them (#60)

* Revert "Add podman support by omitting the --name arg from volume creation. (#58)"

This reverts commit 3e8b45b.

* Stop creating the VOLUME before just using it - this allows podman to populate it as well. Also switch order of deletion so hab is deleted first.

* Stop wrapping errors.

* Remove invalid test cases and fix expectations for hab teardown first.

* Put the sudo back in expectations for sudo test.

Co-authored-by: Sheridan Rawlins <scr@dev122.sieve.corp.gq1.yahoo.com>

cmd/build.go

@@ -73,7 +73,6 @@ func newBuildCmd() *cobra.Command {
 	var metaFilePath string
 	var socketPath string
 	var localVolumes []string
-	var dockerIsPodman bool
 
 	buildCmd := &cobra.Command{
 		Use:   "build [job name]",
@@ -251,7 +250,6 @@ func newBuildCmd() *cobra.Command {
 				SocketPath:      socketPath,
 				FlagVerbose:     flagVerbose,
 				LocalVolumes:    localVolumes,
-				DockerIsPodman:  dockerIsPodman,
 			}
 
 			launch := launchNew(option)
@@ -353,15 +351,5 @@ ex) git@github.com:<org>/<repo>.git[#<branch>]
 		[]string{},
 		"Volumes to mount into build container.")
 
-	defaultDockerIsPodman, err := launch.DockerIsPodman()
-	if err != nil {
-		logrus.Errorf("Error determining whether docker is podman; assuming it is not")
-		defaultDockerIsPodman = false
-	}
-	buildCmd.Flags().BoolVar(&dockerIsPodman,
-		"dockerIsPodman",
-		defaultDockerIsPodman,
-		"Whether docker is podman")
-
 	return buildCmd
 }

cmd/build_test.go

@@ -20,7 +20,6 @@ Usage:
 
 Flags:
       --artifacts-dir string   Path to the host side directory which is mounted into $SD_ARTIFACTS_DIR. (default "sd-artifacts")
-      --dockerIsPodman         Whether docker is podman
   -e, --env stringToString     Set key and value relationship which is set as environment variables of Build Container. (<key>=<value>) (default [])
       --env-file string        Path to config file of environment variables. '.env' format file can be used.
   -h, --help                   help for build

cmd/root_test.go

@@ -108,7 +108,6 @@ Usage:
 
 Flags:
       --artifacts-dir string   Path to the host side directory which is mounted into $SD_ARTIFACTS_DIR. (default "sd-artifacts")
-      --dockerIsPodman         Whether docker is podman
   -e, --env stringToString     Set key and value relationship which is set as environment variables of Build Container. (<key>=<value>) (default [])
       --env-file string        Path to config file of environment variables. '.env' format file can be used.
   -h, --help                   help for build

launch/docker.go

@@ -31,7 +31,6 @@ type docker struct {
 	interact          Interacter
 	socketPath        string
 	localVolumes      []string
-	dockerIsPodman    bool
 }
 
 var _ runner = (*docker)(nil)
@@ -47,17 +46,7 @@ const (
 	orgRepo = "sd-local/local-build"
 )
 
-// DockerIsPodman determines whether docker is podman by asking its version and looking for "podman".
-func DockerIsPodman() (bool, error) {
-	c := exec.Command("docker", "--version")
-	data, err := c.Output()
-	if err != nil {
-		return false, fmt.Errorf("cannot determine whether docker is podman: %w", err)
-	}
-	return strings.HasPrefix(string(data), "podman"), nil
-}
-
-func newDocker(setupImage, setupImageVer string, useSudo bool, interactiveMode bool, socketPath string, flagVerbose bool, localVolumes []string, dockerIsPodman bool) runner {
+func newDocker(setupImage, setupImageVer string, useSudo bool, interactiveMode bool, socketPath string, flagVerbose bool, localVolumes []string) runner {
 	return &docker{
 		volume:            "SD_LAUNCH_BIN",
 		habVolume:         "SD_LAUNCH_HAB",
@@ -71,36 +60,23 @@ func newDocker(setupImage, setupImageVer string, useSudo bool, interactiveMode b
 		interact:          &Interact{},
 		socketPath:        socketPath,
 		localVolumes:      localVolumes,
-		dockerIsPodman:    dockerIsPodman,
 	}
 }
 
 func (d *docker) setupBin() error {
-	args := []string{"volume", "create"}
-	if !d.dockerIsPodman {
-		args = append(args, "--name")
-	}
-	args = append(args, d.volume)
-
-	_, err := d.execDockerCommand(args...)
-	if err != nil {
-		return fmt.Errorf("failed to create docker volume: %v", err)
-	}
-
-	args[len(args)-1] = d.habVolume
-	_, err = d.execDockerCommand(args...)
-	if err != nil {
-		return fmt.Errorf("failed to create docker hab volume: %v", err)
-	}
-
 	mount := fmt.Sprintf("%s:/opt/sd/", d.volume)
 	habMount := fmt.Sprintf("%s:/hab", d.habVolume)
 	image := fmt.Sprintf("%s:%s", d.setupImage, d.setupImageVersion)
-	_, err = d.execDockerCommand("pull", image)
+	_, err := d.execDockerCommand("pull", image)
 	if err != nil {
 		return fmt.Errorf("failed to pull launcher image: %v", err)
 	}
 
+	// The mechanism for population is that VOLUMEs were declared in the image, so they copy what was in their layer to
+	// the mounted location on first mount of non-existing volumes
+	// NOTE: docker allows copying to first-time mounted as well, but both docker and podman copy to non-existing ones.
+	//       therefore, volumes are not pre-created, but created on first mention by the image that populates them
+	//       and then used by subsequent images that then use their content.
 	_, err = d.execDockerCommand("container", "run", "--rm", "-v", mount, "-v", habMount, "--entrypoint", "/bin/echo", image, "set up bin")
 	if err != nil {
 		return fmt.Errorf("failed to prepare build scripts: %v", err)
@@ -273,13 +249,14 @@ func (d *docker) kill(sig os.Signal) {
 }
 
 func (d *docker) clean() {
-	_, err := d.execDockerCommand("volume", "rm", "--force", d.volume)
+	// Since the habVolume is mounted inside the mountpoint for volume, it must be removed first.
+	_, err := d.execDockerCommand("volume", "rm", "--force", d.habVolume)
 
 	if err != nil {
 		logrus.Warn(fmt.Errorf("failed to remove volume: %v", err))
 	}
 
-	_, err = d.execDockerCommand("volume", "rm", "--force", d.habVolume)
+	_, err = d.execDockerCommand("volume", "rm", "--force", d.volume)
 
 	if err != nil {
 		logrus.Warn(fmt.Errorf("failed to remove hab volume: %v", err))

launch/docker_test.go

@@ -66,7 +66,7 @@ func TestNewDocker(t *testing.T) {
 			localVolumes:      []string{"path:path"},
 		}
 
-		d := newDocker("launcher", "latest", false, false, "/auth.sock", false, []string{"path:path"}, false)
+		d := newDocker("launcher", "latest", false, false, "/auth.sock", false, []string{"path:path"})
 
 		assert.Equal(t, expected, d)
 	})
@@ -89,7 +89,6 @@ func TestSetupBin(t *testing.T) {
 		expectError error
 	}{
 		{"success", "SUCCESS_SETUP_BIN", nil},
-		{"failure volume create", "FAIL_CREATING_VOLUME", fmt.Errorf("failed to create docker volume: exit status 1")},
 		{"failure container run", "FAIL_CONTAINER_RUN", fmt.Errorf("failed to prepare build scripts: exit status 1")},
 		{"failure launcher image pull", "FAIL_LAUNCHER_PULL", fmt.Errorf("failed to pull launcher image: exit status 1")},
 	}
@@ -123,7 +122,6 @@ func TestSetupBinWithSudo(t *testing.T) {
 		expectError error
 	}{
 		{"success", "SUCCESS_SETUP_BIN_SUDO", nil},
-		{"failure volume create", "FAIL_CREATING_VOLUME_SUDO", fmt.Errorf("failed to create docker volume: exit status 1")},
 		{"failure container run", "FAIL_CONTAINER_RUN_SUDO", fmt.Errorf("failed to prepare build scripts: exit status 1")},
 		{"failure launcher image pull", "FAIL_LAUNCHER_PULL_SUDO", fmt.Errorf("failed to pull launcher image: exit status 1")},
 	}
@@ -425,6 +423,7 @@ func TestDockerClean(t *testing.T) {
 		c := newFakeExecCommand("SUCCESS_TO_CLEAN")
 		execCommand = c.execCmd
 		d := &docker{
+			habVolume:         "SD_LAUNCH_HAB",
 			volume:            "SD_LAUNCH_BIN",
 			setupImage:        "launcher",
 			setupImageVersion: "latest",
@@ -433,7 +432,8 @@ func TestDockerClean(t *testing.T) {
 		}
 
 		d.clean()
-		assert.Equal(t, fmt.Sprintf("docker volume rm --force %v", d.volume), c.commands[0])
+		assert.Equal(t, fmt.Sprintf("docker volume rm --force %v", d.habVolume), c.commands[0])
+		assert.Equal(t, fmt.Sprintf("docker volume rm --force %v", d.volume), c.commands[1])
 	})
 
 	t.Run("success with sudo", func(t *testing.T) {
@@ -443,6 +443,7 @@ func TestDockerClean(t *testing.T) {
 		c := newFakeExecCommand("SUCCESS_TO_CLEAN")
 		execCommand = c.execCmd
 		d := &docker{
+			habVolume:         "SD_LAUNCH_HAB",
 			volume:            "SD_LAUNCH_BIN",
 			setupImage:        "launcher",
 			setupImageVersion: "latest",
@@ -451,7 +452,8 @@ func TestDockerClean(t *testing.T) {
 		}
 
 		d.clean()
-		assert.Equal(t, fmt.Sprintf("sudo docker volume rm --force %v", d.volume), c.commands[0])
+		assert.Equal(t, fmt.Sprintf("sudo docker volume rm --force %v", d.habVolume), c.commands[0])
+		assert.Equal(t, fmt.Sprintf("sudo docker volume rm --force %v", d.volume), c.commands[1])
 	})
 
 	t.Run("failure", func(t *testing.T) {

launch/launch.go

@@ -84,7 +84,6 @@ type Option struct {
 	SocketPath      string
 	FlagVerbose     bool
 	LocalVolumes    []string
-	DockerIsPodman  bool
 }
 
 const (
@@ -169,7 +168,7 @@ func createBuildEntry(option Option) buildEntry {
 func New(option Option) Launcher {
 	l := new(launch)
 
-	l.runner = newDocker(option.Entry.Launcher.Image, option.Entry.Launcher.Version, option.UseSudo, option.InteractiveMode, option.SocketPath, option.FlagVerbose, option.LocalVolumes, option.DockerIsPodman)
+	l.runner = newDocker(option.Entry.Launcher.Image, option.Entry.Launcher.Version, option.UseSudo, option.InteractiveMode, option.SocketPath, option.FlagVerbose, option.LocalVolumes)
 	l.buildEntry = createBuildEntry(option)
 
 	return l