From 905514a12bc89b9fd36b3479e969277ea30b4e0a Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 4 Jan 2015 17:30:30 -0800 Subject: [PATCH] Ensure Namespace and UID are set in kubelet Make all kubelet config sources ensure that UID and Namespace are defaulted, if need be. We can *almost* disable the "if blank" logic for UID, except for tests that call APIs that do not run through SyncPods. We really ought to be enforcing invariants better. --- cmd/integration/integration.go | 2 +- pkg/kubelet/config/config.go | 51 ++++++--- pkg/kubelet/config/config_test.go | 1 + pkg/kubelet/config/etcd.go | 6 +- pkg/kubelet/config/etcd_test.go | 14 +-- pkg/kubelet/config/file.go | 41 +++---- pkg/kubelet/config/file_test.go | 150 ++++++++++--------------- pkg/kubelet/config/http.go | 30 +++-- pkg/kubelet/config/http_test.go | 30 +++-- pkg/kubelet/dockertools/docker.go | 12 +- pkg/kubelet/dockertools/docker_test.go | 28 ++--- pkg/kubelet/kubelet.go | 3 +- pkg/kubelet/kubelet_test.go | 27 +++-- pkg/kubelet/types.go | 2 +- 14 files changed, 206 insertions(+), 191 deletions(-) diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index 1333ca9b064..a2e44080506 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -622,7 +622,7 @@ func main() { createdPods.Insert(p[:n-8]) } } - // We expect 5: 2 net containers + 2 pods from the replication controller + + // We expect 9: 2 net containers + 2 pods from the replication controller + // 1 net container + 2 pods from the URL + // 1 net container + 1 pod from the service test. if len(createdPods) != 9 { diff --git a/pkg/kubelet/config/config.go b/pkg/kubelet/config/config.go index 2b1b4cefc21..261eed87b33 100644 --- a/pkg/kubelet/config/config.go +++ b/pkg/kubelet/config/config.go @@ -24,9 +24,11 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" apierrs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" + "github.com/GoogleCloudPlatform/kubernetes/pkg/client/record" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/pkg/util/config" + utilerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors" "github.com/golang/glog" ) @@ -293,21 +295,28 @@ func (s *podStorage) seenSources(sources ...string) bool { func filterInvalidPods(pods []api.BoundPod, source string) (filtered []*api.BoundPod) { names := util.StringSet{} for i := range pods { - var errors []error - name := podUniqueName(&pods[i]) - if names.Has(name) { - errors = append(errors, apierrs.NewFieldDuplicate("name", pods[i].Name)) + pod := &pods[i] + var errlist []error + if errs := validation.ValidateBoundPod(pod); len(errs) != 0 { + errlist = append(errlist, errs...) + // If validation fails, don't trust it any further - + // even Name could be bad. } else { - names.Insert(name) + name := podUniqueName(pod) + if names.Has(name) { + errlist = append(errlist, apierrs.NewFieldDuplicate("name", pod.Name)) + } else { + names.Insert(name) + } } - if errs := validation.ValidateBoundPod(&pods[i]); len(errs) != 0 { - errors = append(errors, errs...) - } - if len(errors) > 0 { - glog.Warningf("Pod %d (%s) from %s failed validation, ignoring: %v", i+1, pods[i].Name, source, errors) + if len(errlist) > 0 { + name := bestPodIdentString(pod) + err := utilerrors.NewAggregate(errlist) + glog.Warningf("Pod[%d] (%s) from %s failed validation, ignoring: %v", i+1, name, source, err) + record.Eventf(pod, "", "failedValidation", "Error validating pod %s from %s, ignoring: %v", name, source, err) continue } - filtered = append(filtered, &pods[i]) + filtered = append(filtered, pod) } return } @@ -337,11 +346,19 @@ func (s *podStorage) MergedState() interface{} { } // podUniqueName returns a value for a given pod that is unique across a source, -// which is the combination of namespace and ID. +// which is the combination of namespace and name. func podUniqueName(pod *api.BoundPod) string { - namespace := pod.Namespace - if len(namespace) == 0 { - namespace = api.NamespaceDefault - } - return fmt.Sprintf("%s.%s", pod.Name, namespace) + return fmt.Sprintf("%s.%s", pod.Name, pod.Namespace) +} + +func bestPodIdentString(pod *api.BoundPod) string { + namespace := pod.Namespace + if namespace == "" { + namespace = "" + } + name := pod.Name + if name == "" { + name = "" + } + return fmt.Sprintf("%s.%s", name, namespace) } diff --git a/pkg/kubelet/config/config_test.go b/pkg/kubelet/config/config_test.go index fdf7f9fed9f..29aee090eca 100644 --- a/pkg/kubelet/config/config_test.go +++ b/pkg/kubelet/config/config_test.go @@ -52,6 +52,7 @@ func (s sortedPods) Less(i, j int) bool { func CreateValidPod(name, namespace, source string) api.BoundPod { return api.BoundPod{ ObjectMeta: api.ObjectMeta{ + UID: name, // for the purpose of testing, this is unique enough Name: name, Namespace: namespace, Annotations: map[string]string{kubelet.ConfigSourceAnnotationKey: source}, diff --git a/pkg/kubelet/config/etcd.go b/pkg/kubelet/config/etcd.go index 19c0533f2fd..b2577bbf66c 100644 --- a/pkg/kubelet/config/etcd.go +++ b/pkg/kubelet/config/etcd.go @@ -20,7 +20,6 @@ package config import ( "errors" "path" - "strconv" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -105,11 +104,8 @@ func eventToPods(ev watch.Event) ([]api.BoundPod, error) { } for _, pod := range boundPods.Items { - // TODO: generate random UID if not present - if pod.UID == "" && !pod.CreationTimestamp.IsZero() { - pod.UID = strconv.FormatInt(pod.CreationTimestamp.Unix(), 10) - } // Backwards compatibility with old api servers + // TODO: Remove this after 1.0 release. if len(pod.Namespace) == 0 { pod.Namespace = api.NamespaceDefault } diff --git a/pkg/kubelet/config/etcd_test.go b/pkg/kubelet/config/etcd_test.go index 80f25da514e..35f2c57226c 100644 --- a/pkg/kubelet/config/etcd_test.go +++ b/pkg/kubelet/config/etcd_test.go @@ -44,14 +44,14 @@ func TestEventToPods(t *testing.T) { input: watch.Event{ Object: &api.BoundPods{ Items: []api.BoundPod{ - {ObjectMeta: api.ObjectMeta{Name: "foo"}}, - {ObjectMeta: api.ObjectMeta{Name: "bar"}}, + {ObjectMeta: api.ObjectMeta{UID: "111", Name: "foo", Namespace: "foo"}}, + {ObjectMeta: api.ObjectMeta{UID: "222", Name: "bar", Namespace: "bar"}}, }, }, }, pods: []api.BoundPod{ - {ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: api.PodSpec{}}, - {ObjectMeta: api.ObjectMeta{Name: "bar", Namespace: "default"}, Spec: api.PodSpec{}}, + {ObjectMeta: api.ObjectMeta{UID: "111", Name: "foo", Namespace: "foo"}, Spec: api.PodSpec{}}, + {ObjectMeta: api.ObjectMeta{UID: "222", Name: "bar", Namespace: "bar"}, Spec: api.PodSpec{}}, }, fail: false, }, @@ -59,14 +59,12 @@ func TestEventToPods(t *testing.T) { input: watch.Event{ Object: &api.BoundPods{ Items: []api.BoundPod{ - {ObjectMeta: api.ObjectMeta{Name: "1"}}, - {ObjectMeta: api.ObjectMeta{Name: "2", Namespace: "foo"}}, + {ObjectMeta: api.ObjectMeta{UID: "111", Name: "foo"}}, }, }, }, pods: []api.BoundPod{ - {ObjectMeta: api.ObjectMeta{Name: "1", Namespace: "default"}, Spec: api.PodSpec{}}, - {ObjectMeta: api.ObjectMeta{Name: "2", Namespace: "foo"}, Spec: api.PodSpec{}}, + {ObjectMeta: api.ObjectMeta{UID: "111", Name: "foo", Namespace: "default"}, Spec: api.PodSpec{}}, }, fail: false, }, diff --git a/pkg/kubelet/config/file.go b/pkg/kubelet/config/file.go index 9af7ccdd8fa..f1300d9d680 100644 --- a/pkg/kubelet/config/file.go +++ b/pkg/kubelet/config/file.go @@ -18,15 +18,14 @@ limitations under the License. package config import ( - "crypto/sha1" - "encoding/base32" + "crypto/md5" + "encoding/hex" "fmt" + "hash/adler32" "io/ioutil" "os" "path/filepath" - "regexp" "sort" - "strings" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -155,11 +154,27 @@ func extractFromFile(filename string) (api.BoundPod, error) { return pod, fmt.Errorf("can't convert pod from file %q: %v", filename, err) } + hostname, err := os.Hostname() //TODO: kubelet name would be better + if err != nil { + return pod, err + } + if len(pod.UID) == 0 { - pod.UID = simpleSubdomainSafeHash(filename) + hasher := md5.New() + fmt.Fprintf(hasher, "host:%s", hostname) + fmt.Fprintf(hasher, "file:%s", filename) + util.DeepHashObject(hasher, pod) + pod.UID = hex.EncodeToString(hasher.Sum(nil)[0:]) + glog.V(5).Infof("Generated UID %q for pod %q from file %s", pod.UID, pod.Name, filename) } if len(pod.Namespace) == 0 { - pod.Namespace = api.NamespaceDefault + hasher := adler32.New() + fmt.Fprint(hasher, filename) + // TODO: file-.hostname would be better, if DNS subdomains + // are allowed for namespace (some places only allow DNS + // labels). + pod.Namespace = fmt.Sprintf("file-%08x-%s", hasher.Sum32(), hostname) + glog.V(5).Infof("Generated namespace %q for pod %q from file %s", pod.Namespace, pod.Name, filename) } // TODO(dchen1107): BoundPod is not type of runtime.Object. Once we allow kubelet talks // about Pod directly, we can use SelfLinker defined in package: latest @@ -174,17 +189,3 @@ func extractFromFile(filename string) (api.BoundPod, error) { } return pod, nil } - -var simpleSubdomainSafeEncoding = base32.NewEncoding("0123456789abcdefghijklmnopqrstuv") -var unsafeDNSLabelReplacement = regexp.MustCompile("[^a-z0-9]+") - -// simpleSubdomainSafeHash generates a pod name for the given path that is -// suitable as a subdomain label. -func simpleSubdomainSafeHash(path string) string { - name := strings.ToLower(filepath.Base(path)) - name = unsafeDNSLabelReplacement.ReplaceAllString(name, "") - hasher := sha1.New() - hasher.Write([]byte(path)) - sha := simpleSubdomainSafeEncoding.EncodeToString(hasher.Sum(nil)) - return fmt.Sprintf("%.15s%.30s", name, sha) -} diff --git a/pkg/kubelet/config/file_test.go b/pkg/kubelet/config/file_test.go index 163163621bc..0c8874b938c 100644 --- a/pkg/kubelet/config/file_test.go +++ b/pkg/kubelet/config/file_test.go @@ -21,20 +21,19 @@ import ( "io/ioutil" "os" "sort" + "strings" "testing" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" - - "github.com/ghodss/yaml" ) func ExampleManifestAndPod(id string) (api.ContainerManifest, api.BoundPod) { manifest := api.ContainerManifest{ ID: id, - UUID: "uid", + UUID: id, Containers: []api.Container{ { Name: "c" + id, @@ -53,9 +52,8 @@ func ExampleManifestAndPod(id string) (api.ContainerManifest, api.BoundPod) { } expectedPod := api.BoundPod{ ObjectMeta: api.ObjectMeta{ - Name: id, - UID: "uid", - Namespace: "default", + Name: id, + UID: id, }, Spec: api.PodSpec{ Containers: []api.Container{ @@ -116,7 +114,13 @@ func writeTestFile(t *testing.T, dir, name string, contents string) *os.File { } func TestReadFromFile(t *testing.T) { - file := writeTestFile(t, os.TempDir(), "test_pod_config", "version: v1beta1\nid: test\ncontainers:\n- image: test/image") + file := writeTestFile(t, os.TempDir(), "test_pod_config", + `{ + "version": "v1beta1", + "uuid": "12345", + "id": "test", + "containers": [{ "image": "test/image" }] + }`) defer os.Remove(file.Name()) ch := make(chan interface{}) @@ -127,14 +131,28 @@ func TestReadFromFile(t *testing.T) { expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.BoundPod{ ObjectMeta: api.ObjectMeta{ Name: "test", - UID: simpleSubdomainSafeHash(file.Name()), - Namespace: "default", - SelfLink: "/api/v1beta2/pods/test?namespace=default", + UID: "12345", + Namespace: "", + SelfLink: "", }, Spec: api.PodSpec{ Containers: []api.Container{{Image: "test/image", TerminationMessagePath: "/dev/termination-log"}}, }, }) + + // There's no way to provide namespace in ContainerManifest, so + // it will be defaulted. + if !strings.HasPrefix(update.Pods[0].ObjectMeta.Namespace, "file-") { + t.Errorf("Unexpected namespace: %s", update.Pods[0].ObjectMeta.Namespace) + } + update.Pods[0].ObjectMeta.Namespace = "" + + // SelfLink depends on namespace. + if !strings.HasPrefix(update.Pods[0].ObjectMeta.SelfLink, "/api/") { + t.Errorf("Unexpected selflink: %s", update.Pods[0].ObjectMeta.SelfLink) + } + update.Pods[0].ObjectMeta.SelfLink = "" + if !api.Semantic.DeepEqual(expected, update) { t.Fatalf("Expected %#v, Got %#v", expected, update) } @@ -144,6 +162,29 @@ func TestReadFromFile(t *testing.T) { } } +func TestReadFromFileWithDefaults(t *testing.T) { + file := writeTestFile(t, os.TempDir(), "test_pod_config", + `{ + "version": "v1beta1", + "id": "test", + "containers": [{ "image": "test/image" }] + }`) + defer os.Remove(file.Name()) + + ch := make(chan interface{}) + NewSourceFile(file.Name(), time.Millisecond, ch) + select { + case got := <-ch: + update := got.(kubelet.PodUpdate) + if update.Pods[0].ObjectMeta.UID == "" { + t.Errorf("Unexpected UID: %s", update.Pods[0].ObjectMeta.UID) + } + + case <-time.After(2 * time.Millisecond): + t.Errorf("Expected update, timeout instead") + } +} + func TestExtractFromBadDataFile(t *testing.T) { file := writeTestFile(t, os.TempDir(), "test_pod_config", string([]byte{1, 2, 3})) defer os.Remove(file.Name()) @@ -157,30 +198,6 @@ func TestExtractFromBadDataFile(t *testing.T) { expectEmptyChannel(t, ch) } -func TestExtractFromValidDataFile(t *testing.T) { - manifest, expectedPod := ExampleManifestAndPod("id") - - text, err := json.Marshal(manifest) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - file := writeTestFile(t, os.TempDir(), "test_pod_config", string(text)) - defer os.Remove(file.Name()) - - expectedPod.ObjectMeta.SelfLink = "/api/v1beta2/pods/" + expectedPod.Name + "?namespace=default" - ch := make(chan interface{}, 1) - c := sourceFile{file.Name(), ch} - err = c.extractFromPath() - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - update := (<-ch).(kubelet.PodUpdate) - expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, expectedPod) - if !api.Semantic.DeepEqual(expected, update) { - t.Errorf("Expected %#v, Got %#v", expected, update) - } -} - func TestExtractFromEmptyDir(t *testing.T) { dirName, err := ioutil.TempDir("", "foo") if err != nil { @@ -233,7 +250,6 @@ func TestExtractFromDir(t *testing.T) { } ioutil.WriteFile(name, data, 0755) files[i] = file - pods[i].ObjectMeta.SelfLink = "/api/v1beta2/pods/" + pods[i].Name + "?namespace=default" } ch := make(chan interface{}, 1) @@ -244,7 +260,14 @@ func TestExtractFromDir(t *testing.T) { } update := (<-ch).(kubelet.PodUpdate) + for i := range update.Pods { + update.Pods[i].Namespace = "foobar" + update.Pods[i].SelfLink = "" + } expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, pods...) + for i := range expected.Pods { + expected.Pods[i].Namespace = "foobar" + } sort.Sort(sortedPods(update.Pods)) sort.Sort(sortedPods(expected.Pods)) if !api.Semantic.DeepEqual(expected, update) { @@ -256,60 +279,3 @@ func TestExtractFromDir(t *testing.T) { } } } - -func TestSubdomainSafeName(t *testing.T) { - type Case struct { - Input string - Expected string - } - testCases := []Case{ - {"/some/path/invalidUPPERCASE", "invaliduppercasa6hlenc0vpqbbdtt26ghneqsq3pvud"}, - {"/some/path/_-!%$#&@^&*(){}", "nvhc03p016m60huaiv3avts372rl2p"}, - } - for _, testCase := range testCases { - value := simpleSubdomainSafeHash(testCase.Input) - if value != testCase.Expected { - t.Errorf("Expected %s, Got %s", testCase.Expected, value) - } - value2 := simpleSubdomainSafeHash(testCase.Input) - if value != value2 { - t.Errorf("Value for %s was not stable across runs: %s %s", testCase.Input, value, value2) - } - } -} - -// These are used for testing extract json (below) -type TestData struct { - Value string - Number int -} - -type TestObject struct { - Name string - Data TestData -} - -func verifyStringEquals(t *testing.T, actual, expected string) { - if actual != expected { - t.Errorf("Verification failed. Expected: %s, Found %s", expected, actual) - } -} - -func verifyIntEquals(t *testing.T, actual, expected int) { - if actual != expected { - t.Errorf("Verification failed. Expected: %d, Found %d", expected, actual) - } -} - -func TestExtractJSON(t *testing.T) { - obj := TestObject{} - data := `{ "name": "foo", "data": { "value": "bar", "number": 10 } }` - - if err := yaml.Unmarshal([]byte(data), &obj); err != nil { - t.Fatalf("Could not unmarshal JSON: %v", err) - } - - verifyStringEquals(t, obj.Name, "foo") - verifyStringEquals(t, obj.Data.Value, "bar") - verifyIntEquals(t, obj.Data.Number, 10) -} diff --git a/pkg/kubelet/config/http.go b/pkg/kubelet/config/http.go index 20185999f7e..9703aa22d40 100644 --- a/pkg/kubelet/config/http.go +++ b/pkg/kubelet/config/http.go @@ -19,7 +19,10 @@ package config import ( "bytes" + "crypto/md5" + "encoding/hex" "fmt" + "hash/adler32" "io/ioutil" "net/http" "time" @@ -93,9 +96,7 @@ func (s *sourceURL) extractFromURL() error { if err := api.Scheme.Convert(&manifest, &pod); err != nil { return err } - if len(pod.Namespace) == 0 { - pod.Namespace = api.NamespaceDefault - } + applyDefaults(&pod, s.url) s.updates <- kubelet.PodUpdate{[]api.BoundPod{pod}, kubelet.SET, kubelet.HTTPSource} return nil } @@ -130,12 +131,7 @@ func (s *sourceURL) extractFromURL() error { } for i := range boundPods.Items { pod := &boundPods.Items[i] - if len(pod.Name) == 0 { - pod.Name = fmt.Sprintf("%d", i+1) - } - if len(pod.Namespace) == 0 { - pod.Namespace = api.NamespaceDefault - } + applyDefaults(pod, s.url) } s.updates <- kubelet.PodUpdate{boundPods.Items, kubelet.SET, kubelet.HTTPSource} return nil @@ -145,3 +141,19 @@ func (s *sourceURL) extractFromURL() error { "single manifest (%v: %+v) or as multiple manifests (%v: %+v).\n", s.url, string(data), singleErr, manifest, multiErr, manifests) } + +func applyDefaults(pod *api.BoundPod, url string) { + if len(pod.UID) == 0 { + hasher := md5.New() + fmt.Fprintf(hasher, "url:%s", url) + util.DeepHashObject(hasher, pod) + pod.UID = hex.EncodeToString(hasher.Sum(nil)[0:]) + glog.V(5).Infof("Generated UID %q for pod %q from URL %s", pod.UID, pod.Name, url) + } + if len(pod.Namespace) == 0 { + hasher := adler32.New() + fmt.Fprint(hasher, url) + pod.Namespace = fmt.Sprintf("url-%08x", hasher.Sum32()) + glog.V(5).Infof("Generated namespace %q for pod %q from URL %s", pod.Namespace, pod.Name, url) + } +} diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index 016b70373fc..f0e0f08b71f 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -19,6 +19,7 @@ package config import ( "encoding/json" "net/http/httptest" + "strings" "testing" "time" @@ -26,6 +27,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors" ) func TestURLErrorNotExistNoUpdate(t *testing.T) { @@ -121,13 +123,14 @@ func TestExtractFromHTTP(t *testing.T) { }{ { desc: "Single manifest", - manifests: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, + manifests: api.ContainerManifest{Version: "v1beta1", ID: "foo", UUID: "111"}, expected: CreatePodUpdate(kubelet.SET, kubelet.HTTPSource, api.BoundPod{ ObjectMeta: api.ObjectMeta{ + UID: "111", Name: "foo", - Namespace: "default", + Namespace: "foobar", }, Spec: api.PodSpec{ RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, @@ -138,15 +141,16 @@ func TestExtractFromHTTP(t *testing.T) { { desc: "Multiple manifests", manifests: []api.ContainerManifest{ - {Version: "v1beta1", ID: "", Containers: []api.Container{{Name: "1", Image: "foo"}}}, - {Version: "v1beta1", ID: "bar", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + {Version: "v1beta1", ID: "foo", UUID: "111", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + {Version: "v1beta1", ID: "bar", UUID: "222", Containers: []api.Container{{Name: "1", Image: "foo"}}}, }, expected: CreatePodUpdate(kubelet.SET, kubelet.HTTPSource, api.BoundPod{ ObjectMeta: api.ObjectMeta{ - Name: "1", - Namespace: "default", + UID: "111", + Name: "foo", + Namespace: "foobar", }, Spec: api.PodSpec{ Containers: []api.Container{{ @@ -157,8 +161,9 @@ func TestExtractFromHTTP(t *testing.T) { }, api.BoundPod{ ObjectMeta: api.ObjectMeta{ + UID: "222", Name: "bar", - Namespace: "default", + Namespace: "foobar", }, Spec: api.PodSpec{ Containers: []api.Container{{ @@ -192,12 +197,21 @@ func TestExtractFromHTTP(t *testing.T) { continue } update := (<-ch).(kubelet.PodUpdate) + + for i := range update.Pods { + // There's no way to provide namespace in ContainerManifest, so + // it will be defaulted. + if !strings.HasPrefix(update.Pods[i].ObjectMeta.Namespace, "url-") { + t.Errorf("Unexpected namespace: %s", update.Pods[0].ObjectMeta.Namespace) + } + update.Pods[i].ObjectMeta.Namespace = "foobar" + } if !api.Semantic.DeepEqual(testCase.expected, update) { t.Errorf("%s: Expected: %#v, Got: %#v", testCase.desc, testCase.expected, update) } for i := range update.Pods { if errs := validation.ValidateBoundPod(&update.Pods[i]); len(errs) != 0 { - t.Errorf("%s: Expected no validation errors on %#v, Got %#v", testCase.desc, update.Pods[i], errs) + t.Errorf("%s: Expected no validation errors on %#v, Got %v", testCase.desc, update.Pods[i], errors.NewAggregate(errs)) } } } diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 72ea98d221f..aa3b702f349 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -545,10 +545,10 @@ 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 { +func BuildDockerName(podUID, podFullName string, container *api.Container) string { containerName := container.Name + "." + strconv.FormatUint(HashContainer(container), 16) // Note, manifest.ID could be blank. - if len(manifestUUID) == 0 { + if len(podUID) == 0 { return fmt.Sprintf("%s_%s_%s_%08x", containerNamePrefix, containerName, @@ -559,7 +559,7 @@ func BuildDockerName(manifestUUID, podFullName string, container *api.Container) containerNamePrefix, containerName, podFullName, - manifestUUID, + podUID, rand.Uint32()) } } @@ -590,6 +590,12 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash if len(parts) > 2 { podFullName = parts[2] } + // This is not an off-by-one. We check for > 4 here because (sadly) the + // format generated by BuildDockerName() has an optional field in the + // middle. If len(parts) > 3, parts[3] might be the optional field or + // the (poorly documented) random suffix. If len(parts) > 4, then we + // know [3] is the UUID and [4] is the suffix. Sort of pukey, should + // be fixed by making UID non-optional. if len(parts) > 4 { uuid = parts[3] } diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index cf15a5a37df..29347ace9ba 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -85,31 +85,31 @@ func TestGetContainerID(t *testing.T) { } } -func verifyPackUnpack(t *testing.T, podNamespace, manifestUUID, podName, containerName string) { +func verifyPackUnpack(t *testing.T, podNamespace, podUID, podName, containerName string) { container := &api.Container{Name: containerName} hasher := adler32.New() util.DeepHashObject(hasher, *container) computedHash := uint64(hasher.Sum32()) podFullName := fmt.Sprintf("%s.%s", podName, podNamespace) - 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) + name := BuildDockerName(podUID, podFullName, container) + returnedPodFullName, returnedUID, returnedContainerName, hash := ParseDockerName(name) + if podFullName != returnedPodFullName || podUID != returnedUID || containerName != returnedContainerName || computedHash != hash { + t.Errorf("For (%s, %s, %s, %d), unpacked (%s, %s, %s, %d)", podFullName, podUID, containerName, computedHash, returnedPodFullName, returnedUID, returnedContainerName, hash) } } func TestContainerManifestNaming(t *testing.T) { - 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") + podUID := "d1b925c9-444a-11e4-a576-42010af0a203" + verifyPackUnpack(t, "file", podUID, "manifest1234", "container5678") + verifyPackUnpack(t, "file", podUID, "mani-fest-1234", "container5678") + // UID is same as pod name + verifyPackUnpack(t, "file", podUID, podUID, "container123") // empty namespace - verifyPackUnpack(t, "", manifestUUID, manifestUUID, "container123") - // No UUID - verifyPackUnpack(t, "other", "", manifestUUID, "container456") + verifyPackUnpack(t, "", podUID, podUID, "container123") + // No UID + verifyPackUnpack(t, "other", "", podUID, "container456") // No Container name - verifyPackUnpack(t, "other", "", manifestUUID, "") + verifyPackUnpack(t, "other", "", podUID, "") container := &api.Container{Name: "container"} podName := "foo" diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index b8aaaf2ca56..229bd230037 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1056,8 +1056,7 @@ func (kl *Kubelet) SyncPods(pods []api.BoundPod) error { // Run the sync in an async manifest worker. kl.podWorkers.Run(podFullName, func() { - err := kl.syncPod(pod, dockerContainers) - if err != nil { + if err := kl.syncPod(pod, dockerContainers); err != nil { glog.Errorf("Error syncing pod, skipping: %v", err) record.Eventf(pod, "", "failedSync", "Error syncing pod, skipping: %v", err) } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index d23b9e97786..16c00f8c30f 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -291,19 +291,20 @@ func TestSyncPodsDoesNothing(t *testing.T) { container := api.Container{Name: "bar"} fakeDocker.ContainerList = []docker.APIContainers{ { - // format is k8s__ - Names: []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "_foo.new.test"}, + // format is // k8s____ + Names: []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "_foo.new.test_12345678_0"}, ID: "1234", }, { // network container - Names: []string{"/k8s_net_foo.new.test_"}, + Names: []string{"/k8s_net_foo.new.test_12345678_0"}, ID: "9876", }, } err := kubelet.SyncPods([]api.BoundPod{ { ObjectMeta: api.ObjectMeta{ + UID: "12345678", Name: "foo", Namespace: "new", Annotations: map[string]string{ConfigSourceAnnotationKey: "test"}, @@ -476,13 +477,14 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) { fakeDocker.ContainerList = []docker.APIContainers{ { // network container - Names: []string{"/k8s_net_foo.new.test_"}, + Names: []string{"/k8s_net_foo.new.test_12345678_0"}, ID: "9876", }, } err := kubelet.SyncPods([]api.BoundPod{ { ObjectMeta: api.ObjectMeta{ + UID: "12345678", Name: "foo", Namespace: "new", Annotations: map[string]string{ConfigSourceAnnotationKey: "test"}, @@ -517,13 +519,14 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) { fakeDocker.ContainerList = []docker.APIContainers{ { // network container - Names: []string{"/k8s_net_foo.new.test_"}, + Names: []string{"/k8s_net_foo.new.test_12345678_0"}, ID: "9876", }, } err := kubelet.SyncPods([]api.BoundPod{ { ObjectMeta: api.ObjectMeta{ + UID: "12345678", Name: "foo", Namespace: "new", Annotations: map[string]string{ConfigSourceAnnotationKey: "test"}, @@ -569,14 +572,15 @@ func TestSyncPodsDeletesWithNoNetContainer(t *testing.T) { kubelet, _, fakeDocker := newTestKubelet(t) fakeDocker.ContainerList = []docker.APIContainers{ { - // format is k8s__ - Names: []string{"/k8s_bar_foo.new.test"}, + // format is // k8s___ + Names: []string{"/k8s_bar_foo.new.test_12345678_0"}, ID: "1234", }, } err := kubelet.SyncPods([]api.BoundPod{ { ObjectMeta: api.ObjectMeta{ + UID: "12345678", Name: "foo", Namespace: "new", Annotations: map[string]string{ConfigSourceAnnotationKey: "test"}, @@ -694,17 +698,17 @@ 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.new.test_1"}, + Names: []string{"/k8s_foo_bar.new.test_12345678_1111"}, ID: "1234", }, "9876": &docker.APIContainers{ // network container - Names: []string{"/k8s_net_bar.new.test_"}, + Names: []string{"/k8s_net_bar.new.test_12345678_2222"}, ID: "9876", }, "4567": &docker.APIContainers{ // Duplicate for the same container. - Names: []string{"/k8s_foo_bar.new.test_2"}, + Names: []string{"/k8s_foo_bar.new.test_12345678_3333"}, ID: "4567", }, "2304": &docker.APIContainers{ @@ -715,6 +719,7 @@ func TestSyncPodDeletesDuplicate(t *testing.T) { } err := kubelet.syncPod(&api.BoundPod{ ObjectMeta: api.ObjectMeta{ + UID: "12345678", Name: "bar", Namespace: "new", Annotations: map[string]string{ConfigSourceAnnotationKey: "test"}, @@ -732,7 +737,7 @@ func TestSyncPodDeletesDuplicate(t *testing.T) { verifyCalls(t, fakeDocker, []string{"list", "stop"}) // Expect one of the duplicates to be killed. - if len(fakeDocker.Stopped) != 1 || (len(fakeDocker.Stopped) != 0 && fakeDocker.Stopped[0] != "1234" && fakeDocker.Stopped[0] != "4567") { + if len(fakeDocker.Stopped) != 1 || (fakeDocker.Stopped[0] != "1234" && fakeDocker.Stopped[0] != "4567") { t.Errorf("Wrong containers were stopped: %v", fakeDocker.Stopped) } } diff --git a/pkg/kubelet/types.go b/pkg/kubelet/types.go index 28787fb0ac1..11408ce5b29 100644 --- a/pkg/kubelet/types.go +++ b/pkg/kubelet/types.go @@ -22,7 +22,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" ) -const ConfigSourceAnnotationKey = "kubernetes/config.source" +const ConfigSourceAnnotationKey = "kubernetes.io/config.source" // PodOperation defines what changes will be made on a pod configuration. type PodOperation int