Adds Container Runtime Binary Autodetection #1738

schnie · 2024-10-29T16:58:00Z

Description

This PR adds the ability for the CLI to autodetect the container runtime binary to use for the commands that utilize containers. If the already existing configuration (containers.binary) is set in the config.yaml file, we use that, otherwise we search file paths in $PATH for docker, then podman. Others can be added if needed.

Previously all commands that used the container runtime would grab the configuration directly (config.CFG.DockerCommand.GetString()). We've replaced all instances of this with calls to a new function that wraps up this configuration and layers on the auto-detection piece.

This is a small piece of a larger project to simplify getting started with Astro CLI. We're also exploring the idea of bundling podman with our package and using it as our default, rather than Docker Desktop. This is part of that effort, but also just useful on its own.

New functionality is in the new files here. The other files are mostly just adapting to the new method signature.

This effectively makes the highlighted command show below unnecessary when using podman.

🎟 Issue(s)

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

🧪 Functional Testing

Tested on Mac and Windows with Docker Desktop, Orbstack (docker binary), and Podman.

Mac OS

astro dev start with no container runtime installed

❯ /Users/schnie/repos/astro-cli/astro dev start
Error: Failed to find a container runtime. See the Astro CLI prerequisites for more information. https://www.astronomer.io/docs/astro/cli/install-cli

astro dev start after running brew install podman
Automatically detects podman binary and uses it.

❯ podman ps
CONTAINER ID  IMAGE                                      COMMAND               CREATED       STATUS                   PORTS                               NAMES
b6bfe6d579f1  docker.io/library/postgres:12.6            postgres              24 hours ago  Up 9 seconds             127.0.0.1:5432->5432/tcp, 5432/tcp  test-dags_434792-postgres-1
abfede8c9e6b  localhost/test-dags_434792/airflow:latest  bash -c (airflow ...  24 hours ago  Up 9 seconds                                                 test-dags_434792-scheduler-1
767acd8d3b6a  localhost/test-dags_434792/airflow:latest  bash -c (airflow ...  24 hours ago  Up 9 seconds                                                 test-dags_434792-triggerer-1
8e927abccec2  localhost/test-dags_434792/airflow:latest  bash -c if [[ -z ...  24 hours ago  Up 8 seconds (starting)  127.0.0.1:8080->8080/tcp, 8080/tcp  test-dags_434792-webserver-1

astro dev start after additionally running brew install orbstack (docker binary)
Both binaries are installed and docker is selected, as it's first in our list.

❯ docker ps
CONTAINER ID   IMAGE                             COMMAND                  CREATED        STATUS                    PORTS                      NAMES
868eba18c5c9   test-dags_434792/airflow:latest   "tini -- /entrypoint…"   27 hours ago   Up 29 seconds (healthy)   127.0.0.1:8080->8080/tcp   test-dags_434792-webserver-1
d35e13b7616b   test-dags_434792/airflow:latest   "tini -- /entrypoint…"   27 hours ago   Up 30 seconds                                        test-dags_434792-scheduler-1
b9f4e14b0a4f   test-dags_434792/airflow:latest   "tini -- /entrypoint…"   27 hours ago   Up 30 seconds                                        test-dags_434792-triggerer-1
868f8a3ce33b   postgres:12.6                     "docker-entrypoint.s…"   3 weeks ago    Up 31 seconds             127.0.0.1:5432->5432/tcp   test-dags_434792-postgres-1
❯ podman ps
Cannot connect to Podman. Please verify your connection to the Linux system using `podman system connection list`, or try `podman machine init` and `podman machine start` to manage a new Linux VM
Error: unable to connect to Podman socket: failed to connect: dial tcp 127.0.0.1:61555: connect: connection refused

astro dev start after installing both binaries, but manually overriding to podman with the config file

❯ /Users/schnie/repos/astro-cli/astro config set container.binary podman -g
Setting container.binary to podman successfully
❯ podman ps
CONTAINER ID  IMAGE                                      COMMAND               CREATED       STATUS                   PORTS                               NAMES
b6bfe6d579f1  docker.io/library/postgres:12.6            postgres              24 hours ago  Up 9 seconds             127.0.0.1:5432->5432/tcp, 5432/tcp  test-dags_434792-postgres-1
abfede8c9e6b  localhost/test-dags_434792/airflow:latest  bash -c (airflow ...  24 hours ago  Up 9 seconds                                                 test-dags_434792-scheduler-1
767acd8d3b6a  localhost/test-dags_434792/airflow:latest  bash -c (airflow ...  24 hours ago  Up 9 seconds                                                 test-dags_434792-triggerer-1
8e927abccec2  localhost/test-dags_434792/airflow:latest  bash -c if [[ -z ...  24 hours ago  Up 8 seconds (starting)  127.0.0.1:8080->8080/tcp, 8080/tcp  test-dags_434792-webserver-1

Windows OS

Uninstalled podman and attempted to astro dev start.
We see the helpful error message.
Installed podman and attempt to astro dev start again.
Once podman is installed, we can successfully run astro dev start and view the webserver in the browser.

📋 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-10-30T00:00:28Z

config/config.go

@@ -56,7 +56,7 @@ var (
 		CloudAPIToken:         newCfg("cloud.api.token", ""),
 		Context:               newCfg("context", ""),
 		Contexts:              newCfg("contexts", ""),
-		DockerCommand:         newCfg("container.binary", "docker"),
+		DockerCommand:         newCfg("container.binary", ""),


docker is no longer the "default" here. We "default" to an empty string here and only access the configuration from our new wrapper function where we search the $PATH for an appropriate binary. If one is not found, we now throw a more helpful error.

schnie · 2024-10-30T19:10:24Z

Tagging @lzdanski since this can help simplify the podman docs a bit.

jeremybeard

This looks really good! Just a question on scanning PATH for the container runtime.

airflow/container_runtime.go

schnie · 2024-10-30T19:44:45Z

airflow/docker.go

@@ -46,7 +46,7 @@ const (
 	RuntimeImageLabel              = "io.astronomer.docker.runtime.version"
 	AirflowImageLabel              = "io.astronomer.docker.airflow.version"
 	componentName                  = "airflow"
-	podman                         = "podman"
+	podmanCmd                      = "podman"


Just renamed this to be consistent with dockerCmd. dockerCmd is not simply docker because it conflicts with our imports of the docker go libs. This just makes them more similar.

airflow/container_runtime_test.go

jeremybeard · 2024-10-31T13:50:56Z

airflow/container_runtime.go

+	// Although programs can be called without the .exe extension,
+	// we need to append it here when searching the file system.
+	if isWindows() {
+		binaryName += ".exe"


Have we tested the changes in this branch on a Windows machine?

Hah, was just doing that this morning. Just posted some screenshots on the PR description.

It feels risky that we rely on Docker Desktop and Podman continuing to call their executable "docker.exe" and "podman.exe" but I guess the CLI already does when building the image?

Yea that's true. If something breaks in the wild, I think we'll always have our config file to manually override the binary name, until we have a chance to cut an updated release with a new binary name in the search array here.

Just curious, if we would run into this code path if running in WSL2 on Windows, I doubt it, but just in case you have already tried out that option

We run into this code path when running astro on Windows, with podman managing the WSL2 VM. The podman binary is podman.exe in Windows. If we were running purely within WSL2 on something like an Ubuntu machine where astro and podman were both installed in Linux, then this path shouldn't be hit. But, I wasn't actually able to get that scenario to work at all, and I don't believe we have that documented as an option yet. Do we have customers operating purely within WSL2?

We may have customers operating purely within WSl2 on the Software side - it's hard to know. I also would expect IsWindows to be false under WSL2.

jeremybeard · 2024-10-31T13:54:08Z

airflow/container_runtime_test.go

+	}{
+		{
+			name:    "Find docker",
+			pathEnv: "/usr/local/bin:/usr/bin:/bin",


This assumes we won't run these tests on Windows, but that's probably safe enough?

The tests should run on windows because the MockFileChecker just looks for a match in the mockFiles map. I originally had the os.Stat call directly in the function here, but that wouldn't work even across unixy machines since it was actually checking the filesystem which could differ drastically.

While we're on the topic though, we should probably think about some automated way to run the tests on Windows (a future PR). @sam-awezec was digging into the traffic coming through our onboarding flow and we're seeing a pretty even 50/50 split between mac and windows. Seems like the winget release process could use some love too.

jeremybeard · 2024-10-31T13:59:52Z

airflow/docker.go

+	if err != nil {
+		return err
+	}
+	if runtime.GOOS == "darwin" && containerRuntime == dockerCmd {
 		err := startDocker()


How do we feel about the CLI taking it upon itself to try to start Docker if it's not running? And only for macOS? But not for Podman?

I think as we work deeper into this doc, we'll add this support for podman. For podman, we need to manage the underlying podman machine as well, basically automating the steps we document manually today here - https://www.astronomer.io/docs/astro/cli/use-podman#configure-the-astro-cli-to-use-podman.

The goal would be that a fresh user doesn't need to understand anything about containers to get started. It won't stay "hidden" forever, but at least it won't be something that stands in the way of getting something running in airflow ASAP.

…Users/schnie/go/bin:/Users/schnie/.bun/bin:/Users/schnie/.docker/bin:/Users/schnie/.nvm/versions/node/v16.15.1/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/Users/schnie/bin:/usr/local/bin:/Users/schnie/go/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Library/Apple/usr/bin:/usr/local/MacGPG2/bin:/usr/local/go/bin:/Users/schnie/.orbstack/bin:/Users/schnie/.local/bin:/Users/schnie/.local/bin

schnie · 2024-10-31T17:52:58Z

@rob-1126 I think you originally add the podman binary support. Any concerns here?

neel-astro

LGTM, apart from a minor curious question

neel-astro · 2024-11-01T10:33:49Z

airflow/container_runtime.go

+	// Although programs can be called without the .exe extension,
+	// we need to append it here when searching the file system.
+	if isWindows() {
+		binaryName += ".exe"


Just curious, if we would run into this code path if running in WSL2 on Windows, I doubt it, but just in case you have already tried out that option

rob-1126

thanks for this.

lzdanski · 2024-11-21T15:47:53Z

@yanmastin-astro Move step 2 to a "tip" box and note that it's required for versions less than 1.31. In release notes, add a section called ### Behavior changes that this step is no longer necessary. In additional improvements this bit: the ability for the CLI to autodetect the container runtime binary to use for the commands that utilize containers.

Adds support for container runtime binary autodetection

3c600f6

schnie changed the title ~~Adds support for container runtime binary autodetection~~ Adds Container Runtime Binary Autodetection Oct 29, 2024

Adds concurrent path checks and tests

bdb52b7

schnie commented Oct 30, 2024

View reviewed changes

schnie changed the title ~~Adds Container Runtime Binary Autodetection~~ Adds Container Runtime Binary Auto-detection Oct 30, 2024

schnie changed the title ~~Adds Container Runtime Binary Auto-detection~~ Adds Container Runtime Binary Autodetection Oct 30, 2024

schnie force-pushed the feature/container-runtime-autodetection branch 4 times, most recently from e541a81 to 8676726 Compare October 30, 2024 18:38

schnie requested a review from rob-1126 October 30, 2024 19:05

schnie marked this pull request as ready for review October 30, 2024 19:05

schnie requested review from neel-astro, jeremybeard and kushalmalani as code owners October 30, 2024 19:05

schnie requested a review from lzdanski October 30, 2024 19:09

schnie force-pushed the feature/container-runtime-autodetection branch from 8676726 to 88987c7 Compare October 30, 2024 19:40

Move new code to new files

d6a2c1d

schnie force-pushed the feature/container-runtime-autodetection branch from 88987c7 to d6a2c1d Compare October 30, 2024 19:42

jeremybeard reviewed Oct 30, 2024

View reviewed changes

airflow/container_runtime.go Outdated Show resolved Hide resolved

schnie commented Oct 30, 2024

View reviewed changes

schnie force-pushed the feature/container-runtime-autodetection branch from a8aa1a5 to 20bfb81 Compare October 31, 2024 00:33

Fix binary search goroutine to avoid deadlocks

c3121e4

schnie force-pushed the feature/container-runtime-autodetection branch from 20bfb81 to c3121e4 Compare October 31, 2024 00:39

jeremybeard reviewed Oct 31, 2024

View reviewed changes

jeremybeard approved these changes Oct 31, 2024

View reviewed changes

neel-astro approved these changes Nov 1, 2024

View reviewed changes

rob-1126 approved these changes Nov 3, 2024

View reviewed changes

schnie requested a review from pritt20 November 4, 2024 15:10

Merge branch 'main' into feature/container-runtime-autodetection

29a5e86

schnie mentioned this pull request Nov 12, 2024

[WIP] Automatically Manage Podman Machine #1741

Closed

8 tasks

schnie merged commit 4cad4e2 into main Nov 14, 2024
3 checks passed

schnie deleted the feature/container-runtime-autodetection branch November 14, 2024 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Container Runtime Binary Autodetection #1738

Adds Container Runtime Binary Autodetection #1738

schnie commented Oct 29, 2024 •

edited

Loading

schnie Oct 30, 2024 •

edited

Loading

schnie commented Oct 30, 2024

jeremybeard left a comment

schnie Oct 30, 2024 •

edited

Loading

jeremybeard Oct 31, 2024

schnie Oct 31, 2024

jeremybeard Oct 31, 2024

schnie Oct 31, 2024

neel-astro Nov 1, 2024

schnie Nov 1, 2024 •

edited

Loading

rob-1126 Nov 3, 2024

jeremybeard Oct 31, 2024

schnie Oct 31, 2024

schnie Oct 31, 2024

jeremybeard Oct 31, 2024

schnie Oct 31, 2024

schnie commented Oct 31, 2024

neel-astro left a comment

neel-astro Nov 1, 2024

rob-1126 left a comment

lzdanski commented Nov 21, 2024

Adds Container Runtime Binary Autodetection #1738

Adds Container Runtime Binary Autodetection #1738

Conversation

schnie commented Oct 29, 2024 • edited Loading

Description

🎟 Issue(s)

🧪 Functional Testing

Mac OS

Windows OS

📋 Checklist

schnie Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

schnie commented Oct 30, 2024

jeremybeard left a comment

Choose a reason for hiding this comment

schnie Oct 30, 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

schnie Nov 1, 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

schnie commented Oct 31, 2024

neel-astro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-1126 left a comment

Choose a reason for hiding this comment

lzdanski commented Nov 21, 2024

schnie commented Oct 29, 2024 •

edited

Loading

schnie Oct 30, 2024 •

edited

Loading

schnie Oct 30, 2024 •

edited

Loading

schnie Nov 1, 2024 •

edited

Loading