Merge pull request #25974 from kunalkushwaha/mount-add

correct handling of volumes while service update.

command/service/update.go

@@ -181,7 +181,9 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
 	updateEnvironment(flags, &cspec.Env)
 	updateString(flagWorkdir, &cspec.Dir)
 	updateString(flagUser, &cspec.User)
-	updateMounts(flags, &cspec.Mounts)
+	if err := updateMounts(flags, &cspec.Mounts); err != nil {
+		return err
+	}
 
 	if flags.Changed(flagLimitCPU) || flags.Changed(flagLimitMemory) {
 		taskResources().Limits = &swarm.Resources{}
@@ -402,20 +404,53 @@ func removeItems(
 	return newSeq
 }
 
-func updateMounts(flags *pflag.FlagSet, mounts *[]mounttypes.Mount) {
+type byMountSource []mounttypes.Mount
+
+func (m byMountSource) Len() int      { return len(m) }
+func (m byMountSource) Swap(i, j int) { m[i], m[j] = m[j], m[i] }
+func (m byMountSource) Less(i, j int) bool {
+	a, b := m[i], m[j]
+
+	if a.Source == b.Source {
+		return a.Target < b.Target
+	}
+
+	return a.Source < b.Source
+}
+
+func updateMounts(flags *pflag.FlagSet, mounts *[]mounttypes.Mount) error {
+
+	mountsByTarget := map[string]mounttypes.Mount{}
+
 	if flags.Changed(flagMountAdd) {
 		values := flags.Lookup(flagMountAdd).Value.(*opts.MountOpt).Value()
-		*mounts = append(*mounts, values...)
+		for _, mount := range values {
+			if _, ok := mountsByTarget[mount.Target]; ok {
+				return fmt.Errorf("duplicate mount target")
+			}
+			mountsByTarget[mount.Target] = mount
+		}
+	}
+
+	// Add old list of mount points minus updated one.
+	for _, mount := range *mounts {
+		if _, ok := mountsByTarget[mount.Target]; !ok {
+			mountsByTarget[mount.Target] = mount
+		}
 	}
-	toRemove := buildToRemoveSet(flags, flagMountRemove)
 
 	newMounts := []mounttypes.Mount{}
-	for _, mount := range *mounts {
+
+	toRemove := buildToRemoveSet(flags, flagMountRemove)
+
+	for _, mount := range mountsByTarget {
 		if _, exists := toRemove[mount.Target]; !exists {
 			newMounts = append(newMounts, mount)
 		}
 	}
+	sort.Sort(byMountSource(newMounts))
 	*mounts = newMounts
+	return nil
 }
 
 func updateGroups(flags *pflag.FlagSet, groups *[]string) error {

command/service/update_test.go

@@ -122,18 +122,36 @@ func TestUpdateGroups(t *testing.T) {
 
 func TestUpdateMounts(t *testing.T) {
 	flags := newUpdateCommand(nil).Flags()
-	flags.Set("mount-add", "type=volume,target=/toadd")
+	flags.Set("mount-add", "type=volume,source=vol2,target=/toadd")
 	flags.Set("mount-rm", "/toremove")
 
 	mounts := []mounttypes.Mount{
-		{Target: "/toremove", Type: mounttypes.TypeBind},
-		{Target: "/tokeep", Type: mounttypes.TypeBind},
+		{Target: "/toremove", Source: "vol1", Type: mounttypes.TypeBind},
+		{Target: "/tokeep", Source: "vol3", Type: mounttypes.TypeBind},
 	}
 
 	updateMounts(flags, &mounts)
 	assert.Equal(t, len(mounts), 2)
-	assert.Equal(t, mounts[0].Target, "/tokeep")
-	assert.Equal(t, mounts[1].Target, "/toadd")
+	assert.Equal(t, mounts[0].Target, "/toadd")
+	assert.Equal(t, mounts[1].Target, "/tokeep")
+
+}
+
+func TestUpdateMountsWithDuplicateMounts(t *testing.T) {
+	flags := newUpdateCommand(nil).Flags()
+	flags.Set("mount-add", "type=volume,source=vol4,target=/toadd")
+
+	mounts := []mounttypes.Mount{
+		{Target: "/tokeep1", Source: "vol1", Type: mounttypes.TypeBind},
+		{Target: "/toadd", Source: "vol2", Type: mounttypes.TypeBind},
+		{Target: "/tokeep2", Source: "vol3", Type: mounttypes.TypeBind},
+	}
+
+	updateMounts(flags, &mounts)
+	assert.Equal(t, len(mounts), 3)
+	assert.Equal(t, mounts[0].Target, "/tokeep1")
+	assert.Equal(t, mounts[1].Target, "/tokeep2")
+	assert.Equal(t, mounts[2].Target, "/toadd")
 }
 
 func TestUpdatePorts(t *testing.T) {