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

feat: build and publish weekly alpha/development container image #594

Merged
merged 1 commit into from
Dec 26, 2022

Conversation

tamcore
Copy link
Contributor

@tamcore tamcore commented Dec 23, 2022

Closes #308. Exerpt from commit message:

  • make use of docker buildx caching when possible (helpful with local docker builds)
  • introduce a reusable container workflow which is triggered by docker-release and docker-weekly workflows
  • added an alpine-dev Dockerfile
  • split release.sh contents into different Makefile targets
  • make use of job matrix to build alpine + ubuntu in parallel
  • make alpine build optional by checking for Dockerfile presence
    • as the pre-built release binaries don't work with alpine, because of glibc <-> musl incompatibilities

Tagged images will look like

  • From pre-existing docker-release workflow (which will not output an alpine image, as Dockerfile.alpine-prod has been renamed)
    • For release v0.12.2
      • docker-release yaml v0 12 2 non-prerelease FOR UBUNTU (ALPINE SKIPPED)
    • For pre-release v0.12.2
      • Screenshot 2022-12-26 at 10 12 14
  • From newly introduced docker-weekly workflow
    • For alpine variant
      • docker-weekly yaml ALPINE VARIANT
    • For ubuntu variant
      • docker-weekly yaml UBUNTU VARIANT

@romange
Copy link
Collaborator

romange commented Dec 23, 2022

Thank you, Phillip. Curious, why did you decide to switch to makefile? Ergonomics?

@tamcore
Copy link
Contributor Author

tamcore commented Dec 24, 2022

Felt like it would be overkill to have a bunch of separate scripts for it and didn't want to parameterize release.sh either. So the Makefile felt the "most natural" to place for a handful of one-liners.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,59 @@
# syntax=docker/dockerfile:1
FROM ubuntu:20.04 as builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. what's a AIO?
  2. Why do you prefer building dev env here instead of reusing the already built one : ghcr.io/romange/ubuntu-dev:20 ?

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 makes it more self-contained and comprehensive for the end-user. It's just an apt-get install, not special tooling :) Also, this way, the outputted container is more up2date on patches (and I feel like, Trivy imagine scanning should also be incorporated here)

Additionally, a special case which comes to my mind: I had a client with an air-gapped environment, where I would've been able to bring in a container image like ubuntu:20.04 and this Git repo as source code, but not neccessarily your dev container.

But honestly, I haven't investigated your dev container too much. Just couldn't find it's origin with 2 clicks and was relying on the instructions in the existing release workflow. Please just let me know your preference, and I'll change it accordingly :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's fine, just wanted to understand your rationale.

@tamcore tamcore force-pushed the 308 branch 7 times, most recently from f3841cc to b2ef9a8 Compare December 24, 2022 14:19
echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache

RUN \
--mount=type=cache,target=/var/cache/apt,sharing=locked \
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain what you do here? I understand that it's somehow connected to apt caching but I do not understand exactly what it is. If you have a reference to a blog post or documentation this will also 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.

Theoretically, that's supposed to cache and share /var/cache/apt and /var/lib/apt between both builds, but also between runs (through the cache-{to|from}: type=gha parameter). But doesn't seem to work as I'd hoped.

@tamcore tamcore force-pushed the 308 branch 15 times, most recently from 7a1b742 to b90c66b Compare December 24, 2022 19:40
@tamcore tamcore force-pushed the 308 branch 11 times, most recently from f367036 to ef5506e Compare December 26, 2022 09:07
@tamcore tamcore changed the title feat: build and publish weekly alpha/development container image (WIP) feat: build and publish weekly alpha/development container image Dec 26, 2022
@tamcore tamcore marked this pull request as ready for review December 26, 2022 09:13
@@ -0,0 +1,56 @@
# syntax=docker/dockerfile:1
Copy link
Collaborator

Choose a reason for hiding this comment

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

unless I misunderstand this what this dockerfile does, I think ubuntu-dev is a wrong name for this file.
Would you like that we publish builder images inside dragonflydb org instead of bundling them together with the docker release logic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

another option is to rename the files to Dockerfile.ubuntu-release (the same applies to alpine of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'd keep it as-is. That way, the *-dev Dockerfiles can progress (if needed), as development goes on. The only additional change, I'd suggest, is building the actual release containers from source as well. That would make it easier, to add support for alpine, for example.

But I also wanted to have a look at distroless images, to potentially come up with something even smaller. And with the matrix builds now, throwing in another flavor isn't too difficult :D

The alpine build is already 45% smaller than the ubuntu one :)

philipp@ ~ $ docker image ls | grep tamcore/dragonfly
ghcr.io/tamcore/dragonfly   alpha-alpine             36c1d38c6f2e   2 hours ago         107MB
ghcr.io/tamcore/dragonfly   alpha-ubuntu             78dbf284cc5c   2 hours ago         194MB

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, I had trouble with alpine in the past due to its particular choice of using musl library. I do not remember specifics but I refrain from recommending alpine as a default dragonfly container.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding your comment - I do not understand it. dev containers are for building dev environment, but you added the next build step to build a container equivalent to a release container, so not i's not a dev container anymore. Right? So what do you mean by "keeping as is" and what do you mean by "building actual release containers from source" ?

Copy link
Contributor Author

@tamcore tamcore Dec 26, 2022

Choose a reason for hiding this comment

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

FYI, I had trouble with alpine in the past due to its particular choice of using musl library. I do not remember specifics but I refrain from recommending alpine as a default dragonfly container.

Fair point. I had a ton of issues trying to get the pre-compiled Dragonfly binaries working under alpine. But so far it seems fine, with the version compiled directly on alpine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding your comment - I do not understand it. dev containers are for building dev environment, but you added the next build step to build a container equivalent to a release container, so not i's not a dev container anymore. Right? So what do you mean by "keeping as is" and what do you mean by "building actual release containers from source" ?

I guess we're thinking about different things here 😅

Maybe the name *-dev is indeed not optimal. But if we rename them to *-release, they kinda obsolete the *-prod images, no? Then we could just remove the suffix altogether and have the same Dockerfile for both the weekly and release builds, where the latter would then be built from source as well, rather than relying on the build artifacts from the normal release workflow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, building a binary for aarch64 takes like 2 hours on qemu.... In addition, if we reuse the same binary we will avoid weird synchronization bugs of us having different binaries for the same release.

Ok, now I understand that dev applies to a release "flavor". Lets keep the name as is.

./helio/blaze.sh $(HELIO_FLAGS)

build:
cd build-opt; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless I am mistaken, this makefile is intended to build only the release version of dragonfly.
From this code it seems like it because you cd to build-opt which is created only for release.

if you intend to extend the makefile functionality to debug variant as well, then it support "cd build-dbg".

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you do not want to support debug builds then this HELIO_RELEASE logic is over-complication - you can just hardcode these flags into HELIO_FLAGS without requiring a caller to pass a special flag.

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 guess we should support debug builds as well, while we're already in here. Might make it easier down the road.

@tamcore tamcore force-pushed the 308 branch 3 times, most recently from efbe670 to 1146992 Compare December 26, 2022 11:38
weekly-container-build:
uses: ./.github/workflows/reusable-container-workflow.yaml
with:
dockerfile: dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe rename "dockerfile" to "flavour" but besides this LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this spot to build_type, because, usually, the distro is referenced as the flavor. Now called that way as well.

romange
romange previously approved these changes Dec 26, 2022
- make use of docker buildx caching when possible (helpful with local docker builds)
- introduce a reusable container workflow which is triggered by docker-release and docker-weekly workflows
- added an alpine-dev Dockerfile
- split release.sh contents into different Makefile targets
- make use of job matrix to build alpine + ubuntu in parallel
- make alpine build optional by checking for Dockerfile presence
-- as the pre-built binaries don't work with alpine, because of glibc <-> musl incompatibilities

Signed-off-by: Philipp Born <git@pborn.eu>
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Well, I hope you will be nearby when we cut our next release 😆

@tamcore
Copy link
Contributor Author

tamcore commented Dec 26, 2022

I've built it so often over the last few days, no need for automations anymore. Can do it by hand 😂

The latest changes still seem to work:

@romange
Copy link
Collaborator

romange commented Dec 26, 2022

Thanks Philipp, much appreciated! 🤝

@romange romange merged commit 98b92a0 into dragonflydb:main Dec 26, 2022
@tamcore tamcore deleted the 308 branch December 26, 2022 18:44
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.

CI: introduce weekly alpha releases
2 participants