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

Pass in index annotations from builds on multiple nodes #2546

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

treuherz
Copy link
Contributor

I've tried to put together the smallest possible change that I think should solve #2540, but I'm not sure where or if to add a good test.

Fixes #2540

build/build.go Outdated
@@ -607,7 +608,15 @@ func BuildWithResultHandler(ctx context.Context, nodes []builder.Node, opt map[s
}
}

dt, desc, err := itpull.Combine(ctx, srcs, nil, false)
filteredAnnotations := map[exptypes.AnnotationKey]string{}
Copy link
Member

@tonistiigi tonistiigi Jun 25, 2024

Choose a reason for hiding this comment

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

Could you move this to helper function filterManifestIndexAnnotations() (ignore if #2546 (comment))

build/build.go Outdated
@@ -61,6 +61,7 @@ type Options struct {

Ref string
Allow []entitlements.Entitlement
Annotations map[exptypes.AnnotationKey]string
Copy link
Member

Choose a reason for hiding this comment

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

The annotations should already exist in the Exports array. Can we make a helper function to extract it from there instead of new field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. In Exports they've all had their keys stringified into e.g. annotation-index to fit into the Attrs map, so we'd need to re-parse those keys if we wanted to pull them back out. I agree the duplication doesn't feel great but at least this way it's the same parsing function twice from the same source, rather than parsing once, serialising and parsing agian.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is correct though. The serialized format is the format the builder accepts and this multi-node merge case is a special case where we are doing part of the build ourself. If/when buildkit supported multiple nodes then we would not do the client side and only sent the Exports to buildkit. Maybe some part of the parsing code from BuildKit can be reused in here to reduce code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see what you mean. I'll take care of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully sorted

@treuherz
Copy link
Contributor Author

TIL GitHub doesn't handle force-pushes to PRs the way I expected. Hope this isn't a problem, will use follow-up commits next time 😬

Fixes docker#2540

Signed-off-by: Eli Treuherz <et@arenko.group>
Signed-off-by: Eli Treuherz <et@arenko.group>
@tonistiigi tonistiigi merged commit 59a0ffc into docker:master Jun 28, 2024
103 checks passed
@crazy-max crazy-max added this to the v0.16.0 milestone Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index annotations aren't pushed after multinode builds
3 participants