From 15cab4d0531b5e2e75c3addb0b5e41fb14e44af8 Mon Sep 17 00:00:00 2001 From: Dawn Chen Date: Tue, 26 Aug 2014 11:25:17 -0700 Subject: [PATCH] Introduce the simplest RestartPolicy and handling. --- pkg/api/types.go | 32 +++++++++-------- pkg/api/v1beta1/types.go | 34 +++++++++--------- pkg/api/validation/validation.go | 30 +++++++++++----- pkg/api/validation/validation_test.go | 51 ++++++++++++++++++++++++--- pkg/kubelet/config/config_test.go | 3 +- pkg/kubelet/config/http_test.go | 8 +++-- pkg/kubelet/docker.go | 27 ++++++++++++++ pkg/kubelet/kubelet.go | 26 +++++++++++++- pkg/kubelet/kubelet_test.go | 14 ++++---- 9 files changed, 171 insertions(+), 54 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index 88827128df4..385f1b2b22f 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -58,9 +58,10 @@ type ContainerManifest struct { // TODO: UUID on Manifest is deprecated in the future once we are done // with the API refactoring. It is required for now to determine the instance // of a Pod. - UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` - Volumes []Volume `yaml:"volumes" json:"volumes"` - Containers []Container `yaml:"containers" json:"containers"` + UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` + Volumes []Volume `yaml:"volumes" json:"volumes"` + Containers []Container `yaml:"containers" json:"containers"` + RestartPolicy RestartPolicy `json:"restartPolicy,omitempty" yaml:"restartPolicy,omitempty"` } // ContainerManifestList is used to communicate container manifests to kubelet. @@ -254,19 +255,21 @@ const ( // PodInfo contains one entry for every container with available info. type PodInfo map[string]docker.Container -// RestartPolicyType represents a restart policy for a pod. -type RestartPolicyType string +type RestartPolicyAlways struct{} -// Valid restart policies defined for a PodState.RestartPolicy. -const ( - RestartAlways RestartPolicyType = "RestartAlways" - RestartOnFailure RestartPolicyType = "RestartOnFailure" - RestartNever RestartPolicyType = "RestartNever" -) +// TODO(dchen1107): Define what kinds of failures should restart. +// TODO(dchen1107): Decide whether to support policy knobs, and, if so, which ones. +type RestartPolicyOnFailure struct{} + +type RestartPolicyNever struct{} type RestartPolicy struct { - // Optional: Defaults to "RestartAlways". - Type RestartPolicyType `yaml:"type,omitempty" json:"type,omitempty"` + // Only one of the following restart policies may be specified. + // If none of the following policies is specified, the default one + // is RestartPolicyAlways. + Always *RestartPolicyAlways `json:"always,omitempty" yaml:"always,omitempty"` + OnFailure *RestartPolicyOnFailure `json:"onFailure,omitempty" yaml:"onFailure,omitempty"` + Never *RestartPolicyNever `json:"never,omitempty" yaml:"never,omitempty"` } // PodState is the state of a pod, used as either input (desired state) or output (current state). @@ -283,8 +286,7 @@ type PodState struct { // upon. // TODO: Make real decisions about what our info should look like. Re-enable fuzz test // when we have done this. - Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` - RestartPolicy RestartPolicy `json:"restartpolicy,omitempty" yaml:"restartpolicy,omitempty"` + Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` } // PodList is a list of Pods. diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index bc2a9c34fe7..f3f02480dda 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -17,10 +17,10 @@ limitations under the License. package v1beta1 import ( - "github.com/fsouza/go-dockerclient" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" + "github.com/fsouza/go-dockerclient" ) // Common string formats @@ -58,9 +58,10 @@ type ContainerManifest struct { // TODO: UUID on Manifext is deprecated in the future once we are done // with the API refactory. It is required for now to determine the instance // of a Pod. - UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` - Volumes []Volume `yaml:"volumes" json:"volumes"` - Containers []Container `yaml:"containers" json:"containers"` + UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` + Volumes []Volume `yaml:"volumes" json:"volumes"` + Containers []Container `yaml:"containers" json:"containers"` + RestartPolicy RestartPolicy `json:"restartPolicy,omitempty" yaml:"restartPolicy,omitempty"` } // ContainerManifestList is used to communicate container manifests to kubelet. @@ -267,19 +268,21 @@ const ( // PodInfo contains one entry for every container with available info. type PodInfo map[string]docker.Container -// RestartPolicyType represents a restart policy for a pod. -type RestartPolicyType string +type RestartPolicyAlways struct{} -// Valid restart policies defined for a PodState.RestartPolicy. -const ( - RestartAlways RestartPolicyType = "RestartAlways" - RestartOnFailure RestartPolicyType = "RestartOnFailure" - RestartNever RestartPolicyType = "RestartNever" -) +// TODO(dchen1107): Define what kinds of failures should restart +// TODO(dchen1107): Decide whether to support policy knobs, and, if so, which ones. +type RestartPolicyOnFailure struct{} + +type RestartPolicyNever struct{} type RestartPolicy struct { - // Optional: Defaults to "RestartAlways". - Type RestartPolicyType `yaml:"type,omitempty" json:"type,omitempty"` + // Only one of the following restart policy may be specified. + // If none of the following policies is specified, the default one + // is RestartPolicyAlways. + Always *RestartPolicyAlways `json:"always,omitempty" yaml:"always,omitempty"` + OnFailure *RestartPolicyOnFailure `json:"onFailure,omitempty" yaml:"onFailure,omitempty"` + Never *RestartPolicyNever `json:"never,omitempty" yaml:"never,omitempty"` } // PodState is the state of a pod, used as either input (desired state) or output (current state). @@ -296,8 +299,7 @@ type PodState struct { // upon. To allow marshalling/unmarshalling, we copied the client's structs and added // json/yaml tags. // TODO: Make real decisions about what our info should look like. - Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` - RestartPolicy RestartPolicy `json:"restartpolicy,omitempty" yaml:"restartpolicy,omitempty"` + Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` } // PodList is a list of Pods. diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index e362699e1e4..3115150b805 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -231,19 +231,33 @@ func ValidateManifest(manifest *api.ContainerManifest) errs.ErrorList { allVolumes, errs := validateVolumes(manifest.Volumes) allErrs = append(allErrs, errs.Prefix("volumes")...) allErrs = append(allErrs, validateContainers(manifest.Containers, allVolumes).Prefix("containers")...) + allErrs = append(allErrs, validateRestartPolicy(&manifest.RestartPolicy).Prefix("restartPolicy")...) return allErrs } +func validateRestartPolicy(restartPolicy *api.RestartPolicy) errs.ErrorList { + numPolicies := 0 + allErrors := errs.ErrorList{} + if restartPolicy.Always != nil { + numPolicies++ + } + if restartPolicy.OnFailure != nil { + numPolicies++ + } + if restartPolicy.Never != nil { + numPolicies++ + } + if numPolicies == 0 { + restartPolicy.Always = &api.RestartPolicyAlways{} + } + if numPolicies > 1 { + allErrors = append(allErrors, errs.NewFieldInvalid("", restartPolicy)) + } + return allErrors +} + func ValidatePodState(podState *api.PodState) errs.ErrorList { allErrs := errs.ErrorList(ValidateManifest(&podState.Manifest)).Prefix("manifest") - if podState.RestartPolicy.Type == "" { - podState.RestartPolicy.Type = api.RestartAlways - } else if podState.RestartPolicy.Type != api.RestartAlways && - podState.RestartPolicy.Type != api.RestartOnFailure && - podState.RestartPolicy.Type != api.RestartNever { - allErrs = append(allErrs, errs.NewFieldNotSupported("restartPolicy.type", podState.RestartPolicy.Type)) - } - return allErrs } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 0b4d07fab9b..1027577dbfd 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -216,6 +216,40 @@ func TestValidateContainers(t *testing.T) { } } +func TestValidateRestartPolicy(t *testing.T) { + successCases := []api.RestartPolicy{ + {}, + {Always: &api.RestartPolicyAlways{}}, + {OnFailure: &api.RestartPolicyOnFailure{}}, + {Never: &api.RestartPolicyNever{}}, + } + for _, policy := range successCases { + if errs := validateRestartPolicy(&policy); len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + } + + errorCases := []api.RestartPolicy{ + {Always: &api.RestartPolicyAlways{}, Never: &api.RestartPolicyNever{}}, + {Never: &api.RestartPolicyNever{}, OnFailure: &api.RestartPolicyOnFailure{}}, + } + for k, policy := range errorCases { + if errs := validateRestartPolicy(&policy); len(errs) == 0 { + t.Errorf("expected failure for %s", k) + } + } + + noPolicySpecified := api.RestartPolicy{} + errs := validateRestartPolicy(&noPolicySpecified) + if len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } + if noPolicySpecified.Always == nil { + t.Errorf("expected Always policy specified") + } + +} + func TestValidateManifest(t *testing.T) { successCases := []api.ContainerManifest{ {Version: "v1beta1", ID: "abc"}, @@ -286,8 +320,13 @@ func TestValidatePod(t *testing.T) { "foo": "bar", }, DesiredState: api.PodState{ - Manifest: api.ContainerManifest{Version: "v1beta1", ID: "abc"}, - RestartPolicy: api.RestartPolicy{Type: "RestartAlways"}, + Manifest: api.ContainerManifest{ + Version: "v1beta1", + ID: "abc", + RestartPolicy: api.RestartPolicy{ + Always: &api.RestartPolicyAlways{}, + }, + }, }, }) if len(errs) != 0 { @@ -312,8 +351,12 @@ func TestValidatePod(t *testing.T) { "foo": "bar", }, DesiredState: api.PodState{ - Manifest: api.ContainerManifest{Version: "v1beta1", ID: "abc"}, - RestartPolicy: api.RestartPolicy{Type: "WhatEver"}, + Manifest: api.ContainerManifest{ + Version: "v1beta1", + ID: "abc", + RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}, + Never: &api.RestartPolicyNever{}}, + }, }, }) if len(errs) != 1 { diff --git a/pkg/kubelet/config/config_test.go b/pkg/kubelet/config/config_test.go index 52c1faf0eb7..05d85386255 100644 --- a/pkg/kubelet/config/config_test.go +++ b/pkg/kubelet/config/config_test.go @@ -52,7 +52,8 @@ func CreateValidPod(name, namespace string) kubelet.Pod { Name: name, Namespace: namespace, Manifest: api.ContainerManifest{ - Version: "v1beta1", + Version: "v1beta1", + RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, }, } } diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index 6bf466c0441..992f25b6dc1 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -105,8 +105,12 @@ func TestExtractFromHTTP(t *testing.T) { manifests: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, expected: CreatePodUpdate(kubelet.SET, kubelet.Pod{ - Name: "foo", - Manifest: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, + Name: "foo", + Manifest: api.ContainerManifest{ + Version: "v1beta1", + ID: "foo", + RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, + }, }), }, { diff --git a/pkg/kubelet/docker.go b/pkg/kubelet/docker.go index 46cdb788752..cc701ed8848 100644 --- a/pkg/kubelet/docker.go +++ b/pkg/kubelet/docker.go @@ -153,6 +153,33 @@ func getKubeletDockerContainers(client DockerInterface) (DockerContainers, error return result, nil } +// getRecentDockerContainersWithName returns a list of dead docker containers which matches the name +// and uuid given. +func getRecentDockerContainersWithNameAndUUID(client DockerInterface, podFullName, uuid, containerName string) ([]*docker.Container, error) { + var result []*docker.Container + containers, err := client.ListContainers(docker.ListContainersOptions{All: true}) + if err != nil { + return nil, err + } + for _, dockerContainer := range containers { + dockerPodName, dockerUUID, dockerContainerName, _ := parseDockerName(dockerContainer.Names[0]) + if dockerPodName != podFullName { + continue + } + if uuid != "" && dockerUUID != uuid { + continue + } + if dockerContainerName != containerName { + continue + } + inspectResult, _ := client.InspectContainer(dockerContainer.ID) + if inspectResult != nil && !inspectResult.State.Running && !inspectResult.State.Paused { + result = append(result, inspectResult) + } + } + return result, nil +} + // ErrNoContainersInPod is returned when there are no running containers for a given pod var ErrNoContainersInPod = errors.New("no containers exist for this pod") diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 9ff13f3ada2..8bfd8f75c49 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -516,11 +516,35 @@ func (kl *Kubelet) syncPod(pod *Pod, dockerContainers DockerContainers) error { killedContainers[containerID] = empty{} } - glog.Infof("Container doesn't exist, creating %#v", container) + // Check RestartPolicy for container + recentContainers, err := getRecentDockerContainersWithNameAndUUID(kl.dockerClient, podFullName, uuid, container.Name) + if err != nil { + glog.Errorf("Error listing recent containers with name and uuid:%s--%s--%s", podFullName, uuid, container.Name) + // TODO(dawnchen): error handling here? + } + + if len(recentContainers) > 0 && pod.Manifest.RestartPolicy.Always == nil { + if pod.Manifest.RestartPolicy.Never != nil { + glog.Infof("Already ran container with name %s--%s--%s, do nothing", + podFullName, uuid, container.Name) + continue + } + if pod.Manifest.RestartPolicy.OnFailure != nil { + // Check the exit code of last run + if recentContainers[0].State.ExitCode == 0 { + glog.Infof("Already successfully ran container with name %s--%s--%s, do nothing", + podFullName, uuid, container.Name) + continue + } + } + } + + glog.Infof("Container with name %s--%s--%s doesn't exist, creating %#v", podFullName, uuid, container.Name, container) if err := kl.dockerPuller.Pull(container.Image); err != nil { glog.Errorf("Failed to pull image %s: %v skipping pod %s container %s.", container.Image, err, podFullName, container.Name) continue } + // TODO(dawnchen): Check RestartPolicy.DelaySeconds before restart a container containerID, err := kl.runContainer(pod, &container, podVolumes, "container:"+string(netID)) if err != nil { // TODO(bburns) : Perhaps blacklist a container after N failures? diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index bc9dbee5997..dd88466f757 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -300,7 +300,7 @@ func TestSyncPodsCreatesNetAndContainer(t *testing.T) { kubelet.drainWorkers() verifyCalls(t, fakeDocker, []string{ - "list", "list", "create", "start", "list", "inspect", "create", "start"}) + "list", "list", "create", "start", "list", "inspect", "list", "create", "start"}) fakeDocker.lock.Lock() if len(fakeDocker.Created) != 2 || @@ -338,7 +338,7 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) { kubelet.drainWorkers() verifyCalls(t, fakeDocker, []string{ - "list", "list", "list", "inspect", "create", "start"}) + "list", "list", "list", "inspect", "list", "create", "start"}) fakeDocker.lock.Lock() if len(fakeDocker.Created) != 1 || @@ -388,7 +388,7 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) { kubelet.drainWorkers() verifyCalls(t, fakeDocker, []string{ - "list", "list", "list", "inspect", "create", "start"}) + "list", "list", "list", "inspect", "list", "create", "start"}) fakeDocker.lock.Lock() if len(fakeDocker.Created) != 1 || @@ -428,7 +428,7 @@ func TestSyncPodsDeletesWithNoNetContainer(t *testing.T) { kubelet.drainWorkers() verifyCalls(t, fakeDocker, []string{ - "list", "list", "stop", "create", "start", "list", "list", "inspect", "create", "start"}) + "list", "list", "stop", "create", "start", "list", "list", "inspect", "list", "create", "start"}) // A map iteration is used to delete containers, so must not depend on // order here. @@ -561,7 +561,7 @@ func TestSyncPodBadHash(t *testing.T) { t.Errorf("unexpected error: %v", err) } - verifyCalls(t, fakeDocker, []string{"list", "stop", "create", "start"}) + verifyCalls(t, fakeDocker, []string{"list", "stop", "list", "create", "start"}) // A map interation is used to delete containers, so must not depend on // order here. @@ -608,7 +608,7 @@ func TestSyncPodUnhealthy(t *testing.T) { t.Errorf("unexpected error: %v", err) } - verifyCalls(t, fakeDocker, []string{"list", "stop", "create", "start"}) + verifyCalls(t, fakeDocker, []string{"list", "stop", "list", "create", "start"}) // A map interation is used to delete containers, so must not depend on // order here. @@ -1329,7 +1329,7 @@ func TestSyncPodEventHandlerFails(t *testing.T) { t.Errorf("unexpected error: %v", err) } - verifyCalls(t, fakeDocker, []string{"list", "create", "start", "stop"}) + verifyCalls(t, fakeDocker, []string{"list", "list", "create", "start", "stop"}) if len(fakeDocker.stopped) != 1 { t.Errorf("Wrong containers were stopped: %v", fakeDocker.stopped)