From c70f8b3c9c7a6dc6a219354acaa2e650d1403ecf Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 10 Dec 2015 15:35:53 +0100 Subject: [PATCH] builder: remove container package dependency Signed-off-by: Tibor Vass --- builder/builder.go | 47 +++++++++--------- builder/dockerfile/builder.go | 4 +- builder/dockerfile/dispatchers.go | 18 ++----- builder/dockerfile/internals.go | 81 +++++++++++++++--------------- container/container_unix.go | 13 ++--- container/container_windows.go | 4 -- daemon/daemonbuilder/builder.go | 82 +++++++------------------------ daemon/start.go | 6 +-- pkg/system/path_unix.go | 8 +++ pkg/system/path_windows.go | 7 +++ 10 files changed, 107 insertions(+), 163 deletions(-) create mode 100644 pkg/system/path_unix.go create mode 100644 pkg/system/path_windows.go diff --git a/builder/builder.go b/builder/builder.go index 0ca77c4fc7..bbf0a30e15 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -7,9 +7,10 @@ package builder import ( "io" "os" + "time" "github.com/docker/docker/api/types" - "github.com/docker/docker/container" + "github.com/docker/docker/daemon" "github.com/docker/docker/image" "github.com/docker/docker/runconfig" ) @@ -106,30 +107,40 @@ func (fi *HashedFileInfo) SetHash(h string) { fi.FileHash = h } -// Docker abstracts calls to a Docker Daemon. -type Docker interface { +// Backend abstracts calls to a Docker Daemon. +type Backend interface { // TODO: use digest reference instead of name // LookupImage looks up a Docker image referenced by `name`. - LookupImage(name string) (*image.Image, error) + GetImage(name string) (*image.Image, error) // Pull tells Docker to pull image referenced by `name`. Pull(name string) (*image.Image, error) - - // Container looks up a Docker container referenced by `id`. - Container(id string) (*container.Container, error) - // Create creates a new Docker container and returns potential warnings - // TODO: put warnings in the error - Create(*runconfig.Config, *runconfig.HostConfig) (*container.Container, []string, error) - // Remove removes a container specified by `id`. - Remove(id string, cfg *types.ContainerRmConfig) error + // ContainerWsAttachWithLogs attaches to container. + ContainerWsAttachWithLogs(name string, cfg *daemon.ContainerWsAttachWithLogsConfig) error + // ContainerCreate creates a new Docker container and returns potential warnings + ContainerCreate(params *daemon.ContainerCreateConfig) (types.ContainerCreateResponse, error) + // ContainerRm removes a container specified by `id`. + ContainerRm(name string, config *types.ContainerRmConfig) error // Commit creates a new Docker image from an existing Docker container. Commit(string, *types.ContainerCommitConfig) (string, error) - // Copy copies/extracts a source FileInfo to a destination path inside a container + // Kill stops the container execution abruptly. + ContainerKill(containerID string, sig uint64) error + // Start starts a new container + ContainerStart(containerID string, hostConfig *runconfig.HostConfig) error + // ContainerWait stops processing until the given container is stopped. + ContainerWait(containerID string, timeout time.Duration) (int, error) + + // ContainerUpdateCmd updates container.Path and container.Args + ContainerUpdateCmd(containerID string, cmd []string) error + + // ContainerCopy copies/extracts a source FileInfo to a destination path inside a container // specified by a container object. // TODO: make an Extract method instead of passing `decompress` // TODO: do not pass a FileInfo, instead refactor the archive package to export a Walk function that can be used // with Context.Walk - Copy(c *container.Container, destPath string, src FileInfo, decompress bool) error + //ContainerCopy(name string, res string) (io.ReadCloser, error) + // TODO: use copyBackend api + BuilderCopy(containerID string, destPath string, src FileInfo, decompress bool) error // Retain retains an image avoiding it to be removed or overwritten until a corresponding Release() call. // TODO: remove @@ -137,14 +148,6 @@ type Docker interface { // Release releases a list of images that were retained for the time of a build. // TODO: remove Release(sessionID string, activeImages []string) - // Kill stops the container execution abruptly. - Kill(c *container.Container) error - // Mount mounts the root filesystem for the container. - Mount(c *container.Container) error - // Unmount unmounts the root filesystem for the container. - Unmount(c *container.Container) error - // Start starts a new container - Start(c *container.Container) error } // ImageCache abstracts an image cache store. diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index d9d59e4ffe..554e0c9558 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -77,7 +77,7 @@ type Builder struct { Stdout io.Writer Stderr io.Writer - docker builder.Docker + docker builder.Backend context builder.Context dockerfile *parser.Node @@ -102,7 +102,7 @@ type Builder struct { // NewBuilder creates a new Dockerfile builder from an optional dockerfile and a Config. // If dockerfile is nil, the Dockerfile specified by Config.DockerfileName, // will be read from the Context passed to Build(). -func NewBuilder(config *Config, docker builder.Docker, context builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) { +func NewBuilder(config *Config, docker builder.Backend, context builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) { if config == nil { config = new(Config) } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 08f64f1ac3..038a2c8c61 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -215,7 +215,7 @@ func from(b *Builder, args []string, attributes map[string]bool, original string ) // TODO: don't use `name`, instead resolve it to a digest if !b.Pull { - image, err = b.docker.LookupImage(name) + image, err = b.docker.GetImage(name) // TODO: shouldn't we error out if error is different from "not found" ? } if image == nil { @@ -394,18 +394,12 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) logrus.Debugf("[BUILDER] Command to be executed: %v", b.runConfig.Cmd) - c, err := b.create() + cID, err := b.create() if err != nil { return err } - // Ensure that we keep the container mounted until the commit - // to avoid unmounting and then mounting directly again - b.docker.Mount(c) - defer b.docker.Unmount(c) - - err = b.run(c) - if err != nil { + if err := b.run(cID); err != nil { return err } @@ -414,11 +408,7 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) // properly match it. b.runConfig.Env = env b.runConfig.Cmd = saveCmd - if err := b.commit(c.ID, cmd, "run"); err != nil { - return err - } - - return nil + return b.commit(cID, cmd, "run") } // CMD foo diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index bf407f278e..91b2c0eb02 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -23,7 +23,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/builder" "github.com/docker/docker/builder/dockerfile/parser" - "github.com/docker/docker/container" + "github.com/docker/docker/daemon" "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/httputils" @@ -56,21 +56,16 @@ func (b *Builder) commit(id string, autoCmd *stringutils.StrSlice, comment strin } defer func(cmd *stringutils.StrSlice) { b.runConfig.Cmd = cmd }(cmd) - if hit, err := b.probeCache(); err != nil { + hit, err := b.probeCache() + if err != nil { return err } else if hit { return nil } - container, err := b.create() + id, err = b.create() if err != nil { return err } - id = container.ID - - if err := b.docker.Mount(container); err != nil { - return err - } - defer b.docker.Unmount(container) } // Note: Actually copy the struct @@ -192,11 +187,10 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD return nil } - container, _, err := b.docker.Create(b.runConfig, nil) + container, err := b.docker.ContainerCreate(&daemon.ContainerCreateConfig{Config: b.runConfig}) if err != nil { return err } - defer b.docker.Unmount(container) b.tmpContainers[container.ID] = struct{}{} comment := fmt.Sprintf("%s %s in %s", cmdName, origPaths, dest) @@ -214,15 +208,12 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD } for _, info := range infos { - if err := b.docker.Copy(container, dest, info.FileInfo, info.decompress); err != nil { + if err := b.docker.BuilderCopy(container.ID, dest, info.FileInfo, info.decompress); err != nil { return err } } - if err := b.commit(container.ID, cmd, comment); err != nil { - return err - } - return nil + return b.commit(container.ID, cmd, comment) } func (b *Builder) download(srcURL string) (fi builder.FileInfo, err error) { @@ -414,8 +405,8 @@ func (b *Builder) processImageFrom(img *image.Image) error { } // The default path will be blank on Windows (set by HCS) - if len(b.runConfig.Env) == 0 && container.DefaultPathEnv != "" { - b.runConfig.Env = append(b.runConfig.Env, "PATH="+container.DefaultPathEnv) + if len(b.runConfig.Env) == 0 && system.DefaultPathEnv != "" { + b.runConfig.Env = append(b.runConfig.Env, "PATH="+system.DefaultPathEnv) } // Process ONBUILD triggers if they exist @@ -487,9 +478,9 @@ func (b *Builder) probeCache() (bool, error) { return true, nil } -func (b *Builder) create() (*container.Container, error) { +func (b *Builder) create() (string, error) { if b.image == "" && !b.noBaseImage { - return nil, fmt.Errorf("Please provide a source image with `from` prior to run") + return "", fmt.Errorf("Please provide a source image with `from` prior to run") } b.runConfig.Image = b.image @@ -515,12 +506,14 @@ func (b *Builder) create() (*container.Container, error) { config := *b.runConfig // Create the container - c, warnings, err := b.docker.Create(b.runConfig, hostConfig) + c, err := b.docker.ContainerCreate(&daemon.ContainerCreateConfig{ + Config: b.runConfig, + HostConfig: hostConfig, + }) if err != nil { - return nil, err + return "", err } - defer b.docker.Unmount(c) - for _, warning := range warnings { + for _, warning := range c.Warnings { fmt.Fprintf(b.Stdout, " ---> [Warning] %s\n", warning) } @@ -529,23 +522,24 @@ func (b *Builder) create() (*container.Container, error) { if config.Cmd.Len() > 0 { // override the entry point that may have been picked up from the base image - s := config.Cmd.Slice() - c.Path = s[0] - c.Args = s[1:] + if err := b.docker.ContainerUpdateCmd(c.ID, config.Cmd.Slice()); err != nil { + return "", err + } } - return c, nil + return c.ID, nil } -func (b *Builder) run(c *container.Container) error { - var errCh chan error +func (b *Builder) run(cID string) (err error) { + errCh := make(chan error) if b.Verbose { - errCh = c.Attach(nil, b.Stdout, b.Stderr) - } - - //start the container - if err := b.docker.Start(c); err != nil { - return err + go func() { + errCh <- b.docker.ContainerWsAttachWithLogs(cID, &daemon.ContainerWsAttachWithLogsConfig{ + OutStream: b.Stdout, + ErrStream: b.Stderr, + Stream: true, + }) + }() } finished := make(chan struct{}) @@ -553,13 +547,17 @@ func (b *Builder) run(c *container.Container) error { go func() { select { case <-b.cancelled: - logrus.Debugln("Build cancelled, killing and removing container:", c.ID) - b.docker.Kill(c) - b.removeContainer(c.ID) + logrus.Debugln("Build cancelled, killing and removing container:", cID) + b.docker.ContainerKill(cID, 0) + b.removeContainer(cID) case <-finished: } }() + if err := b.docker.ContainerStart(cID, nil); err != nil { + return err + } + if b.Verbose { // Block on reading output from container, stop on err or chan closed if err := <-errCh; err != nil { @@ -567,8 +565,7 @@ func (b *Builder) run(c *container.Container) error { } } - // Wait for it to finish - if ret, _ := c.WaitStop(-1 * time.Second); ret != 0 { + if ret, _ := b.docker.ContainerWait(cID, -1); ret != 0 { // TODO: change error type, because jsonmessage.JSONError assumes HTTP return &jsonmessage.JSONError{ Message: fmt.Sprintf("The command '%s' returned a non-zero code: %d", b.runConfig.Cmd.ToString(), ret), @@ -584,7 +581,7 @@ func (b *Builder) removeContainer(c string) error { ForceRemove: true, RemoveVolume: true, } - if err := b.docker.Remove(c, rmConfig); err != nil { + if err := b.docker.ContainerRm(c, rmConfig); err != nil { fmt.Fprintf(b.Stdout, "Error removing intermediate container %s: %v\n", stringid.TruncateID(c), err) return err } diff --git a/container/container_unix.go b/container/container_unix.go index c680405c61..972e6585d3 100644 --- a/container/container_unix.go +++ b/container/container_unix.go @@ -29,15 +29,8 @@ import ( "github.com/opencontainers/runc/libcontainer/label" ) -const ( - // DefaultPathEnv is unix style list of directories to search for - // executables. Each directory is separated from the next by a colon - // ':' character . - DefaultPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - - // DefaultSHMSize is the default size (64MB) of the SHM which will be mounted in the container - DefaultSHMSize int64 = 67108864 -) +// DefaultSHMSize is the default size (64MB) of the SHM which will be mounted in the container +const DefaultSHMSize int64 = 67108864 // Container holds the fields specific to unixen implementations. See // CommonContainer for standard fields common to all containers. @@ -66,7 +59,7 @@ func (container *Container) CreateDaemonEnvironment(linkedEnv []string) []string } // Setup environment env := []string{ - "PATH=" + DefaultPathEnv, + "PATH=" + system.DefaultPathEnv, "HOSTNAME=" + fullHostname, // Note: we don't set HOME here because it'll get autoset intelligently // based on the value of USER inside dockerinit, but only if it isn't diff --git a/container/container_windows.go b/container/container_windows.go index 683c59bb09..04a09c55eb 100644 --- a/container/container_windows.go +++ b/container/container_windows.go @@ -7,10 +7,6 @@ import ( "github.com/docker/docker/volume" ) -// DefaultPathEnv is deliberately empty on Windows as the default path will be set by -// the container. Docker has no context of what the default path should be. -const DefaultPathEnv = "" - // Container holds fields specific to the Windows implementation. See // CommonContainer for standard fields common to all containers. type Container struct { diff --git a/daemon/daemonbuilder/builder.go b/daemon/daemonbuilder/builder.go index add33a3f97..61d09f6a76 100644 --- a/daemon/daemonbuilder/builder.go +++ b/daemon/daemonbuilder/builder.go @@ -13,7 +13,6 @@ import ( "github.com/docker/docker/api" "github.com/docker/docker/api/types" "github.com/docker/docker/builder" - "github.com/docker/docker/container" "github.com/docker/docker/daemon" "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" @@ -25,21 +24,16 @@ import ( "github.com/docker/docker/runconfig" ) -// Docker implements builder.Docker for the docker Daemon object. +// Docker implements builder.Backend for the docker Daemon object. type Docker struct { - Daemon *daemon.Daemon + *daemon.Daemon OutOld io.Writer AuthConfigs map[string]types.AuthConfig Archiver *archive.Archiver } -// ensure Docker implements builder.Docker -var _ builder.Docker = Docker{} - -// LookupImage looks up a Docker image referenced by `name`. -func (d Docker) LookupImage(name string) (*image.Image, error) { - return d.Daemon.GetImage(name) -} +// ensure Docker implements builder.Backend +var _ builder.Backend = Docker{} // Pull tells Docker to pull image referenced by `name`. func (d Docker) Pull(name string) (*image.Image, error) { @@ -79,38 +73,15 @@ func (d Docker) Pull(name string) (*image.Image, error) { return d.Daemon.GetImage(name) } -// Container looks up a Docker container referenced by `id`. -func (d Docker) Container(id string) (*container.Container, error) { - return d.Daemon.GetContainer(id) -} - -// Create creates a new Docker container and returns potential warnings -func (d Docker) Create(cfg *runconfig.Config, hostCfg *runconfig.HostConfig) (*container.Container, []string, error) { - ccr, err := d.Daemon.ContainerCreate(&daemon.ContainerCreateConfig{ - Name: "", - Config: cfg, - HostConfig: hostCfg, - AdjustCPUShares: true, - }) +// ContainerUpdateCmd updates Path and Args for the container with ID cID. +func (d Docker) ContainerUpdateCmd(cID string, cmd []string) error { + c, err := d.Daemon.GetContainer(cID) if err != nil { - return nil, nil, err + return err } - container, err := d.Container(ccr.ID) - if err != nil { - return nil, ccr.Warnings, err - } - - return container, ccr.Warnings, d.Mount(container) -} - -// Remove removes a container specified by `id`. -func (d Docker) Remove(id string, cfg *types.ContainerRmConfig) error { - return d.Daemon.ContainerRm(id, cfg) -} - -// Commit creates a new Docker image from an existing Docker container. -func (d Docker) Commit(name string, cfg *types.ContainerCommitConfig) (string, error) { - return d.Daemon.Commit(name, cfg) + c.Path = cmd[0] + c.Args = cmd[1:] + return nil } // Retain retains an image avoiding it to be removed or overwritten until a corresponding Release() call. @@ -125,11 +96,11 @@ func (d Docker) Release(sessionID string, activeImages []string) { //d.Daemon.Graph().Release(sessionID, activeImages...) } -// Copy copies/extracts a source FileInfo to a destination path inside a container +// BuilderCopy copies/extracts a source FileInfo to a destination path inside a container // specified by a container object. // TODO: make sure callers don't unnecessarily convert destPath with filepath.FromSlash (Copy does it already). -// Copy should take in abstract paths (with slashes) and the implementation should convert it to OS-specific paths. -func (d Docker) Copy(c *container.Container, destPath string, src builder.FileInfo, decompress bool) error { +// BuilderCopy should take in abstract paths (with slashes) and the implementation should convert it to OS-specific paths. +func (d Docker) BuilderCopy(cID string, destPath string, src builder.FileInfo, decompress bool) error { srcPath := src.Path() destExists := true rootUID, rootGID := d.Daemon.GetRemappedUIDGID() @@ -137,6 +108,10 @@ func (d Docker) Copy(c *container.Container, destPath string, src builder.FileIn // Work in daemon-local OS specific file paths destPath = filepath.FromSlash(destPath) + c, err := d.Daemon.GetContainer(cID) + if err != nil { + return err + } dest, err := c.GetResourcePath(destPath) if err != nil { return err @@ -211,27 +186,6 @@ func (d Docker) GetCachedImage(imgID string, cfg *runconfig.Config) (string, err return cache.ID().String(), nil } -// Kill stops the container execution abruptly. -func (d Docker) Kill(container *container.Container) error { - return d.Daemon.Kill(container) -} - -// Mount mounts the root filesystem for the container. -func (d Docker) Mount(c *container.Container) error { - return d.Daemon.Mount(c) -} - -// Unmount unmounts the root filesystem for the container. -func (d Docker) Unmount(c *container.Container) error { - d.Daemon.Unmount(c) - return nil -} - -// Start starts a container -func (d Docker) Start(c *container.Container) error { - return d.Daemon.Start(c) -} - // Following is specific to builder contexts // DetectContextFromRemoteURL returns a context and in certain cases the name of the dockerfile to be used diff --git a/daemon/start.go b/daemon/start.go index 0f30d487b1..44ad1d15a3 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -50,11 +50,7 @@ func (daemon *Daemon) ContainerStart(name string, hostConfig *runconfig.HostConf return err } - if err := daemon.containerStart(container); err != nil { - return err - } - - return nil + return daemon.containerStart(container) } // Start starts a container diff --git a/pkg/system/path_unix.go b/pkg/system/path_unix.go new file mode 100644 index 0000000000..1b6cc9cbd9 --- /dev/null +++ b/pkg/system/path_unix.go @@ -0,0 +1,8 @@ +// +build !windows + +package system + +// DefaultPathEnv is unix style list of directories to search for +// executables. Each directory is separated from the next by a colon +// ':' character . +const DefaultPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" diff --git a/pkg/system/path_windows.go b/pkg/system/path_windows.go new file mode 100644 index 0000000000..09e7f89fed --- /dev/null +++ b/pkg/system/path_windows.go @@ -0,0 +1,7 @@ +// +build windows + +package system + +// DefaultPathEnv is deliberately empty on Windows as the default path will be set by +// the container. Docker has no context of what the default path should be. +const DefaultPathEnv = ""