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

ci(docker): use Depot for multiarch images #10039

Merged
merged 3 commits into from
Sep 13, 2024
Merged

ci(docker): use Depot for multiarch images #10039

merged 3 commits into from
Sep 13, 2024

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Sep 6, 2024

closes: #9043

Description

Update docker.yml image publishing to use Depot for efficient multiarch builds.

Before (~2h):
image

After (~15m):
image

Also, stop building the agoric/agoric-sdk:ibc-alpha tag since it is no longer relevant (only was needed by an IBC test long, long ago).

Security Considerations

Introduces CI dependency on Depot. agoric-3-proposals already uses this successfully, so it is not new to our organisation.

Scaling Considerations

Improves scalability of our CI system (2h Docker multiarch build times are reduced to 15m).

Documentation Considerations

n/a

Testing Considerations

Manually launched Docker builds show that the new CI works correctly.

Upgrade Considerations

n/a

@michaelfig michaelfig marked this pull request as ready for review September 6, 2024 16:20
Copy link

cloudflare-workers-and-pages bot commented Sep 6, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 91d2990
Status: ✅  Deploy successful!
Preview URL: https://99e7711b.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-docker-depot.agoric-sdk.pages.dev

View logs

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

This one is flying a bit above my head. One thing however, I don't see an equivalent PR in agoric-3-proposals for the patch here.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@michaelfig michaelfig force-pushed the mfig-docker-depot branch 2 times, most recently from 82c3e3f to e7fb573 Compare September 9, 2024 20:54
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

So far this seems overly complex to me. Let's figure out how to simplify.

- fromTag: "latest"
+ fromTag: "latest",
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:",
+ sdkRepositoryColon: "ghcr.io/agoric/agoric-sdk:",
Copy link
Member

Choose a reason for hiding this comment

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

where is this value changed?

or repositoryColon for that matter?

// Must match the Bake file and CI config
- fromTag: "latest"
+ fromTag: "latest",
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:",
Copy link
Member

Choose a reason for hiding this comment

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

why is the colon necessary to include?

Copy link
Member

Choose a reason for hiding this comment

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

given there are multiple repositories in play, both git and Docker, please give this a more descriptive name. assuming we can drop the colon,

Suggested change
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:",
+ a3pDockerRepository: "ghcr.io/agoric/agoric-3-proposals",

RESUME(fromTag) {
return `
## RESUME
-FROM ghcr.io/agoric/agoric-3-proposals:${fromTag} as use-${fromTag}
Copy link
Member

Choose a reason for hiding this comment

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

why are these Docker vars necessary? the Dockerfile is generated by the CI job. Couldn't it just produce the desired file contents?

Copy link
Member Author

Choose a reason for hiding this comment

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

The buildConfig is part of the package.json. Would you prefer that CI edited that before prepare-build instead of creating a docker-bake.override.json to modify the Dockerfile's behaviour at build time?

Copy link
Member

Choose a reason for hiding this comment

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

yes, very much

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed any dependencies on a synthetic-chain patch, and reduced the postprocessing of the prepare-build-generated files to a single sed. Let's talk about the best way to make that change robust.

@michaelfig michaelfig force-pushed the mfig-docker-depot branch 14 times, most recently from 62d8aaf to d901bda Compare September 12, 2024 17:27
@@ -12,7 +12,7 @@
"doctor": "yarn synthetic-chain doctor"
},
"dependencies": {
"@agoric/synthetic-chain": "patch:@agoric/synthetic-chain@npm%3A0.1.0#~/.yarn/patches/@agoric-synthetic-chain-npm-0.1.0-148de716a6.patch",
"@agoric/synthetic-chain": "^0.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

might as well bump to 0.3.0

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@michaelfig michaelfig changed the title ci(docker): use Depot multiarch, publish use-upgrade-next ci(docker): use Depot for multiarch images Sep 12, 2024
@michaelfig
Copy link
Member Author

I extracted the controversial "publish a3p-use-upgrade-next" code into #10083.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

I don't feel knowledgeable enough with the depot stuff to approve, but from what I'm seeing this look fine to me.

- name: Save BUILD_TAG
run: |
ARCH=$(echo '${{ matrix.platform }}' | tr / _)
echo "BUILD_TAG=${{ needs.snapshot.outputs.tag }}-$ARCH" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Are we still generating -$ARCH tags? I have used those in the past, but mostly because the arm64 and thus combined image took forever to publish, so possibly this is no longer necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right; building and publishing the combined image now takes about the same time as the -linux_amd64 tag used to take. So, no more -$ARCH tags.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Nice improvement!

LGTM save for moving : into the variable. I trust you'll address before merging

Comment on lines 177 to 179
PREFIX="$REGISTRY/agoric/cosmic-swingset-solo:"
for TAG in ${{ needs.docker-sdk.outputs.tags }}; do
PREFIXED="$PREFIXED$sep$IMAGE:$TAG"
PREFIXED="$PREFIXED$sep$PREFIX$TAG"
Copy link
Member

Choose a reason for hiding this comment

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

why this change? the previous was more clear. please revert or justify.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay.

@michaelfig michaelfig self-assigned this Sep 13, 2024
@michaelfig michaelfig added tooling repo-wide infrastructure automerge:no-update (expert!) Automatically merge without updates labels Sep 13, 2024
@mergify mergify bot merged commit 2abdc28 into master Sep 13, 2024
100 checks passed
@mergify mergify bot deleted the mfig-docker-depot branch September 13, 2024 21:41
mergify bot added a commit that referenced this pull request Sep 13, 2024
Project ID file accidentally missing from #10039 which caused the following error in CI:

```
Error: could not configure buildx: unknown project ID (run `depot init` or use --project or $DEPOT_PROJECT_ID)
```

This was confirmed fixed with a manual run of `Docker Build Release Images`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build SDK release image with Depot
3 participants