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

fix: default to legacy builder instead of BuildKit with podman #900

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eqladios
Copy link

Description

Podman doesn't support buildkit and defaults to "DOCKER_BUILDKIT=0", See https://github.com/containers/podman/blob/f7be7a365ad3e90db5f96f269a555f6f380f9275/cmd/podman/compose.go#L157-L170

This results in Podman failing builds when BUILDKIT_INLINE_CACHE or additional_contexts is used, as for example in

if (Object.keys(featureBuildInfo.buildArgs).length > 0 || params.buildKitVersion) {
buildOverrideContent += ' args:\n';
if (params.buildKitVersion) {
buildOverrideContent += ' - BUILDKIT_INLINE_CACHE=1\n';
}
for (const buildArg in featureBuildInfo.buildArgs) {
buildOverrideContent += ` - ${buildArg}=${featureBuildInfo.buildArgs[buildArg]}\n`;
}
}
if (Object.keys(featureBuildInfo.buildKitContexts).length > 0) {
buildOverrideContent += ' additional_contexts:\n';
for (const buildKitContext in featureBuildInfo.buildKitContexts) {
buildOverrideContent += ` - ${buildKitContext}=${featureBuildInfo.buildKitContexts[buildKitContext]}\n`;
}
}

A simple fix would to make cli ignore --buildkit parameter, default to never and log a warning when Podman is used.

Fixes #863

Testing

A failing example would be

Dockerfile

ARG RUBY_VERSION=3.2.5
FROM ghcr.io/rails/devcontainer/images/ruby:$RUBY_VERSION

devcontainer.json

{
  "name": "app",
  "dockerComposeFile": "compose.yaml",
  "service": "rails-app",
  "workspaceFolder": "/workspaces/${localWorkspaceFolderBasename}",
  "features": {
    "ghcr.io/devcontainers/features/github-cli:1": {},
    "ghcr.io/rails/devcontainer/features/sqlite3": {}
  },
  "containerUser": "vscode",
  "updateRemoteUserUID": true,
  "containerEnv": {
    "REDIS_URL": "redis://redis:6379/1",
    "HOME": "/home/vscode"
  },
  "forwardPorts": [3000, 6379],
  "postCreateCommand": "bin/setup"
}

compose.yaml

name: "app"

services:
  rails-app:
    build:
      context: ..
      dockerfile: .devcontainer/Dockerfile
    volumes:
    - ../..:/workspaces:cached
    command: sleep infinity
    depends_on:
    - redis

  redis:
    image: redis:7.2
    restart: unless-stopped
    volumes:
    - redis-data:/data

volumes:
  redis-data:

when features is used in devcontainer.json this would always fail with log the classic builder doesn't support additional contexts, set DOCKER_BUILDKIT=1 to use BuildKit

I have compiled this on my machine and used it with --buildkit auto and --buildkit none and it works correctly and tests are passing normally, maybe an additional testing can be added for this.

@eqladios eqladios requested a review from a team as a code owner September 20, 2024 16:46
@eqladios
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks good to me, looping in @chrmarti for extra 👀

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Should this only target the compose case?

@eqladios
Copy link
Author

eqladios commented Sep 27, 2024

Should this only target the compose case?

I believe this would still fail with devcontainer build --buildkit auto while using podman

if (buildParams.buildKitVersion) {
args.push('buildx', 'build');
if (buildParams.buildxPlatform) {
output.write('Setting BuildKit platform(s): ' + buildParams.buildxPlatform, LogLevel.Trace);
args.push('--platform', buildParams.buildxPlatform);
}
if (buildParams.buildxPush) {
args.push('--push');
} else {
if (buildParams.buildxOutput) {
args.push('--output', buildParams.buildxOutput);
} else {
args.push('--load'); // (short for --output=docker, i.e. load into normal 'docker images' collection)
}
}
if (buildParams.buildxCacheTo) {
args.push('--cache-to', buildParams.buildxCacheTo);
}
args.push('--build-arg', 'BUILDKIT_INLINE_CACHE=1');
} else {
args.push('build');
}

I guess I can add the check for podman in these lines and the compose lines instead, thought to short-circuit it from the --buildkit argument, not sure.

also maybe I'm missing it but is there a way to define Docker Path to be podman or docker before running the tests?

@eqladios eqladios requested a review from chrmarti October 8, 2024 12:58
@eqladios eqladios force-pushed the fix/default-to-legacy-builder-with-podman branch from 48be600 to 3a80b14 Compare November 1, 2024 11:52
- Podman doesn't support all buildkit features.
- Podman main branch is now always defaulting to "DOCKER_BUILDKIT=0", See https://github.com/containers/podman/blob/f7be7a365ad3e90db5f96f269a555f6f380f9275/cmd/podman/compose.go#L164
- fix: make cli ignore `--buildkit` parameter and default to `never` when Podman is used.
@eqladios eqladios force-pushed the fix/default-to-legacy-builder-with-podman branch from 3a80b14 to 8afbc9c Compare November 2, 2024 11:47
@chrmarti
Copy link
Contributor

chrmarti commented Nov 4, 2024

With Podman 4.5.1 I see buildah 1.30 with podman buildx version and that supports --build-context which we use in containerFeatures.ts. Due to that I think single containers might still benefit from podman buildx.

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.

devcontainer up not working with podman and docker-compose file
4 participants