From dc33fc1ff433fcc70efc22f5cea9b87c6ec64a3b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 20 Jun 2016 12:57:57 -0400 Subject: [PATCH 1/4] Add remove flags for service update with unit tests Signed-off-by: Daniel Nephin --- api/client/service/opts.go | 8 +- api/client/service/update.go | 138 +++++++++++++----- api/client/service/update_test.go | 80 ++++++++++ .../docker_cli_service_update_test.go | 2 +- 4 files changed, 187 insertions(+), 41 deletions(-) diff --git a/api/client/service/opts.go b/api/client/service/opts.go index d0b4e7e846..fcf7f87df8 100644 --- a/api/client/service/opts.go +++ b/api/client/service/opts.go @@ -461,7 +461,7 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { flags.StringVar(&opts.name, flagName, "", "Service name") flags.VarP(&opts.labels, flagLabel, "l", "Service labels") - flags.VarP(&opts.env, "env", "e", "Set environment variables") + flags.VarP(&opts.env, flagEnv, "e", "Set environment variables") flags.StringVarP(&opts.workdir, "workdir", "w", "", "Working directory inside the container") flags.StringVarP(&opts.user, flagUser, "u", "", "Username or UID") flags.Var(&opts.mounts, flagMount, "Attach a mount to the service") @@ -494,14 +494,20 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { const ( flagConstraint = "constraint" flagEndpointMode = "endpoint-mode" + flagEnv = "env" + flagEnvRemove = "remove-env" flagLabel = "label" + flagLabelRemove = "remove-label" flagLimitCPU = "limit-cpu" flagLimitMemory = "limit-memory" flagMode = "mode" flagMount = "mount" + flagMountRemove = "remove-mount" flagName = "name" flagNetwork = "network" + flagNetworkRemove = "remove-network" flagPublish = "publish" + flagPublishRemove = "remove-publish" flagReplicas = "replicas" flagReserveCPU = "reserve-cpu" flagReserveMemory = "reserve-memory" diff --git a/api/client/service/update.go b/api/client/service/update.go index 6aa7c10e73..b08b150a87 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -2,6 +2,7 @@ package service import ( "fmt" + "strings" "time" "golang.org/x/net/context" @@ -34,6 +35,11 @@ func newUpdateCommand(dockerCli *client.DockerCli) *cobra.Command { flags.String("image", "", "Service image tag") flags.String("args", "", "Service command args") addServiceFlags(cmd, opts) + flags.StringSlice(flagEnvRemove, []string{}, "Remove an environment variable") + flags.StringSlice(flagLabelRemove, []string{}, "The key of a label to remove") + flags.StringSlice(flagMountRemove, []string{}, "The mount target for a mount to remove") + flags.StringSlice(flagPublishRemove, []string{}, "The target port to remove") + flags.StringSlice(flagNetworkRemove, []string{}, "The name of a network to remove") return cmd } @@ -136,7 +142,7 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { updateLabels(flags, &spec.Labels) updateString("image", &cspec.Image) updateStringToSlice(flags, "args", &cspec.Args) - updateListOpts("env", &cspec.Env) + updateEnvironment(flags, &cspec.Env) updateString("workdir", &cspec.Dir) updateString(flagUser, &cspec.User) updateMounts(flags, &cspec.Mounts) @@ -169,7 +175,10 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { updateDurationOpt((flagRestartWindow), task.RestartPolicy.Window) } - // TODO: The constraints field is fixed in #23773 + if flags.Changed(flagConstraint) { + task.Placement = &swarm.Placement{} + updateSlice(flagConstraint, &task.Placement.Constraints) + } if err := updateReplicas(flags, &spec.Mode); err != nil { return err @@ -209,20 +218,6 @@ func updateStringToSlice(flags *pflag.FlagSet, flag string, field *[]string) err return err } -func updateLabels(flags *pflag.FlagSet, field *map[string]string) { - if !flags.Changed(flagLabel) { - return - } - - values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll() - - localLabels := map[string]string{} - for key, value := range runconfigopts.ConvertKVStringsToMap(values) { - localLabels[key] = value - } - *field = localLabels -} - func anyChanged(flags *pflag.FlagSet, fields ...string) bool { for _, flag := range fields { if flags.Changed(flag) { @@ -232,42 +227,107 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { return false } -// TODO: should this override by destination path, or does swarm handle that? -func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { - if !flags.Changed(flagMount) { - return +func updateLabels(flags *pflag.FlagSet, field *map[string]string) { + if flags.Changed(flagLabel) { + if field == nil { + *field = map[string]string{} + } + + values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll() + for key, value := range runconfigopts.ConvertKVStringsToMap(values) { + (*field)[key] = value + } } - *mounts = flags.Lookup(flagMount).Value.(*MountOpt).Value() + if field != nil && flags.Changed(flagLabelRemove) { + toRemove, _ := flags.GetStringSlice(flagLabelRemove) + for _, label := range toRemove { + delete(*field, label) + } + } +} + +func updateEnvironment(flags *pflag.FlagSet, field *[]string) { + if flags.Changed(flagEnv) { + value := flags.Lookup(flagEnv).Value.(*opts.ListOpts) + *field = append(*field, value.GetAll()...) + } + if flags.Changed(flagEnvRemove) { + toRemove, _ := flags.GetStringSlice(flagEnvRemove) + for _, envRemove := range toRemove { + for i, env := range *field { + key := envKey(env) + if key == envRemove { + *field = append((*field)[:i], (*field)[i+1:]...) + } + } + } + } +} + +func envKey(value string) string { + kv := strings.SplitN(value, "=", 2) + return kv[0] +} + +func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { + if flags.Changed(flagMount) { + values := flags.Lookup(flagMount).Value.(*MountOpt).Value() + *mounts = append(*mounts, values...) + } + if flags.Changed(flagMountRemove) { + toRemove, _ := flags.GetStringSlice(flagMountRemove) + for _, mountTarget := range toRemove { + for i, mount := range *mounts { + if mount.Target == mountTarget { + *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) + } + } + } + } } -// TODO: should this override by name, or does swarm handle that? func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { - if !flags.Changed(flagPublish) { - return + if flags.Changed(flagPublish) { + values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll() + ports, portBindings, _ := nat.ParsePortSpecs(values) + + for port := range ports { + *portConfig = append(*portConfig, convertPortToPortConfig(port, portBindings)...) + } } - values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll() - ports, portBindings, _ := nat.ParsePortSpecs(values) - - var localPortConfig []swarm.PortConfig - for port := range ports { - localPortConfig = append(localPortConfig, convertPortToPortConfig(port, portBindings)...) + if flags.Changed(flagPublishRemove) { + toRemove, _ := flags.GetStringSlice(flagPublishRemove) + for _, rawTargetPort := range toRemove { + targetPort := nat.Port(rawTargetPort) + for i, port := range *portConfig { + if string(port.Protocol) == targetPort.Proto() && + port.TargetPort == uint32(targetPort.Int()) { + *portConfig = append((*portConfig)[:i], (*portConfig)[i+1:]...) + } + } + } } - *portConfig = localPortConfig } func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) { - if !flags.Changed(flagNetwork) { - return + if flags.Changed(flagNetwork) { + networks, _ := flags.GetStringSlice(flagNetwork) + for _, network := range networks { + *attachments = append(*attachments, swarm.NetworkAttachmentConfig{Target: network}) + } } - networks, _ := flags.GetStringSlice(flagNetwork) - - var localAttachments []swarm.NetworkAttachmentConfig - for _, network := range networks { - localAttachments = append(localAttachments, swarm.NetworkAttachmentConfig{Target: network}) + if flags.Changed(flagNetworkRemove) { + toRemove, _ := flags.GetStringSlice(flagNetworkRemove) + for _, networkTarget := range toRemove { + for i, network := range *attachments { + if network.Target == networkTarget { + *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) + } + } + } } - *attachments = localAttachments } func updateReplicas(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error { diff --git a/api/client/service/update_test.go b/api/client/service/update_test.go index 130e8b3e23..7f1797b06b 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -18,3 +18,83 @@ func TestUpdateServiceArgs(t *testing.T) { updateService(flags, spec) assert.EqualStringSlice(t, cspec.Args, []string{"the", "new args"}) } + +func TestUpdateLabels(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("label", "toadd=newlabel") + flags.Set("remove-label", "toremove") + + labels := map[string]string{ + "toremove": "thelabeltoremove", + "tokeep": "value", + } + + updateLabels(flags, &labels) + assert.Equal(t, len(labels), 2) + assert.Equal(t, labels["tokeep"], "value") + assert.Equal(t, labels["toadd"], "newlabel") +} + +func TestUpdateEnvironment(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("env", "toadd=newenv") + flags.Set("remove-env", "toremove") + + envs := []string{ + "toremove=theenvtoremove", + "tokeep=value", + } + + updateEnvironment(flags, &envs) + assert.Equal(t, len(envs), 2) + assert.Equal(t, envs[0], "tokeep=value") + assert.Equal(t, envs[1], "toadd=newenv") +} + +func TestUpdateMounts(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("mount", "type=volume,target=/toadd") + flags.Set("remove-mount", "/toremove") + + mounts := []swarm.Mount{ + {Target: "/toremove", Type: swarm.MountType("BIND")}, + {Target: "/tokeep", Type: swarm.MountType("BIND")}, + } + + updateMounts(flags, &mounts) + assert.Equal(t, len(mounts), 2) + assert.Equal(t, mounts[0].Target, "/tokeep") + assert.Equal(t, mounts[1].Target, "/toadd") +} + +func TestUpdateNetworks(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("network", "toadd") + flags.Set("remove-network", "toremove") + + attachments := []swarm.NetworkAttachmentConfig{ + {Target: "toremove", Aliases: []string{"foo"}}, + {Target: "tokeep"}, + } + + updateNetworks(flags, &attachments) + assert.Equal(t, len(attachments), 2) + assert.Equal(t, attachments[0].Target, "tokeep") + assert.Equal(t, attachments[1].Target, "toadd") +} + +func TestUpdatePorts(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("publish", "1000:1000") + flags.Set("remove-publish", "333/udp") + + portConfigs := []swarm.PortConfig{ + {TargetPort: 333, Protocol: swarm.PortConfigProtocol("udp")}, + {TargetPort: 555}, + } + + updatePorts(flags, &portConfigs) + assert.Equal(t, len(portConfigs), 2) + assert.Equal(t, portConfigs[0].TargetPort, uint32(555)) + assert.Equal(t, portConfigs[1].TargetPort, uint32(1000)) +} diff --git a/integration-cli/docker_cli_service_update_test.go b/integration-cli/docker_cli_service_update_test.go index b5e6d35c08..680cb16245 100644 --- a/integration-cli/docker_cli_service_update_test.go +++ b/integration-cli/docker_cli_service_update_test.go @@ -22,7 +22,7 @@ func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) { waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, 1) // Update the service: changed the port mapping from 8080:8081 to 8082:8083. - _, err = d.Cmd("service", "update", "-p", "8082:8083", serviceName) + _, err = d.Cmd("service", "update", "-p", "8082:8083", "--remove-publish", "8081", serviceName) c.Assert(err, checker.IsNil) // Inspect the service and verify port mapping From ead1f62abae5e5ad188536a01fb88d55339e3f63 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 24 Jun 2016 11:53:49 -0400 Subject: [PATCH 2/4] PR feedback improve help text for service update remove flags implement proper merge update of placement flag more code re-use in update functions using a toRemove set. Signed-off-by: Daniel Nephin --- api/client/service/opts.go | 1 + api/client/service/update.go | 81 +++++++++++++++++++------------ api/client/service/update_test.go | 20 ++++++-- 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/api/client/service/opts.go b/api/client/service/opts.go index fcf7f87df8..93dbc3ba6c 100644 --- a/api/client/service/opts.go +++ b/api/client/service/opts.go @@ -493,6 +493,7 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { const ( flagConstraint = "constraint" + flagConstraintRemove = "remove-constraint" flagEndpointMode = "endpoint-mode" flagEnv = "env" flagEnvRemove = "remove-env" diff --git a/api/client/service/update.go b/api/client/service/update.go index b08b150a87..6af1a47d9d 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -36,10 +36,11 @@ func newUpdateCommand(dockerCli *client.DockerCli) *cobra.Command { flags.String("args", "", "Service command args") addServiceFlags(cmd, opts) flags.StringSlice(flagEnvRemove, []string{}, "Remove an environment variable") - flags.StringSlice(flagLabelRemove, []string{}, "The key of a label to remove") - flags.StringSlice(flagMountRemove, []string{}, "The mount target for a mount to remove") - flags.StringSlice(flagPublishRemove, []string{}, "The target port to remove") - flags.StringSlice(flagNetworkRemove, []string{}, "The name of a network to remove") + flags.StringSlice(flagLabelRemove, []string{}, "Remove a label by its key") + flags.StringSlice(flagMountRemove, []string{}, "Remove a mount by its target path") + flags.StringSlice(flagPublishRemove, []string{}, "Remove a published port by its target port") + flags.StringSlice(flagNetworkRemove, []string{}, "Remove a network by name") + flags.StringSlice(flagConstraintRemove, []string{}, "Remove a constraint") return cmd } @@ -176,8 +177,10 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { } if flags.Changed(flagConstraint) { - task.Placement = &swarm.Placement{} - updateSlice(flagConstraint, &task.Placement.Constraints) + if task.Placement == nil { + task.Placement = &swarm.Placement{} + } + updatePlacement(flags, task.Placement) } if err := updateReplicas(flags, &spec.Mode); err != nil { @@ -227,6 +230,19 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { return false } +func updatePlacement(flags *pflag.FlagSet, placement *swarm.Placement) { + field, _ := flags.GetStringSlice(flagConstraint) + constraints := &placement.Constraints + placement.Constraints = append(placement.Constraints, field...) + + toRemove := buildToRemoveSet(flags, flagConstraintRemove) + for i, constraint := range placement.Constraints { + if _, exists := toRemove[constraint]; exists { + *constraints = append((*constraints)[:i], (*constraints)[i+1:]...) + } + } +} + func updateLabels(flags *pflag.FlagSet, field *map[string]string) { if flags.Changed(flagLabel) { if field == nil { @@ -252,15 +268,11 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) { value := flags.Lookup(flagEnv).Value.(*opts.ListOpts) *field = append(*field, value.GetAll()...) } - if flags.Changed(flagEnvRemove) { - toRemove, _ := flags.GetStringSlice(flagEnvRemove) - for _, envRemove := range toRemove { - for i, env := range *field { - key := envKey(env) - if key == envRemove { - *field = append((*field)[:i], (*field)[i+1:]...) - } - } + toRemove := buildToRemoveSet(flags, flagEnvRemove) + for i, env := range *field { + key := envKey(env) + if _, exists := toRemove[key]; exists { + *field = append((*field)[:i], (*field)[i+1:]...) } } } @@ -270,19 +282,30 @@ func envKey(value string) string { return kv[0] } +func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { + var empty struct{} + toRemove := make(map[string]struct{}) + + if !flags.Changed(flag) { + return toRemove + } + + toRemoveSlice, _ := flags.GetStringSlice(flag) + for _, key := range toRemoveSlice { + toRemove[key] = empty + } + return toRemove +} + func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { if flags.Changed(flagMount) { values := flags.Lookup(flagMount).Value.(*MountOpt).Value() *mounts = append(*mounts, values...) } - if flags.Changed(flagMountRemove) { - toRemove, _ := flags.GetStringSlice(flagMountRemove) - for _, mountTarget := range toRemove { - for i, mount := range *mounts { - if mount.Target == mountTarget { - *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) - } - } + toRemove := buildToRemoveSet(flags, flagMountRemove) + for i, mount := range *mounts { + if _, exists := toRemove[mount.Target]; exists { + *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) } } } @@ -318,14 +341,10 @@ func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachment *attachments = append(*attachments, swarm.NetworkAttachmentConfig{Target: network}) } } - if flags.Changed(flagNetworkRemove) { - toRemove, _ := flags.GetStringSlice(flagNetworkRemove) - for _, networkTarget := range toRemove { - for i, network := range *attachments { - if network.Target == networkTarget { - *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) - } - } + toRemove := buildToRemoveSet(flags, flagNetworkRemove) + for i, network := range *attachments { + if _, exists := toRemove[network.Target]; exists { + *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) } } } diff --git a/api/client/service/update_test.go b/api/client/service/update_test.go index 7f1797b06b..4a2ccc6169 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -35,15 +35,27 @@ func TestUpdateLabels(t *testing.T) { assert.Equal(t, labels["toadd"], "newlabel") } +func TestUpdatePlacement(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("constraint", "node=toadd") + flags.Set("remove-constraint", "node!=toremove") + + placement := &swarm.Placement{ + Constraints: []string{"node!=toremove", "container=tokeep"}, + } + + updatePlacement(flags, placement) + assert.Equal(t, len(placement.Constraints), 2) + assert.Equal(t, placement.Constraints[0], "container=tokeep") + assert.Equal(t, placement.Constraints[1], "node=toadd") +} + func TestUpdateEnvironment(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("env", "toadd=newenv") flags.Set("remove-env", "toremove") - envs := []string{ - "toremove=theenvtoremove", - "tokeep=value", - } + envs := []string{"toremove=theenvtoremove", "tokeep=value"} updateEnvironment(flags, &envs) assert.Equal(t, len(envs), 2) From 4c6faa434071b87a55256e86020cb78495e9951d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 29 Jun 2016 12:28:33 -0400 Subject: [PATCH 3/4] Change the add/update flags to include 'add' Signed-off-by: Daniel Nephin --- api/client/service/create.go | 10 +++- api/client/service/opts.go | 25 ++++----- api/client/service/update.go | 56 +++++++++++-------- api/client/service/update_test.go | 24 ++++---- .../docker_cli_service_update_test.go | 2 +- 5 files changed, 68 insertions(+), 49 deletions(-) diff --git a/api/client/service/create.go b/api/client/service/create.go index be707c5f5d..f6210be288 100644 --- a/api/client/service/create.go +++ b/api/client/service/create.go @@ -28,7 +28,15 @@ func newCreateCommand(dockerCli *client.DockerCli) *cobra.Command { flags := cmd.Flags() flags.StringVar(&opts.mode, flagMode, "replicated", "Service mode (replicated or global)") addServiceFlags(cmd, opts) - cmd.Flags().SetInterspersed(false) + + flags.VarP(&opts.labels, flagLabel, "l", "Service labels") + flags.VarP(&opts.env, flagEnv, "e", "Set environment variables") + flags.Var(&opts.mounts, flagMount, "Attach a mount to the service") + flags.StringSliceVar(&opts.constraints, flagConstraint, []string{}, "Placement constraints") + flags.StringSliceVar(&opts.networks, flagNetwork, []string{}, "Network attachments") + flags.VarP(&opts.endpoint.ports, flagPublish, "p", "Publish a port as a node port") + + flags.SetInterspersed(false) return cmd } diff --git a/api/client/service/opts.go b/api/client/service/opts.go index 93dbc3ba6c..7c0b0761c6 100644 --- a/api/client/service/opts.go +++ b/api/client/service/opts.go @@ -459,12 +459,9 @@ func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) { func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { flags := cmd.Flags() flags.StringVar(&opts.name, flagName, "", "Service name") - flags.VarP(&opts.labels, flagLabel, "l", "Service labels") - flags.VarP(&opts.env, flagEnv, "e", "Set environment variables") flags.StringVarP(&opts.workdir, "workdir", "w", "", "Working directory inside the container") flags.StringVarP(&opts.user, flagUser, "u", "", "Username or UID") - flags.Var(&opts.mounts, flagMount, "Attach a mount to the service") flags.Var(&opts.resources.limitCPU, flagLimitCPU, "Limit CPUs") flags.Var(&opts.resources.limitMemBytes, flagLimitMemory, "Limit Memory") @@ -479,36 +476,38 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) { flags.Var(&opts.restartPolicy.maxAttempts, flagRestartMaxAttempts, "Maximum number of restarts before giving up") flags.Var(&opts.restartPolicy.window, flagRestartWindow, "Window used to evaluate the restart policy") - flags.StringSliceVar(&opts.constraints, flagConstraint, []string{}, "Placement constraints") - flags.Uint64Var(&opts.update.parallelism, flagUpdateParallelism, 0, "Maximum number of tasks updated simultaneously") flags.DurationVar(&opts.update.delay, flagUpdateDelay, time.Duration(0), "Delay between updates") - flags.StringSliceVar(&opts.networks, flagNetwork, []string{}, "Network attachments") flags.StringVar(&opts.endpoint.mode, flagEndpointMode, "", "Endpoint mode (vip or dnsrr)") - flags.VarP(&opts.endpoint.ports, flagPublish, "p", "Publish a port as a node port") flags.BoolVar(&opts.registryAuth, flagRegistryAuth, false, "Send registry authentication details to Swarm agents") } const ( flagConstraint = "constraint" - flagConstraintRemove = "remove-constraint" + flagConstraintRemove = "constraint-rm" + flagConstraintAdd = "constraint-add" flagEndpointMode = "endpoint-mode" flagEnv = "env" - flagEnvRemove = "remove-env" + flagEnvRemove = "env-rm" + flagEnvAdd = "env-add" flagLabel = "label" - flagLabelRemove = "remove-label" + flagLabelRemove = "label-rm" + flagLabelAdd = "label-add" flagLimitCPU = "limit-cpu" flagLimitMemory = "limit-memory" flagMode = "mode" flagMount = "mount" - flagMountRemove = "remove-mount" + flagMountRemove = "mount-rm" + flagMountAdd = "mount-add" flagName = "name" flagNetwork = "network" - flagNetworkRemove = "remove-network" + flagNetworkRemove = "network-rm" + flagNetworkAdd = "network-add" flagPublish = "publish" - flagPublishRemove = "remove-publish" + flagPublishRemove = "publish-rm" + flagPublishAdd = "publish-add" flagReplicas = "replicas" flagReserveCPU = "reserve-cpu" flagReserveMemory = "reserve-memory" diff --git a/api/client/service/update.go b/api/client/service/update.go index 6af1a47d9d..74b0c85dfc 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -35,15 +35,26 @@ func newUpdateCommand(dockerCli *client.DockerCli) *cobra.Command { flags.String("image", "", "Service image tag") flags.String("args", "", "Service command args") addServiceFlags(cmd, opts) - flags.StringSlice(flagEnvRemove, []string{}, "Remove an environment variable") - flags.StringSlice(flagLabelRemove, []string{}, "Remove a label by its key") - flags.StringSlice(flagMountRemove, []string{}, "Remove a mount by its target path") - flags.StringSlice(flagPublishRemove, []string{}, "Remove a published port by its target port") - flags.StringSlice(flagNetworkRemove, []string{}, "Remove a network by name") - flags.StringSlice(flagConstraintRemove, []string{}, "Remove a constraint") + + flags.Var(newListOptsVar(), flagEnvRemove, "Remove an environment variable") + flags.Var(newListOptsVar(), flagLabelRemove, "Remove a label by its key") + flags.Var(newListOptsVar(), flagMountRemove, "Remove a mount by its target path") + flags.Var(newListOptsVar(), flagPublishRemove, "Remove a published port by its target port") + flags.Var(newListOptsVar(), flagNetworkRemove, "Remove a network by name") + flags.Var(newListOptsVar(), flagConstraintRemove, "Remove a constraint") + flags.Var(&opts.labels, flagLabelAdd, "Add or update service labels") + flags.Var(&opts.env, flagEnvAdd, "Add or update environment variables") + flags.Var(&opts.mounts, flagMountAdd, "Add or update a mount on a service") + flags.StringSliceVar(&opts.constraints, flagConstraintAdd, []string{}, "Add or update placement constraints") + flags.StringSliceVar(&opts.networks, flagNetworkAdd, []string{}, "Add or update network attachments") + flags.Var(&opts.endpoint.ports, flagPublishAdd, "Add or update a published port") return cmd } +func newListOptsVar() *opts.ListOpts { + return opts.NewListOptsRef(&[]string{}, nil) +} + func runUpdate(dockerCli *client.DockerCli, flags *pflag.FlagSet, serviceID string) error { apiClient := dockerCli.Client() ctx := context.Background() @@ -176,7 +187,7 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { updateDurationOpt((flagRestartWindow), task.RestartPolicy.Window) } - if flags.Changed(flagConstraint) { + if anyChanged(flags, flagConstraintAdd, flagConstraintRemove) { if task.Placement == nil { task.Placement = &swarm.Placement{} } @@ -201,7 +212,7 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { spec.EndpointSpec.Mode = swarm.ResolutionMode(value) } - if flags.Changed(flagPublish) { + if anyChanged(flags, flagPublishAdd, flagPublishRemove) { if spec.EndpointSpec == nil { spec.EndpointSpec = &swarm.EndpointSpec{} } @@ -231,7 +242,7 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { } func updatePlacement(flags *pflag.FlagSet, placement *swarm.Placement) { - field, _ := flags.GetStringSlice(flagConstraint) + field, _ := flags.GetStringSlice(flagConstraintAdd) constraints := &placement.Constraints placement.Constraints = append(placement.Constraints, field...) @@ -244,19 +255,19 @@ func updatePlacement(flags *pflag.FlagSet, placement *swarm.Placement) { } func updateLabels(flags *pflag.FlagSet, field *map[string]string) { - if flags.Changed(flagLabel) { + if flags.Changed(flagLabelAdd) { if field == nil { *field = map[string]string{} } - values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll() + values := flags.Lookup(flagLabelAdd).Value.(*opts.ListOpts).GetAll() for key, value := range runconfigopts.ConvertKVStringsToMap(values) { (*field)[key] = value } } if field != nil && flags.Changed(flagLabelRemove) { - toRemove, _ := flags.GetStringSlice(flagLabelRemove) + toRemove := flags.Lookup(flagLabelRemove).Value.(*opts.ListOpts).GetAll() for _, label := range toRemove { delete(*field, label) } @@ -264,8 +275,8 @@ func updateLabels(flags *pflag.FlagSet, field *map[string]string) { } func updateEnvironment(flags *pflag.FlagSet, field *[]string) { - if flags.Changed(flagEnv) { - value := flags.Lookup(flagEnv).Value.(*opts.ListOpts) + if flags.Changed(flagEnvAdd) { + value := flags.Lookup(flagEnvAdd).Value.(*opts.ListOpts) *field = append(*field, value.GetAll()...) } toRemove := buildToRemoveSet(flags, flagEnvRemove) @@ -290,7 +301,7 @@ func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { return toRemove } - toRemoveSlice, _ := flags.GetStringSlice(flag) + toRemoveSlice := flags.Lookup(flag).Value.(*opts.ListOpts).GetAll() for _, key := range toRemoveSlice { toRemove[key] = empty } @@ -298,8 +309,8 @@ func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { } func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { - if flags.Changed(flagMount) { - values := flags.Lookup(flagMount).Value.(*MountOpt).Value() + if flags.Changed(flagMountAdd) { + values := flags.Lookup(flagMountAdd).Value.(*MountOpt).Value() *mounts = append(*mounts, values...) } toRemove := buildToRemoveSet(flags, flagMountRemove) @@ -311,8 +322,8 @@ func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { } func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { - if flags.Changed(flagPublish) { - values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll() + if flags.Changed(flagPublishAdd) { + values := flags.Lookup(flagPublishAdd).Value.(*opts.ListOpts).GetAll() ports, portBindings, _ := nat.ParsePortSpecs(values) for port := range ports { @@ -321,13 +332,14 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { } if flags.Changed(flagPublishRemove) { - toRemove, _ := flags.GetStringSlice(flagPublishRemove) + toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll() for _, rawTargetPort := range toRemove { targetPort := nat.Port(rawTargetPort) for i, port := range *portConfig { if string(port.Protocol) == targetPort.Proto() && port.TargetPort == uint32(targetPort.Int()) { *portConfig = append((*portConfig)[:i], (*portConfig)[i+1:]...) + break } } } @@ -335,8 +347,8 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { } func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) { - if flags.Changed(flagNetwork) { - networks, _ := flags.GetStringSlice(flagNetwork) + if flags.Changed(flagNetworkAdd) { + networks, _ := flags.GetStringSlice(flagNetworkAdd) for _, network := range networks { *attachments = append(*attachments, swarm.NetworkAttachmentConfig{Target: network}) } diff --git a/api/client/service/update_test.go b/api/client/service/update_test.go index 4a2ccc6169..86321e3de7 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -21,8 +21,8 @@ func TestUpdateServiceArgs(t *testing.T) { func TestUpdateLabels(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("label", "toadd=newlabel") - flags.Set("remove-label", "toremove") + flags.Set("label-add", "toadd=newlabel") + flags.Set("label-rm", "toremove") labels := map[string]string{ "toremove": "thelabeltoremove", @@ -37,8 +37,8 @@ func TestUpdateLabels(t *testing.T) { func TestUpdatePlacement(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("constraint", "node=toadd") - flags.Set("remove-constraint", "node!=toremove") + flags.Set("constraint-add", "node=toadd") + flags.Set("constraint-rm", "node!=toremove") placement := &swarm.Placement{ Constraints: []string{"node!=toremove", "container=tokeep"}, @@ -52,8 +52,8 @@ func TestUpdatePlacement(t *testing.T) { func TestUpdateEnvironment(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("env", "toadd=newenv") - flags.Set("remove-env", "toremove") + flags.Set("env-add", "toadd=newenv") + flags.Set("env-rm", "toremove") envs := []string{"toremove=theenvtoremove", "tokeep=value"} @@ -65,8 +65,8 @@ func TestUpdateEnvironment(t *testing.T) { func TestUpdateMounts(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("mount", "type=volume,target=/toadd") - flags.Set("remove-mount", "/toremove") + flags.Set("mount-add", "type=volume,target=/toadd") + flags.Set("mount-rm", "/toremove") mounts := []swarm.Mount{ {Target: "/toremove", Type: swarm.MountType("BIND")}, @@ -81,8 +81,8 @@ func TestUpdateMounts(t *testing.T) { func TestUpdateNetworks(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("network", "toadd") - flags.Set("remove-network", "toremove") + flags.Set("network-add", "toadd") + flags.Set("network-rm", "toremove") attachments := []swarm.NetworkAttachmentConfig{ {Target: "toremove", Aliases: []string{"foo"}}, @@ -97,8 +97,8 @@ func TestUpdateNetworks(t *testing.T) { func TestUpdatePorts(t *testing.T) { flags := newUpdateCommand(nil).Flags() - flags.Set("publish", "1000:1000") - flags.Set("remove-publish", "333/udp") + flags.Set("publish-add", "1000:1000") + flags.Set("publish-rm", "333/udp") portConfigs := []swarm.PortConfig{ {TargetPort: 333, Protocol: swarm.PortConfigProtocol("udp")}, diff --git a/integration-cli/docker_cli_service_update_test.go b/integration-cli/docker_cli_service_update_test.go index 680cb16245..67d1641747 100644 --- a/integration-cli/docker_cli_service_update_test.go +++ b/integration-cli/docker_cli_service_update_test.go @@ -22,7 +22,7 @@ func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) { waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, 1) // Update the service: changed the port mapping from 8080:8081 to 8082:8083. - _, err = d.Cmd("service", "update", "-p", "8082:8083", "--remove-publish", "8081", serviceName) + _, err = d.Cmd("service", "update", "--publish-add", "8082:8083", "--publish-rm", "8081", serviceName) c.Assert(err, checker.IsNil) // Inspect the service and verify port mapping From 3249c1d0e79f57642b96f6692ffa44f46f15b602 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 13 Jul 2016 15:59:23 -0400 Subject: [PATCH 4/4] Fix multi-remove during service update. Signed-off-by: Daniel Nephin --- api/client/service/update.go | 80 ++++++++++++++++++------------- api/client/service/update_test.go | 21 ++++++++ 2 files changed, 68 insertions(+), 33 deletions(-) diff --git a/api/client/service/update.go b/api/client/service/update.go index 74b0c85dfc..8ad3f58e90 100644 --- a/api/client/service/update.go +++ b/api/client/service/update.go @@ -103,13 +103,6 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error { } } - updateListOpts := func(flag string, field *[]string) { - if flags.Changed(flag) { - value := flags.Lookup(flag).Value.(*opts.ListOpts) - *field = value.GetAll() - } - } - updateInt64Value := func(flag string, field *int64) { if flags.Changed(flag) { *field = flags.Lookup(flag).Value.(int64Value).Value() @@ -243,15 +236,10 @@ func anyChanged(flags *pflag.FlagSet, fields ...string) bool { func updatePlacement(flags *pflag.FlagSet, placement *swarm.Placement) { field, _ := flags.GetStringSlice(flagConstraintAdd) - constraints := &placement.Constraints placement.Constraints = append(placement.Constraints, field...) toRemove := buildToRemoveSet(flags, flagConstraintRemove) - for i, constraint := range placement.Constraints { - if _, exists := toRemove[constraint]; exists { - *constraints = append((*constraints)[:i], (*constraints)[i+1:]...) - } - } + placement.Constraints = removeItems(placement.Constraints, toRemove, itemKey) } func updateLabels(flags *pflag.FlagSet, field *map[string]string) { @@ -280,12 +268,7 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) { *field = append(*field, value.GetAll()...) } toRemove := buildToRemoveSet(flags, flagEnvRemove) - for i, env := range *field { - key := envKey(env) - if _, exists := toRemove[key]; exists { - *field = append((*field)[:i], (*field)[i+1:]...) - } - } + *field = removeItems(*field, toRemove, envKey) } func envKey(value string) string { @@ -293,6 +276,10 @@ func envKey(value string) string { return kv[0] } +func itemKey(value string) string { + return value +} + func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { var empty struct{} toRemove := make(map[string]struct{}) @@ -308,17 +295,34 @@ func buildToRemoveSet(flags *pflag.FlagSet, flag string) map[string]struct{} { return toRemove } +func removeItems( + seq []string, + toRemove map[string]struct{}, + keyFunc func(string) string, +) []string { + newSeq := []string{} + for _, item := range seq { + if _, exists := toRemove[keyFunc(item)]; !exists { + newSeq = append(newSeq, item) + } + } + return newSeq +} + func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) { if flags.Changed(flagMountAdd) { values := flags.Lookup(flagMountAdd).Value.(*MountOpt).Value() *mounts = append(*mounts, values...) } toRemove := buildToRemoveSet(flags, flagMountRemove) - for i, mount := range *mounts { - if _, exists := toRemove[mount.Target]; exists { - *mounts = append((*mounts)[:i], (*mounts)[i+1:]...) + + newMounts := []swarm.Mount{} + for _, mount := range *mounts { + if _, exists := toRemove[mount.Target]; !exists { + newMounts = append(newMounts, mount) } } + *mounts = newMounts } func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { @@ -331,19 +335,27 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) { } } - if flags.Changed(flagPublishRemove) { - toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll() + if !flags.Changed(flagPublishRemove) { + return + } + toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.ListOpts).GetAll() + newPorts := []swarm.PortConfig{} +portLoop: + for _, port := range *portConfig { for _, rawTargetPort := range toRemove { targetPort := nat.Port(rawTargetPort) - for i, port := range *portConfig { - if string(port.Protocol) == targetPort.Proto() && - port.TargetPort == uint32(targetPort.Int()) { - *portConfig = append((*portConfig)[:i], (*portConfig)[i+1:]...) - break - } + if equalPort(targetPort, port) { + continue portLoop } } + newPorts = append(newPorts, port) } + *portConfig = newPorts +} + +func equalPort(targetPort nat.Port, port swarm.PortConfig) bool { + return (string(port.Protocol) == targetPort.Proto() && + port.TargetPort == uint32(targetPort.Int())) } func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) { @@ -354,11 +366,13 @@ func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachment } } toRemove := buildToRemoveSet(flags, flagNetworkRemove) - for i, network := range *attachments { - if _, exists := toRemove[network.Target]; exists { - *attachments = append((*attachments)[:i], (*attachments)[i+1:]...) + newNetworks := []swarm.NetworkAttachmentConfig{} + for _, network := range *attachments { + if _, exists := toRemove[network.Target]; !exists { + newNetworks = append(newNetworks, network) } } + *attachments = newNetworks } func updateReplicas(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error { diff --git a/api/client/service/update_test.go b/api/client/service/update_test.go index 86321e3de7..104a9e53e1 100644 --- a/api/client/service/update_test.go +++ b/api/client/service/update_test.go @@ -35,6 +35,15 @@ func TestUpdateLabels(t *testing.T) { assert.Equal(t, labels["toadd"], "newlabel") } +func TestUpdateLabelsRemoveALabelThatDoesNotExist(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("label-rm", "dne") + + labels := map[string]string{"foo": "theoldlabel"} + updateLabels(flags, &labels) + assert.Equal(t, len(labels), 1) +} + func TestUpdatePlacement(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("constraint-add", "node=toadd") @@ -63,6 +72,18 @@ func TestUpdateEnvironment(t *testing.T) { assert.Equal(t, envs[1], "toadd=newenv") } +func TestUpdateEnvironmentWithDuplicateValues(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + flags.Set("env-add", "foo=newenv") + flags.Set("env-add", "foo=dupe") + flags.Set("env-rm", "foo") + + envs := []string{"foo=value"} + + updateEnvironment(flags, &envs) + assert.Equal(t, len(envs), 0) +} + func TestUpdateMounts(t *testing.T) { flags := newUpdateCommand(nil).Flags() flags.Set("mount-add", "type=volume,target=/toadd")