Change `docker service update` semantics

Change `docker service update` to replace attributes of the target
service rather than augment them. One particular occurrence where the
previous behavior proved problematic is when trying to update a port
mapping: the merge semantics provided no way of removing published
ports, but strictly of adding more.

The utility merge* functions where renamed accordingly to update*.

Signed-off-by: Arnaud Porterie (icecrime) <arnaud.porterie@docker.com>

api/client/service/update.go

@@ -46,7 +46,7 @@ func runUpdate(dockerCli *client.DockerCli, flags *pflag.FlagSet, serviceID stri
 		return err
 	}
 
-	err = mergeService(&service.Spec, flags)
+	err = updateService(&service.Spec, flags)
 	if err != nil {
 		return err
 	}
@@ -59,52 +59,52 @@ func runUpdate(dockerCli *client.DockerCli, flags *pflag.FlagSet, serviceID stri
 	return nil
 }
 
-func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
+func updateService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 
-	mergeString := func(flag string, field *string) {
+	updateString := func(flag string, field *string) {
 		if flags.Changed(flag) {
 			*field, _ = flags.GetString(flag)
 		}
 	}
 
-	mergeListOpts := func(flag string, field *[]string) {
+	updateListOpts := func(flag string, field *[]string) {
 		if flags.Changed(flag) {
 			value := flags.Lookup(flag).Value.(*opts.ListOpts)
 			*field = value.GetAll()
 		}
 	}
 
-	mergeSlice := func(flag string, field *[]string) {
+	updateSlice := func(flag string, field *[]string) {
 		if flags.Changed(flag) {
 			*field, _ = flags.GetStringSlice(flag)
 		}
 	}
 
-	mergeInt64Value := func(flag string, field *int64) {
+	updateInt64Value := func(flag string, field *int64) {
 		if flags.Changed(flag) {
 			*field = flags.Lookup(flag).Value.(int64Value).Value()
 		}
 	}
 
-	mergeDuration := func(flag string, field *time.Duration) {
+	updateDuration := func(flag string, field *time.Duration) {
 		if flags.Changed(flag) {
 			*field, _ = flags.GetDuration(flag)
 		}
 	}
 
-	mergeDurationOpt := func(flag string, field *time.Duration) {
+	updateDurationOpt := func(flag string, field *time.Duration) {
 		if flags.Changed(flag) {
 			*field = *flags.Lookup(flag).Value.(*DurationOpt).Value()
 		}
 	}
 
-	mergeUint64 := func(flag string, field *uint64) {
+	updateUint64 := func(flag string, field *uint64) {
 		if flags.Changed(flag) {
 			*field, _ = flags.GetUint64(flag)
 		}
 	}
 
-	mergeUint64Opt := func(flag string, field *uint64) {
+	updateUint64Opt := func(flag string, field *uint64) {
 		if flags.Changed(flag) {
 			*field = *flags.Lookup(flag).Value.(*Uint64Opt).Value()
 		}
@@ -112,23 +112,23 @@ func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 
 	cspec := &spec.TaskTemplate.ContainerSpec
 	task := &spec.TaskTemplate
-	mergeString(flagName, &spec.Name)
-	mergeLabels(flags, &spec.Labels)
-	mergeString("image", &cspec.Image)
-	mergeSlice("command", &cspec.Command)
-	mergeSlice("arg", &cspec.Command)
-	mergeListOpts("env", &cspec.Env)
-	mergeString("workdir", &cspec.Dir)
-	mergeString("user", &cspec.User)
-	mergeMounts(flags, &cspec.Mounts)
+	updateString(flagName, &spec.Name)
+	updateLabels(flags, &spec.Labels)
+	updateString("image", &cspec.Image)
+	updateSlice("command", &cspec.Command)
+	updateSlice("arg", &cspec.Command)
+	updateListOpts("env", &cspec.Env)
+	updateString("workdir", &cspec.Dir)
+	updateString("user", &cspec.User)
+	updateMounts(flags, &cspec.Mounts)
 
 	if flags.Changed(flagLimitCPU) || flags.Changed(flagLimitMemory) {
 		if task.Resources == nil {
 			task.Resources = &swarm.ResourceRequirements{}
 		}
 		task.Resources.Limits = &swarm.Resources{}
-		mergeInt64Value(flagLimitCPU, &task.Resources.Limits.NanoCPUs)
-		mergeInt64Value(flagLimitMemory, &task.Resources.Limits.MemoryBytes)
+		updateInt64Value(flagLimitCPU, &task.Resources.Limits.NanoCPUs)
+		updateInt64Value(flagLimitMemory, &task.Resources.Limits.MemoryBytes)
 
 	}
 	if flags.Changed(flagReserveCPU) || flags.Changed(flagReserveMemory) {
@@ -136,11 +136,11 @@ func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 			task.Resources = &swarm.ResourceRequirements{}
 		}
 		task.Resources.Reservations = &swarm.Resources{}
-		mergeInt64Value(flagReserveCPU, &task.Resources.Reservations.NanoCPUs)
-		mergeInt64Value(flagReserveMemory, &task.Resources.Reservations.MemoryBytes)
+		updateInt64Value(flagReserveCPU, &task.Resources.Reservations.NanoCPUs)
+		updateInt64Value(flagReserveMemory, &task.Resources.Reservations.MemoryBytes)
 	}
 
-	mergeDurationOpt("stop-grace-period", cspec.StopGracePeriod)
+	updateDurationOpt("stop-grace-period", cspec.StopGracePeriod)
 
 	if flags.Changed(flagRestartCondition) || flags.Changed(flagRestartDelay) || flags.Changed(flagRestartMaxAttempts) || flags.Changed(flagRestartWindow) {
 		if task.RestartPolicy == nil {
@@ -151,17 +151,17 @@ func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 			value, _ := flags.GetString(flagRestartCondition)
 			task.RestartPolicy.Condition = swarm.RestartPolicyCondition(value)
 		}
-		mergeDurationOpt(flagRestartDelay, task.RestartPolicy.Delay)
-		mergeUint64Opt(flagRestartMaxAttempts, task.RestartPolicy.MaxAttempts)
-		mergeDurationOpt((flagRestartWindow), task.RestartPolicy.Window)
+		updateDurationOpt(flagRestartDelay, task.RestartPolicy.Delay)
+		updateUint64Opt(flagRestartMaxAttempts, task.RestartPolicy.MaxAttempts)
+		updateDurationOpt((flagRestartWindow), task.RestartPolicy.Window)
 	}
 
 	if flags.Changed(flagConstraint) {
 		task.Placement = &swarm.Placement{}
-		mergeSlice(flagConstraint, &task.Placement.Constraints)
+		updateSlice(flagConstraint, &task.Placement.Constraints)
 	}
 
-	if err := mergeMode(flags, &spec.Mode); err != nil {
+	if err := updateMode(flags, &spec.Mode); err != nil {
 		return err
 	}
 
@@ -169,11 +169,11 @@ func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 		if spec.UpdateConfig == nil {
 			spec.UpdateConfig = &swarm.UpdateConfig{}
 		}
-		mergeUint64(flagUpdateParallelism, &spec.UpdateConfig.Parallelism)
-		mergeDuration(flagUpdateDelay, &spec.UpdateConfig.Delay)
+		updateUint64(flagUpdateParallelism, &spec.UpdateConfig.Parallelism)
+		updateDuration(flagUpdateDelay, &spec.UpdateConfig.Delay)
 	}
 
-	mergeNetworks(flags, &spec.Networks)
+	updateNetworks(flags, &spec.Networks)
 	if flags.Changed(flagEndpointMode) {
 		value, _ := flags.GetString(flagEndpointMode)
 		spec.EndpointSpec.Mode = swarm.ResolutionMode(value)
@@ -183,38 +183,36 @@ func mergeService(spec *swarm.ServiceSpec, flags *pflag.FlagSet) error {
 		if spec.EndpointSpec == nil {
 			spec.EndpointSpec = &swarm.EndpointSpec{}
 		}
-		mergePorts(flags, &spec.EndpointSpec.Ports)
+		updatePorts(flags, &spec.EndpointSpec.Ports)
 	}
 	return nil
 }
 
-func mergeLabels(flags *pflag.FlagSet, field *map[string]string) {
+func updateLabels(flags *pflag.FlagSet, field *map[string]string) {
 	if !flags.Changed(flagLabel) {
 		return
 	}
 
-	if *field == nil {
-		*field = make(map[string]string)
-	}
-
 	values := flags.Lookup(flagLabel).Value.(*opts.ListOpts).GetAll()
+
+	localLabels := map[string]string{}
 	for key, value := range runconfigopts.ConvertKVStringsToMap(values) {
-		(*field)[key] = value
+		localLabels[key] = value
 	}
+	*field = localLabels
 }
 
 // TODO: should this override by destination path, or does swarm handle that?
-func mergeMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) {
+func updateMounts(flags *pflag.FlagSet, mounts *[]swarm.Mount) {
 	if !flags.Changed(flagMount) {
 		return
 	}
 
-	values := flags.Lookup(flagMount).Value.(*MountOpt).Value()
-	*mounts = append(*mounts, values...)
+	*mounts = flags.Lookup(flagMount).Value.(*MountOpt).Value()
 }
 
 // TODO: should this override by name, or does swarm handle that?
-func mergePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) {
+func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) {
 	if !flags.Changed(flagPublish) {
 		return
 	}
@@ -222,22 +220,27 @@ func mergePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) {
 	values := flags.Lookup(flagPublish).Value.(*opts.ListOpts).GetAll()
 	ports, portBindings, _ := nat.ParsePortSpecs(values)
 
+	var localPortConfig []swarm.PortConfig
 	for port := range ports {
-		*portConfig = append(*portConfig, convertPortToPortConfig(port, portBindings)...)
+		localPortConfig = append(localPortConfig, convertPortToPortConfig(port, portBindings)...)
 	}
+	*portConfig = localPortConfig
 }
 
-func mergeNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) {
+func updateNetworks(flags *pflag.FlagSet, attachments *[]swarm.NetworkAttachmentConfig) {
 	if !flags.Changed(flagNetwork) {
 		return
 	}
 	networks, _ := flags.GetStringSlice(flagNetwork)
+
+	var localAttachments []swarm.NetworkAttachmentConfig
 	for _, network := range networks {
-		*attachments = append(*attachments, swarm.NetworkAttachmentConfig{Target: network})
+		localAttachments = append(localAttachments, swarm.NetworkAttachmentConfig{Target: network})
 	}
+	*attachments = localAttachments
 }
 
-func mergeMode(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error {
+func updateMode(flags *pflag.FlagSet, serviceMode *swarm.ServiceMode) error {
 	if !flags.Changed(flagMode) && !flags.Changed(flagReplicas) {
 		return nil
 	}

integration-cli/docker_api_service_update_test.go

@@ -0,0 +1,39 @@
+// +build !windows
+
+package main
+
+import (
+	"github.com/docker/docker/pkg/integration/checker"
+	"github.com/docker/engine-api/types/swarm"
+	"github.com/go-check/check"
+)
+
+func setPortConfig(portConfig []swarm.PortConfig) serviceConstructor {
+	return func(s *swarm.Service) {
+		if s.Spec.EndpointSpec == nil {
+			s.Spec.EndpointSpec = &swarm.EndpointSpec{}
+		}
+		s.Spec.EndpointSpec.Ports = portConfig
+	}
+}
+
+func (s *DockerSwarmSuite) TestApiServiceUpdatePort(c *check.C) {
+	d := s.AddDaemon(c, true, true)
+
+	// Create a service with a port mapping of 8080:8081.
+	portConfig := []swarm.PortConfig{{TargetPort: 8081, PublishedPort: 8080}}
+	serviceID := d.createService(c, simpleTestService, setInstances(1), setPortConfig(portConfig))
+	waitAndAssert(c, defaultReconciliationTimeout, d.checkActiveContainerCount, checker.Equals, 1)
+
+	// Update the service: changed the port mapping from 8080:8081 to 8082:8083.
+	updatedPortConfig := []swarm.PortConfig{{TargetPort: 8083, PublishedPort: 8082}}
+	remoteService := d.getService(c, serviceID)
+	d.updateService(c, remoteService, setPortConfig(updatedPortConfig))
+
+	// Inspect the service and verify port mapping.
+	updatedService := d.getService(c, serviceID)
+	c.Assert(updatedService.Spec.EndpointSpec, check.NotNil)
+	c.Assert(len(updatedService.Spec.EndpointSpec.Ports), check.Equals, 1)
+	c.Assert(updatedService.Spec.EndpointSpec.Ports[0].TargetPort, check.Equals, uint32(8083))
+	c.Assert(updatedService.Spec.EndpointSpec.Ports[0].PublishedPort, check.Equals, uint32(8082))
+}

integration-cli/docker_cli_service_update_test.go

@@ -0,0 +1,45 @@
+// +build !windows
+
+package main
+
+import (
+	"encoding/json"
+
+	"github.com/docker/docker/pkg/integration/checker"
+	"github.com/docker/engine-api/types/swarm"
+	"github.com/go-check/check"
+)
+
+func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) {
+	d := s.AddDaemon(c, true, true)
+
+	serviceName := "TestServiceUpdatePort"
+	serviceArgs := append([]string{"create", "--name", serviceName, "-p", "8080:8081", defaultSleepImage}, defaultSleepCommand...)
+
+	// Create a service with a port mapping of 8080:8081.
+	out, err := d.Cmd("service", serviceArgs...)
+	c.Assert(err, checker.IsNil)
+	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)
+	c.Assert(err, checker.IsNil)
+
+	// Inspect the service and verify port mapping
+	expected := []swarm.PortConfig{
+		{
+			Protocol:      "tcp",
+			PublishedPort: 8082,
+			TargetPort:    8083,
+		},
+	}
+
+	out, err = d.Cmd("service", "inspect", "--format", "{{ json .Spec.EndpointSpec.Ports }}", serviceName)
+	c.Assert(err, checker.IsNil)
+
+	var portConfig []swarm.PortConfig
+	if err := json.Unmarshal([]byte(out), &portConfig); err != nil {
+		c.Fatalf("invalid JSON in inspect result: %v (%s)", err, out)
+	}
+	c.Assert(portConfig, checker.DeepEquals, expected)
+}