Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup: Remove unnecessary dash escape when building docker container name #1441

Merged
merged 1 commit into from
Sep 26, 2014
Merged

Cleanup: Remove unnecessary dash escape when building docker container name #1441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 14 additions & 25 deletions pkg/kubelet/dockertools/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ func GetKubeletDockerContainers(client DockerInterface) (DockerContainers, error
container := &containers[i]
// Skip containers that we didn't create to allow users to manually
// spin up their own containers if they want.
if !strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"--") {
// TODO(dchen1107): Remove the old separator "--" by end of Oct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you check for prefix + "--" or prefix + "_", then we will kill stale containers and restart them - non-breaking change. We can remove the -- check in a couple weeks (leave a TODO)

if !strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"_") &&
!strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"--") {
glog.Infof("Docker Container:%s is not managed by kubelet.", container.Names[0])
continue
}
result[DockerID(container.ID)] = container
Expand Down Expand Up @@ -287,20 +290,6 @@ func GetDockerPodInfo(client DockerInterface, podFullName, uuid string) (api.Pod
return info, nil
}

// Converts "-" to "_-_" and "_" to "___" so that we can use "--" to meaningfully separate parts of a docker name.
func escapeDash(in string) (out string) {
out = strings.Replace(in, "_", "___", -1)
out = strings.Replace(out, "-", "_-_", -1)
return
}

// Reverses the transformation of escapeDash.
func unescapeDash(in string) (out string) {
out = strings.Replace(in, "_-_", "-", -1)
out = strings.Replace(out, "___", "_", -1)
return
}

const containerNamePrefix = "k8s"

func HashContainer(container *api.Container) uint64 {
Expand All @@ -311,20 +300,20 @@ func HashContainer(container *api.Container) uint64 {

// Creates a name which can be reversed to identify both full pod name and container name.
func BuildDockerName(manifestUUID, podFullName string, container *api.Container) string {
containerName := escapeDash(container.Name) + "." + strconv.FormatUint(HashContainer(container), 16)
containerName := container.Name + "." + strconv.FormatUint(HashContainer(container), 16)
// Note, manifest.ID could be blank.
if len(manifestUUID) == 0 {
return fmt.Sprintf("%s--%s--%s--%08x",
return fmt.Sprintf("%s_%s_%s_%08x",
containerNamePrefix,
containerName,
escapeDash(podFullName),
podFullName,
rand.Uint32())
} else {
return fmt.Sprintf("%s--%s--%s--%s--%08x",
return fmt.Sprintf("%s_%s_%s_%s_%08x",
containerNamePrefix,
containerName,
escapeDash(podFullName),
escapeDash(manifestUUID),
podFullName,
manifestUUID,
rand.Uint32())
}
}
Expand All @@ -337,13 +326,13 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash
if name[0] == '/' {
name = name[1:]
}
parts := strings.Split(name, "--")
parts := strings.Split(name, "_")
if len(parts) == 0 || parts[0] != containerNamePrefix {
return
}
if len(parts) > 1 {
pieces := strings.Split(parts[1], ".")
containerName = unescapeDash(pieces[0])
containerName = pieces[0]
if len(pieces) > 1 {
var err error
hash, err = strconv.ParseUint(pieces[1], 16, 32)
Expand All @@ -353,10 +342,10 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash
}
}
if len(parts) > 2 {
podFullName = unescapeDash(parts[2])
podFullName = parts[2]
}
if len(parts) > 4 {
uuid = unescapeDash(parts[3])
uuid = parts[3]
}
return
}
Expand Down
32 changes: 19 additions & 13 deletions pkg/kubelet/dockertools/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ func TestGetContainerID(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
ID: "foobar",
Names: []string{"/k8s--foo--qux--1234"},
Names: []string{"/k8s_foo_qux_1234"},
},
{
ID: "barbar",
Names: []string{"/k8s--bar--qux--2565"},
Names: []string{"/k8s_bar_qux_2565"},
},
}
fakeDocker.Container = &docker.Container{
Expand Down Expand Up @@ -83,31 +83,37 @@ func TestGetContainerID(t *testing.T) {
}
}

func verifyPackUnpack(t *testing.T, podNamespace, podName, containerName string) {
func verifyPackUnpack(t *testing.T, podNamespace, manifestUUID, podName, containerName string) {
container := &api.Container{Name: containerName}
hasher := adler32.New()
data := fmt.Sprintf("%#v", *container)
hasher.Write([]byte(data))
computedHash := uint64(hasher.Sum32())
podFullName := fmt.Sprintf("%s.%s", podName, podNamespace)
name := BuildDockerName("", podFullName, container)
returnedPodFullName, _, returnedContainerName, hash := ParseDockerName(name)
if podFullName != returnedPodFullName || containerName != returnedContainerName || computedHash != hash {
t.Errorf("For (%s, %s, %d), unpacked (%s, %s, %d)", podFullName, containerName, computedHash, returnedPodFullName, returnedContainerName, hash)
name := BuildDockerName(manifestUUID, podFullName, container)
returnedPodFullName, returnedUUID, returnedContainerName, hash := ParseDockerName(name)
if podFullName != returnedPodFullName || manifestUUID != returnedUUID || containerName != returnedContainerName || computedHash != hash {
t.Errorf("For (%s, %s, %s, %d), unpacked (%s, %s, %s, %d)", podFullName, manifestUUID, containerName, computedHash, returnedPodFullName, returnedUUID, returnedContainerName, hash)
}
}

func TestContainerManifestNaming(t *testing.T) {
verifyPackUnpack(t, "file", "manifest1234", "container5678")
verifyPackUnpack(t, "file", "manifest--", "container__")
verifyPackUnpack(t, "file", "--manifest", "__container")
verifyPackUnpack(t, "", "m___anifest_", "container-_-")
verifyPackUnpack(t, "other", "_m___anifest", "-_-container")
manifestUUID := "d1b925c9-444a-11e4-a576-42010af0a203"
verifyPackUnpack(t, "file", manifestUUID, "manifest1234", "container5678")
verifyPackUnpack(t, "file", manifestUUID, "mani-fest-1234", "container5678")
// UUID is same as pod name
verifyPackUnpack(t, "file", manifestUUID, manifestUUID, "container123")
// empty namespace
verifyPackUnpack(t, "", manifestUUID, manifestUUID, "container123")
// No UUID
verifyPackUnpack(t, "other", "", manifestUUID, "container456")
// No Container name
verifyPackUnpack(t, "other", "", manifestUUID, "")

container := &api.Container{Name: "container"}
podName := "foo"
podNamespace := "test"
name := fmt.Sprintf("k8s--%s--%s.%s--12345", container.Name, podName, podNamespace)
name := fmt.Sprintf("k8s_%s_%s.%s_12345", container.Name, podName, podNamespace)

podFullName := fmt.Sprintf("%s.%s", podName, podNamespace)
returnedPodFullName, _, returnedContainerName, hash := ParseDockerName(name)
Expand Down
62 changes: 31 additions & 31 deletions pkg/kubelet/kubelet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ func TestKillContainerWithError(t *testing.T) {
ContainerList: []docker.APIContainers{
{
ID: "1234",
Names: []string{"/k8s--foo--qux--1234"},
Names: []string{"/k8s_foo_qux_1234"},
},
{
ID: "5678",
Names: []string{"/k8s--bar--qux--5678"},
Names: []string{"/k8s_bar_qux_5678"},
},
},
}
Expand All @@ -105,11 +105,11 @@ func TestKillContainer(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
ID: "1234",
Names: []string{"/k8s--foo--qux--1234"},
Names: []string{"/k8s_foo_qux_1234"},
},
{
ID: "5678",
Names: []string{"/k8s--bar--qux--5678"},
Names: []string{"/k8s_bar_qux_5678"},
},
}
fakeDocker.Container = &docker.Container{
Expand Down Expand Up @@ -154,13 +154,13 @@ func TestSyncPodsDoesNothing(t *testing.T) {
container := api.Container{Name: "bar"}
fakeDocker.ContainerList = []docker.APIContainers{
{
// format is k8s--<container-id>--<pod-fullname>
Names: []string{"/k8s--bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "--foo.test"},
// format is k8s_<container-id>_<pod-fullname>
Names: []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "_foo.test"},
ID: "1234",
},
{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand Down Expand Up @@ -229,8 +229,8 @@ func TestSyncPodsCreatesNetAndContainer(t *testing.T) {

fakeDocker.Lock()
if len(fakeDocker.Created) != 2 ||
!matchString(t, "k8s--net\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) ||
!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[1]) {
!matchString(t, "k8s_net\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) ||
!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[1]) {
t.Errorf("Unexpected containers created %v", fakeDocker.Created)
}
fakeDocker.Unlock()
Expand All @@ -241,7 +241,7 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand All @@ -267,7 +267,7 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) {

fakeDocker.Lock()
if len(fakeDocker.Created) != 1 ||
!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) {
!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) {
t.Errorf("Unexpected containers created %v", fakeDocker.Created)
}
fakeDocker.Unlock()
Expand All @@ -280,7 +280,7 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand Down Expand Up @@ -317,7 +317,7 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) {

fakeDocker.Lock()
if len(fakeDocker.Created) != 1 ||
!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) {
!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) {
t.Errorf("Unexpected containers created %v", fakeDocker.Created)
}
fakeDocker.Unlock()
Expand All @@ -330,8 +330,8 @@ func TestSyncPodsDeletesWithNoNetContainer(t *testing.T) {
kubelet, _, fakeDocker := newTestKubelet(t)
fakeDocker.ContainerList = []docker.APIContainers{
{
// format is k8s--<container-id>--<pod-fullname>
Names: []string{"/k8s--bar--foo.test"},
// format is k8s_<container-id>_<pod-fullname>
Names: []string{"/k8s_bar_foo.test"},
ID: "1234",
},
}
Expand Down Expand Up @@ -372,12 +372,12 @@ func TestSyncPodsDeletes(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
// the k8s prefix is required for the kubelet to manage the container
Names: []string{"/k8s--foo--bar.test"},
Names: []string{"/k8s_foo_bar.test"},
ID: "1234",
},
{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
{
Expand Down Expand Up @@ -410,22 +410,22 @@ func TestSyncPodDeletesDuplicate(t *testing.T) {
dockerContainers := dockertools.DockerContainers{
"1234": &docker.APIContainers{
// the k8s prefix is required for the kubelet to manage the container
Names: []string{"/k8s--foo--bar.test--1"},
Names: []string{"/k8s_foo_bar.test_1"},
ID: "1234",
},
"9876": &docker.APIContainers{
// network container
Names: []string{"/k8s--net--bar.test--"},
Names: []string{"/k8s_net_bar.test_"},
ID: "9876",
},
"4567": &docker.APIContainers{
// Duplicate for the same container.
Names: []string{"/k8s--foo--bar.test--2"},
Names: []string{"/k8s_foo_bar.test_2"},
ID: "4567",
},
"2304": &docker.APIContainers{
// Container for another pod, untouched.
Names: []string{"/k8s--baz--fiz.test--6"},
Names: []string{"/k8s_baz_fiz.test_6"},
ID: "2304",
},
}
Expand Down Expand Up @@ -463,12 +463,12 @@ func TestSyncPodBadHash(t *testing.T) {
dockerContainers := dockertools.DockerContainers{
"1234": &docker.APIContainers{
// the k8s prefix is required for the kubelet to manage the container
Names: []string{"/k8s--bar.1234--foo.test"},
Names: []string{"/k8s_bar.1234_foo.test"},
ID: "1234",
},
"9876": &docker.APIContainers{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand Down Expand Up @@ -505,12 +505,12 @@ func TestSyncPodUnhealthy(t *testing.T) {
dockerContainers := dockertools.DockerContainers{
"1234": &docker.APIContainers{
// the k8s prefix is required for the kubelet to manage the container
Names: []string{"/k8s--bar--foo.test"},
Names: []string{"/k8s_bar_foo.test"},
ID: "1234",
},
"9876": &docker.APIContainers{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand Down Expand Up @@ -776,7 +776,7 @@ func TestGetContainerInfo(t *testing.T) {
ID: containerID,
// pod id: qux
// container id: foo
Names: []string{"/k8s--foo--qux--1234"},
Names: []string{"/k8s_foo_qux_1234"},
},
}

Expand Down Expand Up @@ -826,7 +826,7 @@ func TestGetContainerInfoWithoutCadvisor(t *testing.T) {
ID: "foobar",
// pod id: qux
// container id: foo
Names: []string{"/k8s--foo--qux--uuid--1234"},
Names: []string{"/k8s_foo_qux_uuid_1234"},
},
}

Expand Down Expand Up @@ -855,7 +855,7 @@ func TestGetContainerInfoWhenCadvisorFailed(t *testing.T) {
ID: containerID,
// pod id: qux
// container id: foo
Names: []string{"/k8s--foo--qux--uuid--1234"},
Names: []string{"/k8s_foo_qux_uuid_1234"},
},
}

Expand Down Expand Up @@ -934,7 +934,7 @@ func TestRunInContainer(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
ID: containerID,
Names: []string{"/k8s--" + containerName + "--" + podName + "." + podNamespace + "--1234"},
Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + "_1234"},
},
}

Expand Down Expand Up @@ -968,7 +968,7 @@ func TestRunHandlerExec(t *testing.T) {
fakeDocker.ContainerList = []docker.APIContainers{
{
ID: containerID,
Names: []string{"/k8s--" + containerName + "--" + podName + "." + podNamespace + "--1234"},
Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + "_1234"},
},
}

Expand Down Expand Up @@ -1072,7 +1072,7 @@ func TestSyncPodEventHandlerFails(t *testing.T) {
dockerContainers := dockertools.DockerContainers{
"9876": &docker.APIContainers{
// network container
Names: []string{"/k8s--net--foo.test--"},
Names: []string{"/k8s_net_foo.test_"},
ID: "9876",
},
}
Expand Down
1 change: 1 addition & 0 deletions pkg/kubelet/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func TestValidatePodNoName(t *testing.T) {
"zero-length name": {Name: "", Manifest: api.ContainerManifest{Version: "v1beta1"}},
"name > 255 characters": {Name: strings.Repeat("a", 256), Manifest: api.ContainerManifest{Version: "v1beta1"}},
"name not a DNS subdomain": {Name: "a.b.c.", Manifest: api.ContainerManifest{Version: "v1beta1"}},
"name with underscore": {Name: "a_b_c", Manifest: api.ContainerManifest{Version: "v1beta1"}},
}
for k, v := range errorCases {
if errs := ValidatePod(&v); len(errs) == 0 {
Expand Down