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

enable multi export #1575

Closed
wants to merge 248 commits into from
Closed

Conversation

fahedouch
Copy link
Contributor

@fahedouch fahedouch commented Jul 16, 2020

building #1555

@tonistiigi
Copy link
Member

tonistiigi commented Jul 16, 2020

the proto files are generated, you should not change them manually. For backward compatibility new indexes are also needed. Switching the type from single to repeated may not need a new index iirc (@hinshun not sure if this was the case for all types).

update: make generated-files is what you need to run for proto changes.

@hinshun
Copy link
Collaborator

hinshun commented Jul 16, 2020

Switching the type from single to repeated may not need a new index iirc (@hinshun not sure if this was the case for all types).

Not true for all types, for example repeated int32 is not compatible with int32.

@fahedouch
Copy link
Contributor Author

fahedouch commented Aug 2, 2020

@tonistiigi @hinshun ok I will pay attention to backward compatibility ( but I don't think we have backward compatibility problem with string and ExporterAttrs types).

For the moment I need your help :
I don't know how to enables writing into multiple directories to be reachable through the grpc session here

I got this message Server.RegisterService found duplicate service registration for "moby.filesync.v1.FileSend" when

@tonistiigi
Copy link
Member

There is no need to register filesync target server twice. I think for multi-export we are mainly talking about different types of exporters. Even if there is some use case for doing 2 local exports (that I think is just wasteful) there is one handler and splitting between targets is done by the call parameters.

@fahedouch fahedouch changed the title [WIP] enable multi export enable multi export Aug 8, 2020
@fahedouch fahedouch marked this pull request as ready for review August 8, 2020 19:44
@fahedouch
Copy link
Contributor Author

@tonistiigi
Thank you for your reply. I think I am done with this PR. Do you see any additional changes? Should I do activate the multi-export in buildx project now ?

@fahedouch fahedouch force-pushed the enable-multi-export branch 2 times, most recently from 08c51cd to 269bad0 Compare August 8, 2020 20:07
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.

Many failures in the CI, please take a look at the logs.

We will also need integration tests for this feature. PTAL https://github.com/moby/buildkit/blob/master/client/client_test.go and https://github.com/moby/buildkit/blob/master/.github/CONTRIBUTING.md#run-the-unit--and-integration-tests for running the tests.

string Exporter = 3;
map<string, string> ExporterAttrs = 4;
repeated string Exporters = 3;
repeated ExporterAttrs ExportersAttrs = 4;
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this is not backward compatible. @hinshun . At least it can't be compatible if we ever wish to extend ExporterAttrs in the future.

if ex.OutputDir == "" {
return nil, errors.New("output directory is required for local exporter")
}
s.Allow(filesync.NewFSSyncTargetDir(ex.OutputDir))
Copy link
Member

Choose a reason for hiding this comment

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

Some things like output directory do not seem to support multiple afaics. That is fine but please add validation for not allowing 2 local exporters etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Would you recheck please :) ?

if req.Exporter != "" {
exp, err := w.Exporter(req.Exporter, c.opt.SessionManager)
if req.Exporters[0] != "" {
exp, err := w.Exporter(req.Exporters[0], c.opt.SessionManager)
Copy link
Member

Choose a reason for hiding this comment

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

Afaics this exports to first item of the array, not to all items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. For the moment I commented some parts in code, I will reactive them.

@AkihiroSuda
Copy link
Member

needs rebase

@fahedouch
Copy link
Contributor Author

needs rebase

@AkihiroSuda after rebasing it is hard to fix DCO job by signing previous commits. Can you help plz!

@AkihiroSuda
Copy link
Member

git rebase -i HEAD~N (N is a number)

fahedouch and others added 16 commits November 9, 2020 16:26
more steps on enabling multi-export

fix variable name

fix variable name && lint code

Signed-off-by: fahedouch <fahed.dorgaa@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Avoid hidden session passing and allow one session to drop when
multiple builds share a vertex.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Cory Bennett <cbennett@netflix.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
update run/exec tests for stdin and expected failures
move common tests for runc and container to shared tests package

Signed-off-by: Cory Bennett <cbennett@netflix.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Cory Bennett <cbennett@netflix.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Cory Bennett <cbennett@netflix.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
This is the shim used by the containerd Runtime V2 on Linux, per the
default setting of `io.containerd.runc.v2`.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
fate-grand-order and others added 25 commits November 9, 2020 16:34
…matting

text does not appear to contain a placeholder

Signed-off-by: Helen Xie <chenjg@harmonycloud.cn>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
According to documentation (https://docs.docker.com/engine/reference/builder/#dockerignore-file), absolute paths like `/foo/bar` should have the same effect as `foo/bar`. This is not the case today.

This fix normalize paths when reading the .dockerignore file by removing
leading slashes.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: wingkwong <wingkwong.code@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
…0ab3ab

full diff: microsoft/go-winio@fc70bd9...5b44b70

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
full diff: golang/sys@2334cc1...aee5d88

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
…951081b35f

full diff: moby/libnetwork@d8334cc...d095108

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
…adc38

full diff: docker/cli@2298e6a...1d20b15

relevant changes:
- config: don't call homedir on init()
- config: fix error message using incorrect filename
- config: remove redundant os.Stat()
- config: Handle errors on close in config file write.
- config: ignore empty config file instead of printing warning
- config: Fix ConfigFile.Save() replacing symlink with file
- Config-file: remove User-Agent from config.json when saving

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
…352355d4

full diff: moby/moby@4634ce6...c2cc352

also adds github.com/cilium/ebpf as a dependency, which I set to the same
version as is set in containerd/cgroups version

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Cory Bennett <cbennett@netflix.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Vlad A. Ionescu <vladaionescu@users.noreply.github.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Vlad A. Ionescu <vladaionescu@users.noreply.github.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Allows readonly passed cleanly.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Same defaults already used on buildkitd side

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Currently moby/buildkit image misses fuse dependency (fusermount) so currently
stargz support doesn't work on that image. This commit fixes this issue.

Signed-off-by: ktock <ktokunaga.mail@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: fahedouch <fahed.dorgaa@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
Signed-off-by: fahedouch <fahed.dorgaa@gmail.com>
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
@fahedouch
Copy link
Contributor Author

I close this PR for #1788

@fahedouch fahedouch closed this Nov 10, 2020
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.

None yet