Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

[CI] Docker image updates #1799

Merged
merged 19 commits into from
Oct 19, 2020
Merged

[CI] Docker image updates #1799

merged 19 commits into from
Oct 19, 2020

Conversation

s3krit
Copy link
Contributor

@s3krit s3krit commented Oct 9, 2020

This PR makes a number of changes:

  1. Do not push the parity/polkadot:latest docker image on new tag pushes
  2. Introduce a new Github Action that will push to parity/polkadot:latest when a new release is published, that will build the image based on our deb package repo (this is done with scripts/docker/Dockerfile.release
  3. Update the documentation in doc/docker.md to specify using our official docker images, rather than ones based on phusion. As per our Docker images maintenance policy: Use only official (in the _ namespace) third-party DockerHub images are allowed, both in direct use and inherited with FROM.
  4. Update docker/Dockerfile to be based on paritytech/ci-linux:production as per the aforementioned Docker images maintenance policy.

@s3krit s3krit added A0-please_review Pull request needs code review. B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. labels Oct 9, 2020
@s3krit s3krit self-assigned this Oct 9, 2020
uses: docker/build-push-action@v2
with:
push: true
file: scripts/docker/Dockerfile.release
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually cache layers in docker builds with Buildx, which gives a significant performance boost here.

First you'd need to set up caching, with a separate step (prior build-push-action):

      - name: Cache Docker layers
        uses: actions/cache@v2
        with:
          path: /tmp/.buildx-cache
          key: ${{ runner.os }}-buildx-${{ github.sha }}
          restore-keys: |
            ${{ runner.os }}-buildx-

And then in your build-push-action itself you just add

          cache-from: type=local,src=/tmp/.buildx-cache
          cache-to: type=local,dest=/tmp/.buildx-cache

properties, and everything seems to work. (At least on that silly side-project of mine.)

@@ -0,0 +1,46 @@
FROM debian:buster-slim
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to use "Dockerfile" as an extension: release.Dockerfile. Other way many linters do not recognize it.

io.parity.image.vendor="Parity Technologies" \
io.parity.image.title="parity/polkadot" \
io.parity.image.description="polkadot: a platform for web3" \
io.parity.image.source="https://github.com/paritytech/polkadot/blob/${VCS_REF}/scripts/docker/Dockerfile" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
io.parity.image.source="https://github.com/paritytech/polkadot/blob/${VCS_REF}/scripts/docker/Dockerfile" \
io.parity.image.source="https://github.com/paritytech/polkadot/blob/${VCS_REF}/scripts/docker/release.Dockerfile" \


# ===== SECOND STAGE ======

FROM phusion/baseimage:0.11
LABEL maintainer "chevdor@gmail.com"
FROM debian:buster-slim
LABEL description="This is the 2nd stage: a very small image where we copy the Polkadot binary."
Copy link
Contributor

Choose a reason for hiding this comment

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

could use the same Label as in release.Dockerfile below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of this Dockerfile is to be used by users - i.e., for dev purposes etc., so it doesn't make sense to have vendor info and references to variables (like $VCS_REF) that won't be present

Copy link
Contributor

Choose a reason for hiding this comment

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

OCI recommends filling labels as a best practice. At least it can be handy to know where is the code of this Dockerfile, this could be in label.

VOLUME ["/polkadot"]

ENTRYPOINT ["/usr/bin/polkadot"]

Copy link
Contributor

Choose a reason for hiding this comment

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

also I have some questions on diff scripts/docker/Dockerfile scripts/docker/Dockerfile.release

  • why different base?
  • why different endpoint for the binary?
  • why did you omit creating a user?

Copy link
Contributor Author

@s3krit s3krit Oct 12, 2020

Choose a reason for hiding this comment

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

why different base?

As per our README.md, I only claim our packages support the latest release versions of debian + ubuntu. It won't work with debian stretch - this is actually because the image used to build our release bins is base-ci-linux, which is based on buster also.

why different endpoint for the binary?

This dockerfile installs polkadot from our apt repository, so it gets placed in /usr/bin/ rather than /usr/local/bin.

why did you omit creating a user?

The polkadot user is created as part of installing the debian package.

Copy link
Contributor

Choose a reason for hiding this comment

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

which is based on buster also

oh, then we should change the base for the other image.

Copy link
Contributor

Choose a reason for hiding this comment

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

The polkadot user is created as part of installing the debian package.

It's worth mentioning in the comment there.

ca-certificates \
curl \
gnupg && \
gpg --recv-keys --keyserver hkps://keys.mailvelope.com 9D4B2B6EB8F97156D19669A9FF0812D491B96798 && \
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation here and below

@s3krit s3krit requested a review from TriplEight October 13, 2020 12:35
Copy link
Contributor

@gabreal gabreal 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 very good to me. what are the arguments to build and release docker images from github actions rather than gitlab ci (it's actually already there, could also use the deb package, runs on self-run infra)?

@s3krit
Copy link
Contributor Author

s3krit commented Oct 14, 2020

overall looks very good to me. what are the arguments to build and release docker images from github actions rather than gitlab ci (it's actually already there, could also use the deb package, runs on self-run infra)?

The main advantage is that we trigger it when the release is published rather than tagged. Since we perform a bunch of tests after the tag is pushed but before the release is published, it makes more sense to only push our docker images once the release is actually published.

@s3krit s3krit removed the A0-please_review Pull request needs code review. label Oct 14, 2020
@s3krit s3krit added A8-mergeoncegreen A3-in_progress Pull request is in progress. No review needed at this stage. and removed A8-mergeoncegreen labels Oct 14, 2020
Copy link
Contributor

@TriplEight TriplEight 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, thanks for updating both Dockerfiles to a new base.

@s3krit s3krit merged commit e1a5c6b into master Oct 19, 2020
@s3krit s3krit deleted the mp-docker-release branch October 19, 2020 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants