Add s3 remote cache #2824

bpaquet · 2022-04-25T18:10:47Z

Largely inspired from #2323

bpaquet · 2022-04-25T18:11:05Z

I'm currently testing this under load @doctolib

tonistiigi · 2022-04-26T00:37:36Z

Same questions as #2323 .

How do we test this? We don't necessarily need to run existing tests with it but we should have something that verifies the basics on the CI so we don't accidentally break it. Especially because none of the current popular downstream would have many users on it(at least initially). A separate workflow would be ok. Or if that is hard then minimally external service that tests master branch and notifies if things go wrong.
Are you committing to maintaining this?

Why is this 40k lines bigger than the previous PR?

Take a look at the CI results as well.

cc @sethpollack

bpaquet · 2022-04-26T12:41:14Z

Same questions as #2323 .

How do we test this?

I'm currenlty testing this on my CI.

We don't necessarily need to run existing tests with it but we should have something that verifies the basics on the CI so we don't accidentally break it. Especially because none of the current popular downstream would have many users on it(at least initially). A separate workflow would be ok. Or if that is hard then minimally external service that tests master branch and notifies if things go wrong.

What do you mean exactly? A dedicated workflow in github which test this? An integration test? Do you have any base I can start from?

Are you committing to maintaining this?

Yes.

Why is this 40k lines bigger than the previous PR?

New version of the aws s3 sdk :(

Take a look at the CI results as well.

cc @sethpollack

tonistiigi · 2022-04-26T16:57:54Z

What do you mean exactly? A dedicated workflow in github which test this? An integration test? Do you have any base I can start from?

I assume testing something like this requires either token credentials or some kind of mock service. Our regular integration tests for things like this are in client_test.go or dockerfile_test.go. But as this would likely be something that can't just be run in a container with no external dependencies it can be a special github actions workflow. Eg. we currently have special workflow for windows testing. In that workflow/test you should run the basic loop of building, exporting cache, prune local cache, rebuild with cache and verify it matched. If you want you can use external tools in that workflow, eg. you could use buildx create for simplicity. @crazy-max

bpaquet · 2022-05-05T12:29:52Z

@tonistiigi Can you review?

Tests are in hack/s3_test. I can move them where you want. Any feedback is welcome. Not sure how to taunch them in GHA, especially with the --privileged.
I duplicated your readerat.go from the go-actions-cache. Should we move it in buildkit repo?
Test under load still in progress.

tonistiigi

Add the ./hack/s3_test/run_test.sh invocation to https://github.com/moby/buildkit/blob/master/.github/workflows/build.yml with needs: [base] .

I added a commit that fixes up the Dockerfile and includes the correct buildkit binaries.

It doesn't work for me locally though

Documentation: https://docs.min.io
Finished loading IAM sub-system (took 0.0s of 0.0s to load data).
+ mc alias set myminio http://172.17.0.2:9000 minioadmin minioadmin
mc: Configuration written to `/root/.mc/config.json`. Please update your access credentials.
mc: Successfully created `/root/.mc/share`.
mc: Initialized share uploads `/root/.mc/share/uploads.json` file.
mc: Initialized share downloads `/root/.mc/share/downloads.json` file.
mc: <ERROR> Unable to initialize new alias from the provided credentials. Get "http://172.17.0.2:9000/probe-bucket-sign-tszmiipmum4w/?location=": dial tcp 172.17.0.2:9000: connect: connection refused.

Assuming this works it doesn't actually look as complicated as I would have assumed. You can also consider adding the minio to the main Dockerfile integration-tests stage and adding a regular Go integration test.

FYI @crazy-max

tonistiigi · 2022-05-06T06:17:11Z

hack/s3_test/Dockerfile

+FROM debian:bullseye-slim
+
+RUN apt-get update \
+  && apt-get install -y --no-install-recommends wget ca-certificates containerd


Why is containerd needed here?

Buildkit does not start without it

WARN[2022-05-06T08:27:58Z] skipping oci worker, as runc does not exist WARN[2022-05-06T08:27:58Z] skipping containerd worker, as "/run/containerd/containerd.sock" does not exist buildkitd: no worker found, rebuild the buildkit daemon?

May be you have a magical option to add to buildkitd -debugaddr 0.0.0.0:8060 :)

tonistiigi · 2022-05-06T06:40:53Z

hack/s3_test/test1/Dockerfile

@@ -0,0 +1,4 @@
+FROM public.ecr.aws/debian/debian:bullseye-slim
+
+RUN echo first


These commands should write a constant plus a random value. See https://github.com/moby/buildkit/blob/master/client/client_test.go#L1410 . Then when you build them export the result to a local directory and check that the unique part has not change. Therefore cache matched.

tonistiigi · 2022-05-06T06:49:03Z

Not sure how to taunch them in GHA, especially with the --privileged.

--privileged is not a problem in GH actions.

I duplicated your readerat.go from the go-actions-cache. Should we move it in buildkit repo?

Need to think about this later. I don't want an import cycle there.

crazy-max · 2022-05-06T09:04:05Z

You can also consider adding the minio to the main Dockerfile integration-tests stage and adding a regular Go integration test.

Added a commit to add minio to integration tests stage and also use it through linked context in the bake definition for s3 test.

I will add another commit to create a new job in our GHA workflow.

bpaquet · 2022-05-06T09:14:15Z

Add the ./hack/s3_test/run_test.sh invocation to https://github.com/moby/buildkit/blob/master/.github/workflows/build.yml with needs: [base] .

I added a commit that fixes up the Dockerfile and includes the correct buildkit binaries.

It doesn't work for me locally though

Documentation: https://docs.min.io
Finished loading IAM sub-system (took 0.0s of 0.0s to load data).
+ mc alias set myminio http://172.17.0.2:9000 minioadmin minioadmin
mc: Configuration written to `/root/.mc/config.json`. Please update your access credentials.
mc: Successfully created `/root/.mc/share`.
mc: Initialized share uploads `/root/.mc/share/uploads.json` file.
mc: Initialized share downloads `/root/.mc/share/downloads.json` file.
mc: <ERROR> Unable to initialize new alias from the provided credentials. Get "http://172.17.0.2:9000/probe-bucket-sign-tszmiipmum4w/?location=": dial tcp 172.17.0.2:9000: connect: connection refused.

Fixed

Assuming this works it doesn't actually look as complicated as I would have assumed. You can also consider adding the minio to the main Dockerfile integration-tests stage and adding a regular Go integration test.

I prefer to keep it like this if you dont mind.

FYI @crazy-max

I

bpaquet · 2022-05-06T09:15:28Z

You can also consider adding the minio to the main Dockerfile integration-tests stage and adding a regular Go integration test.

Added a commit to add minio to integration tests stage and also use it through linked context in the bake definition for s3 test.

I will add another commit to create a new job in our GHA workflow.

@crazy-max just force pushed, sorry may be I drop some stuff. I added something in the build workflow, please feel free to refact.

crazy-max · 2022-05-06T09:23:14Z

@crazy-max just force pushed, sorry may be I drop some stuff. I added something in the build workflow, please feel free to refact.

Yes just saw that, that's fine. I bring it back.

I prefer to keep it like this if you dont mind.

In the future I think we don't want to rely on scripting but Go integration tests like we do for containerd and dockerd. See https://github.com/moby/buildkit/tree/master/util/testutil/integration

crazy-max · 2022-05-06T11:05:30Z

Not linked to this PR but I noticed that when we use linked context in bake a warning from buildx appears: https://github.com/moby/buildkit/runs/6320810388?check_suite_focus=true#step:5:622

WARNING: No output specified for docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load

Looks linked to docker/buildx#968.

There is another one less obvious that seems to appear when using COPY --link and bake linked context. Maybe caching issue but as it's an error it should fail but build continues instead: https://github.com/moby/buildkit/runs/6320810388?check_suite_focus=true#step:5:590

#56 extracting sha256:5979a4d386a9a20337edbeed4404a714b811c40e4bf73aa97fa73ca0bcdc3b9e 0.3s done
#56 extracting sha256:7964a4d2bbf168b770bc9ee09ce82d556bb466804b07f06b9c749cf928b37fa2 0.4s done
#56 extracting sha256:7964a4d2bbf168b770bc9ee09ce82d556bb466804b07f06b9c749cf928b37fa2 0.4s done
#56 merging done
#56 ERROR: maximum timeout reached

cc @tonistiigi @sipsma

tonistiigi · 2022-05-09T21:38:28Z

cache/remotecache/s3/s3.go

+}
+
+func (e *exporter) Config() remotecache.Config {
+	return remotecache.Config{}


This should return Compression: default

tonistiigi · 2022-05-09T21:42:38Z

cache/remotecache/s3/s3.go

+	return config.Prefix + config.BlobsPrefix + dgst.String()
+}
+
+type cache struct {


Calling this cache seems kind of confusing. It's more like s3Client

tonistiigi · 2022-05-09T21:43:13Z

cache/remotecache/s3/s3.go

+}
+
+func (c *cache) getManifest(key string, config *v1.CacheConfig) (bool, error) {
+	fmt.Printf("Start downloading s3://%s/%s\n", c.config.Bucket, key)


Writing to stdout is not allowed in here. If you want debug logs then you need to write them to the context logger.

removed all debug

tonistiigi · 2022-05-09T21:45:06Z

cache/remotecache/s3/s3.go

+}
+
+func isNotFound(err error) bool {
+	awsErr, ok := err.(awserr.Error)


This should use errors.As

tonistiigi · 2022-05-09T21:49:36Z

hack/s3_test/test1/Dockerfile

+FROM public.ecr.aws/debian/debian:bullseye-slim
+
+RUN cat /dev/urandom | head -c 100 | sha256sum > unique_first
+RUN cat /dev/urandom | head -c 100 | sha256sum > unique_second


Add a another stage in here

FROM scratch COPY --link --from=build /unique_first / COPY --link --from=build /unique_second /

And in the test add --output destdir . After cache import the contents of the files need to remain the same.

tonistiigi

LGTM but try to squash the commits a bit. It doesn't need to be a single commit but we should avoid commits that just say "fixes" or "code review". Maybe squash these 2 with the first commit. The others can remain as they are.

tonistiigi · 2022-05-11T18:18:04Z

cache/remotecache/s3/s3.go

+func news3Client(config Config) (*s3Client, error) {
+	s, err := sess.NewSession(&aws.Config{Region: &config.Region, Endpoint: &config.EndpointURL, S3ForcePathStyle: &config.S3ForcePathStyle})
+	if err != nil {
+		return nil, fmt.Errorf("failed to create session: %w", err)


Use errors.Errorf always for stacktraces

bpaquet · 2022-05-12T10:09:53Z

@tonistiigi I squashed everything and moved to aws-sdk-go-v2. Ready for review :)

tonistiigi · 2022-05-12T22:02:10Z

hack/s3_test/run_test.sh

@@ -0,0 +1,7 @@
+#!/bin/sh -ex


This file looks unused now

I'm using it to launch the test locally.

If we have a helper script we should call it in the CI. Otherwise nobody notices if it breaks or starts to behave differently than CI invocations.

Used it in the CI

crazy-max · 2022-05-13T09:35:30Z

go.mod

@@ -76,7 +80,22 @@ require (
 	google.golang.org/grpc v1.45.0
 )

+require github.com/aws/aws-sdk-go-v2/config v1.15.5


should be moved in the previous require block

Fixed. I should do something wrong when I update mod / vendor :(

Signed-off-by: Bertrand Paquet <[email protected]>

crazy-max

LGTM

in a follow-up we should think about proper integration tests in testutil but might need to split first as suggested by @tonistiigi in #2734 (comment)

also README starts to be quite huge. wonder if we should not move some sections in dedicated pages under ./docs like the cache exporters.

zchee · 2022-06-17T19:32:09Z

@tonistiigi Can I send support GCS as well?

bpaquet · 2022-10-11T07:02:50Z

I’m using it to run the test locally.

…

On Fri 13 May 2022 at 00:03, Tõnis Tiigi ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hack/s3_test/run_test.sh <#2824 (comment)>: > @@ -0,0 +1,7 @@ +#!/bin/sh -ex This file looks unused now — Reply to this email directly, view it on GitHub <#2824 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACSJXKMF2JOLXFTVW7POXTVJV53HANCNFSM5UJLRGCA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

bpaquet force-pushed the s3_cache_master branch from cbbe072 to ac4ed5f Compare April 25, 2022 19:49

bpaquet changed the title ~~Add s3 remot cache~~ Add s3 remote cache Apr 25, 2022

bpaquet force-pushed the s3_cache_master branch from ac4ed5f to f874cd6 Compare April 25, 2022 19:49

bpaquet force-pushed the s3_cache_master branch 2 times, most recently from fe68519 to d856c7d Compare April 26, 2022 12:46

bpaquet force-pushed the s3_cache_master branch 3 times, most recently from 4a5b507 to d77e1d7 Compare May 5, 2022 12:26

tonistiigi self-requested a review May 5, 2022 18:28

tonistiigi reviewed May 6, 2022

View reviewed changes

tonistiigi force-pushed the s3_cache_master branch from bf0fb60 to 2cb6501 Compare May 6, 2022 06:47

bpaquet force-pushed the s3_cache_master branch from d9ff1ab to 3570e94 Compare May 6, 2022 08:59

bpaquet force-pushed the s3_cache_master branch from 954c3fd to c4f992c Compare May 6, 2022 09:13

crazy-max force-pushed the s3_cache_master branch from 7bb2711 to 1034b0a Compare May 6, 2022 09:21

crazy-max force-pushed the s3_cache_master branch from 1034b0a to 7a0c6c0 Compare May 6, 2022 09:27

bpaquet force-pushed the s3_cache_master branch from 4650a20 to 774cc45 Compare May 6, 2022 09:44

crazy-max force-pushed the s3_cache_master branch 2 times, most recently from 178546a to 34e8962 Compare May 6, 2022 16:32

bpaquet requested a review from tonistiigi May 9, 2022 10:07

tonistiigi reviewed May 9, 2022

View reviewed changes

bpaquet force-pushed the s3_cache_master branch 3 times, most recently from a971027 to ede8d29 Compare May 10, 2022 11:28

tonistiigi reviewed May 10, 2022

View reviewed changes

tonistiigi reviewed May 11, 2022

View reviewed changes

bpaquet force-pushed the s3_cache_master branch 3 times, most recently from 3bb72f7 to 86c98b4 Compare May 12, 2022 10:05

bpaquet force-pushed the s3_cache_master branch from 86c98b4 to cc4af04 Compare May 12, 2022 12:39

tonistiigi reviewed May 12, 2022

View reviewed changes

bpaquet force-pushed the s3_cache_master branch 2 times, most recently from 349c24c to e0b54a4 Compare May 13, 2022 09:34

crazy-max requested changes May 13, 2022

View reviewed changes

Add s3 remote cache

09c5a7c

Signed-off-by: Bertrand Paquet <[email protected]>

bpaquet force-pushed the s3_cache_master branch from e0b54a4 to 09c5a7c Compare May 13, 2022 11:17

sethpollack mentioned this pull request May 13, 2022

Add S3 remote cache #2323

Closed

bpaquet requested review from crazy-max and tonistiigi May 14, 2022 07:35

tonistiigi approved these changes May 17, 2022

View reviewed changes

crazy-max approved these changes May 20, 2022

View reviewed changes

tonistiigi merged commit 5d2ca98 into moby:master May 20, 2022

crazy-max mentioned this pull request Aug 25, 2022

s3 cache client-side support docker/buildx#1294

Merged

thaJeztah mentioned this pull request Oct 12, 2022

ERROR: failed to solve: unknown cache exporter: "s3" docker/buildx#1355

Closed

		@@ -0,0 +1,4 @@
		FROM public.ecr.aws/debian/debian:bullseye-slim

		RUN echo first

Add s3 remote cache #2824

Add s3 remote cache #2824

Conversation

bpaquet commented Apr 25, 2022

bpaquet commented Apr 25, 2022

tonistiigi commented Apr 26, 2022

bpaquet commented Apr 26, 2022

tonistiigi commented Apr 26, 2022

bpaquet commented May 5, 2022 • edited Loading

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented May 6, 2022

crazy-max commented May 6, 2022

bpaquet commented May 6, 2022

bpaquet commented May 6, 2022

crazy-max commented May 6, 2022

crazy-max commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpaquet commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy-max left a comment • edited Loading

Choose a reason for hiding this comment

zchee commented Jun 17, 2022

bpaquet commented Oct 11, 2022 via email

bpaquet commented May 5, 2022 •

edited

Loading

crazy-max left a comment •

edited

Loading