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.

Sign up for GitHub

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

Jump to bottom

[v0.10][Moby LTS] Backport 4600 4602 4603 4604 #4642

Closed
wants to merge 10 commits into from
Closed

[v0.10][Moby LTS] Backport 4600 4602 4603 4604 #4642

wants to merge 10 commits into from

Conversation

aepifanov
Copy link

No description provided.

@aepifanov aepifanov force-pushed the v10_backport-4600-4601-4602-4603-4604 branch from 1448d6c to 254b150 Compare February 14, 2024 10:43
@aepifanov aepifanov changed the title V10 backport 4600 4601 4602 4603 4604 [v0.10][Moby LTS] Backport 4600 4601 4602 4603 4604 Feb 15, 2024
corhere
corhere suggested changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

Please include the commits you cherry-picked from in the commit messages (git cherry-pick -x)

@corhere
Copy link
Contributor

corhere commented Feb 15, 2024

22c6007 looks like a bad cherry-pick. It adds tests but does not change any code, and the tests are asserting for error strings like "invalid empty platforms index for exporter" which have no other hits when grepped.

corhere
corhere suggested changes Feb 15, 2024
View reviewed changes
frontend/gateway/forwarder/forward.go
Comment on lines -28 to +30
func llbBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLLBBridge, opts map[string]string, inputs map[string]*opspb.Definition, w worker.Infos, sid string, sm *session.Manager) (*bridgeClient, error) {
bc := &bridgeClient{
func LLBBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLLBBridge, exec executor.Executor, opts map[string]string, inputs map[string]*opspb.Definition, w worker.Infos, sid string, sm *session.Manager) (*BridgeClient, error) {
bc := &BridgeClient{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were func llbBridgeToGatewayClient and type bridgeClient renamed? I don't see why they have to be exported. ("They were renamed in v0.12" is not a valid reason.)

Copy link
Author

Choose a reason for hiding this comment

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

bridgeClient is made exported since exported func LLBBridgeToGatewayClient should have exported-return
> [stage-0 5/6] RUN --mount=target=/go/src/github.com/moby/buildkit --mount=target=/root/.cache,type=cache GOARCH=amd64 golangci-lint run && GOARCH=arm64 golangci-lint run: 2.809 frontend/gateway/forwarder/forward.go:29:225: unexported-return: exported func LLBBridgeToGatewayClient returns unexported type *forwarder.bridgeClient, which can be annoying to use (revive) 2.809 func LLBBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLLBBridge, exec executor.Executor, opts map[string]string, inputs map[string]*opspb.Definition, w worker.Infos, sid string, sm *session.Manager) (*bridgeClient, error) {

Copy link
Contributor

@corhere corhere Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Don't change func llbBridgeToGatewayClient to be exported and you won't have to export the type! Why are you changing a function which is not referenced outside of the defining package to be exported?

frontend/gateway/gateway.go Outdated
lbf, ctx, err := serveLLBBridgeForwarder(ctx, llbBridge, gf.workers, inputs, sid, sm)
defer lbf.conn.Close() //nolint
lbf, ctx, err := serveLLBBridgeForwarder(ctx, llbBridge, exec, gf.workers, inputs, sid, sm)
defer lbf.conn.Close() // nolint
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful, adding a space between // and the comment text semantically changes it from a (linter) directive to a comment for humans. Your editor might have done it automatically because it doesn't match the directive regex defined in versions of Go newer than the Buildkit v0.10 branch.

solver/llbsolver/solver.go Outdated
Comment on lines 10 to 11
"golang.org/x/sync/errgroup"

Copy link
Contributor

Choose a reason for hiding this comment

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

Get your editor under control! Rearranging imports in a cherry-pick needlessly increases the size of the diff, making it harder to review.

client/build_test.go Outdated
Comment on lines 19 to 20
"golang.org/x/crypto/ssh/agent"

Copy link
Contributor

Choose a reason for hiding this comment

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

And another one

executor/oci/spec.go Outdated
Comment on lines 21 to 20
specs "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-spec/specs-go"
Copy link
Contributor

Choose a reason for hiding this comment

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

And another one

aepifanov and others added 10 commits February 16, 2024 01:37
@aepifanov
Move container.go to a separate module
7a3af2a 
Signed-off-by: Andrey Epifanov <[email protected]>
@tonistiigi @aepifanov
gateway: pass executor with build and not access worker directly
a9b0450 
Running interactive container APIs was done by giving
the gateway implementation access to worker controller
directly, but it should be passed with a build job instead.

Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit 0971dffaab93d91e51af984b44c745b35b3c5b4d)
(cherry picked from commit 564f884e7bb6db9c63e03c3b081ea71e15aa7980)
(cherry picked from commit 5026d95)
Signed-off-by: Andrey Epifanov <[email protected]>

`bridgeClient` is made exported since exported func LLBBridgeToGatewayClient should have exported-return

Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	executor/executor.go
#	frontend/gateway/container/container.go
#	frontend/gateway/forwarder/forward.go
#	frontend/gateway/forwarder/frontend.go
#	frontend/gateway/gateway.go
#	solver/llbsolver/bridge.go
#	solver/llbsolver/provenance.go
#	solver/llbsolver/solver.go
#	worker/workercontroller.go
@tonistiigi @aepifanov
llbsolver: make sure interactive container API validates entitlements
8487af1 
Ensure interactive calls validate same conditions that
the build requests do. Refactor of the build side is to ensure
we use the same validation function for both cases. There
was no validation issue with the LLB validation.

Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit d1970522d7145be5f4a1f1a028b1910bb527126c)
(cherry picked from commit e1e30278d0a491dfd34bd80fa66b54106614cffa)
(cherry picked from commit 92cc595)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	client/build_test.go
#	solver/llbsolver/bridge.go
@AkihiroSuda @aepifanov
MountStubsCleaner: preserve timestamps
f58634f 
Fix issue 3148

Signed-off-by: Akihiro Suda <[email protected]>
(cherry picked from commit 0b5a315)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	client/client_test.go
@jedevc @aepifanov
executor: stubs cleaner should remove empty directory mounts
5c3e1eb 
On Linux, an empty directory is usually 4096 bytes, not 0, so we need an
additional explicit check here.

Signed-off-by: Justin Chadwell <[email protected]>
(cherry picked from commit 6778973)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	client/client_test.go
@jedevc @aepifanov
chore: tidy atime_unix.go to use errors pkg
067baa8 
Signed-off-by: Justin Chadwell <[email protected]>
(cherry picked from commit 32b5e4d)
@tonistiigi @aepifanov
executor: recheck mount stub path within root after container run
51f9450 
Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit 96ccaec09c51176a6d954fd7c4ce57d519bae1b2)
(cherry picked from commit a9523c6476f39bb44dd02bcab19e8cb25c5bc37b)
(cherry picked from commit 00fe637)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	executor/stubs.go
@tonistiigi @aepifanov
oci: fix error handling on submount calls
eb5b326 
Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit 42d866e)
(cherry picked from commit e81066f8a8623dc876f3d64fae8f693c17ecdc1a)
(cherry picked from commit d089e0b)
@tonistiigi @aepifanov
exec: add extra validation for submount sources
b379ed2 
While submount paths were already validated there are some
cases where the parent mount may not be immutable while the
submount is created.

Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit 2529ec4121bcd8c35bcd96218083da175c2e5b77)
(cherry picked from commit cbc233b3b695918d92fd5b1407b829296c53db70)
(cherry picked from commit f781267)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	executor/oci/spec.go
#	executor/oci/spec_windows.go
#	snapshot/localmounter_unix.go
@tonistiigi @aepifanov
update runc to v1.1.12
7f34166 
Signed-off-by: Tonis Tiigi <[email protected]>
(cherry picked from commit bac3f2b)
Signed-off-by: Andrey Epifanov <[email protected]>

# Conflicts:
#	Dockerfile
@aepifanov aepifanov force-pushed the v10_backport-4600-4601-4602-4603-4604 branch from a71ff7f to 7f34166 Compare February 16, 2024 12:58
@aepifanov aepifanov changed the title [v0.10][Moby LTS] Backport 4600 4601 4602 4603 4604 [v0.10][Moby LTS] Backport 4600 4602 4603 4604 Feb 16, 2024
@aepifanov
Copy link
Author

aepifanov commented Feb 16, 2024 •
edited
Loading

22c6007 looks like a bad cherry-pick. It adds tests but does not change any code, and the tests are asserting for error strings like "invalid empty platforms index for exporter" which have no other hits when grepped.

This PR has been removed, since it needs to be more carefully investigated

corhere
corhere reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

Why 7a3af2a? I'm sure you have a very good reason for refactoring that file in this PR. But I can't read your mind. Please document your rationale in the commit message.

@aepifanov aepifanov closed this Feb 26, 2024
@corhere corhere mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corhere corhere corhere requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aepifanov @corhere @tonistiigi @AkihiroSuda @jedevc