From 7a7e64331c5f06de46e8f13c4882b245247c2296 Mon Sep 17 00:00:00 2001 From: Wojciech Tyczynski Date: Thu, 19 Mar 2015 15:39:30 +0100 Subject: [PATCH] Kubelet file read both ContainerManifest and Pod --- pkg/kubelet/config/common.go | 57 +++++++++ pkg/kubelet/config/file.go | 87 ++++--------- pkg/kubelet/config/file_test.go | 213 ++++++++++++++++++++++++-------- pkg/kubelet/config/http.go | 62 +--------- 4 files changed, 240 insertions(+), 179 deletions(-) diff --git a/pkg/kubelet/config/common.go b/pkg/kubelet/config/common.go index beae2d552d4dd..04cbd89946297 100644 --- a/pkg/kubelet/config/common.go +++ b/pkg/kubelet/config/common.go @@ -25,11 +25,13 @@ import ( "strings" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" "github.com/GoogleCloudPlatform/kubernetes/pkg/types" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + "github.com/ghodss/yaml" "github.com/golang/glog" ) @@ -120,3 +122,58 @@ func tryDecodePodList(data []byte, source string, isFile bool) (parsed bool, pod } return true, *newPods, err } + +func tryDecodeSingleManifest(data []byte) (parsed bool, manifest v1beta1.ContainerManifest, pod api.Pod, err error) { + // TODO: should be api.Scheme.Decode + // This is awful. DecodeInto() expects to find an APIObject, which + // Manifest is not. We keep reading manifest for now for compat, but + // we will eventually change it to read Pod (at which point this all + // becomes nicer). Until then, we assert that the ContainerManifest + // structure on disk is always v1beta1. Read that, convert it to a + // "current" ContainerManifest (should be ~identical), then convert + // that to a Pod (which is a well-understood conversion). This + // avoids writing a v1beta1.ContainerManifest -> api.Pod + // conversion which would be identical to the api.ContainerManifest -> + // api.Pod conversion. + if err = yaml.Unmarshal(data, &manifest); err != nil { + return false, manifest, pod, err + } + newManifest := api.ContainerManifest{} + if err = api.Scheme.Convert(&manifest, &newManifest); err != nil { + return false, manifest, pod, err + } + if errs := validation.ValidateManifest(&newManifest); len(errs) > 0 { + err = fmt.Errorf("invalid manifest: %v", errs) + return false, manifest, pod, err + } + if err = api.Scheme.Convert(&newManifest, &pod); err != nil { + return true, manifest, pod, err + } + // Success. + return true, manifest, pod, nil +} + +func tryDecodeManifestList(data []byte) (parsed bool, manifests []v1beta1.ContainerManifest, pods api.PodList, err error) { + // TODO: should be api.Scheme.Decode + // See the comment in tryDecodeSingle(). + if err = yaml.Unmarshal(data, &manifests); err != nil { + return false, manifests, pods, err + } + newManifests := []api.ContainerManifest{} + if err = api.Scheme.Convert(&manifests, &newManifests); err != nil { + return false, manifests, pods, err + } + for i := range newManifests { + manifest := &newManifests[i] + if errs := validation.ValidateManifest(manifest); len(errs) > 0 { + err = fmt.Errorf("invalid manifest: %v", errs) + return false, manifests, pods, err + } + } + list := api.ContainerManifestList{Items: newManifests} + if err = api.Scheme.Convert(&list, &pods); err != nil { + return true, manifests, pods, err + } + // Success. + return true, manifests, pods, nil +} diff --git a/pkg/kubelet/config/file.go b/pkg/kubelet/config/file.go index 0bd370e8ed14b..ab8c9423a45e2 100644 --- a/pkg/kubelet/config/file.go +++ b/pkg/kubelet/config/file.go @@ -18,23 +18,17 @@ limitations under the License. package config import ( - "crypto/md5" - "encoding/hex" "fmt" "io/ioutil" "os" "path/filepath" "sort" - "strings" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" - "github.com/GoogleCloudPlatform/kubernetes/pkg/types" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" - "github.com/ghodss/yaml" "github.com/golang/glog" ) @@ -131,9 +125,7 @@ func extractFromDir(name string) ([]api.Pod, error) { return pods, nil } -func extractFromFile(filename string) (api.Pod, error) { - var pod api.Pod - +func extractFromFile(filename string) (pod api.Pod, err error) { glog.V(3).Infof("Reading config file %q", filename) file, err := os.Open(filename) if err != nil { @@ -146,65 +138,28 @@ func extractFromFile(filename string) (api.Pod, error) { return pod, err } - // TODO: use api.Scheme.DecodeInto - // This is awful. DecodeInto() expects to find an APIObject, which - // Manifest is not. We keep reading manifest for now for compat, but - // we will eventually change it to read Pod (at which point this all - // becomes nicer). Until then, we assert that the ContainerManifest - // structure on disk is always v1beta1. Read that, convert it to a - // "current" ContainerManifest (should be ~identical), then convert - // that to a Pod (which is a well-understood conversion). This - // avoids writing a v1beta1.ContainerManifest -> api.Pod - // conversion which would be identical to the api.ContainerManifest -> - // api.Pod conversion. - oldManifest := &v1beta1.ContainerManifest{} - if err := yaml.Unmarshal(data, oldManifest); err != nil { - return pod, fmt.Errorf("can't unmarshal file %q: %v", filename, err) - } - newManifest := &api.ContainerManifest{} - if err := api.Scheme.Convert(oldManifest, newManifest); err != nil { - return pod, fmt.Errorf("can't convert pod from file %q: %v", filename, err) - } - if err := api.Scheme.Convert(newManifest, &pod); err != nil { - return pod, fmt.Errorf("can't convert pod from file %q: %v", filename, err) + parsed, _, pod, manifestErr := tryDecodeSingleManifest(data) + if parsed { + if manifestErr != nil { + // It parsed but could not be used. + return pod, manifestErr + } + // It parsed! + if err = applyDefaults(&pod, filename, true); err != nil { + return pod, err + } + return pod, nil } - hostname, err := os.Hostname() //TODO: kubelet name would be better - if err != nil { - return pod, err - } - hostname = strings.ToLower(hostname) - - if len(pod.UID) == 0 { - hasher := md5.New() - fmt.Fprintf(hasher, "host:%s", hostname) - fmt.Fprintf(hasher, "file:%s", filename) - util.DeepHashObject(hasher, pod) - pod.UID = types.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) - } - // This is required for backward compatibility, and should be removed once we - // completely deprecate ContainerManifest. - if len(pod.Name) == 0 { - pod.Name = string(pod.UID) - } - if pod.Name, err = GeneratePodName(pod.Name); err != nil { - return pod, err + parsed, pod, podErr := tryDecodeSinglePod(data, filename, true) + if parsed { + if podErr != nil { + return pod, podErr + } + return pod, nil } - glog.V(5).Infof("Generated Name %q for UID %q from file %s", pod.Name, pod.UID, filename) - // Always overrides the namespace provided by the file. - pod.Namespace = kubelet.NamespaceDefault - glog.V(5).Infof("Using namespace %q for pod %q from file %s", pod.Namespace, pod.Name, filename) - - // Currently just simply follow the same format in resthandler.go - pod.ObjectMeta.SelfLink = fmt.Sprintf("/api/v1beta2/pods/%s?namespace=%s", - pod.Name, pod.Namespace) - - if glog.V(4) { - glog.Infof("Got pod from file %q: %#v", filename, pod) - } else { - glog.V(5).Infof("Got pod from file %q: %s.%s (%s)", filename, pod.Namespace, pod.Name, pod.UID) - } - return pod, nil + return pod, fmt.Errorf("%v: read '%v', but couldn't parse as neither "+ + "manifest (%v) nor pod (%v).\n", + filename, string(data), manifestErr, podErr) } diff --git a/pkg/kubelet/config/file_test.go b/pkg/kubelet/config/file_test.go index e287f36931722..eebbcb1576228 100644 --- a/pkg/kubelet/config/file_test.go +++ b/pkg/kubelet/config/file_test.go @@ -32,10 +32,17 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/types" ) +// TODO(wojtek-t): Most of the test cases are pretty similar and introduce +// the same boilerplate. Refactor them similarly to what is done in http_test.go + func ExampleManifestAndPod(id string) (v1beta1.ContainerManifest, api.Pod) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + manifest := v1beta1.ContainerManifest{ - ID: id, - UUID: types.UID(id), + Version: "v1beta1", + ID: id, + UUID: types.UID(id), Containers: []v1beta1.Container{ { Name: "c" + id, @@ -54,9 +61,10 @@ func ExampleManifestAndPod(id string) (v1beta1.ContainerManifest, api.Pod) { } expectedPod := api.Pod{ ObjectMeta: api.ObjectMeta{ - Name: id, + Name: id + "-" + hostname, UID: types.UID(id), Namespace: kubelet.NamespaceDefault, + SelfLink: "/api/v1beta2/pods/" + id + "-" + hostname + "?namespace=default", }, Spec: api.PodSpec{ Containers: []api.Container{ @@ -115,13 +123,16 @@ func writeTestFile(t *testing.T, dir, name string, contents string) *os.File { return file } -func TestReadFromFile(t *testing.T) { +func TestReadManifestFromFile(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + file := writeTestFile(t, os.TempDir(), "test_pod_config", `{ "version": "v1beta1", "uuid": "12345", "id": "test", - "containers": [{ "image": "test/image", imagePullPolicy: "PullAlways"}] + "containers": [{ "name": "image", "image": "test/image", imagePullPolicy: "PullAlways"}] }`) defer os.Remove(file.Name()) @@ -132,30 +143,14 @@ func TestReadFromFile(t *testing.T) { update := got.(kubelet.PodUpdate) expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ ObjectMeta: api.ObjectMeta{ - Name: "", + Name: "test-" + hostname, UID: "12345", Namespace: kubelet.NamespaceDefault, - SelfLink: "", + SelfLink: "/api/v1beta2/pods/test-" + hostname + "?namespace=default", }, Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, }) - if !strings.HasPrefix(update.Pods[0].Name, "test-") { - t.Errorf("Unexpected name: %s", update.Pods[0].Name) - } - // There's no way to provide namespace in ContainerManifest, so - // it will be defaulted. - if update.Pods[0].Namespace != kubelet.NamespaceDefault { - t.Errorf("Unexpected namespace: %s", update.Pods[0].Namespace) - } - // SelfLink depends on namespace. - if !strings.HasPrefix(update.Pods[0].SelfLink, "/api/") { - t.Errorf("Unexpected selflink: %s", update.Pods[0].SelfLink) - } - - // Reset the fileds that we don't want to compare. - update.Pods[0].Name = "" - update.Pods[0].SelfLink = "" if !api.Semantic.DeepDerivative(expected, update) { t.Fatalf("Expected %#v, Got %#v", expected, update) } @@ -165,12 +160,15 @@ func TestReadFromFile(t *testing.T) { } } -func TestReadFromFileWithoutID(t *testing.T) { +func TestReadManifestFromFileWithoutID(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + file := writeTestFile(t, os.TempDir(), "test_pod_config", `{ "version": "v1beta1", "uuid": "12345", - "containers": [{ "image": "test/image", imagePullPolicy: "PullAlways"}] + "containers": [{ "name": "image", "image": "test/image", imagePullPolicy: "PullAlways"}] }`) defer os.Remove(file.Name()) @@ -181,20 +179,14 @@ func TestReadFromFileWithoutID(t *testing.T) { update := got.(kubelet.PodUpdate) expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ ObjectMeta: api.ObjectMeta{ - Name: "", + Name: "12345-" + hostname, UID: "12345", Namespace: kubelet.NamespaceDefault, - SelfLink: "", + SelfLink: "/api/v1beta2/pods/12345-" + hostname + "?namespace=default", }, Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, }) - if len(update.Pods[0].ObjectMeta.Name) == 0 { - t.Errorf("Name did not get defaulted") - } - // Reset the fileds that we don't want to compare. - update.Pods[0].Name = "" - update.Pods[0].SelfLink = "" if !api.Semantic.DeepDerivative(expected, update) { t.Fatalf("Expected %#v, Got %#v", expected, update) } @@ -204,13 +196,16 @@ func TestReadFromFileWithoutID(t *testing.T) { } } -func TestReadV1Beta2FromFile(t *testing.T) { +func TestReadManifestV1Beta2FromFile(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + file := writeTestFile(t, os.TempDir(), "test_pod_config", `{ "version": "v1beta2", "uuid": "12345", "id": "test", - "containers": [{ "image": "test/image", imagePullPolicy: "PullAlways"}] + "containers": [{ "name": "image", "image": "test/image", imagePullPolicy: "PullAlways"}] }`) defer os.Remove(file.Name()) @@ -221,17 +216,14 @@ func TestReadV1Beta2FromFile(t *testing.T) { update := got.(kubelet.PodUpdate) expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ ObjectMeta: api.ObjectMeta{ - Name: "", + Name: "test-" + hostname, UID: "12345", Namespace: kubelet.NamespaceDefault, - SelfLink: "", + SelfLink: "/api/v1beta2/pods/test-" + hostname + "?namespace=default", }, Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, }) - // Reset the fileds that we don't want to compare. - update.Pods[0].Name = "" - update.Pods[0].SelfLink = "" if !api.Semantic.DeepDerivative(expected, update) { t.Fatalf("Expected %#v, Got %#v", expected, update) } @@ -241,12 +233,12 @@ func TestReadV1Beta2FromFile(t *testing.T) { } } -func TestReadFromFileWithDefaults(t *testing.T) { +func TestReadManifestFromFileWithDefaults(t *testing.T) { file := writeTestFile(t, os.TempDir(), "test_pod_config", `{ "version": "v1beta1", "id": "test", - "containers": [{ "image": "test/image" }] + "containers": [{ "name": "image", "image": "test/image" }] }`) defer os.Remove(file.Name()) @@ -264,6 +256,132 @@ func TestReadFromFileWithDefaults(t *testing.T) { } } +func TestReadPodFromFile(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + + file := writeTestFile(t, os.TempDir(), "test_pod_config", + `{ + "kind": "Pod", + "apiVersion": "v1beta1", + "uid": "12345", + "id": "test", + "namespace": "mynamespace", + "desiredState": { + "manifest": { + "containers": [{ "name": "image", "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) + expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "test-" + hostname, + UID: "12345", + Namespace: "mynamespace", + SelfLink: "/api/v1beta2/pods/test-" + hostname + "?namespace=mynamespace", + }, + Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, + }) + + if !api.Semantic.DeepDerivative(expected, update) { + t.Fatalf("Expected %#v, Got %#v", expected, update) + } + + case <-time.After(time.Second): + t.Errorf("Expected update, timeout instead") + } +} + +func TestReadPodV1Beta3FromFile(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + + file := writeTestFile(t, os.TempDir(), "test_pod_config", + `{ + "kind": "Pod", + "apiversion": "v1beta3", + "metadata": { + "uid": "12345", + "name": "test", + }, + "spec": { + "containers": [{ "name": "image", "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) + expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "test-" + hostname, + UID: "12345", + Namespace: kubelet.NamespaceDefault, + SelfLink: "/api/v1beta2/pods/test-" + hostname + "?namespace=default", + }, + Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, + }) + + if !api.Semantic.DeepDerivative(expected, update) { + t.Fatalf("Expected %#v, Got %#v", expected, update) + } + + case <-time.After(time.Second): + t.Errorf("Expected update, timeout instead") + } +} + +func TestReadPodFromFileWithoutID(t *testing.T) { + hostname, _ := os.Hostname() + hostname = strings.ToLower(hostname) + + file := writeTestFile(t, os.TempDir(), "test_pod_config", + `{ + "kind": "Pod", + "apiversion": "v1beta1", + "uid": "12345", + "DesiredState": { + "Manifest": { + "containers": [{ "name": "image", "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) + expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "12345-" + hostname, + UID: "12345", + Namespace: kubelet.NamespaceDefault, + SelfLink: "/api/v1beta2/pods/12345-" + hostname + "?namespace=default", + }, + Spec: api.PodSpec{Containers: []api.Container{{Image: "test/image"}}}, + }) + + if !api.Semantic.DeepDerivative(expected, update) { + t.Fatalf("Expected %#v, Got %#v", expected, update) + } + + case <-time.After(time.Second): + 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()) @@ -339,18 +457,7 @@ func TestExtractFromDir(t *testing.T) { } update := (<-ch).(kubelet.PodUpdate) - for i := range update.Pods { - // Pod name is generated with hash and is unique. Skip the comparision - // here by setting it to a simple value. - update.Pods[i].Name = manifests[i].ID - update.Pods[i].SelfLink = "" - } expected := CreatePodUpdate(kubelet.SET, kubelet.FileSource, pods...) - for i := range expected.Pods { - // Pod name is generated with hash and is unique. Skip the comparision - // here by setting it to a simple value. - expected.Pods[i].Name = manifests[i].ID - } sort.Sort(sortedPods(update.Pods)) sort.Sort(sortedPods(expected.Pods)) if !api.Semantic.DeepDerivative(expected, update) { diff --git a/pkg/kubelet/config/http.go b/pkg/kubelet/config/http.go index fb573f35387e4..bb9e95f311b27 100644 --- a/pkg/kubelet/config/http.go +++ b/pkg/kubelet/config/http.go @@ -25,12 +25,9 @@ import ( "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" - "github.com/ghodss/yaml" "github.com/golang/glog" ) @@ -81,7 +78,7 @@ func (s *sourceURL) extractFromURL() error { s.data = data // First try as if it's a single manifest - parsed, manifest, pod, singleErr := tryDecodeSingle(data) + parsed, manifest, pod, singleErr := tryDecodeSingleManifest(data) if parsed { if singleErr != nil { // It parsed but could not be used. @@ -96,7 +93,7 @@ func (s *sourceURL) extractFromURL() error { } // That didn't work, so try an array of manifests. - parsed, manifests, pods, multiErr := tryDecodeList(data) + parsed, manifests, pods, multiErr := tryDecodeManifestList(data) if parsed { if multiErr != nil { // It parsed but could not be used. @@ -151,58 +148,3 @@ func (s *sourceURL) extractFromURL() error { s.url, string(data), singleErr, manifest, multiErr, manifests, singlePodErr, multiPodErr) } - -func tryDecodeSingle(data []byte) (parsed bool, manifest v1beta1.ContainerManifest, pod api.Pod, err error) { - // TODO: should be api.Scheme.Decode - // This is awful. DecodeInto() expects to find an APIObject, which - // Manifest is not. We keep reading manifest for now for compat, but - // we will eventually change it to read Pod (at which point this all - // becomes nicer). Until then, we assert that the ContainerManifest - // structure on disk is always v1beta1. Read that, convert it to a - // "current" ContainerManifest (should be ~identical), then convert - // that to a Pod (which is a well-understood conversion). This - // avoids writing a v1beta1.ContainerManifest -> api.Pod - // conversion which would be identical to the api.ContainerManifest -> - // api.Pod conversion. - if err = yaml.Unmarshal(data, &manifest); err != nil { - return false, manifest, pod, err - } - newManifest := api.ContainerManifest{} - if err = api.Scheme.Convert(&manifest, &newManifest); err != nil { - return false, manifest, pod, err - } - if errs := validation.ValidateManifest(&newManifest); len(errs) > 0 { - err = fmt.Errorf("invalid manifest: %v", errs) - return false, manifest, pod, err - } - if err = api.Scheme.Convert(&newManifest, &pod); err != nil { - return true, manifest, pod, err - } - // Success. - return true, manifest, pod, nil -} - -func tryDecodeList(data []byte) (parsed bool, manifests []v1beta1.ContainerManifest, pods api.PodList, err error) { - // TODO: should be api.Scheme.Decode - // See the comment in tryDecodeSingle(). - if err = yaml.Unmarshal(data, &manifests); err != nil { - return false, manifests, pods, err - } - newManifests := []api.ContainerManifest{} - if err = api.Scheme.Convert(&manifests, &newManifests); err != nil { - return false, manifests, pods, err - } - for i := range newManifests { - manifest := &newManifests[i] - if errs := validation.ValidateManifest(manifest); len(errs) > 0 { - err = fmt.Errorf("invalid manifest: %v", errs) - return false, manifests, pods, err - } - } - list := api.ContainerManifestList{Items: newManifests} - if err = api.Scheme.Convert(&list, &pods); err != nil { - return true, manifests, pods, err - } - // Success. - return true, manifests, pods, nil -}