From 1a65d7d84ffdde1baf2a82d28d2b63c226d5777f Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Wed, 15 Nov 2017 19:18:23 -0800 Subject: [PATCH 1/5] attach: Use ContainerWait instead of Inspect Signed-off-by: Kenfe-Mickael Laventure Upstream-commit: 66661761d58d444edcc9c29e643fc7d5ce1abb61 Component: cli --- .../cli/cli/command/container/attach.go | 40 ++++++++------- .../cli/cli/command/container/attach_test.go | 51 +++++++++++-------- components/cli/e2e/container/attach_test.go | 17 +++++++ 3 files changed, 71 insertions(+), 37 deletions(-) create mode 100644 components/cli/e2e/container/attach_test.go diff --git a/components/cli/cli/command/container/attach.go b/components/cli/cli/command/container/attach.go index 6e4628d95d..dca84d5ddb 100644 --- a/components/cli/cli/command/container/attach.go +++ b/components/cli/cli/command/container/attach.go @@ -1,12 +1,14 @@ package container import ( + "fmt" "io" "net/http/httputil" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/docker/docker/pkg/signal" "github.com/pkg/errors" @@ -66,6 +68,9 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { ctx := context.Background() client := dockerCli.Client() + // request channel to wait for client + resultC, errC := client.ContainerWait(ctx, opts.container, "") + c, err := inspectContainerAndCheckState(ctx, client, opts.container) if err != nil { return err @@ -140,7 +145,24 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { if errAttach != nil { return errAttach } - return getExitStatus(ctx, dockerCli.Client(), opts.container) + + return getExitStatus(errC, resultC) +} + +func getExitStatus(errC <-chan error, resultC <-chan container.ContainerWaitOKBody) error { + select { + case result := <-resultC: + if result.Error != nil { + return fmt.Errorf(result.Error.Message) + } + if result.StatusCode != 0 { + return cli.StatusError{StatusCode: int(result.StatusCode)} + } + case err := <-errC: + return err + } + + return nil } func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) { @@ -157,19 +179,3 @@ func resizeTTY(ctx context.Context, dockerCli command.Cli, containerID string) { logrus.Debugf("Error monitoring TTY size: %s", err) } } - -func getExitStatus(ctx context.Context, apiclient client.ContainerAPIClient, containerID string) error { - container, err := apiclient.ContainerInspect(ctx, containerID) - if err != nil { - // If we can't connect, then the daemon probably died. - if !client.IsErrConnectionFailed(err) { - return err - } - return cli.StatusError{StatusCode: -1} - } - status := container.State.ExitCode - if status != 0 { - return cli.StatusError{StatusCode: status} - } - return nil -} diff --git a/components/cli/cli/command/container/attach_test.go b/components/cli/cli/command/container/attach_test.go index 1ca775c6d5..1c305aa7a1 100644 --- a/components/cli/cli/command/container/attach_test.go +++ b/components/cli/cli/command/container/attach_test.go @@ -1,6 +1,7 @@ package container import ( + "fmt" "io/ioutil" "testing" @@ -8,9 +9,9 @@ import ( "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/testutil" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/container" "github.com/pkg/errors" "github.com/stretchr/testify/assert" - "golang.org/x/net/context" ) func TestNewAttachCommandErrors(t *testing.T) { @@ -78,40 +79,50 @@ func TestNewAttachCommandErrors(t *testing.T) { } func TestGetExitStatus(t *testing.T) { - containerID := "the exec id" - expecatedErr := errors.New("unexpected error") + var ( + expectedErr = fmt.Errorf("unexpected error") + errC = make(chan error, 1) + resultC = make(chan container.ContainerWaitOKBody, 1) + ) testcases := []struct { - inspectError error - exitCode int + result *container.ContainerWaitOKBody + err error expectedError error }{ { - inspectError: nil, - exitCode: 0, + result: &container.ContainerWaitOKBody{ + StatusCode: 0, + }, }, { - inspectError: expecatedErr, - expectedError: expecatedErr, + err: expectedErr, + expectedError: expectedErr, }, { - exitCode: 15, + result: &container.ContainerWaitOKBody{ + Error: &container.ContainerWaitOKBodyError{ + expectedErr.Error(), + }, + }, + expectedError: expectedErr, + }, + { + result: &container.ContainerWaitOKBody{ + StatusCode: 15, + }, expectedError: cli.StatusError{StatusCode: 15}, }, } for _, testcase := range testcases { - client := &fakeClient{ - inspectFunc: func(id string) (types.ContainerJSON, error) { - assert.Equal(t, containerID, id) - return types.ContainerJSON{ - ContainerJSONBase: &types.ContainerJSONBase{ - State: &types.ContainerState{ExitCode: testcase.exitCode}, - }, - }, testcase.inspectError - }, + if testcase.err != nil { + errC <- testcase.err } - err := getExitStatus(context.Background(), client, containerID) + if testcase.result != nil { + resultC <- *testcase.result + } + err := getExitStatus(errC, resultC) assert.Equal(t, testcase.expectedError, err) } } diff --git a/components/cli/e2e/container/attach_test.go b/components/cli/e2e/container/attach_test.go new file mode 100644 index 0000000000..a2b6188b50 --- /dev/null +++ b/components/cli/e2e/container/attach_test.go @@ -0,0 +1,17 @@ +package container + +import ( + "testing" + + "github.com/gotestyourself/gotestyourself/icmd" +) + +func TestAttachExitCode(t *testing.T) { + cName := "test-attach-exit-code" + icmd.RunCommand("docker", "run", "-d", "--rm", "--name", cName, + alpineImage, "sh", "-c", "sleep 5 ; exit 21").Assert(t, icmd.Success) + cmd := icmd.Command("docker", "wait", cName) + res := icmd.StartCmd(cmd) + icmd.RunCommand("docker", "attach", cName).Assert(t, icmd.Expected{ExitCode: 21}) + icmd.WaitOnCmd(8, res).Assert(t, icmd.Expected{ExitCode: 0, Out: "21"}) +} From 3acd4904bf367aff0d554ce37f821e142e63d7e1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 17 Nov 2017 12:45:27 -0500 Subject: [PATCH 2/5] Fix TestAttachExitCode Signed-off-by: Daniel Nephin Upstream-commit: bace33d7a8486371e8a595bd21eb3e4b43db1b23 Component: cli --- components/cli/e2e/container/attach_test.go | 26 +++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/components/cli/e2e/container/attach_test.go b/components/cli/e2e/container/attach_test.go index a2b6188b50..d26cae90ff 100644 --- a/components/cli/e2e/container/attach_test.go +++ b/components/cli/e2e/container/attach_test.go @@ -1,17 +1,29 @@ package container import ( + "strings" "testing" "github.com/gotestyourself/gotestyourself/icmd" ) func TestAttachExitCode(t *testing.T) { - cName := "test-attach-exit-code" - icmd.RunCommand("docker", "run", "-d", "--rm", "--name", cName, - alpineImage, "sh", "-c", "sleep 5 ; exit 21").Assert(t, icmd.Success) - cmd := icmd.Command("docker", "wait", cName) - res := icmd.StartCmd(cmd) - icmd.RunCommand("docker", "attach", cName).Assert(t, icmd.Expected{ExitCode: 21}) - icmd.WaitOnCmd(8, res).Assert(t, icmd.Expected{ExitCode: 0, Out: "21"}) + containerID := runBackgroundContainsWithExitCode(t, 21) + + result := icmd.RunCmd( + icmd.Command("docker", "attach", containerID), + withStdinNewline) + + result.Assert(t, icmd.Expected{ExitCode: 21}) +} + +func runBackgroundContainsWithExitCode(t *testing.T, exitcode int) string { + result := icmd.RunCmd(shell(t, + "docker run -d -i --rm %s sh -c 'read; exit %d'", alpineImage, exitcode)) + result.Assert(t, icmd.Success) + return strings.TrimSpace(result.Stdout()) +} + +func withStdinNewline(cmd *icmd.Cmd) { + cmd.Stdin = strings.NewReader("\n") } From 69799110950e07b6afb30d02b8e22fbbc61386f3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 3 Jan 2018 15:40:55 +0100 Subject: [PATCH 3/5] Fix --network-add adding duplicate networks When adding a network using `docker service update --network-add`, the new network was added by _name_. Existing entries in a service spec are listed by network ID, which resulted in the CLI not detecting duplicate entries for the same network. This patch changes the behavior to always use the network-ID, so that duplicate entries are correctly caught. Before this change; $ docker network create -d overlay foo $ docker service create --name=test --network=foo nginx:alpine $ docker service update --network-add foo test $ docker service inspect --format '{{ json .Spec.TaskTemplate.Networks}}' test [ { "Target": "9ot0ieagg5xv1gxd85m7y33eq" }, { "Target": "9ot0ieagg5xv1gxd85m7y33eq" } ] After this change: $ docker network create -d overlay foo $ docker service create --name=test --network=foo nginx:alpine $ docker service update --network-add foo test service is already attached to network foo Signed-off-by: Sebastiaan van Stijn Upstream-commit: e6ebaf55ddc1dde118c0164dc71a83f977a0adab Component: cli --- .../cli/cli/command/service/client_test.go | 8 +++ components/cli/cli/command/service/opts.go | 26 ++++--- .../cli/cli/command/service/opts_test.go | 39 +++++++++++ components/cli/cli/command/service/update.go | 12 ++-- .../cli/cli/command/service/update_test.go | 67 +++++++++++++++++++ 5 files changed, 136 insertions(+), 16 deletions(-) diff --git a/components/cli/cli/command/service/client_test.go b/components/cli/cli/command/service/client_test.go index 896892b943..b1349844c9 100644 --- a/components/cli/cli/command/service/client_test.go +++ b/components/cli/cli/command/service/client_test.go @@ -16,6 +16,7 @@ type fakeClient struct { serviceListFunc func(context.Context, types.ServiceListOptions) ([]swarm.Service, error) taskListFunc func(context.Context, types.TaskListOptions) ([]swarm.Task, error) infoFunc func(ctx context.Context) (types.Info, error) + networkInspectFunc func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) } func (f *fakeClient) NodeList(ctx context.Context, options types.NodeListOptions) ([]swarm.Node, error) { @@ -60,6 +61,13 @@ func (f *fakeClient) Info(ctx context.Context) (types.Info, error) { return f.infoFunc(ctx) } +func (f *fakeClient) NetworkInspect(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) { + if f.networkInspectFunc != nil { + return f.networkInspectFunc(ctx, networkID, options) + } + return types.NetworkResource{}, nil +} + func newService(id string, name string) swarm.Service { return swarm.Service{ ID: id, diff --git a/components/cli/cli/command/service/opts.go b/components/cli/cli/command/service/opts.go index b57c5c8c4e..19915050b9 100644 --- a/components/cli/cli/command/service/opts.go +++ b/components/cli/cli/command/service/opts.go @@ -353,22 +353,21 @@ func (c *credentialSpecOpt) Value() *swarm.CredentialSpec { return c.value } -func convertNetworks(ctx context.Context, apiClient client.NetworkAPIClient, networks opts.NetworkOpt) ([]swarm.NetworkAttachmentConfig, error) { +func resolveNetworkID(ctx context.Context, apiClient client.NetworkAPIClient, networkIDOrName string) (string, error) { + nw, err := apiClient.NetworkInspect(ctx, networkIDOrName, types.NetworkInspectOptions{Scope: "swarm"}) + return nw.ID, err +} + +func convertNetworks(networks opts.NetworkOpt) []swarm.NetworkAttachmentConfig { var netAttach []swarm.NetworkAttachmentConfig for _, net := range networks.Value() { - networkIDOrName := net.Target - _, err := apiClient.NetworkInspect(ctx, networkIDOrName, types.NetworkInspectOptions{Scope: "swarm"}) - if err != nil { - return nil, err - } netAttach = append(netAttach, swarm.NetworkAttachmentConfig{ Target: net.Target, Aliases: net.Aliases, DriverOpts: net.DriverOpts, }) } - sort.Sort(byNetworkTarget(netAttach)) - return netAttach, nil + return netAttach } type endpointOptions struct { @@ -590,10 +589,15 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N return service, err } - networks, err := convertNetworks(ctx, apiClient, options.networks) - if err != nil { - return service, err + networks := convertNetworks(options.networks) + for i, net := range networks { + nwID, err := resolveNetworkID(ctx, apiClient, net.Target) + if err != nil { + return service, err + } + networks[i].Target = nwID } + sort.Sort(byNetworkTarget(networks)) resources, err := options.resources.ToResourceRequirements() if err != nil { diff --git a/components/cli/cli/command/service/opts_test.go b/components/cli/cli/command/service/opts_test.go index e373c6479d..f68cbb3c5a 100644 --- a/components/cli/cli/command/service/opts_test.go +++ b/components/cli/cli/command/service/opts_test.go @@ -1,12 +1,17 @@ package service import ( + "context" + "fmt" "testing" "time" "github.com/docker/cli/opts" + "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/swarm" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMemBytesString(t *testing.T) { @@ -123,3 +128,37 @@ func TestResourceOptionsToResourceRequirements(t *testing.T) { } } + +func TestToServiceNetwork(t *testing.T) { + nws := []types.NetworkResource{ + {Name: "aaa-network", ID: "id555"}, + {Name: "mmm-network", ID: "id999"}, + {Name: "zzz-network", ID: "id111"}, + } + + client := &fakeClient{ + networkInspectFunc: func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) { + for _, network := range nws { + if network.ID == networkID || network.Name == networkID { + return network, nil + } + } + return types.NetworkResource{}, fmt.Errorf("network not found: %s", networkID) + }, + } + + nwo := opts.NetworkOpt{} + nwo.Set("zzz-network") + nwo.Set("mmm-network") + nwo.Set("aaa-network") + + o := newServiceOptions() + o.mode = "replicated" + o.networks = nwo + + ctx := context.Background() + flags := newCreateCommand(nil).Flags() + service, err := o.ToService(ctx, client, flags) + require.NoError(t, err) + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id111"}, {Target: "id555"}, {Target: "id999"}}, service.TaskTemplate.Networks) +} diff --git a/components/cli/cli/command/service/update.go b/components/cli/cli/command/service/update.go index d657871ff2..ca9752a656 100644 --- a/components/cli/cli/command/service/update.go +++ b/components/cli/cli/command/service/update.go @@ -1119,14 +1119,16 @@ func updateNetworks(ctx context.Context, apiClient client.NetworkAPIClient, flag if flags.Changed(flagNetworkAdd) { values := flags.Lookup(flagNetworkAdd).Value.(*opts.NetworkOpt) - networks, err := convertNetworks(ctx, apiClient, *values) - if err != nil { - return err - } + networks := convertNetworks(*values) for _, network := range networks { - if _, exists := existingNetworks[network.Target]; exists { + nwID, err := resolveNetworkID(ctx, apiClient, network.Target) + if err != nil { + return err + } + if _, exists := existingNetworks[nwID]; exists { return errors.Errorf("service is already attached to network %s", network.Target) } + network.Target = nwID newNetworks = append(newNetworks, network) existingNetworks[network.Target] = struct{}{} } diff --git a/components/cli/cli/command/service/update_test.go b/components/cli/cli/command/service/update_test.go index ba55269cb0..e7e428192f 100644 --- a/components/cli/cli/command/service/update_test.go +++ b/components/cli/cli/command/service/update_test.go @@ -1,6 +1,7 @@ package service import ( + "fmt" "reflect" "sort" "testing" @@ -586,3 +587,69 @@ func TestRemoveGenericResources(t *testing.T) { assert.NoError(t, removeGenericResources(flags, task)) assert.Len(t, task.Resources.Reservations.GenericResources, 1) } + +func TestUpdateNetworks(t *testing.T) { + ctx := context.Background() + nws := []types.NetworkResource{ + {Name: "aaa-network", ID: "id555"}, + {Name: "mmm-network", ID: "id999"}, + {Name: "zzz-network", ID: "id111"}, + } + + client := &fakeClient{ + networkInspectFunc: func(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error) { + for _, network := range nws { + if network.ID == networkID || network.Name == networkID { + return network, nil + } + } + return types.NetworkResource{}, fmt.Errorf("network not found: %s", networkID) + }, + } + + svc := swarm.ServiceSpec{ + TaskTemplate: swarm.TaskSpec{ + ContainerSpec: &swarm.ContainerSpec{}, + Networks: []swarm.NetworkAttachmentConfig{ + {Target: "id999"}, + }, + }, + } + + flags := newUpdateCommand(nil).Flags() + err := flags.Set(flagNetworkAdd, "aaa-network") + require.NoError(t, err) + err = updateService(ctx, client, flags, &svc) + require.NoError(t, err) + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks) + + flags = newUpdateCommand(nil).Flags() + err = flags.Set(flagNetworkAdd, "aaa-network") + require.NoError(t, err) + err = updateService(ctx, client, flags, &svc) + assert.EqualError(t, err, "service is already attached to network aaa-network") + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks) + + flags = newUpdateCommand(nil).Flags() + err = flags.Set(flagNetworkAdd, "id555") + require.NoError(t, err) + err = updateService(ctx, client, flags, &svc) + assert.EqualError(t, err, "service is already attached to network id555") + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}, {Target: "id999"}}, svc.TaskTemplate.Networks) + + flags = newUpdateCommand(nil).Flags() + err = flags.Set(flagNetworkRemove, "id999") + require.NoError(t, err) + err = updateService(ctx, client, flags, &svc) + assert.NoError(t, err) + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id555"}}, svc.TaskTemplate.Networks) + + flags = newUpdateCommand(nil).Flags() + err = flags.Set(flagNetworkAdd, "mmm-network") + require.NoError(t, err) + err = flags.Set(flagNetworkRemove, "aaa-network") + require.NoError(t, err) + err = updateService(ctx, client, flags, &svc) + assert.NoError(t, err) + assert.Equal(t, []swarm.NetworkAttachmentConfig{{Target: "id999"}}, svc.TaskTemplate.Networks) +} From 3fa889eb3ff166834d6e5ef5d95a5c21ae7a7555 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 18 Jan 2018 12:53:22 -0500 Subject: [PATCH 4/5] Add dockerfile for building on non-amd64 platforms Signed-off-by: Daniel Nephin Upstream-commit: 02ca1c85735291d8a6c99da75501b389b7249751 Component: cli --- components/cli/docker.Makefile | 11 +++++++++-- components/cli/dockerfiles/Dockerfile.binary-native | 8 ++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 components/cli/dockerfiles/Dockerfile.binary-native diff --git a/components/cli/docker.Makefile b/components/cli/docker.Makefile index a881593dda..a94b81a81d 100644 --- a/components/cli/docker.Makefile +++ b/components/cli/docker.Makefile @@ -5,6 +5,7 @@ # DEV_DOCKER_IMAGE_NAME = docker-cli-dev$(IMAGE_TAG) +BINARY_NATIVE_IMAGE_NAME = docker-cli-native$(IMAGE_TAG) LINTER_IMAGE_NAME = docker-cli-lint$(IMAGE_TAG) CROSS_IMAGE_NAME = docker-cli-cross$(IMAGE_TAG) VALIDATE_IMAGE_NAME = docker-cli-shell-validate$(IMAGE_TAG) @@ -30,12 +31,18 @@ build_cross_image: build_shell_validate_image: docker build -t $(VALIDATE_IMAGE_NAME) -f ./dockerfiles/Dockerfile.shellcheck . +.PHONY: build_binary_native_image +build_binary_native_image: + docker build -t $(BINARY_NATIVE_IMAGE_NAME) -f ./dockerfiles/Dockerfile.binary-native . + + # build executable using a container -binary: build_docker_image - docker run --rm $(ENVVARS) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make binary +binary: build_binary_native_image + docker run --rm $(ENVVARS) $(MOUNTS) $(BINARY_NATIVE_IMAGE_NAME) build: binary + # clean build artifacts using a container .PHONY: clean clean: build_docker_image diff --git a/components/cli/dockerfiles/Dockerfile.binary-native b/components/cli/dockerfiles/Dockerfile.binary-native new file mode 100644 index 0000000000..e130e8240e --- /dev/null +++ b/components/cli/dockerfiles/Dockerfile.binary-native @@ -0,0 +1,8 @@ +FROM golang:1.9.2-alpine3.6 + +RUN apk add -U git bash coreutils gcc musl-dev + +ENV CGO_ENABLED=0 \ + DISABLE_WARN_OUTSIDE_CONTAINER=1 +WORKDIR /go/src/github.com/docker/cli +CMD ./scripts/build/binary From bc95dc9fb38e87580d98996a7b6b4f6e9868ac8b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 22 Jan 2018 18:09:47 -0500 Subject: [PATCH 5/5] Fix merge conflict Signed-off-by: Daniel Nephin Upstream-commit: ae03b006a6ad113ef3674493b83b82cb43ba748e Component: cli --- components/cli/e2e/container/attach_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/cli/e2e/container/attach_test.go b/components/cli/e2e/container/attach_test.go index d26cae90ff..3e7d0ed244 100644 --- a/components/cli/e2e/container/attach_test.go +++ b/components/cli/e2e/container/attach_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/docker/cli/e2e/internal/fixtures" "github.com/gotestyourself/gotestyourself/icmd" ) @@ -19,7 +20,7 @@ func TestAttachExitCode(t *testing.T) { func runBackgroundContainsWithExitCode(t *testing.T, exitcode int) string { result := icmd.RunCmd(shell(t, - "docker run -d -i --rm %s sh -c 'read; exit %d'", alpineImage, exitcode)) + "docker run -d -i --rm %s sh -c 'read; exit %d'", fixtures.AlpineImage, exitcode)) result.Assert(t, icmd.Success) return strings.TrimSpace(result.Stdout()) }