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.

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

Conversation

uranusjr
Copy link
Member

I hit this when trying to set up a contributor’s Windows PC for development. The Windows host talks to a WSL2 container with a Docker context (if you enable it in Docker Desktop) and it would show up in docker context ls, and Breeze’s detection code can mistakenly select it for building an image and fail. This adds explicit logic to exclude those contexts so Breeze can find a more appropriate one.

@uranusjr uranusjr force-pushed the avoid-wsl2-desktop-linux-context branch from 66057cb to 7746dd5 Compare August 20, 2023 07:15
@uranusjr uranusjr force-pushed the avoid-wsl2-desktop-linux-context branch from 7746dd5 to 9fb506b Compare August 20, 2023 08:17
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

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.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

One thing - we should either switch back to docker ls -q or find out in which (relatively recent) version the --format switch was added.

potiuk added a commit to potiuk/airflow that referenced this pull request Aug 21, 2023
It's been quite some time since we bumped min versions for docker
and docker-compose for Breeze and there are already a few cases where
it held us back from more efficiently using some newer docker or
docker-compose features (for example in apache#33547). Also docker-compose
v1 is sufficiently old an unmaintained, that it's about time to
get rid of it and ask the users to migrate to docker-compose v2.

The new min versions propose:

* Docker: 23.0.0 released 2023-02-01 (6 months ago)
* Docker Compose 2.14.0 release 2022-12-15 (8 months ago)

Docker v1 is not supported any more and the users are directed
to migrate to v2 to continue using Breeze.
potiuk added a commit that referenced this pull request Aug 21, 2023
It's been quite some time since we bumped min versions for docker
and docker-compose for Breeze and there are already a few cases where
it held us back from more efficiently using some newer docker or
docker-compose features (for example in #33547). Also docker-compose
v1 is sufficiently old an unmaintained, that it's about time to
get rid of it and ask the users to migrate to docker-compose v2.

The new min versions propose:

* Docker: 23.0.0 released 2023-02-01 (6 months ago)
* Docker Compose 2.14.0 release 2022-12-15 (8 months ago)

Docker v1 is not supported any more and the users are directed
to migrate to v2 to continue using Breeze.
@potiuk
Copy link
Member

potiuk commented Aug 21, 2023

Ok. Should be good to just rebase it - we have the min docker version now that supports --format json :)

@uranusjr uranusjr force-pushed the avoid-wsl2-desktop-linux-context branch from 9fb506b to 7f35725 Compare August 23, 2023 01:18
@potiuk
Copy link
Member

potiuk commented Aug 23, 2023

Cannot rebas ehit one - but the botocore issue is solved so rebasing should bring it back

@uranusjr uranusjr force-pushed the avoid-wsl2-desktop-linux-context branch from 7f35725 to 7eddc4a Compare August 24, 2023 01:34
@potiuk potiuk merged commit df7c170 into apache:main Aug 24, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 24, 2023
@potiuk
Copy link
Member

potiuk commented Aug 24, 2023

Hey @uranusjr -> something was wrong when retrieving context during image building - I need to revert it temporarily.

Can you please recreate the PR but push your branch to apache/airflow repo - it should be reproducible if you run it from the apache one rather than from the fork. (building image is running frommain in PRs from the fork so the way to test build image failures is to make a PR from the apache/airflow.

I am curious what went wrong there.

potiuk added a commit that referenced this pull request Aug 24, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
It's been quite some time since we bumped min versions for docker
and docker-compose for Breeze and there are already a few cases where
it held us back from more efficiently using some newer docker or
docker-compose features (for example in #33547). Also docker-compose
v1 is sufficiently old an unmaintained, that it's about time to
get rid of it and ask the users to migrate to docker-compose v2.

The new min versions propose:

* Docker: 23.0.0 released 2023-02-01 (6 months ago)
* Docker Compose 2.14.0 release 2022-12-15 (8 months ago)

Docker v1 is not supported any more and the users are directed
to migrate to v2 to continue using Breeze.

(cherry picked from commit 73a3733)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants