From f1f2461e09f46d254161ff8e7f16a272ac74f25b Mon Sep 17 00:00:00 2001 From: "Arnaud Porterie (icecrime)" Date: Fri, 17 Jun 2016 16:24:07 -0700 Subject: [PATCH] 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) (cherry picked from commit 1f8ab93b4422a88ef3053105c17acf1933576fbe) --- api/client/service/update.go | 97 ++++++++++--------- .../docker_api_service_update_test.go | 39 ++++++++ .../docker_cli_service_update_test.go | 45 +++++++++ 3 files changed, 134 insertions(+), 47 deletions(-) create mode 100644 integration-cli/docker_api_service_update_test.go create mode 100644 integration-cli/docker_cli_service_update_test.go diff --git a/api/client/service/update.go b/api/client/service/update.go index c8e3a6d7a5..03da4b3dd6 100644 --- a/api/client/service/update.go +++ b/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 } diff --git a/integration-cli/docker_api_service_update_test.go b/integration-cli/docker_api_service_update_test.go new file mode 100644 index 0000000000..7fdef97121 --- /dev/null +++ b/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)) +} diff --git a/integration-cli/docker_cli_service_update_test.go b/integration-cli/docker_cli_service_update_test.go new file mode 100644 index 0000000000..b5e6d35c08 --- /dev/null +++ b/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) +}