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

ops: merge go op-stack build into dockerfile targets #8242

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

protolambda
Copy link
Contributor

Description

Previously we simplified the main Go op-stack components down to one shared docker build, and individual docker images per target.

To avoid the manual relation between separate Dockerfiles, we can use docker targets within a unified Dockerfile, and isolate the the build stage per docker target, while still sharing the same Go mod and build caches.

This is experimental: the hypothesis is that now that the Circle CI docker-layer-cache works, we don't have to manually specify the common relationship between the different targets. There may be some less overlap due to things running in parallel, but cache re-use across different pipelines should help handle most of the work.

We can watch the CI time / build results, and revert the PR later if it affects the CI time badly.

By doing this, we avoid the need for the explicit intermediate "op-stack-go" docker image, simplifying our CI and devnet setup.

Bonus: the version build-args are now set in the docker bake config, and default to the global GIT_VERSION bake var, which is set during the CI build. So when running docker run --rm -it us-docker.pkg.dev/oplabs-tools-artifacts/images/op-node:versionhere op-node --version it should print the correct version.

Metadata

Fix https://github.com/ethereum-optimism/client-pod/issues/238

@protolambda protolambda requested review from a team as code owners November 22, 2023 19:18
@protolambda protolambda requested a review from Inphi November 22, 2023 19:18
@ajsutton
Copy link
Contributor

Looking at CI build times it looks like this makes no difference to the total build time (still 25mins on this PR and on the latest develop build). However the docker build steps are way faster. op-node-build for example is 46s in this PR vs 1m34s for op-stack-docker-build plus 1m27s for op-node-docker-build in the last develop build. Maybe it hit caches better - I can see one build from develop that took 55s and 34s but it looks like the split approach is costing a bunch of time in saving artefacts and schlepping them between build jobs.

Haven't looked at the code changes yet but on those timings this seems like a win all round - simpler and faster.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Do we need to update the Makefile golang-docker target? It's passing for me but the built images don't work on Mac:

$ make golang-docker
#61 [op-challenger] exporting to image
#61 exporting layers done
#61 writing image sha256:8f22a06b949a369a792e419cc034f2b85becd02de29708464ce2b5b1cdee09d3 done
#61 naming to us-docker.pkg.dev/oplabs-tools-artifacts/images/op-challenger:4775de60371bb7d2844baa14549d0cc86c11f3df done
#61 naming to us-docker.pkg.dev/oplabs-tools-artifacts/images/op-challenger:latest done
#61 DONE 0.0s
 $ docker run us-docker.pkg.dev/oplabs-tools-artifacts/images/op-challenger:4775de60371bb7d2844baa14549d0cc86c11f3df
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
exec /usr/local/bin/op-challenger: no such file or directory

I'm not sure why it built amd64 or why it couldn't find the op-challenger command. I've pruned all the existing docker images locally so it should have been a clean build.

The devnet does work on Mac and is building an arm64 image as expected.

@protolambda
Copy link
Contributor Author

@ajsutton looks like I missed the --platform somewhere in the dockerfile, will investigate.

@protolambda
Copy link
Contributor Author

@ajsutton turns out I didn't set the default target platform variable to the local build platform, so it was cross-building for amd64 while on arm. I made all the platform choices in the dockerfile explicit now to be sure, and fixed the plaform default, and tested it on a macbook air m2 (arm v8) running asahi linux. Should be good to go now.

@ajsutton
Copy link
Contributor

I was just testing the latest commit you pushed locally and it was failing with:

ERROR: failed to solve: alpine:3.18: no match for platform in manifest sha256:eece025e432126ce23f223450a0326fbebde39cdf496a85d8c016293fc851978: not found

it passed when I set that default to linux/arm64 though so I wonder if the default I'm getting is something dumb like mac/arm64. Good to know it worked for you though - leave it with me and I'll have a play locally after office hours to see what's going on.

@ajsutton
Copy link
Contributor

Yeah I'm getting:

      "platforms": [
        "darwin/arm64/v8"
      ],

Works if I set the PLATFORMS env var to linux/arm64. Not sure why we have different defaults yet but it's happening on both my work and personal Mac. I do tend to configure them the same way though so...

@ajsutton
Copy link
Contributor

It does work for me if I set the default for PLATFORMS to be the empty string.

variable "PLATFORMS" {
  // You can override this as "linux/amd64,linux/arm64".
  // Only a specify a single platform when `--load` ing into docker.
  // Multi-platform is supported when outputting to disk or pushing to a registry.
  // Multi-platform builds can be tested locally with:  --set="*.output=type=image,push=false"
  default = ""
}

That also seems to work in a linux VM for me but I don't have an intel linux machine to test with.

@ajsutton
Copy link
Contributor

ok, confirmed it also works in an linux/amd64 VM and correctly generates amd images there. So I think no default value is the trick to make it work.

@ajsutton ajsutton enabled auto-merge November 23, 2023 20:20
@ajsutton ajsutton added this pull request to the merge queue Nov 23, 2023
Merged via the queue into develop with commit c12a946 Nov 23, 2023
59 checks passed
@ajsutton ajsutton deleted the go-docker-targets branch November 23, 2023 21:05
trianglesphere added a commit that referenced this pull request Nov 28, 2023
…gets"

This reverts commit c12a946, reversing
changes made to caa9d27.
github-merge-queue bot pushed a commit that referenced this pull request Nov 29, 2023
protolambda added a commit that referenced this pull request Dec 21, 2023
…cker-targets""

This reverts commit d972c46.

It also fixes up some conflicts / inconsistencies,
 since op-conductor was added after the original revert.
bitwiseguy pushed a commit that referenced this pull request Apr 30, 2024
…cker-targets""

This reverts commit d972c46.

It also fixes up some conflicts / inconsistencies,
 since op-conductor was added after the original revert.
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2024
* Revert "Revert "Merge pull request #8242 from ethereum-optimism/go-docker-targets""

This reverts commit d972c46.

It also fixes up some conflicts / inconsistencies,
 since op-conductor was added after the original revert.

* ops: experimental cross-build fixes

* Retrieve git tag in Circle CI and use to set op version within docker image

* Update dispute-mon and da-server to use new docker build flow

* Fix GIT_VERSION script in Circle CI config.yml

* Update ops-bedrock docker-compose to use new docker build flow

* Load pre-built op-challenger image in devnet tests

* Save op-challenger.tar to avoid docker rebuild in devnet tests

* Add Circle CI job for check-cross-platform

* Allow env var to override VERSION in Makefiles

* Pass version to op-program components except op-program-client

* Wrap all docker-bake variable names in quotes

---------

Co-authored-by: protolambda <[email protected]>
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.

2 participants