Refactor Docker Init with `ContainerRuntime` Interface #1748

schnie · 2024-11-22T18:50:02Z

Description

Functionally, this really doesn't change much of anything. It is really just refactoring the startDocker() command to a new place. We've defined a new ContainerRuntime interface that will grow a bit in the next PR to cover container runtime lifecycle management functions. This places the original, directly in-line code into this new lifecycle management structure. We then call these functions from cobra pre-run hooks on our astro dev commands.

My original podman PR grew too large, so this is part 1 to get some of the basic structure in place for expansion, while not changing any behavior quite yet. A follow up PR will expand on the ContainerRuntime interface and add podman support.

🎟 Issue(s)

Related to https://github.com/astronomer/astro/issues/24344

🧪 Functional Testing

I've updated a few unit tests, refactored some, and added a few new for the new code paths.
All astro dev subcommands have been tested manually and confirmed to be working as expected. (we should automate this as one of our next steps)

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Ran make lint before taking out of draft
Added/updated applicable tests
Tested against Astro-API (if necessary).
Tested against Houston-API and Astronomer (if necessary).
Communicated to/tagged owners of respective clients potentially impacted by these changes.
Updated any related documentation

schnie · 2024-11-22T20:14:11Z

airflow/runtimes/podman_runtime.go

+package runtimes
+
+type PodmanRuntime struct{}
+
+func (p PodmanRuntime) Initialize() error {
+	return nil
+}


Coming in the next PR.

schnie · 2024-11-22T20:15:14Z

airflow/runtimes/docker.go

+func InitializeDocker(d DockerInitializer, timeoutSeconds int) error {
+	// Initialize spinner.
+	timeout := time.After(time.Duration(timeoutSeconds) * time.Second)
+	ticker := time.NewTicker(time.Duration(tickNum) * time.Millisecond)
+	s := spinner.New(spinnerCharSet, spinnerRefresh)
+	s.Suffix = containerRuntimeInitMessage
+	defer s.Stop()
+
+	// Execute `docker ps` to check if Docker is running.
+	_, err := d.CheckDockerCmd()
+
+	// If we didn't get an error, Docker is running, so we can return.
+	if err == nil {
+		return nil
+	}
+
+	// If we got an error, Docker is not running, so we attempt to start it.
+	_, err = d.OpenDockerCmd()
+	if err != nil {
+		return fmt.Errorf(dockerOpenNotice)
+	}
+
+	// Wait for Docker to start.
+	s.Start()
+	for {
+		select {
+		case <-timeout:
+			return fmt.Errorf(timeoutErrMsg)
+		case <-ticker.C:
+			_, err := d.CheckDockerCmd()
+			if err != nil {
+				continue
+			}
+			return nil
+		}
+	}
+}


This function is a refactored, more testable version of the previous startDocker() and waitForDocker() functions that we've deleted above.

schnie · 2024-11-22T20:15:40Z

airflow/docker_test.go

-func (s *Suite) TestStartDocker() {
-	s.Run("start docker success", func() {
-		counter := 0
-		cmdExec = func(cmd string, stdout, stderr io.Writer, args ...string) error {
-			switch cmd {
-			case "open":
-				return nil
-			case "docker":
-				if counter == 0 {
-					counter++
-					return errExecMock
-				}
-				return nil
-			default:
-				return errExecMock
-			}
-		}
-
-		err := startDocker()
-		s.NoError(err)
-	})
-
-	s.Run("start docker fail", func() {
-		timeoutNum = 5
-
-		cmdExec = func(cmd string, stdout, stderr io.Writer, args ...string) error {
-			switch cmd {
-			case "open":
-				return nil
-			case "docker":
-				return errExecMock
-			default:
-				return errExecMock
-			}
-		}
-		err := startDocker()
-		s.Contains(err.Error(), "timed out waiting for docker")
-	})
-}
-


Refactored and replaced below.

schnie · 2024-11-22T20:18:11Z

airflow/docker.go

-
-func startDocker() error {
-	containerRuntime, err := GetContainerRuntimeBinary()
-	if err != nil {
-		return err
-	}
-
-	buf := new(bytes.Buffer)
-	err = cmdExec(containerRuntime, buf, buf, "ps")
-	if err != nil {
-		// open docker
-		fmt.Println("\nDocker is not running. Starting up the Docker engine…")
-		err = cmdExec(OpenCmd, buf, os.Stderr, "-a", dockerCmd)
-		if err != nil {
-			return err
-		}
-		fmt.Println("\nIf you don't see Docker Desktop starting, exit this command and start it manually.")
-		fmt.Println("If you don't have Docker Desktop installed, install it (https://www.docker.com/products/docker-desktop/) and try again.")
-		fmt.Println("If you are using Colima or another Docker alternative, start the engine manually.")
-		// poll for docker
-		err = waitForDocker()
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func waitForDocker() error {
-	containerRuntime, err := GetContainerRuntimeBinary()
-	if err != nil {
-		return err
-	}
-
-	buf := new(bytes.Buffer)
-	timeout := time.After(time.Duration(timeoutNum) * time.Second)
-	ticker := time.NewTicker(time.Duration(tickNum) * time.Millisecond)
-	for {
-		select {
-		// Got a timeout! fail with a timeout error
-		case <-timeout:
-			return errors.New("timed out waiting for docker")
-		// Got a tick, we should check if docker is up & running
-		case <-ticker.C:
-			buf.Reset()
-			err := cmdExec(containerRuntime, buf, buf, "ps")
-			if err != nil {
-				continue
-			} else {
-				return nil
-			}
-		}
-	}
-}


This block of code (startDocker()) was previously called directly by the astro dev start handler. It's now been moved and ultimately called from our new cobra hooks - EnsureRuntime. This hook then ultimately calls the docker initialization code which now does this setup work. The refactored version is below.

schnie · 2024-11-22T20:19:00Z

airflow/runtimes/container_runtime.go

+// ContainerRuntime interface defines the methods that manage
+// the container runtime lifecycle.
+type ContainerRuntime interface {
+	Initialize() error
+}


This interface will grow to include more lifecycle functions when we follow up with podman support in the next PR.

schnie · 2024-11-22T20:23:20Z

cmd/airflow.go

+		// Most astro dev sub-commands require the container runtime,
+		// so we set that configuration in this persistent pre-run hook.
+		// A few sub-commands don't require this, so they explicitly
+		// clobber it with a no-op function.
+		PersistentPreRunE: ConfigureContainerRuntime,


Here we insert a new PersistentPreRunE function to setup the runtime for all the sub commands. This is a quick function and doesn't actually run any initialization steps that take time. When needed, the value set from this function is referenced and we run any lifecycle hooks that interact with the runtime.

With this PR, we're only running this on the astro dev start command, as the other commands explicitly clobber this hook. In the following PR for podman support, we'll utilize this much more.

The methods that are not yet using this functionality explicitly define their own PersistentPreRunE function of DoNothing. This was already happening previously with an inline function definiton. This is really just cleaning that up and defining all our hooks in a single place.

schnie · 2024-11-22T20:24:14Z

cmd/airflow.go

-		// ignore PersistentPreRunE of root command
-		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
-			return nil
-		},


All of these blocks are have been replaced with the same function - DoNothing.

schnie · 2024-11-22T20:25:22Z

airflow/runtimes/command.go

+package runtimes
+
+import (
+	"bytes"
+	"os/exec"
+)
+
+// Command represents a command to be executed.
+type Command struct {
+	Command string
+	Args    []string
+}
+
+// Execute runs the Podman command and returns the output.
+func (p *Command) Execute() (string, error) {
+	cmd := exec.Command(p.Command, p.Args...) //nolint:gosec
+	var out bytes.Buffer
+	cmd.Stdout = &out
+	cmd.Stderr = &out
+	err := cmd.Run()
+	return out.String(), err
+}


Just a generic command execution structure we'll use for shelling out docker, podman, etc commands.

schnie · 2024-11-22T23:01:13Z

airflow/docker.go

-	// check if docker is up for macOS
-	containerRuntime, err := GetContainerRuntimeBinary()
-	if err != nil {
-		return err
-	}
-	if runtime.GOOS == "darwin" && containerRuntime == dockerCmd {
-		err := startDocker()
-		if err != nil {
-			return err
-		}
-	}
-


This has been moved to a cobra PersistentPreRunE function named ConfiguredContainerRuntime and a PreRunE function named EnsureRuntime. These will be rolled out more broadly across the astro dev commands in the next PR.

schnie · 2024-11-22T23:06:58Z

airflow/docker_image.go

+	"github.com/astronomer/astro-cli/airflow/runtimes"
+


Just bumped all this new container runtime management stuff into a sub package runtimes, so that's just getting updated below.

schnie · 2024-11-24T03:17:46Z

airflow/container_runtime_test.go

This is just being moved and expanded within the sub-package runtimes.

schnie · 2024-11-24T03:39:50Z

cmd/airflow_test.go

-func (s *AirflowSuite) TestNewAirflowInitCmd() {
-	cmd := newAirflowInitCmd()
+func (s *AirflowSuite) TestNewAirflowDevRootCmd() {
+	cmd := newDevRootCmd(nil, nil)
 	s.Nil(cmd.PersistentPreRunE(new(cobra.Command), []string{}))
 }

-func (s *AirflowSuite) TestNewAirflowStartCmd() {
-	cmd := newAirflowStartCmd(nil)
+func (s *AirflowSuite) TestNewAirflowInitCmd() {
+	cmd := newAirflowInitCmd()


Weird looking change here, but I've:

removed the test for TestNewAirflowStartRootCmd, the PersistentPreRunE function has been moved to the parent command so this test fails and really isn't necessary any more.

added the test for the TestNewAirflowDevRootCmd since it now holds the shared PersistentPreRunE function.

left the test for TestNewAirflowInitCmd in place.

jeremybeard

This is excellent, I really like the new structure as a way of treating the runtime options equally. Left a couple of small comments.

jeremybeard · 2024-11-25T15:45:06Z

airflow/runtimes/container_runtime.go


 	"github.com/astronomer/astro-cli/config"
 	"github.com/astronomer/astro-cli/pkg/fileutil"
 	"github.com/astronomer/astro-cli/pkg/util"
 	"github.com/pkg/errors"
 )

+var spinnerCharSet = spinner.CharSets[14]


Do we maybe want to match (or reuse? or change?) the charset in pkg/ansi/spinner.go for consistency?

Looks like we created a custom one for login, etc. I think standardizing would be good but I'm trying to keep my changes to a minimum and keep them highly correlated. I've started and stopped this PR a few times because I've gone too far down the refactor path. I do plan to keep going, and intend to standardize as much as possible as we go it'll just be in targeted chunks.

This particular spinner charset actually mimics the ones that docker-compose uses so it looks somewhat intentional at the moment since we're using it for the messages where we're dealing with containers and the runtimes, etc.

Ok fair enough. I appreciate the refactor considering it can feel like a bit of a never-ending rabbit hole.

jeremybeard · 2024-11-25T15:46:40Z

airflow/runtimes/container_runtime.go

+var spinnerCharSet = spinner.CharSets[14]
+
+const (
+	docker                         = "docker"


Do we need these verbatim constants? I'd think if the value changed we'd probably just rename the constant anyway.

What are you thinking? Change them back to dockerCmd, etc?

Could just use the literal string.

jeremybeard · 2024-11-25T15:51:23Z

airflow/runtimes/docker.go

+// DockerInitializer is a struct that contains the functions needed to initialize Docker.
+// The concrete implementation that we use is DefaultDockerInitializer below.
+// When running the tests, we substitute the default implementation with a mock implementation.
+type DockerInitializer struct {


This looks like it should be an interface, what do you think?

Ah yea good call, meant to do that. It's now an interface and I've updated the mock implementation and tests accordingly.

pritt20

Tested the PR changes locally by setting up local astro-project using docker and changes work as expected.

Thanks @schnie ! 🚀

Refactor docker initialization

3da4dd3

schnie commented Nov 22, 2024

View reviewed changes

schnie force-pushed the container-runtime-interface branch from ec87aef to 98016bf Compare November 22, 2024 20:26

Clean up tests

0ad0bbc

schnie force-pushed the container-runtime-interface branch from 98016bf to 0ad0bbc Compare November 22, 2024 22:56

schnie commented Nov 22, 2024

View reviewed changes

schnie force-pushed the container-runtime-interface branch from 588de54 to a5c314d Compare November 22, 2024 23:44

schnie commented Nov 24, 2024

View reviewed changes

airflow/container_runtime_test.go Outdated

Copy link

Member Author

schnie Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just being moved and expanded within the sub-package runtimes.

schnie force-pushed the container-runtime-interface branch from a5c314d to da610b6 Compare November 24, 2024 03:27

Add tests for GetContainerRuntime()

5097786

schnie force-pushed the container-runtime-interface branch from da610b6 to 5097786 Compare November 24, 2024 03:29

schnie commented Nov 24, 2024

View reviewed changes

schnie requested a review from pritt20 November 24, 2024 03:40

Merge branch 'main' into container-runtime-interface

c5d2aa6

schnie marked this pull request as ready for review November 24, 2024 03:41

schnie requested review from neel-astro, jeremybeard, kushalmalani and lzdanski as code owners November 24, 2024 03:41

schnie changed the title ~~Refactor docker initialization~~ Refactor Docker Initialization, Adds ContainerRuntime Interface Nov 24, 2024

schnie changed the title ~~Refactor Docker Initialization, Adds ContainerRuntime Interface~~ Refactor Docker Init with ContainerRuntime Interface Nov 24, 2024

jeremybeard reviewed Nov 25, 2024

View reviewed changes

Refactor DockerIntializer into interface

43c8a93

schnie force-pushed the container-runtime-interface branch from 634a68e to 43c8a93 Compare November 25, 2024 18:18

jeremybeard approved these changes Nov 25, 2024

View reviewed changes

Fix linting issue

aeeb1c5

pritt20 approved these changes Nov 26, 2024

View reviewed changes

schnie merged commit 8316e9a into main Nov 26, 2024
3 checks passed

schnie deleted the container-runtime-interface branch November 26, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Docker Init with `ContainerRuntime` Interface #1748

Refactor Docker Init with `ContainerRuntime` Interface #1748

schnie commented Nov 22, 2024 •

edited

Loading

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 22, 2024 •

edited

Loading

schnie Nov 22, 2024 •

edited

Loading

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 22, 2024

schnie Nov 24, 2024

schnie Nov 24, 2024

jeremybeard left a comment

jeremybeard Nov 25, 2024

schnie Nov 25, 2024

jeremybeard Nov 25, 2024

jeremybeard Nov 25, 2024

schnie Nov 25, 2024

jeremybeard Nov 25, 2024

jeremybeard Nov 25, 2024

schnie Nov 25, 2024

pritt20 left a comment

Refactor Docker Init with ContainerRuntime Interface #1748

Refactor Docker Init with ContainerRuntime Interface #1748

Conversation

schnie commented Nov 22, 2024 • edited Loading

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

📋 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schnie Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

schnie Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremybeard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritt20 left a comment

Choose a reason for hiding this comment

Refactor Docker Init with `ContainerRuntime` Interface #1748

Refactor Docker Init with `ContainerRuntime` Interface #1748

schnie commented Nov 22, 2024 •

edited

Loading

schnie Nov 22, 2024 •

edited

Loading

schnie Nov 22, 2024 •

edited

Loading