Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid WSL2 ones when finding a context for Breeze #33547

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Avoid WSL2 ones when finding a context for Breeze #33547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions dev/breeze/src/airflow_breeze/utils/docker_command_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"""Various utils to prepare docker and docker compose commands."""
from __future__ import annotations

import json
import os
import re
import sys
Expand Down Expand Up @@ -800,23 +801,30 @@ def autodetect_docker_context():

:return: name of the docker context to use
"""
output = run_command(["docker", "context", "ls", "-q"], capture_output=True, check=False, text=True)
if output.returncode != 0:
result = run_command(
["docker", "context", "ls", "--format=json"],
Copy link
Member

Choose a reason for hiding this comment

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

I tried this before, and it's better to stick to "ls -q".

Unfortunately there are still relatively fresh versions of docker out there that do not support --format for this command. We had some cases like that where people's docker (even newly installed on relatively fresh Linux machine) did not contain the --format switch.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe actually we could leave it as is but raise the min-docker version requirement though (but we need to figure out in which version of docker it has been added. I think evan if it is relatively recent addition, we can actually raise the bar a little as we have not updated the min docker version for quite a while. The worst thing, the users will just have to update their docker following instructions on docker website (and I think we already direct the users there when min-version is not reached).

Copy link
Member

@potiuk potiuk Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did some experimenting on my Linux machine (reinstalled some older versions and bisected when the change happened as it was not in the release notes):

Those are versions available for my Mint at least:

5:24.0.5-1~ubuntu.22.04~jammy
5:24.0.4-1~ubuntu.22.04~jammy
5:24.0.3-1~ubuntu.22.04~jammy
5:24.0.2-1~ubuntu.22.04~jammy
5:24.0.1-1~ubuntu.22.04~jammy
5:24.0.0-1~ubuntu.22.04~jammy
5:23.0.6-1~ubuntu.22.04~jammy
5:23.0.5-1~ubuntu.22.04~jammy
5:23.0.4-1~ubuntu.22.04~jammy
5:23.0.3-1~ubuntu.22.04~jammy
5:23.0.2-1~ubuntu.22.04~jammy
5:23.0.1-1~ubuntu.22.04~jammy
5:23.0.0-1~ubuntu.22.04~jammy
5:20.10.24~3-0~ubuntu-jammy
5:20.10.23~3-0~ubuntu-jammy
5:20.10.22~3-0~ubuntu-jammy
5:20.10.21~3-0~ubuntu-jammy
5:20.10.20~3-0~ubuntu-jammy
5:20.10.19~3-0~ubuntu-jammy
5:20.10.18~3-0~ubuntu-jammy
5:20.10.17~3-0~ubuntu-jammy
5:20.10.16~3-0~ubuntu-jammy
5:20.10.15~3-0~ubuntu-jammy
5:20.10.14~3-0~ubuntu-jammy
5:20.10.13~3-0~ubuntu-jammy

For 5:20.10.24~3-0~ubuntu-jammy (our current min-version is 20.10.0):

[jarek@jarek-HP-Z2-Tower-G9-Workstation-Desktop-PC:~/code/airflow] main+ 29s ± docker context ls --help

Usage:  docker context ls [OPTIONS]

List contexts

Aliases:
  ls, list

Options:
      --format string   Pretty-print contexts using a Go template
  -q, --quiet           Only show context names

For 5:23.0.0-1~ubuntu.22.04~jammy:

Usage:  docker context ls [OPTIONS]

List contexts

Aliases:
  docker context ls, docker context list

Options:
      --format string   Format output using a custom template:
                        'table':            Print output in table format with column headers (default)
                        'table TEMPLATE':   Print output in table format using the given Go template
                        'json':             Print in JSON format
                        'TEMPLATE':         Print output using the given Go template.
                        Refer to https://docs.docker.com/go/formatting/ for more information about formatting output with templates
  -q, --quiet           Only show context names

I'd be for setting the min version to 23.0.6 ? WDYT @uranusjr ?

Copy link
Member

Choose a reason for hiding this comment

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

23.0.6🔗 2023-05-08

Released 3 months ago.

I think for a development environment it's sortof enough. We can always experiment and go back if people will complain.

We could also raise the docker-compose min version - but that should be done separately, as we would have to force docker-compose v2. Which I think it's about the time to do anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm since 20.10.24 already supports --format with a Go template, and that syntax is still supported in 23.0, I think it might be better to just use that syntax instead. I’ll work on that.

Copy link
Member

Choose a reason for hiding this comment

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

That is also an option indeed.

Copy link
Member

Choose a reason for hiding this comment

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

But I think it's about right to bump min-version of docker. This is a small annoyance for contributors to upgrade, but it's good for contributors to update and refresh docker client, there might be other things in the future that we won't even be aware that we are lagging behind. If you want I can make a separate PR where I bump both docker-compose (we still support v1 which is > 2 years old) and docker min versions. This is already handled well by messaging in Breeze.

capture_output=True,
check=False,
text=True,
)
if result.returncode != 0:
get_console().print("[warning]Could not detect docker builder. Using default.[/]")
return "default"
context_list = output.stdout.splitlines()
if not context_list:
known_contexts = {info["Name"]: info for info in json.loads(result.stdout)}
if not known_contexts:
get_console().print("[warning]Could not detect docker builder. Using default.[/]")
return "default"
elif len(context_list) == 1:
get_console().print(f"[info]Using {context_list[0]} as context.[/]")
return context_list[0]
else:
for preferred_context in PREFERRED_CONTEXTS:
if preferred_context in context_list:
get_console().print(f"[info]Using {preferred_context} as context.[/]")
return preferred_context
fallback_context = context_list[0]
for preferred_context_name in PREFERRED_CONTEXTS:
try:
context = known_contexts[preferred_context_name]
except KeyError:
continue
# On Windows, some contexts are used for WSL2. We don't want to use those.
if context["DockerEndpoint"] == "npipe:////./pipe/dockerDesktopLinuxEngine":
potiuk marked this conversation as resolved.
Show resolved Hide resolved
continue
get_console().print(f"[info]Using {preferred_context_name} as context.[/]")
return preferred_context_name
fallback_context = next(iter(known_contexts))
get_console().print(
f"[warning]Could not use any of the preferred docker contexts {PREFERRED_CONTEXTS}.\n"
f"Using {fallback_context} as context.[/]"
Expand Down
33 changes: 28 additions & 5 deletions dev/breeze/tests/test_docker_command_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,19 +195,42 @@ def test_check_docker_compose_version_ok(mock_get_console, mock_run_command):
)


def _fake_ctx(name: str) -> dict[str, str]:
return {
"Name": name,
"DockerEndpoint": f"unix://{name}",
}


@pytest.mark.parametrize(
"context_output, selected_context, console_output",
[
(
json.dumps([_fake_ctx("default")]),
"default",
"[info]Using default as context",
),
("[]", "default", "[warning]Could not detect docker builder"),
(
json.dumps([_fake_ctx("a"), _fake_ctx("b")]),
"a",
"[warning]Could not use any of the preferred docker contexts",
),
(
json.dumps([_fake_ctx("a"), _fake_ctx("desktop-linux")]),
"desktop-linux",
"[info]Using desktop-linux as context",
),
(
json.dumps([_fake_ctx("a"), _fake_ctx("default")]),
"default",
"[info]Using default as context",
),
("", "default", "[warning]Could not detect docker builder"),
("a\nb", "a", "[warning]Could not use any of the preferred docker contexts"),
("a\ndesktop-linux", "desktop-linux", "[info]Using desktop-linux as context"),
("a\ndefault", "default", "[info]Using default as context"),
("a\ndefault\ndesktop-linux", "desktop-linux", "[info]Using desktop-linux as context"),
(
json.dumps([_fake_ctx("a"), _fake_ctx("default"), _fake_ctx("desktop-linux")]),
"desktop-linux",
"[info]Using desktop-linux as context",
),
],
)
def test_autodetect_docker_context(context_output: str, selected_context: str, console_output: str):
Expand Down