Merge pull request #732 from vieux/fix_name_unique

fix name checks in create and rename

api/handlers.go

@@ -275,14 +275,13 @@ func postContainersCreate(c *context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if container := c.cluster.Container(name); container != nil {
-		httpError(w, fmt.Sprintf("Conflict, The name %s is already assigned to %s. You have to delete (or rename) that container to be able to assign %s to a container again.", name, container.Id, name), http.StatusConflict)
-		return
-	}
-
 	container, err := c.cluster.CreateContainer(cluster.BuildContainerConfig(config), name)
 	if err != nil {
-		httpError(w, err.Error(), http.StatusInternalServerError)
+		if strings.HasPrefix(err.Error(), "Conflict") {
+			httpError(w, err.Error(), http.StatusConflict)
+		} else {
+			httpError(w, err.Error(), http.StatusInternalServerError)
+		}
 		return
 	}
 
@@ -606,12 +605,12 @@ func postRenameContainer(c *context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	newName := r.Form.Get("name")
-
-	// call cluster rename container
-	err = c.cluster.RenameContainer(container, newName)
-	if err != nil {
-		httpError(w, err.Error(), http.StatusInternalServerError)
+	if err = c.cluster.RenameContainer(container, r.Form.Get("name")); err != nil {
+		if strings.HasPrefix(err.Error(), "Conflict") {
+			httpError(w, err.Error(), http.StatusConflict)
+		} else {
+			httpError(w, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 }

cluster/swarm/cluster.go

@@ -85,6 +85,11 @@ func (c *Cluster) CreateContainer(config *cluster.ContainerConfig, name string)
 	c.scheduler.Lock()
 	defer c.scheduler.Unlock()
 
+	// check new name whether avaliable
+	if cID := c.getIDFromName(name); cID != "" {
+		return nil, fmt.Errorf("Conflict, The name %s is already assigned to %s. You have to delete (or rename) that container to be able to assign %s to a container again.", name, cID, name)
+	}
+
 	n, err := c.scheduler.SelectNodeForContainer(c.listNodes(), config)
 	if err != nil {
 		return nil, err
@@ -304,6 +309,26 @@ func (c *Cluster) Containers() []*cluster.Container {
 	return out
 }
 
+func (c *Cluster) getIDFromName(name string) string {
+	// Abort immediately if the name is empty.
+	if len(name) == 0 {
+		return ""
+	}
+
+	c.RLock()
+	defer c.RUnlock()
+	for _, n := range c.engines {
+		for _, c := range n.Containers() {
+			for _, cname := range c.Names {
+				if cname == name || cname == "/"+name {
+					return c.Id
+				}
+			}
+		}
+	}
+	return ""
+}
+
 // Container returns the container with IDOrName in the cluster
 func (c *Cluster) Container(IDOrName string) *cluster.Container {
 	// Abort immediately if the name is empty.
@@ -392,8 +417,8 @@ func (c *Cluster) RenameContainer(container *cluster.Container, newName string)
 	defer c.RUnlock()
 
 	// check new name whether avaliable
-	if conflictContainer := c.Container(newName); conflictContainer != nil {
-		return fmt.Errorf("Error when allocating new name: Conflict. The name %q is already in use by container %s. You have to delete (or rename) that container to be able to reuse that name.", newName, conflictContainer.Id)
+	if cID := c.getIDFromName(newName); cID != "" {
+		return fmt.Errorf("Conflict, The name %s is already assigned to %s. You have to delete (or rename) that container to be able to assign %s to a container again.", newName, cID, newName)
 	}
 
 	// call engine rename

test/integration/api/create.bats

@@ -16,11 +16,21 @@ function teardown() {
 	[ "${#lines[@]}" -eq 0 ]
 
 	# create
-	run docker_swarm create --name test_container busybox sleep 1000
-	[ "$status" -eq 0 ]
+	docker_swarm create --name test_container busybox sleep 1000
 
 	# verify, created container exists
 	run docker_swarm ps -l
 	[ "${#lines[@]}" -eq 2 ]
 	[[ "${lines[1]}" ==  *"test_container"* ]]
 }
+
+@test "docker create conflict" {
+	start_docker_with_busybox 1
+	swarm_manage
+
+	id=$(docker_swarm create busybox)
+	prefix=$(printf "$id" | cut -c 1-4)
+	docker_swarm create --name "$prefix" busybox
+	docker_swarm create --name "$id" busybox
+	docker_swarm create --name test busybox
+}

test/integration/api/rename.bats

@@ -11,11 +11,9 @@ function teardown() {
 	start_docker_with_busybox 2
 	swarm_manage
 
-	run docker_swarm run -d --name test_container busybox sleep 500
-	[ "$status" -eq 0 ]
+	docker_swarm run -d --name test_container busybox sleep 500
 
-	run docker_swarm run -d --name another_container busybox sleep 500
-	[ "$status" -eq 0 ]
+	docker_swarm run -d --name another_container busybox sleep 500
 
 	# make sure container exist
 	run docker_swarm ps -a
@@ -27,11 +25,10 @@ function teardown() {
 	# rename container, conflict and fail
 	run docker_swarm rename test_container another_container
 	[ "$status" -ne 0 ]
-	[[ "${output}" == *"Error when allocating new name: Conflict."* ]]
+	[[ "${output}" == *"Conflict,"* ]]
 
 	# rename container, sucessful
-	run docker_swarm rename test_container rename_container
-	[ "$status" -eq 0 ]
+	docker_swarm rename test_container rename_container
 
 	# verify after, rename 
 	run docker_swarm ps -a
@@ -40,3 +37,15 @@ function teardown() {
 	[[ "${output}" == *"another_container"* ]]
 	[[ "${output}" != *"test_container"* ]]
 }
+
+@test "docker rename conflict" {
+	start_docker_with_busybox 1
+	swarm_manage
+
+	id=$(docker_swarm create busybox)
+	prefix=$(printf "$id" | cut -c 1-4)
+	docker_swarm create --name test busybox
+	docker_swarm rename test "$prefix"
+	run docker_swarm rename test "$id"
+	[ "$status" -eq 1 ]
+}