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

build: remove --stream (was experimental) #2105

Merged
merged 1 commit into from
Oct 1, 2019
Merged

build: remove --stream (was experimental) #2105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 5 additions & 60 deletions cli/command/image/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"fmt"
"io"
"io/ioutil"
"net"
"os"
"path/filepath"
"regexp"
Expand All @@ -33,7 +32,6 @@ import (
"github.com/docker/docker/pkg/urlutil"
units "github.com/docker/go-units"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -165,9 +163,7 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
flags.SetAnnotation("squash", "version", []string{"1.25"})

flags.BoolVar(&options.stream, "stream", false, "Stream attaches to server to negotiate build context")
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a hidden, deprecated dummy flag so that we can provide a useful error message if someone still uses this?

Similar to https://github.com/moby/moby/pull/24721/files

We can remove if after 1 or 2 releases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was experimental, so the best thing I can do is keep the flag to tell them it's been removed and that they should use buildkit, but I don't want to keep the feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we error out with human-friendly message when the dummy flag was specified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that's what I suggested in the comment above. I'll update the PR

flags.SetAnnotation("stream", "experimental", nil)
flags.SetAnnotation("stream", "version", []string{"1.31"})
flags.SetAnnotation("stream", "no-buildkit", nil)
flags.MarkHidden("stream")

flags.StringVar(&options.progress, "progress", "auto", "Set type of progress output (auto, plain, tty). Use plain to show container output")
flags.SetAnnotation("progress", "buildkit", nil)
Expand Down Expand Up @@ -224,8 +220,8 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
remote string
)

if options.compress && options.stream {
return errors.New("--compress conflicts with --stream options")
if options.stream {
return errors.New("Experimental flag --stream was removed, enable BuildKit instead with DOCKER_BUILDKIT=1")
}

if options.dockerfileFromStdin() {
Expand Down Expand Up @@ -284,7 +280,7 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
}

// read from a directory into tar archive
if buildCtx == nil && !options.stream {
if buildCtx == nil {
excludes, err := build.ReadDockerignore(contextDir)
if err != nil {
return err
Expand Down Expand Up @@ -315,16 +311,6 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
}
}

// if streaming and Dockerfile was not from stdin then read from file
// to the same reader that is usually stdin
if options.stream && dockerfileCtx == nil {
dockerfileCtx, err = os.Open(relDockerfile)
if err != nil {
return errors.Wrapf(err, "failed to open %s", relDockerfile)
}
defer dockerfileCtx.Close()
}

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

Expand Down Expand Up @@ -367,38 +353,11 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
buildCtx = dockerfileCtx
}

s, err := trySession(dockerCli, contextDir, true)
if err != nil {
return err
}

var body io.Reader
if buildCtx != nil && !options.stream {
if buildCtx != nil {
body = progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon")
}

// add context stream to the session
if options.stream && s != nil {
syncDone := make(chan error) // used to signal first progress reporting completed.
// progress would also send errors but don't need it here as errors
// are handled by session.Run() and ImageBuild()
if err := addDirToSession(s, contextDir, progressOutput, syncDone); err != nil {
return err
}

buf := newBufferedWriter(syncDone, buildBuff)
defer func() {
select {
case <-buf.flushed:
case <-ctx.Done():
}
}()
buildBuff = buf

remote = clientSessionRemote
body = buildCtx
}

configFile := dockerCli.ConfigFile()
creds, _ := configFile.GetAllCredentials()
authConfigs := make(map[string]types.AuthConfig, len(creds))
Expand All @@ -411,20 +370,6 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
buildOptions.AuthConfigs = authConfigs
buildOptions.RemoteContext = remote

if s != nil {
go func() {
logrus.Debugf("running session: %v", s.ID())
dialSession := func(ctx context.Context, proto string, meta map[string][]string) (net.Conn, error) {
return dockerCli.Client().DialHijack(ctx, "/session", proto, meta)
}
if err := s.Run(ctx, dialSession); err != nil {
logrus.Error(err)
cancel() // cancel progress context
}
}()
buildOptions.SessionID = s.ID()
}

response, err := dockerCli.Client().ImageBuild(ctx, body, buildOptions)
if err != nil {
if options.quiet {
Expand Down
89 changes: 0 additions & 89 deletions cli/command/image/build_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,20 @@
package image

import (
"bytes"
"context"
"crypto/rand"
"crypto/sha256"
"encoding/hex"
"fmt"
"io"
"io/ioutil"
"os"
"path/filepath"
"sync"
"time"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/image/build"
cliconfig "github.com/docker/cli/cli/config"
"github.com/docker/docker/api/types/versions"
"github.com/docker/docker/pkg/progress"
"github.com/moby/buildkit/session"
"github.com/moby/buildkit/session/filesync"
"github.com/pkg/errors"
"golang.org/x/time/rate"
)

const clientSessionRemote = "client-session"
Expand All @@ -49,87 +41,6 @@ func trySession(dockerCli command.Cli, contextDir string, forStream bool) (*sess
return s, nil
}

func addDirToSession(session *session.Session, contextDir string, progressOutput progress.Output, done chan error) error {
excludes, err := build.ReadDockerignore(contextDir)
if err != nil {
return err
}

p := &sizeProgress{out: progressOutput, action: "Streaming build context to Docker daemon"}

workdirProvider := filesync.NewFSSyncProvider([]filesync.SyncedDir{
{Dir: contextDir, Excludes: excludes},
})
session.Allow(workdirProvider)

// this will be replaced on parallel build jobs. keep the current
// progressbar for now
if snpc, ok := workdirProvider.(interface {
SetNextProgressCallback(func(int, bool), chan error)
}); ok {
snpc.SetNextProgressCallback(p.update, done)
}

return nil
}

type sizeProgress struct {
out progress.Output
action string
limiter *rate.Limiter
}

func (sp *sizeProgress) update(size int, last bool) {
if sp.limiter == nil {
sp.limiter = rate.NewLimiter(rate.Every(100*time.Millisecond), 1)
}
if last || sp.limiter.Allow() {
sp.out.WriteProgress(progress.Progress{Action: sp.action, Current: int64(size), LastUpdate: last})
}
}

type bufferedWriter struct {
done chan error
io.Writer
buf *bytes.Buffer
flushed chan struct{}
mu sync.Mutex
}

func newBufferedWriter(done chan error, w io.Writer) *bufferedWriter {
bw := &bufferedWriter{done: done, Writer: w, buf: new(bytes.Buffer), flushed: make(chan struct{})}
go func() {
<-done
bw.flushBuffer()
}()
return bw
}

func (bw *bufferedWriter) Write(dt []byte) (int, error) {
select {
case <-bw.done:
bw.flushBuffer()
return bw.Writer.Write(dt)
default:
return bw.buf.Write(dt)
}
}

func (bw *bufferedWriter) flushBuffer() {
bw.mu.Lock()
select {
case <-bw.flushed:
default:
bw.Writer.Write(bw.buf.Bytes())
close(bw.flushed)
}
bw.mu.Unlock()
}

func (bw *bufferedWriter) String() string {
return fmt.Sprintf("%s", bw.Writer)
}

func getBuildSharedKey(dir string) (string, error) {
// build session is hash of build dir with node based randomness
s := sha256.Sum256([]byte(fmt.Sprintf("%s:%s", tryNodeIdentifier(), dir)))
Expand Down
5 changes: 0 additions & 5 deletions contrib/completion/bash/docker
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2873,11 +2873,6 @@ _docker_image_build() {
boolean_options+="
--compress
"
if __docker_server_is_experimental ; then
boolean_options+="
--stream
"
fi
fi

local all_options="$options_with_args $boolean_options"
Expand Down