Adds Runtime Agnostic Healthcheck #1740

schnie · 2024-11-01T00:28:43Z

Description

This PR aims to provide a consistent local dev startup experience across container runtimes. Today, astro dev start will print the status update and attempt to open the browser to the airflow dashboard once its up and running. This is nice and it works for Docker Desktop. It does not work as expected for orbstack or podman. For these, the command line hangs and we end up opening the browser to a URL that isn't ready yet. This is a bad experience.

We're exploring the idea of using podman as our default runtime going forward and this is another small fix towards preparing for something like that. This fix will improve todays UX either way.

🎟 Issue(s)

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

🧪 Functional Testing

Have tested this using astro dev start on:

Mac OS, Docker Desktop
Mac OS, Podman
Mac OS, Orbstack
Windows OS, Podman
Windows OS, Docker Desktop

📸 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-01T03:04:37Z

airflow/docker.go

+	}
+
+	// If we've successfully gotten a healthcheck response, print the status.
+	err = printStatus(settingsFile, envConns, project, d.composeService, airflowDockerVersion, noBrowser)


printStatus() was previously called by the original checkWebserverHealth() function. That function has been removed and replaced and doesn't concern itself with printing the message. That now just happens at the "top-level" here.

schnie · 2024-11-01T03:05:08Z

airflow/docker.go

-	} else {
-		fmt.Println("\nProject is running! All components are now available.")
-	}
+	fmt.Println("\nProject is running! All components are now available.")


The healthcheck now works for all runtimes so there's no need to differentiate here.

schnie · 2024-11-01T03:05:39Z

airflow/docker.go

@@ -1433,7 +1397,7 @@ func printStatus(settingsFile string, envConns map[string]astrocore.EnvironmentO
 			fmt.Println("\nUnable to open the webserver URL, please visit the following link: " + webserverURL)
 		}
 	}
-	return errComposeProjectRunning
+	return nil


I don't know why we were returning an error for success, but we don't anymore.

schnie · 2024-11-01T03:06:46Z

airflow/include/composeyml.yml

-    healthcheck:
-      test: curl --fail http://webserver:8080/health || exit 1
-      interval: 2s
-      retries: 15
-      start_period: 5s
-      timeout: 60s


This original check built into the compose file does not work for orbstack or podman. We remove it here and it's been replaced by the new functionality.

schnie · 2024-11-01T03:07:10Z

airflow/suite_test.go

@@ -19,7 +17,7 @@ type Suite struct {
 	origCmdExec              func(cmd string, stdout, stderr io.Writer, args ...string) error
 	origGetDockerClient      func() (client.APIClient, error)
 	origInitSettings         func(id, settingsFile string, envConns map[string]astrocore.EnvironmentObjectConnection, version uint64, connections, variables, pools bool) error
-	origCheckWebserverHealth func(settingsFile string, envConns map[string]astrocore.EnvironmentObjectConnection, project *types.Project, composeService api.Service, airflowDockerVersion uint64, noBrowser bool, timeout time.Duration) error
+	origCheckWebserverHealth func(url string, timeout time.Duration) error


Just updating to the signature.

schnie · 2024-11-01T03:58:17Z

airflow/docker.go

@@ -82,7 +81,6 @@ const (
 var (
 	errNoFile                  = errors.New("file specified does not exist")
 	errSettingsPath            = "error looking for settings.yaml"
-	errComposeProjectRunning   = errors.New("project is up and running")


This was an error that we used to indicate success for some reason. This isn't used anymore as we just return nil if things work (no error).

schnie · 2024-11-01T18:49:58Z

airflow/docker.go

-// CheckM1Image checks if the CLI is currently running on M1 architecture
-// next it checks if the runtime version of the image building is above 6.0.4
-// if the image is M1 architecture and runtime version is less than or equal to 6.0.4 it will print true
-var CheckM1Image = func(imageLabels map[string]string) bool {
-	if !isM1(runtime.GOOS, runtime.GOARCH) {
-		// the architecture is not arm64 no need to print message
-		return false
-	}
-	runtimeVersion, ok := imageLabels[runtimeVersionLabelName]
-	if !ok {
-		// cannot determine runtime version print message by default
-		return true
-	}
-
-	return versions.LessThanOrEqualTo(runtimeVersion, M1ImageRuntimeVersion)
-}


This function is purely used to decide if the astro dev start --wait flag defaults to 1m or 5m. Thinking we just simplify and use 5m. The CLI progresses as soon as it sees the 200 from the webserver /health endpoint.

schnie · 2024-11-01T19:26:56Z

airflow/docker.go

-	startupTimeout = waitTime
-	// check if user provided a waitTime
-	// default is 1 minute
-	if waitTime != 1*time.Minute {
-		startupTimeout = waitTime
-	} else if CheckM1Image(imageLabels) {
-		// user did not provide a waitTime
-		// if running darwin/M1 architecture
-		// we wait for a longer startup time
-		startupTimeout = 5 * time.Minute


I'm removing a chunk of logic around this M1 architecture check and runtime comparison. The only reason we have all this is to bump the startup timeout from 1m to 5m. I've just made 5m the default across the board.

If we feel strongly about not changing the default across the board, we could revert this piece. It just feels like unnecessary cruft at this point, and this feels like a fine time to remove it.

Thanks for cutting out these legacy "M1" bits, it is definitely time to remove it.

I believe we added the 5 minute timeout because when Apple Silicon first came out we did not have arm64 support in the Astro Runtime image and so the macOS emulation was really slow. We have long since added that arm64 support, so I'm wondering if the default timeout should be 1 minute?

Yea, I'm fine with making it 1m across the board now. @neel-astro that sound ok to you?

schnie · 2024-11-01T19:47:17Z

airflow/docker.go

@@ -52,7 +51,6 @@ const (
 	defaultAirflowVersion          = uint64(0x2) //nolint:gomnd
 	triggererAllowedRuntimeVersion = "4.0.0"
 	triggererAllowedAirflowVersion = "2.2.0"
-	M1ImageRuntimeVersion          = "6.0.4"


The 6.x version of runtime has been deprecated as of March 2024 as stated in our docs.

schnie · 2024-11-01T19:49:32Z

airflow/docker.go

-var checkWebserverHealth = func(settingsFile string, envConns map[string]astrocore.EnvironmentObjectConnection, project *types.Project, composeService api.Service, airflowDockerVersion uint64, noBrowser bool, timeout time.Duration) error {
-	if config.CFG.DockerCommand.GetString() == podman {
-		err := printStatus(settingsFile, envConns, project, composeService, airflowDockerVersion, noBrowser)
-		if err != nil {
-			if !errors.Is(err, errComposeProjectRunning) {
-				return err
-			}
-		}
-	} else {
-		ctx, cancel := context.WithTimeout(context.Background(), timeout)
-		defer cancel()
-		// check if webserver is healthy for user
-		err := composeService.Events(ctx, project.Name, api.EventsOptions{
-			Services: []string{WebserverDockerContainerName}, Consumer: func(event api.Event) error {
-				marshal, err := json.Marshal(map[string]interface{}{
-					"action": event.Status,
-				})
-				if err != nil {
-					return err
-				}
-				if string(marshal) == `{"action":"health_status: healthy"}` {
-					err := printStatus(settingsFile, envConns, project, composeService, airflowDockerVersion, noBrowser)
-					if err != nil {
-						return err
-					}
-				}
-
-				return nil
-			},
-		})
-		if err != nil {
-			if errors.Is(err, context.DeadlineExceeded) {
-				fmt.Printf("\n")
-				return fmt.Errorf("there might be a problem with your project starting up. The webserver health check timed out after %s but your project will continue trying to start. Run 'astro dev logs --webserver | --scheduler' for details.\n\nTry again or use the --wait flag to increase the time out", timeout)
-			}
-			if !errors.Is(err, errComposeProjectRunning) {
-				return err
-			}
-		}
-	}
-	return nil
-}
-


This implementation is being removed in favor of the new one declared in its own file below - health_check.go.

jeremybeard · 2024-11-04T15:14:22Z

cmd/airflow_test.go

@@ -407,7 +407,7 @@ func (s *AirflowSuite) TestAirflowStart() {

 		mockContainerHandler := new(mocks.ContainerHandler)
 		containerHandlerInit = func(airflowHome, envFile, dockerfile, imageName string) (airflow.ContainerHandler, error) {
-			mockContainerHandler.On("Start", "", "airflow_settings.yaml", "", "", false, false, 1*time.Minute, map[string]astrocore.EnvironmentObjectConnection(nil)).Return(nil).Once()
+			mockContainerHandler.On("Start", "", "airflow_settings.yaml", "", "", false, false, 5*time.Minute, map[string]astrocore.EnvironmentObjectConnection(nil)).Return(nil).Once()


Ideally unit tests should use the constants.

jeremybeard

This is excellent, it simplifies the code and gains more functionality as a result! Appreciate the proactive PR comments too, makes it all very clear.

Approving ahead of a couple of small comments to look into.

neel-astro

This is a step in the right direction, but I am slightly concerned about potentially breaking the behavior for folks using docker-compose override or custom docker-compose in their local setup (applies to both docker and podman users, and I have seen a decent amount of examples where we suggest customers use it to customize any change), because in those cases we can't for sure say that the port mapping would be 8080 or even the same as that of the webserver.port in config (because changing port number is quite a common occurrence).
We need to think through a way to keep supporting those. Maybe adjust our error messaging to ask the user to set the webserver port in the config even if they are using custom docker-compose or override.

airflow/docker.go

neel-astro · 2024-11-04T15:59:34Z

airflow/health_check.go

+		// This fires on every tick of our timer to run the healthcheck.
+		// We return successfully from this function when we get a 200 status code.
+		case <-ticker.C:
+			statusCode, _ := healthCheck(ctx, client, url)


nit: either we could log the error or drop the error from the return arguments

airflow/health_check_test.go

cmd/airflow.go

schnie · 2024-11-05T17:58:00Z

@neel-astro I've updated the healthcheck URL to get the port from the config now. I also attempted to override compose with:

webserver:
  ports:
  - 9090:9090

This results in both our standard 8080 and 9090 being bound, so the override file can't wipe out our existing port and it'll always be the first in the list of ports (or the overridden port from the config).

Also, the healthcheck that was defined previously via the compose file had 8080 hard-coded, so this should actually result in an improvement here now that we grab it from the config.

neel-astro

LGTM, this change would still be slightly disruptive for anyone using astro dev start --compose-file <custom-compose-file> with a different webserver port, but there is a workaround for them by setting astro config set webserver.port <webserver-port>. But since this solves the problem for a bunch of other broken use cases, I am on board with the change.

schnie · 2024-11-06T14:19:52Z

@neel-astro do you have an example of what that compose file would look like to override the websever port in a problematic way? As far as I know, any ports defined in a custom compose file are added to the standard definition we provide as the base, so 8080 (or whatever the user configures) will always be bound.

Or are you referring to overriding the airflow setting to change the port that airflow webserver listens on?

neel-astro · 2024-11-06T14:35:55Z

@neel-astro do you have an example of what that compose file would look like to override the webserver port in a problematic way? As far as I know, any ports defined in a custom compose file are added to the standard definition we provide as the base, so 8080 (or whatever the user configures) will always be bound.

@schnie there is a functionality within astro dev commands to export the current docker-compose file locally, edit it, and use that local file to spin components up. This is a bit advanced use case, and it is also a backdoor for us to unblock users running into various non-common issues due to how they are running local setups.
astro dev object export --compose --compose-file <compose-file> to export the compose file
astro dev start --compose-file <compose-file> to use the local compose file to spin up components.

So theoretically, users could export the compose file, edit it to replace the existing 8080 webserver port, and spin up the local setup.

schnie · 2024-11-06T15:27:50Z

Hmm, yea I was able to recreate that here locally. Does seem like a bit of an edge case.

I did have some code I was playing with that actually grabbed the port from the webserver service in the compose project. ports is a list, so if someone is really messing with things at this level they could also have multiple ports listed, in which case the port to healthcheck on becomes ambiguous.. unless we scanned all of them, but that doesn't feel quite right either.

Do you know of any use-case where someone out there would be doing this kind of change? If we know of someone actually doing this on the webserver maybe we think about it some more. Otherwise, it does feel like a lot for someone to go through for something like this.

neel-astro · 2024-11-06T15:37:21Z

Yeah, I think it's fine because in such cases the new way to handle the case would be for them to set the webserver.port in CLI config.
The other way around might be for CLI to fetch the port from the container named "webserver", i.e. parse and get it from the docker port webserver output.
I have seen use cases for custom compose file used by the users (mostly around volume mounting), but I am not sure if they would be using that with a custom webserver port or not.

lzdanski · 2024-11-21T15:40:33Z

@yanmastin-astro Add to docs release notes?

"Additional improvements: Streamlined the behavior of astro dev start for Podman users."

schnie force-pushed the universal-webserver-healthcheck branch 3 times, most recently from 075590e to e737fd9 Compare November 1, 2024 02:04

Replaces webserver healthcheck with one that is runtime agnostic

d088cd2

schnie force-pushed the universal-webserver-healthcheck branch from e737fd9 to d088cd2 Compare November 1, 2024 02:12

schnie commented Nov 1, 2024

View reviewed changes

schnie force-pushed the universal-webserver-healthcheck branch 7 times, most recently from 25af892 to ca5e07d Compare November 1, 2024 03:48

Adds unit tests for health check

47a5644

schnie force-pushed the universal-webserver-healthcheck branch from ca5e07d to 47a5644 Compare November 1, 2024 03:51

schnie commented Nov 1, 2024

View reviewed changes

schnie force-pushed the universal-webserver-healthcheck branch 3 times, most recently from bed5d87 to 4caa877 Compare November 1, 2024 19:19

schnie commented Nov 1, 2024

View reviewed changes

schnie changed the title ~~Adds Runtime Agnostic Webserver Healthcheck~~ Adds Runtime Agnostic Healthcheck Nov 1, 2024

schnie marked this pull request as ready for review November 1, 2024 19:53

schnie requested a review from neel-astro as a code owner November 1, 2024 19:53

schnie requested review from jeremybeard, kushalmalani and lzdanski as code owners November 1, 2024 19:53

schnie force-pushed the universal-webserver-healthcheck branch from 4caa877 to 0f96cb3 Compare November 1, 2024 19:55

Remove M1-specific logic and simplify wait-time to 5m

d5c9fcf

schnie force-pushed the universal-webserver-healthcheck branch from 0f96cb3 to d5c9fcf Compare November 1, 2024 19:56

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

jeremybeard reviewed Nov 4, 2024

View reviewed changes

jeremybeard approved these changes Nov 4, 2024

View reviewed changes

neel-astro requested changes Nov 4, 2024

View reviewed changes

Use constant for defaultWaitTime and update webserver port

4867f0e

Reformatted tests to use existing patterns

7352506

schnie force-pushed the universal-webserver-healthcheck branch from feedeb0 to 7352506 Compare November 5, 2024 18:32

neel-astro approved these changes Nov 6, 2024

View reviewed changes

Merge branch 'main' into universal-webserver-healthcheck

9c6e5f1

Fix merge conflicts

4d34319

schnie merged commit 0d3145f into main Nov 14, 2024
2 of 3 checks passed

schnie deleted the universal-webserver-healthcheck branch November 14, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Runtime Agnostic Healthcheck #1740

Adds Runtime Agnostic Healthcheck #1740

schnie commented Nov 1, 2024 •

edited

Loading

schnie Nov 1, 2024

jeremybeard Nov 4, 2024

schnie Nov 1, 2024

schnie Nov 1, 2024

schnie Nov 1, 2024

schnie Nov 1, 2024 •

edited

Loading

schnie Nov 1, 2024 •

edited

Loading

schnie Nov 1, 2024

schnie Nov 1, 2024

schnie Nov 1, 2024

jeremybeard Nov 4, 2024

schnie Nov 4, 2024 •

edited

Loading

schnie Nov 1, 2024

schnie Nov 1, 2024

jeremybeard Nov 4, 2024

jeremybeard left a comment

neel-astro left a comment •

edited

Loading

neel-astro Nov 4, 2024

schnie commented Nov 5, 2024

neel-astro left a comment •

edited

Loading

schnie commented Nov 6, 2024 •

edited

Loading

neel-astro commented Nov 6, 2024

schnie commented Nov 6, 2024

neel-astro commented Nov 6, 2024

lzdanski commented Nov 21, 2024

Adds Runtime Agnostic Healthcheck #1740

Adds Runtime Agnostic Healthcheck #1740

Conversation

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

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

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

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

jeremybeard left a comment

Choose a reason for hiding this comment

neel-astro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schnie commented Nov 5, 2024

neel-astro left a comment • edited Loading

Choose a reason for hiding this comment

schnie commented Nov 6, 2024 • edited Loading

neel-astro commented Nov 6, 2024

schnie commented Nov 6, 2024

neel-astro commented Nov 6, 2024

lzdanski commented Nov 21, 2024

schnie commented Nov 1, 2024 •

edited

Loading

schnie Nov 1, 2024 •

edited

Loading

schnie Nov 1, 2024 •

edited

Loading

schnie Nov 4, 2024 •

edited

Loading

neel-astro left a comment •

edited

Loading

neel-astro left a comment •

edited

Loading

schnie commented Nov 6, 2024 •

edited

Loading