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

[v0.12] Cherry-picks for v0.12.3 #4304

Merged
merged 11 commits into from
Oct 13, 2023
Merged

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Oct 3, 2023

tonistiigi and others added 8 commits October 3, 2023 16:37
If build contains multiple subbuilds all of their sources
are tracked in provenance attestations. When some subbuilds
are coming from same source file (eg. same Dockerfile but
different targets) currently the same file would appear
in multiple times. This detects such duplicates and makes
sure definitions from multiple subbuilds can map to same file.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 1bbf73e)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 641c552)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Chris Goller <goller@gmail.com>
(cherry picked from commit e0ccc47)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Chris Goller <goller@gmail.com>
(cherry picked from commit 4d4fc4d)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Chris Goller <goller@gmail.com>
(cherry picked from commit 40741df)
Signed-off-by: Justin Chadwell <me@jedevc.com>
This reverts commit 1aef766.

Signed-off-by: Justin Chadwell <me@jedevc.com>
(cherry picked from commit d51edce)
Signed-off-by: Justin Chadwell <me@jedevc.com>
When calling client.Wait, we want to avoid the default backoff behavior,
because we want to achieve a quick response back once the server becomes
active.

To do this, without modifying the entire client's exponential backoff
configuration, we can use conn.ResetConnectBackoff, while attempting to
reconnect every second.

Here are some common scenarios:
- Server is listening: the call to Info succeeds quickly, and we return.
- Server is listening, but is behind several proxies and so latency is
  high: the call to Info succeeds slowly (up to minConnectTimeout=20s),
  and we return.
- Server is not listening and gets "connection refused": the
  call to Info fails quickly, and we wait a second before retrying.
- Server is not listening and does not respond (e.g. firewall dropping
  packets): the call to Info fails slowly (by default after
  minConnectTimeout=20s). After the call fails, we wait a second before
  retrying.

Signed-off-by: Justin Chadwell <me@jedevc.com>
(cherry picked from commit f1d7f2e)
Signed-off-by: Justin Chadwell <me@jedevc.com>
ResolveImageConfig can be called concurrently - for example, by
dockerfile2llb during conversion, we loop through each stage and resolve
the base image for that stage.

In the case that two calls to ResolveImageConfig finish at roughly the
same time, we can hit an edge case where we attempt to modify the
bridge's image records at the same time.

To fix this, we just need to use the bridge's mutex to prevent
concurrent access here.

This should fix the following stack trace found in CI:

    sandbox.go:144: goroutine 1079 [running]:
    sandbox.go:144: github.com/moby/buildkit/solver/llbsolver.(*provenanceBridge).ResolveImageConfig(0xc000431e00, {0x1c2b040?, 0xc0008e5b30?}, {0xc00094ba00?, 0xc0003728f0?}, {0x0, 0xc0006cb580, {0x19ba868, 0x7}, {0xc0008f7500, ...}, ...})
    sandbox.go:144: 	/src/solver/llbsolver/provenance.go:139 +0x1fb
    sandbox.go:144: github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.toDispatchState.func3.1()
    sandbox.go:144: 	/src/frontend/dockerfile/dockerfile2llb/convert.go:405 +0x5fe
    sandbox.go:144: golang.org/x/sync/errgroup.(*Group).Go.func1()
    sandbox.go:144: 	/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75 +0x64
    sandbox.go:144: created by golang.org/x/sync/errgroup.(*Group).Go
    sandbox.go:144: 	/src/vendor/golang.org/x/sync/errgroup/errgroup.go:72 +0xa5
    --- FAIL: TestIntegration/TestNoCache/worker=oci-rootless/frontend=builtin (4.45s)

No other explanation for this failure makes sense - `b` cannot be `nil`
at this point, since a call to `b.llbBridge.ResolveImageConfig` has just
succeeded (also because that would be very strange).

Signed-off-by: Justin Chadwell <me@jedevc.com>
(cherry picked from commit c08f767)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

cc @goller

@thaJeztah
Copy link
Member

@tonistiigi should we include #4311 as well (once merged)?

@jedevc
Copy link
Member Author

jedevc commented Oct 7, 2023

@thaJeztah I'll hold this open for now, in case we want to include #4311 - I'd be happy with the backport, it looks fairly safe to me, but I might not have all the context for it.

@AkihiroSuda
Copy link
Member

We need this too

@jedevc
Copy link
Member Author

jedevc commented Oct 9, 2023

Moving into draft until those others merge :)

@jedevc jedevc marked this pull request as draft October 9, 2023 08:43
mook-as and others added 3 commits October 13, 2023 11:31
Fixes moby#4108

Signed-off-by: Mark Yen <mark.yen@suse.com>
(cherry picked from commit d48bf06)
Signed-off-by: Justin Chadwell <me@jedevc.com>
This responds to review feedback from
moby#4308 (review)

Signed-off-by: Mark Yen <mark.yen@suse.com>
(cherry picked from commit f9ccb09)
Signed-off-by: Justin Chadwell <me@jedevc.com>
Before this change, all platforms that loosely match the provided
platform will be fetched even though we only care about 1 of them.
As an example when linux/amd64 is requested it will also fetch linux/386
because it is a compatible architecture.
This means extra round trips to the registry, potentially even for
content that doesn't exist in the remote.

This is especially a problem when resolve mode is prefer-local because
we'll have the index locally but most likely only one manifest.
In this case we'll end up reaching out to the registry to fetch the
other manifests unncessarily.

With this change instead of fetching all matching platforms it chooses
only the best matching platform.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit 575cb10)
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc marked this pull request as ready for review October 13, 2023 17:32
@jedevc
Copy link
Member Author

jedevc commented Oct 13, 2023

So. This should be all the patches we need - but as discussed with @tonistiigi, the riscv64 failures as in #4316 might make doing a release tricky 😢

So will still hold this for a bit.

@crazy-max
Copy link
Member

So. This should be all the patches we need - but as discussed with @tonistiigi, the riscv64 failures as in #4316 might make doing a release tricky 😢

So will still hold this for a bit.

Opened #4332 as draft to check if we can mitigate this issue while we are investigating on a proper fix.

@thaJeztah
Copy link
Member

Can we merge this without tagging / building a release? That way we can vendor in moby/moby and verify the patches (we're ok for now with an untagged version from the release-branch)

@jedevc jedevc merged commit 6560bb9 into moby:v0.12 Oct 13, 2023
51 of 54 checks passed
@jedevc jedevc deleted the v0.12.3-cherry-picks branch October 16, 2023 09:49
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.

8 participants