Update Moby dependencies, address skipped builds #828
Conversation
AaronFriel
requested review from
mikhailshilkov,
joeduffy and
guineveresaenger
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
Adding additional logging to `sess.Run()` call here, and fixing moby/moby#45935, as a prerequisite to debugging #812. While looking into #812, when using a locally built provider, updates to the Image resource would not result in a new image built and a corresponding new `RepoDigest` output value. This was root caused to the Moby issue below after adding a log statement following `sess.Run()` in `provider/image.go`. - moby/moby#45935 However, the behavior seen while reproducing is *different* from our issue. That local repro was too consistent, it occurred in over 20 consecutive builds following the repro steps at the bottom of this commit. After updating the Moby dependencies, the image build was no longer skipped and I was unable to reproduce #812. Neither [Docker v23 release notes](https://docs.docker.com/engine/release-notes/23.0/) nor the [git diff from 23.0.1 to 23.0.7](moby/moby@v23.0.1...v23.0.7) contain any obvious changes that explain that. The most likely conclusion here is: 1. There is still an unknown bug, likely a race condition, that can trigger #812, and I was unsuccessful in reproducing it. 2. The Moby dependency update fixed an issue with the session dialing that has the same symptoms as #812, which is fixed in the change to `client/client.go` in the Moby diff above. Repro ===== The Pulumi program in `examples/multi-stage-build-go` was run repeatedly to trigger #812 via: ```console $ uuidgen -r > ./app/example-lockfile.json && pulumi up --skip-preview --diff ``` When the symptoms occurred, verbose logging indicated the image build was skipped entirely - `docker.ImageBuild` did not run. provider/go.mod # modified: provider/go.sum # modified: provider/image.go # examples/multi-stage-build-go/go.sum # modified: examples/multi-stage-build-go/main.go # modified: provider/pkg/docs-gen/examples/image.md # examples/aws-container-registry/ts/pulumi-up.log # examples/multi-stage-build-yaml/ # go.work # go.work.sum #
AaronFriel
force-pushed
the
friel/dep-update
branch
from
4e64f91 to
59941c9
Compare
| @@ -253,7 +254,10 @@ func (p *dockerNativeProvider) dockerBuild(ctx context.Context, | |||
|
|
|||
| //Start a session for BuildKit | |||
| if build.BuilderVersion == defaultBuilder { | |||
| sess, _ := session.NewSession(ctx, "pulumi-docker", "") | |||
| sess, err := session.NewSession(ctx, "pulumi-docker", identity.NewID()) | |||
There was a problem hiding this comment.
What does passing identity.NewID() do? Sorry if didn't parse it from your PR description.
There was a problem hiding this comment.
The most complete permissively licensed client I've found is the Tilt tool, here:
While we aren't using all of the same Buildkit options, I couldn't find any documentation on what the third parameter, SharedKey, does.
But it seems to me that we likely want to avoid any inadvertent sharing between concurrent Docker builds, so I followed the Tilt API's here.
I used this GitHub Code Search to try to find areas where we might deviate from norms: https://github.com/search?q=NewSession+DialHijack+lang%3Ago+-org%3Ahashicorp+-is%3Afork&type=code
There was a problem hiding this comment.
I still don't entirely follow what the NewID() does for us.
The source code says that it generates a random identifier for cases where such are required. Is that so here, for our session?
More specifically - do we need this change for skipped builds to no longer happen?
Either way, it seems relatively low risk to add this.
| @@ -253,7 +254,10 @@ func (p *dockerNativeProvider) dockerBuild(ctx context.Context, | |||
|
|
|||
| //Start a session for BuildKit | |||
| if build.BuilderVersion == defaultBuilder { | |||
| sess, _ := session.NewSession(ctx, "pulumi-docker", "") | |||
| sess, err := session.NewSession(ctx, "pulumi-docker", identity.NewID()) | |||
There was a problem hiding this comment.
I tried to check #450 to understand whether the error was ignored on purpose, but I couldn't. @guineveresaenger do you remember?
There was a problem hiding this comment.
It does not seem like we ignored this error on purpose. Explicitly returning this error looks correct to me. 👍
| @@ -253,7 +254,10 @@ func (p *dockerNativeProvider) dockerBuild(ctx context.Context, | |||
|
|
|||
| //Start a session for BuildKit | |||
| if build.BuilderVersion == defaultBuilder { | |||
| sess, _ := session.NewSession(ctx, "pulumi-docker", "") | |||
| sess, err := session.NewSession(ctx, "pulumi-docker", identity.NewID()) | |||
There was a problem hiding this comment.
I still don't entirely follow what the NewID() does for us.
The source code says that it generates a random identifier for cases where such are required. Is that so here, for our session?
More specifically - do we need this change for skipped builds to no longer happen?
Either way, it seems relatively low risk to add this.
Updates the docs with a note that says local repoDigest is now supported. See [comment](#739 (comment)) I'd like to get this in because due to #828 being merged we are ready to cut a patch release, which will ensure that the update appears in the registry.
Adding additional logging to
sess.Run()call here, and fixing moby/moby#45935, as a prerequisite to debugging #812.While looking into #812, when using a locally built provider, updates to the Image resource would not result in a new image built and a corresponding new
RepoDigestoutput value. This was root caused to the Moby issue below after adding a log statement followingsess.Run()inprovider/image.go.However, the behavior seen while reproducing is different from our issue. That local repro was too consistent, it occurred in over 20 consecutive builds following the repro steps at the bottom of this commit.
After updating the Moby dependencies, the image build was no longer skipped and I was unable to reproduce #812. Neither Docker v23 release notes nor the git diff from 23.0.1 to 23.0.7 contain any obvious changes that explain that.
The most likely conclusion here is:
client/client.goin the Moby diff above.Repro
The Pulumi program in
examples/multi-stage-build-gowas run repeatedly to trigger #812 via:$ uuidgen -r > ./app/example-lockfile.json && pulumi up --skip-preview --diffWhen the symptoms occurred, verbose logging indicated the image build was skipped entirely -
docker.ImageBuilddid not run.