From ebf570d04535d541a347e2fe5f5be1fa86ace8fe Mon Sep 17 00:00:00 2001 From: Avi Deitcher Date: Tue, 18 May 2021 12:51:10 +0300 Subject: [PATCH] refactor buildContext into a simple stdin reader Signed-off-by: Avi Deitcher --- src/cmd/linuxkit/pkglib/build.go | 116 +++++++++++++++----------- src/cmd/linuxkit/pkglib/build_test.go | 7 +- src/cmd/linuxkit/pkglib/docker.go | 99 +++++++++------------- 3 files changed, 106 insertions(+), 116 deletions(-) diff --git a/src/cmd/linuxkit/pkglib/build.go b/src/cmd/linuxkit/pkglib/build.go index 5eb6161aa..636245044 100644 --- a/src/cmd/linuxkit/pkglib/build.go +++ b/src/cmd/linuxkit/pkglib/build.go @@ -383,8 +383,6 @@ func (p Pkg) buildArch(d dockerRunner, c lktspec.CacheProvider, arch string, arg // find the desired builder builderName := getBuilderForPlatform(arch, bo.builders) - d.setBuildCtx(&buildCtx{sources: p.sources}) - // set the target var ( buildxOutput string @@ -419,10 +417,11 @@ func (p Pkg) buildArch(d dockerRunner, c lktspec.CacheProvider, arch string, arg }) args = append(args, fmt.Sprintf("--output=%s", buildxOutput)) + buildCtx := &buildCtx{sources: p.sources} platform := fmt.Sprintf("linux/%s", arch) archArgs := append(args, "--platform") archArgs = append(archArgs, platform) - if err := d.build(tagArch, p.path, builderName, platform, stdout, archArgs...); err != nil { + if err := d.build(tagArch, p.path, builderName, platform, buildCtx.Reader(), stdout, archArgs...); err != nil { stdoutCloser() if strings.Contains(err.Error(), "executor failed running [/dev/.buildkit_qemu_emulator") { return nil, fmt.Errorf("buildkit was unable to emulate %s. check binfmt has been set up and works for this platform: %v", platform, err) @@ -441,71 +440,90 @@ func (p Pkg) buildArch(d dockerRunner, c lktspec.CacheProvider, arch string, arg type buildCtx struct { sources []pkgSource + err error + r io.ReadCloser } -// Copy iterates over the sources, tars up the content after rewriting the paths. +// Reader gets an io.Reader by iterating over the sources, tarring up the content after rewriting the paths. // It assumes that sources is sane, ie is well formed and the first part is an absolute path // and that it exists. NewFromCLI() ensures that. -func (c *buildCtx) Copy(w io.WriteCloser) error { +func (c *buildCtx) Reader() io.ReadCloser { + r, w := io.Pipe() tw := tar.NewWriter(w) - defer func() { - tw.Close() - w.Close() - }() - for _, s := range c.sources { - log.Debugf("Adding to build context: %s -> %s", s.src, s.dst) + go func() { + defer func() { + tw.Close() + w.Close() + }() + for _, s := range c.sources { + log.Debugf("Adding to build context: %s -> %s", s.src, s.dst) - f := func(p string, i os.FileInfo, err error) error { - if err != nil { - return fmt.Errorf("ctx: Walk error on %s: %v", p, err) - } - - var link string - if i.Mode()&os.ModeSymlink != 0 { - var err error - link, err = os.Readlink(p) + f := func(p string, i os.FileInfo, err error) error { if err != nil { - return fmt.Errorf("ctx: Failed to read symlink %s: %v", p, err) + return fmt.Errorf("ctx: Walk error on %s: %v", p, err) } - } - h, err := tar.FileInfoHeader(i, link) - if err != nil { - return fmt.Errorf("ctx: Converting FileInfo for %s: %v", p, err) - } - rel, err := filepath.Rel(s.src, p) - if err != nil { - return err - } - h.Name = filepath.ToSlash(filepath.Join(s.dst, rel)) - if err := tw.WriteHeader(h); err != nil { - return fmt.Errorf("ctx: Writing header for %s: %v", p, err) - } + var link string + if i.Mode()&os.ModeSymlink != 0 { + var err error + link, err = os.Readlink(p) + if err != nil { + return fmt.Errorf("ctx: Failed to read symlink %s: %v", p, err) + } + } - if !i.Mode().IsRegular() { + h, err := tar.FileInfoHeader(i, link) + if err != nil { + return fmt.Errorf("ctx: Converting FileInfo for %s: %v", p, err) + } + rel, err := filepath.Rel(s.src, p) + if err != nil { + return err + } + h.Name = filepath.ToSlash(filepath.Join(s.dst, rel)) + if err := tw.WriteHeader(h); err != nil { + return fmt.Errorf("ctx: Writing header for %s: %v", p, err) + } + + if !i.Mode().IsRegular() { + return nil + } + + f, err := os.Open(p) + if err != nil { + return fmt.Errorf("ctx: Open %s: %v", p, err) + } + defer f.Close() + + _, err = io.Copy(tw, f) + if err != nil { + return fmt.Errorf("ctx: Writing %s: %v", p, err) + } return nil } - f, err := os.Open(p) - if err != nil { - return fmt.Errorf("ctx: Open %s: %v", p, err) + if err := filepath.Walk(s.src, f); err != nil { + c.err = err + return } - defer f.Close() - - _, err = io.Copy(tw, f) - if err != nil { - return fmt.Errorf("ctx: Writing %s: %v", p, err) - } - return nil } + }() + c.r = r + return c +} - if err := filepath.Walk(s.src, f); err != nil { - return err - } +// Read wraps the usual read, but allows us to include an error +func (c *buildCtx) Read(data []byte) (n int, err error) { + if c.err != nil { + return 0, err } + return c.r.Read(data) +} - return nil +// Close wraps the usual close +func (c *buildCtx) Close() error { + return c.r.Close() } // getBuilderForPlatform given an arch, find the context for the desired builder. diff --git a/src/cmd/linuxkit/pkglib/build_test.go b/src/cmd/linuxkit/pkglib/build_test.go index 34356613f..bd35f5129 100644 --- a/src/cmd/linuxkit/pkglib/build_test.go +++ b/src/cmd/linuxkit/pkglib/build_test.go @@ -25,7 +25,6 @@ type dockerMocker struct { enableTag bool enableBuild bool enablePull bool - ctx buildContext fixedReadName string builds []buildLog } @@ -51,7 +50,7 @@ func (d *dockerMocker) tag(ref, tag string) error { d.images[tag] = d.images[ref] return nil } -func (d *dockerMocker) build(tag, pkg, dockerContext, platform string, stdout io.Writer, opts ...string) error { +func (d *dockerMocker) build(tag, pkg, dockerContext, platform string, stdin io.Reader, stdout io.Writer, opts ...string) error { if !d.enableBuild { return errors.New("build disabled") } @@ -86,10 +85,6 @@ func (d *dockerMocker) pull(img string) (bool, error) { } return false, errors.New("failed to pull") } -func (d *dockerMocker) setBuildCtx(ctx buildContext) { - d.ctx = ctx - -} type cacheMocker struct { enablePush bool diff --git a/src/cmd/linuxkit/pkglib/docker.go b/src/cmd/linuxkit/pkglib/docker.go index 73dba4e82..15a0536e4 100644 --- a/src/cmd/linuxkit/pkglib/docker.go +++ b/src/cmd/linuxkit/pkglib/docker.go @@ -19,7 +19,6 @@ import ( versioncompare "github.com/hashicorp/go-version" "github.com/linuxkit/linuxkit/src/cmd/linuxkit/registry" log "github.com/sirupsen/logrus" - "golang.org/x/sync/errgroup" ) const ( @@ -34,18 +33,14 @@ var platforms = []string{ type dockerRunner interface { buildkitCheck() error tag(ref, tag string) error - build(tag, pkg, dockerContext, platform string, stdout io.Writer, opts ...string) error + build(tag, pkg, dockerContext, platform string, stdin io.Reader, stdout io.Writer, opts ...string) error save(tgt string, refs ...string) error load(src io.Reader) error pull(img string) (bool, error) - setBuildCtx(ctx buildContext) } type dockerRunnerImpl struct { cache bool - - // Optional build context to use - ctx buildContext } type buildContext interface { @@ -80,8 +75,11 @@ var proxyEnvVars = []string{ "ALL_PROXY", } -func (dr *dockerRunnerImpl) command(stdout, stderr io.Writer, args ...string) error { +func (dr *dockerRunnerImpl) command(stdin io.Reader, stdout, stderr io.Writer, args ...string) error { cmd := exec.Command("docker", args...) + if stdin == nil { + stdin = os.Stdin + } if stdout == nil { stdout = os.Stdout } @@ -90,56 +88,26 @@ func (dr *dockerRunnerImpl) command(stdout, stderr io.Writer, args ...string) er } cmd.Stdout = stdout cmd.Stderr = stderr + cmd.Stdin = stdin cmd.Env = os.Environ() - var eg errgroup.Group - - // special handling for build-args - if args[0] == "buildx" && args[1] == "build" { - buildArgs := []string{} - for _, proxyVarName := range proxyEnvVars { - if value, ok := os.LookupEnv(proxyVarName); ok { - buildArgs = append(buildArgs, - []string{"--build-arg", fmt.Sprintf("%s=%s", proxyVarName, value)}...) - } - } - // cannot use usual append(append( because it overwrites part of it - newArgs := make([]string, len(cmd.Args)+len(buildArgs)) - copy(newArgs[:2], cmd.Args[:2]) - copy(newArgs[2:], buildArgs) - copy(newArgs[2+len(buildArgs):], cmd.Args[2:]) - cmd.Args = newArgs - - if dr.ctx != nil { - stdin, err := cmd.StdinPipe() - if err != nil { - return err - } - eg.Go(func() error { - defer stdin.Close() - return dr.ctx.Copy(stdin) - }) - - cmd.Args = append(cmd.Args[:len(cmd.Args)-1], "-") - } - } - log.Debugf("Executing: %v", cmd.Args) - if err := cmd.Run(); err != nil { + err := cmd.Run() + if err != nil { if isExecErrNotFound(err) { return fmt.Errorf("linuxkit pkg requires docker to be installed") } return err } - return eg.Wait() + return nil } // versionCheck returns the client version and server version, and compares them both // against the minimum required version. func (dr *dockerRunnerImpl) versionCheck(version string) (string, string, error) { var stdout bytes.Buffer - if err := dr.command(&stdout, nil, "version", "--format", "json"); err != nil { + if err := dr.command(nil, &stdout, nil, "version", "--format", "json"); err != nil { return "", "", err } @@ -200,7 +168,7 @@ func (dr *dockerRunnerImpl) versionCheck(version string) (string, string, error) // of docker in Actions, which makes it difficult to tell if buildkit is supported. // See https://github.community/t/what-really-is-docker-3-0-6/16171 func (dr *dockerRunnerImpl) buildkitCheck() error { - return dr.command(ioutil.Discard, ioutil.Discard, "buildx", "ls") + return dr.command(nil, ioutil.Discard, ioutil.Discard, "buildx", "ls") } // builder ensure that a builder exists. Works as follows. @@ -216,7 +184,7 @@ func (dr *dockerRunnerImpl) builder(dockerContext, platform string) (string, err // if we were given a context, we must find a builder and use it, or create one and use it if dockerContext != "" { // does the context exist? - if err := dr.command(ioutil.Discard, ioutil.Discard, "context", "inspect", dockerContext); err != nil { + if err := dr.command(nil, ioutil.Discard, ioutil.Discard, "context", "inspect", dockerContext); err != nil { return "", fmt.Errorf("provided docker context '%s' not found", dockerContext) } builderName = fmt.Sprintf("%s-%s-%s-builder", buildkitBuilderName, dockerContext, strings.ReplaceAll(platform, "/", "-")) @@ -228,7 +196,7 @@ func (dr *dockerRunnerImpl) builder(dockerContext, platform string) (string, err // no provided dockerContext, so look for one based on platform-specific name dockerContext = fmt.Sprintf("%s-%s", buildkitBuilderName, strings.ReplaceAll(platform, "/", "-")) - if err := dr.command(ioutil.Discard, ioutil.Discard, "context", "inspect", dockerContext); err == nil { + if err := dr.command(nil, ioutil.Discard, ioutil.Discard, "context", "inspect", dockerContext); err == nil { // we found an appropriately named context, so let us try to use it or error out builderName = fmt.Sprintf("%s-builder", dockerContext) if err := dr.builderEnsureContainer(builderName, platform, dockerContext, args...); err == nil { @@ -250,7 +218,7 @@ func (dr *dockerRunnerImpl) builderEnsureContainer(name, platform, dockerContext // if no error, then we have a builder already // inspect it to make sure it is of the right type var b bytes.Buffer - if err := dr.command(&b, ioutil.Discard, "buildx", "inspect", name); err != nil { + if err := dr.command(nil, &b, ioutil.Discard, "buildx", "inspect", name); err != nil { // we did not have the named builder, so create the builder args = append(args, "--name", name) msg := fmt.Sprintf("creating builder '%s'", name) @@ -265,7 +233,7 @@ func (dr *dockerRunnerImpl) builderEnsureContainer(name, platform, dockerContext msg = fmt.Sprintf("%s based on docker context '%s'", msg, dockerContext) } fmt.Println(msg) - return dr.command(ioutil.Discard, ioutil.Discard, args...) + return dr.command(nil, ioutil.Discard, ioutil.Discard, args...) } // if we got here, we found a builder already, so let us check its type var ( @@ -295,7 +263,7 @@ func (dr *dockerRunnerImpl) builderEnsureContainer(name, platform, dockerContext } func (dr *dockerRunnerImpl) pull(img string) (bool, error) { - err := dr.command(nil, nil, "image", "pull", img) + err := dr.command(nil, nil, nil, "image", "pull", img) if err == nil { return true, nil } @@ -308,7 +276,7 @@ func (dr *dockerRunnerImpl) pull(img string) (bool, error) { } func (dr dockerRunnerImpl) push(img string) error { - return dr.command(nil, nil, "image", "push", img) + return dr.command(nil, nil, nil, "image", "push", img) } func (dr *dockerRunnerImpl) pushWithManifest(img, suffix string, pushImage, pushManifest bool) error { @@ -341,10 +309,10 @@ func (dr *dockerRunnerImpl) pushWithManifest(img, suffix string, pushImage, push func (dr *dockerRunnerImpl) tag(ref, tag string) error { fmt.Printf("Tagging %s as %s\n", ref, tag) - return dr.command(nil, nil, "image", "tag", ref, tag) + return dr.command(nil, nil, nil, "image", "tag", ref, tag) } -func (dr *dockerRunnerImpl) build(tag, pkg, dockerContext, platform string, stdout io.Writer, opts ...string) error { +func (dr *dockerRunnerImpl) build(tag, pkg, dockerContext, platform string, stdin io.Reader, stdout io.Writer, opts ...string) error { // ensure we have a builder builderName, err := dr.builder(dockerContext, platform) if err != nil { @@ -352,28 +320,37 @@ func (dr *dockerRunnerImpl) build(tag, pkg, dockerContext, platform string, stdo } args := []string{"buildx", "build"} + + for _, proxyVarName := range proxyEnvVars { + if value, ok := os.LookupEnv(proxyVarName); ok { + args = append(args, + []string{"--build-arg", fmt.Sprintf("%s=%s", proxyVarName, value)}...) + } + } if !dr.cache { args = append(args, "--no-cache") } args = append(args, opts...) args = append(args, fmt.Sprintf("--builder=%s", builderName)) - args = append(args, "-t", tag, pkg) + args = append(args, "-t", tag) + + // should docker read from the build path or stdin? + buildPath := pkg + if stdin != nil { + buildPath = "-" + } + args = append(args, buildPath) + fmt.Printf("building for platform %s using builder %s\n", platform, builderName) - return dr.command(stdout, nil, args...) + return dr.command(stdin, stdout, nil, args...) } func (dr *dockerRunnerImpl) save(tgt string, refs ...string) error { args := append([]string{"image", "save", "-o", tgt}, refs...) - return dr.command(nil, nil, args...) + return dr.command(nil, nil, nil, args...) } func (dr *dockerRunnerImpl) load(src io.Reader) error { args := []string{"image", "load"} - dr.ctx = &readerCtx{ - reader: src, - } - return dr.command(nil, nil, args...) -} -func (dr *dockerRunnerImpl) setBuildCtx(ctx buildContext) { - dr.ctx = ctx + return dr.command(src, nil, nil, args...) }