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

docs: tutor uses docker compose now, not docker-compose #868

Closed

Conversation

kdmccormick
Copy link
Collaborator

Per v16 changelog: https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14

@regisb , a couple related questions:

  • Is v20.10.15+ still the right Docker version to require?
  • Do we count on BuildKit now? If so, it's not default in Docker Engine until v23.
  • I didn't update the podman tutorial. Do you if the podman shim still works now that we're using builtin docker compose?

@kdmccormick kdmccormick requested a review from regisb July 10, 2023 13:02
@kdmccormick
Copy link
Collaborator Author

Updating to a newer Docker version helped resolve an issue several folks were having with Tutor Palm: https://discuss.openedx.org/t/issue-in-tutor-palm-release-with-tuotr-dev-launch-while-installing/10629/12

Abdess updated to the latest Engine version (v24.0.3), which is also what I have. Shall I update this PR to make that the minimum Docker version, or should we go with v23.0.0, which is the first version that made Buildx the default?

@regisb
Copy link
Contributor

regisb commented Jul 13, 2023

This is very strange. A couple of notes:

  • Compose does not ship by default with Docker (at least on some OS). This is why we must still specify a minimum version for Compose.
  • Tutor will default to docker compose if it's available, but it should still be compatible with docker-compose. If not, then it's a bug.
  • The minimum required Docker version was updated in Palm, and I don't think we need to bump it to v24.
  • It's difficult to troubleshoot the issue that was reported in the forum without more information, including a full stacktrace of docker build. For instance, it might be that there is a bug in how Tutor generates the Dockerfile based on the presence or absence of buildx.

docs/install.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Looking good outside of one minor point.

@kdmccormick
Copy link
Collaborator Author

@regisb , after digging further, it seems like Tutor implicitly depends on buildx currently, and upgrading Docker only worked because it turns on buildx by default.

I was able to reproduce Yagnesh's issue locally with both 16.0.2 and 16.0.2-nighly by disabling buildx:

export DOCKER_BUILDKIT=0  # generally disable buildx
tutor config save
tutor images build openedx

This yields:

....
Step 30/99 : RUN pip install -r /openedx/edx-platform/requirements/edx/base.txt
 ---> Running in 75c58204f9a7
ERROR: Could not open requirements file: [Errno 2] No such file or directory: '/openedx/edx-platform/requirements/edx/base.txt'

The command '/bin/sh -c pip install -r /openedx/edx-platform/requirements/edx/base.txt' returned a non-zero code: 1
Error: Command failed with status 1: docker build --tag=docker.io/overhangio/openedx:16.0.2-nightly --cache-from=type=registry,ref=docker.io/overhangio/openedx:16.0.2-nightly-cache /home/kyle/tutor-root/env/build/openedx

I believe the issue is here:

###### Install python requirements in virtualenv
FROM python as python-requirements
ENV PATH /openedx/venv/bin:${PATH}
ENV VIRTUAL_ENV /openedx/venv/
ENV XDG_CACHE_HOME /openedx/.cache
RUN {% if is_buildkit_enabled() %}--mount=type=cache,target=/var/cache/apt,sharing=locked \
--mount=type=cache,target=/var/lib/apt,sharing=locked {% endif %}apt update \
&& apt install -y software-properties-common libmysqlclient-dev libxmlsec1-dev libgeos-dev
# Install the right version of pip/setuptools
RUN {% if is_buildkit_enabled() %}--mount=type=cache,target=/openedx/.cache/pip,sharing=shared {% endif %}pip install \
# https://pypi.org/project/setuptools/
# https://pypi.org/project/pip/
# https://pypi.org/project/wheel/
setuptools==67.6.1 pip==23.0.1. wheel==0.40.0
# Install base requirements
RUN {% if is_buildkit_enabled() %}--mount=type=bind,from=edx-platform,source=/requirements/edx/base.txt,target=/openedx/edx-platform/requirements/edx/base.txt \
--mount=type=cache,target=/openedx/.cache/pip,sharing=shared {% endif %}pip install -r /openedx/edx-platform/requirements/edx/base.txt

When builtkit is enabled, requirements/edx/base.txt is mounted from the edx-platform image (or from the local dir).

When it's disabled, though, requirements/edx/base.txt does not exist because it is never COPY'ed in.

Similarly, the NPM install would also fail, since package.json mounted by buildkit, but never COPY'ed for the legacy builder:

RUN {% if is_buildkit_enabled() %}--mount=type=bind,from=edx-platform,source=/package.json,target=/openedx/edx-platform/package.json \
--mount=type=bind,from=edx-platform,source=/package-lock.json,target=/openedx/edx-platform/package-lock.json \
--mount=type=cache,target=/root/.npm,sharing=shared {% endif %}npm clean-install --no-audit --registry=$NPM_REGISTRY

Lastly, using tutor mounts with the legacy builder causes the build to fail:

(venv-tutor) ~/overhangio/tutor 🍀 export DOCKER_BUILDKIT=0
(venv-tutor) ~/overhangio/tutor 🍀 tutor mounts add ~/openedx/edx-platform/
Adding bind-mount: /home/kyle/openedx/edx-platform
Configuration saved to /home/kyle/tutor-root/config.yml
Environment generated in /home/kyle/tutor-root/env
(venv-tutor) ~/overhangio/tutor 🍀  tutor images build openedx
Adding /home/kyle/openedx/edx-platform to the build context 'edx-platform' of image 'openedx'
Adding /home/kyle/openedx/edx-platform to the build context 'edx-platform' of image 'openedx-dev'
Building image docker.io/overhangio/openedx:16.0.2-nightly
docker build --tag=docker.io/overhangio/openedx:16.0.2-nightly --cache-from=type=registry,ref=docker.io/overhangio/openedx:16.0.2-nightly-cache --build-context=edx-platform=/home/kyle/openedx/edx-platform /home/kyle/tutor-root/env/build/openedx
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
            BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
            environment-variable.

unknown flag: --build-context
See 'docker build --help'.
Error: Command failed with status 125: docker build --tag=docker.io/overhangio/openedx:16.0.2-nightly --cache-from=type=registry,ref=docker.io/overhangio/openedx:16.0.2-nightly-cache --build-context=edx-platform=/home/kyle/openedx/edx-platform /home/kyle/tutor-root/env/build/openedx

So, rather than this PR, I think the correct fix is to either:

  1. Make sure that each Tutor feature which takes advantage of buildkit gracefully degrades when using the legacy builder. For example, tutor mounts should say something like "Legacy Docker builder not supported--enable buildkit to use this command.". And, obviously, the Docker image should compile, even if it's much slower.
  2. Retroactively remove legacy builder support from Tutor v16.

I prefer 2 since it's less work, it would simply the Dockerfile a lot, it would simplify maintenance for us, and since Palm I haven't heard anyone asking us to put back legacy builder support. The legacy builder itself has been deprecated since Docker Engine v23 (02 Feb 2023); anyone running Docker v23+ is using buildx whether they know it or not.

@kdmccormick
Copy link
Collaborator Author

kdmccormick commented Jul 19, 2023

@regisb sorry, there is one other approach we would take, as a compromise:

Restore legacy builder support for Palm, but assume BuildKit for Nightly, and communicate this as a breaking change for Quince.

(Selfishly, I want to be able to assume BuildKit in Nightly, because it'll make the static asset caching work I'm wrapping up much simpler :)

@regisb
Copy link
Contributor

regisb commented Jul 31, 2023

Kyle, I still think we need to merge this PR, because the default is now BuildKit's docker compose, and not docker-compose.

That being said, you are right that in the current state Docker without BuildKit is broken in Tutor. We do want to support the legacy builder in Palm because its deprecation was only 5 months ago, which is a bit too short for my taste. Some servers might not have been upgraded since then.

Here's what I suggest:

  1. Merge this PR, after you re-introduce the minimal compose requirement. Optionally, you can add a "Docker recommended version". You may (also optionally) remove the line about arm64 work-in-progress support from the installation docs.
  2. I'll open a PR to fix the build in non-buildkit mode.
  3. You can assume that BuildKit will be required in Quince, and thus ignore non-buildkit mode in nightly.

regisb added a commit that referenced this pull request Jul 31, 2023
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so
this transition should be seamless for users of the latest Tutor.

Relevant discussion:
overhangio#868 (comment)
@kdmccormick
Copy link
Collaborator Author

@regisb

  1. Done 👍🏻 Merge this at your leisure.
  2. Thanks!
  3. I have branch that removes the buildkit conditionals. Next week I'll test it out and then open a PR against nightly.

kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so
this transition should be seamless for users of the latest Tutor.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so
this transition should be seamless for users of the latest Tutor.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 18, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
@regisb
Copy link
Contributor

regisb commented Aug 28, 2023

I merged this PR manually to fix the documentation build issue. Thanks for the PR!

@regisb regisb closed this Aug 28, 2023
@kdmccormick kdmccormick deleted the kdmccormick/docker-compose branch August 28, 2023 14:38
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 31, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 31, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Aug 31, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Sep 1, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Sep 1, 2023
BuildKit replaces and improves the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the
Dockerfile and makes future build performance improvements easier. The
Docker versions which Tutor recommends (v20+) all come with BuildKit, so

As follow-up work, we will need to remove `is_buildkit_enabled` from
the official plugins templates.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Oct 5, 2023
BuildKit replaces and improves upon the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us
to seriously simplify the Dockerfile, and it also unlocks future build
performance improvements based on BuildKit-specific features such as:

* COPY --link
* Multiple build contexts
* More aggressive parallelization during the build process

The Docker versions which Tutor recommends (v20+) all come with
BuildKit, so this should be a seamless transition for Tutor users who
have been installing the recommended version.

We leave the ``is_buildkit_enabled`` template variable in place in order
to avoid breaking plugins, but we now just set it to ``True`` as a
constant. It will be removed some time after Quince, so plugins should
remove any uses of the variable from their Dockerfiles.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Oct 5, 2023
BuildKit replaces and improves upon the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us
to seriously simplify the Dockerfile, and it also unlocks future build
performance improvements based on BuildKit-specific features such as:

* COPY --link
* Multiple build contexts
* More aggressive parallelization during the build process

The Docker versions which Tutor recommends (v20+) all come with
BuildKit, so this should be a seamless transition for Tutor users who
have been installing the recommended version.

We leave the ``is_buildkit_enabled`` template variable in place in order
to avoid breaking plugins, but we now just set it to ``True`` as a
constant. It will be removed some time after Quince, so plugins should
remove any uses of the variable from their Dockerfiles.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Oct 5, 2023
BuildKit replaces and improves upon the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us
to seriously simplify the Dockerfile, and it also unlocks future build
performance improvements based on BuildKit-specific features such as:

* COPY --link
* Multiple build contexts
* More aggressive parallelization during the build process

The Docker versions which Tutor recommends (v20+) all come with
BuildKit, so this should be a seamless transition for Tutor users who
have been installing the recommended version.

We leave the ``is_buildkit_enabled`` template variable in place in order
to avoid breaking plugins, but we now just set it to ``True`` as a
constant. It will be removed some time after Quince, so plugins should
remove any uses of the variable from their Dockerfiles.

Relevant discussion:
overhangio#868 (comment)
kdmccormick added a commit to kdmccormick/tutor that referenced this pull request Oct 16, 2023
BuildKit replaces and improves upon the legacy Docker builder, which was
deprecated back in Feb 2023. Assuming that BuildKit is enabled allows us
to seriously simplify the Dockerfile, and it also unlocks future build
performance improvements based on BuildKit-specific features such as:

* COPY --link
* Multiple build contexts
* More aggressive parallelization during the build process

The Docker versions which Tutor recommends (v20+) all come with
BuildKit, so this should be a seamless transition for Tutor users who
have been installing the recommended version.

We leave the ``is_buildkit_enabled`` template variable in place in order
to avoid breaking plugins, but we now just set it to ``True`` as a
constant. It will be removed some time after Quince, so plugins should
remove any uses of the variable from their Dockerfiles.

Relevant discussion:
overhangio#868 (comment)
moonesque pushed a commit to edSPIRIT/tutor that referenced this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants