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

chore: ability to build container image #47

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

developer-guy
Copy link
Contributor

@developer-guy
Copy link
Contributor Author

kindly ping @ktock. I've built an image of buildg, you can reach out to the image 👇

developer-guy/buildg:latest

Copy link
Owner

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Comment on lines +71 to +73

containerize:
runs-on: ubuntu-20.04
Copy link
Owner

Choose a reason for hiding this comment

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

Can we publish images only on every tag instead of every push for avoiding unstable code published?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already like that?

name: Release
on:
  push:
    tags:
      - 'v*'

Comment on lines +112 to +118
name: Build image
uses: docker/bake-action@v2
with:
files: |
./docker-bake.hcl
${{ steps.meta.outputs.bake-file }}
targets: image-all
pull: true
push: ${{ github.event_name != 'pull_request' }}
Copy link
Owner

Choose a reason for hiding this comment

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

Could we use docker/build-push-action to simplify this? (maybe we don't need to use hcl)
https://github.com/docker/build-push-action/blob/master/docs/advanced/multi-platform.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, the best option would be "bake" because it really simplifies everything and improves reusability.

labels: |
org.opencontainers.image.title=buildg
org.opencontainers.image.description=Interactive debugger for Dockerfile, with support for IDEs (VS Code, Emacs, Neovim, etc.)
org.opencontainers.image.vendor=${{ github.repositor_owner }}
Copy link
Owner

Choose a reason for hiding this comment

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

typo? repositor_owner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, let me fix this, good catch.

Dockerfile Outdated
Comment on lines 15 to 25
ARG TARGETPLATFORM
# https://pkg.go.dev/cmd/go#hdr-Build_and_test_caching
RUN --mount=type=bind,target=. \
--mount=type=cache,target=/root/.cache/go-build \
--mount=type=cache,target=/go/pkg/mod \
xx-go build -o /out/example .

FROM scratch AS bin-unix
COPY --from=build /out/example /

ENTRYPOINT [ "/example"]
Copy link
Owner

Choose a reason for hiding this comment

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

runc should be need to make buildg work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that runc has a apk pkg, should we use this to install runc ?

Dockerfile Outdated
RUN --mount=type=bind,target=. \
--mount=type=cache,target=/root/.cache/go-build \
--mount=type=cache,target=/go/pkg/mod \
xx-go build -o /out/example .
Copy link
Owner

Choose a reason for hiding this comment

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

Can't we use GOARCH instead of xx-go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it uses GOARCH and GOOS under the good, what it really does is that it only parses the variable in form of "linux/amd64" provided by buildx and adds the right values to GOOS and GOARCH.

.dockerignore Outdated
Comment on lines 1 to 9
/.idea
/*.iml
/.vscode

/.dev
/bin
/dist
/site
/coverage.txt
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that there are no such files in this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me replace these with the correct ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated them fyi @ktock

apt-get update && \
apt-get install -y crossbuild-essential-amd64 crossbuild-essential-arm64 git libbtrfs-dev:amd64 libbtrfs-dev:arm64 libseccomp-dev:amd64 libseccomp-dev:arm64

FROM build-base-debian AS build-runc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ktock I've added runc binary to the image, is there anything else we need to add to final image?

@developer-guy developer-guy requested a review from ktock July 5, 2022 08:31
Copy link
Owner

@ktock ktock left a comment

Choose a reason for hiding this comment

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

We need docs about building buildg image with docker build and docker buildx.

Dockerfile Outdated
ARG RUNC_VERSION
RUN git clone https://github.com/opencontainers/runc.git /go/src/github.com/opencontainers/runc
WORKDIR /go/src/github.com/opencontainers/runc
RUN git checkout ${RUNC_VERSION} && \
mkdir -p /out
Copy link
Owner

Choose a reason for hiding this comment

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

We should use the released runc binaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using version 1.1.3 for the runc binary, which is also one of the released versions of it.
Would you mind giving a little bit more context?

Dockerfile Outdated

FROM scratch AS bin-unix
COPY --from=build /out/buildg /
COPY --from=build-runc /out/runc.${TARGETARCH:-amd64} /out/bin/runc
Copy link
Owner

Choose a reason for hiding this comment

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

runc needs to be stored at the location accessible from buildg (through PATH) otherwise buildg doesn't work.

$ docker run --rm -it -v /tmp/ctx:/ctx test debug /ctx/
WARN[2022-07-06T07:17:56Z] using host network as the default            
failed to find runc binary
github.com/moby/buildkit/executor/runcexecutor.New
	/go/pkg/mod/github.com/ktock/buildkit@v0.6.2-0.20220628004512-0a3180ac4985/executor/runcexecutor/executor.go:88
github.com/moby/buildkit/worker/runc.NewWorkerOpt
	/go/pkg/mod/github.com/ktock/buildkit@v0.6.2-0.20220628004512-0a3180ac4985/worker/runc/runc.go:56
github.com/ktock/buildg/pkg/buildkit.newWorker
	/src/pkg/buildkit/client.go:341
github.com/ktock/buildg/pkg/buildkit.newClient
	/src/pkg/buildkit/client.go:242
github.com/ktock/buildg/pkg/buildkit.Debug.func1
	/src/pkg/buildkit/client.go:68
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1571

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thank you for testing this, I've configured the location of runc as one of the executable paths which is a "/usr/local/bin"

Comment on lines 91 to 92
type=ref,event=pr
type=edge
Copy link
Owner

Choose a reason for hiding this comment

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

Needs comments why we need them. I think having only vX.X.X tag is just enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, removed the unnecessary ones.

docker-bake.hcl Outdated
inherits = ["image"]
platforms = [
"linux/amd64",
"linux/arm/v6",
Copy link
Owner

Choose a reason for hiding this comment

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

BuildKit doesn't provide the release image for this platform so we cannot provide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the information 🫶

Comment on lines +40 to +39
RUN GOARCH=amd64 CC=x86_64-linux-gnu-gcc make static && \
cp -a runc /out/runc.amd64
RUN GOARCH=arm64 CC=aarch64-linux-gnu-gcc make static && \
cp -a runc /out/runc.arm64
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that this Dockerfile cannot provide non-amd64 and non-arm64 image so we need to docker-bake.hcl not to build non-amd64 and non-arm64 images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense, thank you.

Dockerfile Outdated

FROM --platform=${BUILDPLATFORM} golang:${GO_VERSION}-bullseye AS build-base-debian
# libbtrfs: for containerd
# libseccomp: for runc and bypass4netns
Copy link
Owner

Choose a reason for hiding this comment

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

We aren't using bypass4netns.

Comment on lines +1 to +3
variable "GO_VERSION" {
default = "1.18"
}
Copy link
Owner

Choose a reason for hiding this comment

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

This also defined in the Dockerfile. We need an explanation comment why we need this duplication.

Copy link
Contributor Author

@developer-guy developer-guy Jul 14, 2022

Choose a reason for hiding this comment

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

It's here because from now on we manage containerization process through bake command, so, if we want to override the Go version, we'll be overriding it through the bake command using a syntax like "--set *.args.GO_VERSION=x.x.x"

docker-bake.hcl Outdated
target "_common" {
args = {
GO_VERSION = GO_VERSION
BUILDKIT_CONTEXT_KEEP_GIT_DIR = 1
Copy link
Owner

Choose a reason for hiding this comment

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

Needs explanation why we need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the builtin build-args provided by the BuildKit itself.

  • BUILDKIT_CONTEXT_KEEP_GIT_DIR= trigger git context to keep the .git directory

You can reach out to the official documentation to get more detail about them.

https://docs.docker.com/engine/reference/commandline/buildx_build/#build-arg

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for the explanation. Yes, I know what is this variable. Could you add the comment to docker-bake.hcl about the reason "why" this variable being needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about that it is necessary thing, so, I'm removing.

COPY --from=build /out/buildg /
COPY --from=build-runc /out/runc.${TARGETARCH:-amd64} /out/bin/runc

ENTRYPOINT [ "/buildg"]
Copy link
Owner

Choose a reason for hiding this comment

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

We should run buildg as a non-root user (can be following-up).

Dockerfile Outdated
@@ -0,0 +1,48 @@
ARG GO_VERSION=1.18
ARG $=v1.1.3
Copy link
Owner

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has to be a RUNC_VERSION environment variable, my bad.

Dockerfile Outdated
COPY --from=build /out/buildg /
COPY --from=build-runc /out/runc.${TARGETARCH:-amd64} /usr/local/bin/runc

USER 65532
Copy link
Owner

Choose a reason for hiding this comment

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

When buildg is executed as a non-root user, it requires rootlesskit wrapper (i.e. buildg.sh) https://github.com/ktock/buildg#rootless-mode
Otherwise buidlg fails with permission errors.

$ docker run --rm -it --privileged -v /tmp/ctx:/ctx test debug /ctx/
mkdir /var/lib/buildg: permission denied

I think we can work on rootless execution in a following PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds like a plan 🤘 what should we do for this, run buildg.sh with RUN instruction?

Dockerfile Outdated
ENV CGO_ENABLED=1
RUN GOARCH=amd64 CC=x86_64-linux-gnu-gcc make static && \
cp -a runc /out/runc.amd64
RUN GOARCH=${TARGETARCH} CC=aarch64-linux-gnu-gcc make static && \
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
RUN GOARCH=${TARGETARCH} CC=aarch64-linux-gnu-gcc make static && \
RUN GOARCH=arm64 CC=aarch64-linux-gnu-gcc make static && \

docker-bake.hcl Outdated
target "_common" {
args = {
GO_VERSION = GO_VERSION
BUILDKIT_CONTEXT_KEEP_GIT_DIR = 1
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for the explanation. Yes, I know what is this variable. Could you add the comment to docker-bake.hcl about the reason "why" this variable being needed?

@developer-guy developer-guy force-pushed the feature/46 branch 2 times, most recently from 23c97e1 to 8d17802 Compare July 20, 2022 19:43
Dockerfile Outdated
Comment on lines 8 to 10
COPY go.* .
# https://go.dev/ref/mod#module-cache
RUN --mount=type=cache,target=/go/pkg/mod go mod download
Copy link
Owner

Choose a reason for hiding this comment

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

base is only used by stage build. The stage build looks like it caches the go modules at /go/pkg/mod. So this pre-filling of modules seems to be unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this pre-filling of modules seems to be unneeded.

Sorry, don't understand what you mean here, can you give a bit more context?

Copy link
Owner

Choose a reason for hiding this comment

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

Downloading go modules will be done in the stage build and the modules are also cached because --mount=type=cache,target=/go/pkg/mod is specified in that stage. So this line running go mod download and cacheing go modules looks duplicated with the stage build. I thought we can safely eliminated this. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, now I get it, yes you are right, removed.

Dockerfile Outdated
Comment on lines 43 to 49
FROM ghcr.io/distroless/static:latest AS bin-unix
COPY --from=build /out/buildg /
COPY --from=build-runc /out/runc.${TARGETARCH:-amd64} /usr/local/bin/runc
Copy link
Owner

Choose a reason for hiding this comment

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

The created image still doesn't seems work. Could we use ubuntu image as the base image instead?

$ docker run --rm -it --privileged -v /tmp/ctx:/ctx test debug /ctx/
mkdir /var/lib/buildg: permission denied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but as you know, it ends up having a large image but anyways, to test whether it is working, we can do that.

@developer-guy developer-guy requested a review from ktock July 27, 2022 06:11
Copy link
Owner

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Overall looks good. Left some nits. Can you squash commits?

Dockerfile Outdated
Comment on lines 43 to 49
FROM ubuntu:bionic AS bin-unix
COPY --from=build /out/buildg /
COPY --from=build-runc /out/runc.${TARGETARCH:-amd64} /usr/local/bin/runc
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use 20.04 which is well tested in our GithubActions CI ?
And, could you add ca-certificates apt pkg for allowing buildg to access DockerHub etc. from the container.
Currently it fails.

$ docker run --rm -it --privileged -v /tmp/ctx:/ctx test debug /ctx/
WARN[2022-07-27T08:30:03Z] using host network as the default            
WARN[2022-07-27T08:30:03Z] git source cannot be enabled: failed to find git binary: exec: "git": executable file not found in $PATH 
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile:
#1 transferring dockerfile: 237B done
#1 DONE 0.2s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.2s

#3 [internal] load metadata for docker.io/library/busybox:latest
INFO[2022-07-27T08:30:04Z] trying next host                              error="failed to do request: Head \"https://registry-1.docker.io/v2/library/busybox/manifests/latest\": x509: certificate signed by unknown authority" host=registry-1.docker.io
#3 ERROR: failed to do request: Head "https://registry-1.docker.io/v2/library/busybox/manifests/latest": x509: certificate signed by unknown authority
------
 > [internal] load metadata for docker.io/library/busybox:latest:
------
failed to build: failed to solve: failed to do request: Head "https://registry-1.docker.io/v2/library/busybox/manifests/latest": x509: certificate signed by unknown authority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course 🙋🏻‍♂️

Dockerfile Outdated
Comment on lines 8 to 10
COPY go.* .
# https://go.dev/ref/mod#module-cache
RUN --mount=type=cache,target=/go/pkg/mod go mod download
Copy link
Owner

Choose a reason for hiding this comment

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

Downloading go modules will be done in the stage build and the modules are also cached because --mount=type=cache,target=/go/pkg/mod is specified in that stage. So this line running go mod download and cacheing go modules looks duplicated with the stage build. I thought we can safely eliminated this. WDYT?

@developer-guy developer-guy requested a review from ktock July 27, 2022 10:08
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

feat: add runc

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

feat: add runc

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

chore: add unnecessary folders to .dockerignore

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

updates according to the feedbacks

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

switch to non-root

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

use ubuntu:bionic as base for final image

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

feat: remove extra statement

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

feat: change base img to ubuntu:20.04ga

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
@ktock
Copy link
Owner

ktock commented Aug 3, 2022

@developer-guy This commit has been merged via #59 . Thanks for your contribution!

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.

containerize buildg
2 participants