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 Docker build and add concurrency limit #22273

Merged
merged 4 commits into from
May 24, 2024
Merged

Fix Docker build and add concurrency limit #22273

merged 4 commits into from
May 24, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented May 23, 2024

fixes: mozilla/addons#14802

Description

Fix the computed image tag in our docker compose file causing tags to be invalid and therrefore failing to push

Context

We added (here) support for running our container via a tag or a digest, but the logic had a bug that meant if you defined DOCKER_VERSION in the shell environment, it would override the formattted value produced by calling make setup.

Essentially, the value in the .env is updated but not in the shel environment.

Fun facts: There is a different between an image Id, and image digest and an image configuration digest.

Field Description
id Image ID is a unique identifier for a Docker image that is different on every build, even if the contents are the same.
digest A hash of the image layer content digests. A hash of hashes that is produced only when an image is pushed to a Docker registry. (To further confuse, it is also produced when building with buildkit, which is kind of like a pseudo-registry because it's running on its own Docker container that exports to the local daemon)
configuration digest This is a hash of the image configuration object which is what you get when you inspect an image. It is not 1:1 with a digest because it doesn't (necessarily) take into account changes in the contents of the image, but in its configuration. For example, you could have a change in a RUN step that produces no change in the content of the layer, but it is still a change. Kind of confusing.

Testing

There are several flows to test.

First and formost. remove your .env file and create a new one!

rm -f .env && make setup

Not doing this likely won't cause any problems but better consistent than sorry.

Run with digest

  1. Grab a digest from a built image in CI. This will NOT work with locally built images.
  2. set the digest and up the environment DOCKER_DIGEST=<value> make up

This should run the container exactly from the build.

Run with tag

  1. set the tag to latest DOCKER_VERSION=latest make up
  2. This should run the latest tag from dockerhub.

Test concurrency.

If you have an open PR from this branch, with jobs in progress. Pushing a new commit should queue a new workflow run and cancel the previous one. There should only be one running at a time. This will save a huge amount of resources as the number of jobs per workflow increases.

@KevinMind KevinMind requested a review from diox May 23, 2024 07:50
@KevinMind KevinMind force-pushed the 14802 branch 4 times, most recently from 97abd6b to 82f8039 Compare May 23, 2024 08:20
@KevinMind KevinMind marked this pull request as draft May 23, 2024 08:20
@KevinMind KevinMind force-pushed the 14802 branch 4 times, most recently from 42b11f3 to 97deb99 Compare May 23, 2024 09:29
@KevinMind
Copy link
Contributor Author

KevinMind commented May 23, 2024

Confirmed run of build-image with push that shows we are pushing the right tag

https://app.circleci.com/pipelines/github/mozilla/addons-server/31635/workflows/e0b785d1-c00b-479d-9642-35abe646683c/jobs/215066

You can run this image locally

DOCKER_DIGEST=sha256:72b39bc5098bb4108599f82b869edfd9971d29abe5a0d79b09cbccc1f310e101 make up

Cool right?

@KevinMind KevinMind force-pushed the 14802 branch 5 times, most recently from 2370ea9 to c018620 Compare May 23, 2024 10:35
@KevinMind KevinMind marked this pull request as ready for review May 23, 2024 10:38
@KevinMind
Copy link
Contributor Author

KevinMind commented May 23, 2024

Interesting result that counters what I've written above.

I reran the CI job which re "built" and pushed the image. I was expecting it to produce the same digest because literally nothing changed. I can verify that is true by container diffing the images.

Build 1: https://github.com/mozilla/addons-server/actions/runs/9206736288/job/25325241509?pr=22273
Build 2: https://github.com/mozilla/addons-server/actions/runs/9206736288/job/25325607566?pr=22273

kmeinhardt@kevins-mbp ~ % container-diff diff remote://mozilla/addons-server@sha256:1e0e0fd67bfec5e6ce4419961315fd13022c01337f5d0c10ebbe387dfd43c7e5 remote://mozilla/addons-server@sha256:01a25462865cfee0f91d2e542b83a3b631a1d52bd167ffe76911fd7d1b4b5a11 --type=history --type=file --type=pip --type=apt --type=node --json
ERRO[0001] error getting diff with HistoryAnalyzer: unsupported status code 404; body: 404 page not found
ERRO[0001] error getting diff with PipAnalyzer: unsupported status code 404; body: 404 page not found
[
  {
    "Image1": "mozilla/addons-server@sha256:1e0e0fd67bfec5e6ce4419961315fd13022c01337f5d0c10ebbe387dfd43c7e5",
    "Image2": "mozilla/addons-server@sha256:01a25462865cfee0f91d2e542b83a3b631a1d52bd167ffe76911fd7d1b4b5a11",
    "DiffType": "Apt",
    "Diff": {
      "Packages1": [],
      "Packages2": [],
      "InfoDiff": []
    }
  },
  {
    "Image1": "mozilla/addons-server@sha256:1e0e0fd67bfec5e6ce4419961315fd13022c01337f5d0c10ebbe387dfd43c7e5",
    "Image2": "mozilla/addons-server@sha256:01a25462865cfee0f91d2e542b83a3b631a1d52bd167ffe76911fd7d1b4b5a11",
    "DiffType": "File",
    "Diff": {
      "Adds": null,
      "Dels": null,
      "Mods": null
    }
  },
  {
    "Image1": "mozilla/addons-server@sha256:1e0e0fd67bfec5e6ce4419961315fd13022c01337f5d0c10ebbe387dfd43c7e5",
    "Image2": "mozilla/addons-server@sha256:01a25462865cfee0f91d2e542b83a3b631a1d52bd167ffe76911fd7d1b4b5a11",
    "DiffType": "Node",
    "Diff": {
      "Packages1": [],
      "Packages2": [],
      "InfoDiff": []
    }
  }
]%

No file/pip/apt/node changes. It is failing on the "history" so maybe there is a slight change in the configuration object but I was under the impression the configuration object wouldn't change if the exact same commit on the repo is re-ran twice.. how could it. maybe there is some time based data in the docker context that I don't know about...

This isn't really a deal breaker, it's just weird.

@@ -27,7 +27,7 @@ x-env-mapping: &env
services:
worker: &worker
<<: *env
image: mozilla/addons-server${DOCKER_VERSION:-}
image: ${DOCKER_TAG:-}
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to warn everyone again that they have to use make up/make update_docker (or at least make setup) when updating, otherwise things will be broken

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. running make up will automatically fix and yes running raw docker commands will break.

.github/workflows/verify-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
- get correct Image ID from the build.
- set docker tag correctly supporting tag and digest
- add concurrency limit to github action ci jobs
Makefile-os Outdated
@@ -7,7 +7,7 @@
DOCKER_BUILDER ?= container
DOCKER_PROGRESS ?= auto
DOCKER_PUSH ?= false
DOCKER_OUTPUT ?=
BUILDX_BAKE_METADATA_FILE ?= buildx-bake-metadata.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving this file a more explanatory name and a default value. It is ignored so we can create it by default. This means you don't have to expcitly opt in or know the location of the file. Useful for CI jobs that want to read this data.

@diox

This comment was marked as outdated.

Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

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

Still unsure about whether or not the added complexity of supporting digests is worth it but we can try it out.

.circleci/config.yml Outdated Show resolved Hide resolved
KevinMind and others added 2 commits May 24, 2024 09:39
Co-authored-by: Mathieu Pillard <diox@users.noreply.github.com>
@KevinMind KevinMind merged commit ddade2a into master May 24, 2024
13 checks passed
@KevinMind KevinMind deleted the 14802 branch May 24, 2024 08:09
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.

[Bug]: Master build is broken pushing the wrong tag.
2 participants