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

vendor: github.com/moby/buildkit v0.13.0-rc2 #47364

Merged
merged 14 commits into from Feb 27, 2024
Merged

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Feb 9, 2024

- What I did
Updated buildkit to v0.13.0-rc2

- How I did it
See individual commits.

- How to verify it

- Description for the changelog

- Update buildkit to v0.13.0-rc2

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added area/builder status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/builder/buildkit Issues affecting buildkit containerd-integration Issues and PRs related to containerd integration labels Feb 9, 2024
@vvoland vvoland added this to the 26.0.0 milestone Feb 9, 2024
@vvoland vvoland self-assigned this Feb 9, 2024
@vvoland

This comment was marked as off-topic.

@vvoland

This comment was marked as resolved.

@vvoland

This comment was marked as resolved.

@thaJeztah
Copy link
Member

@stewartsmith @PeterCai7 are you able to help with the cloudwatch logger?

@vvoland perhaps we can move that update to a separate PR in preparation?

@vvoland

This comment was marked as off-topic.

@thaJeztah

This comment was marked as off-topic.

@vvoland

This comment was marked as off-topic.

vendor.mod Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member

Thanks for looking at it @vvoland, I will review your comments shortly

@vvoland
Copy link
Contributor Author

vvoland commented Feb 12, 2024

Extracted the AWS bump to: #47373

@vvoland
Copy link
Contributor Author

vvoland commented Feb 14, 2024

Looks like increasing the integration tests timeout from 5m to 10m solves the TestWaitConditions failure.
Wondering what's caused the tests to execute longer though 🤔

Comparing (this PR):
https://github.com/moby/moby/actions/runs/7889967599#summary-21563428016
vs (latest master)
https://github.com/moby/moby/actions/runs/7886719263#summary-21521123926

looks like indeed the tests take more time to complete, for example:
TestServicePlugin from ~28s to ~67s
TestLiveRestore/volume_references from ~17s to ~88s

these don't use build related things (and integration tests will use the legacy builder anyway), so wondering if it's the OTEL stuff that's making things slower?

EDIT: Solved with d32b3cf (OTEL was trying to export spans over a wrong protocol and was slowing down all scopes that were being traced).

@vvoland
Copy link
Contributor Author

vvoland commented Feb 15, 2024

This also fails consistently:

=== Failed
=== FAIL: amd64.integration.daemon TestDaemonProxy/environment_variables (11.81s)
    daemon_test.go:229: assertion failed:  (received string) != example.org:5000 (string): should not have used proxy
    --- FAIL: TestDaemonProxy/environment_variables (11.81s)

=== FAIL: amd64.integration.daemon TestDaemonProxy/configuration_file (11.81s)
    daemon_test.go:330: assertion failed:  (received string) != example.org:5002 (string): should not have used proxy
    --- FAIL: TestDaemonProxy/configuration_file (11.81s)

=== FAIL: amd64.integration.daemon TestDaemonProxy/command-line_options (11.82s)
    daemon_test.go:280: assertion failed:  (received string) != example.org:5001 (string): should not have used proxy
    --- FAIL: TestDaemonProxy/command-line_options (11.82s)

=== FAIL: amd64.integration.daemon TestDaemonProxy (0.50s)

@vvoland
Copy link
Contributor Author

vvoland commented Feb 23, 2024

Rebased after #47245 merge

@vvoland
Copy link
Contributor Author

vvoland commented Feb 26, 2024

Updated to rc2

@vvoland vvoland changed the title vendor: github.com/moby/buildkit v0.13.0-rc1 vendor: github.com/moby/buildkit v0.13.0-rc2 Feb 26, 2024
if err != nil {
return struct{}{}, err
}

p.ref = newRef
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch!

Yeah it looks like this shouldn't be deleted.
I restored it as:
p.ref = ref.String()

because ResolveImageConfig no longer returns a reference.
Looking at the old implementation, it didn't really change the ref that was passed as an argument.
The only place where it returned a different ref is where it went to the (remotes.Resolver) Resolve(), but it doesn't seem to change it either (at least the docker remote).

crazy-max and others added 14 commits February 27, 2024 11:25
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Introduced in: moby/buildkit@3713178

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
StaticDirSource definition changed and can no longer be initialized from
the composite literal.

https://github.com/moby/buildkit/commits/a80b48544c029f0e28e1243407b55c916c7757f2

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
moby/buildkit@1c1777b
added an explicit id argument to the Resolve method.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
moby/buildkit@e358792
changed that field to a function and added an `OverrideResource`
function that allows to override it.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
moby/buildkit@8bfd280
added a new argument that allows to specify different runtime.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Adjust to cache sources changes from:
moby/buildkit@6b27487

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
moby/buildkit@30c069c
removed the `ResolveImageConfig` method in favor of more generic
`ResolveSourceMetadata` that can also support other things than images.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Temporarily store the produced config descriptor for the buildkit
history to work.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Buildkit added support for exporting metrics in:
moby/buildkit@7de2e4f

Explicitly set the protocol for exporting metrics like we do for the
traces. We need that because Buildkit defaults to grpc.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

discussed on Slack; looks like the failures are flaky tests, so no need to re-run CI again for now.

@thaJeztah thaJeztah merged commit 2208351 into moby:master Feb 27, 2024
134 of 138 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/builder/buildkit Issues affecting buildkit area/builder area/dependencies containerd-integration Issues and PRs related to containerd integration impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants