Skip to content

fix: should ignore nil client #686

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

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Conversation

morlay
Copy link
Collaborator

@morlay morlay commented Jul 21, 2021

maybe fix #675

I have another issue

With multi-node builder, but single platform build buildx build --platform=linux/amd64, will cause the nil issue too

@morlay morlay requested a review from tonistiigi July 23, 2021 14:57
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM, ptal @AkihiroSuda

@crazy-max crazy-max merged commit 7d312ea into docker:master Jul 23, 2021
@@ -197,6 +197,10 @@ func resolveDrivers(ctx context.Context, drivers []DriverInfo, auth Auth, opt ma

eg, ctx := errgroup.WithContext(ctx)
for i, c := range clients {
if c == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Fix looks ok to address the issue, but perhaps we should also look into why this slice contains nil values in the first place, @crazy-max ?

Copy link
Member

@crazy-max crazy-max Jul 23, 2021

Choose a reason for hiding this comment

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

Multi-node build support and the underlying clients iteration was first introduced while resolving drivers (#37). Client nil check was already there

buildx/build/build.go

Lines 283 to 285 in 7d312ea

if c == nil {
continue
}

but was left out to be included in #535.

but perhaps we should also look into why this slice contains nil values in the first place,

Yes agree, might be an issue with the context dialer while creating the buildkit client for docker-container driver. Will take a look.

Copy link
Member

Choose a reason for hiding this comment

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

Was initially thinking, perhaps the slice is initialized with a length/size (not taking into account that filtering might happen). Haven't looked at the code though (was reading from my phone), but though it wouldn't hurt to leave a comment ☺️🤗

Copy link
Member

Choose a reason for hiding this comment

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

Yes you did well :)

@tonistiigi
Copy link
Member

This does not look correct and iiuc can return cleanly on error cases as skip over the builds. We should first figure out why this happens. Probably the correct solution is to return an error. If skipping over a node is determined to be better it should be in resolveDriversBase and pick a replacement node instead of skipping over a (part of) build.

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.

failed to run any build if using a builder instance with multiple native nodes
5 participants