Merge pull request #3587 from dchen1107/image

Clean up Kubernetes PullPolicy

pkg/api/helpers.go

@@ -18,7 +18,6 @@ package api
 
 import (
 	"reflect"
-	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
@@ -60,27 +59,4 @@ var Semantic = conversion.EqualitiesOrDie(
 		}
 		return a.Amount.Cmp(b.Amount) == 0
 	},
-	pullPoliciesEqual,
 )
-
-// TODO: Address these per #1502
-
-func IsPullAlways(p PullPolicy) bool {
-	return pullPoliciesEqual(p, PullAlways)
-}
-
-func IsPullNever(p PullPolicy) bool {
-	return pullPoliciesEqual(p, PullNever)
-}
-
-func IsPullIfNotPresent(p PullPolicy) bool {
-	// Default to pull if not present
-	if len(p) == 0 {
-		return true
-	}
-	return pullPoliciesEqual(p, PullIfNotPresent)
-}
-
-func pullPoliciesEqual(p1, p2 PullPolicy) bool {
-	return strings.ToLower(string(p1)) == strings.ToLower(string(p2))
-}

pkg/api/helpers_test.go

@@ -59,8 +59,6 @@ func TestSemantic(t *testing.T) {
 			true,
 		},
 		{resource.MustParse("2m"), resource.MustParse("1m"), false},
-		{PullPolicy("NEVER"), PullPolicy("neveR"), true},
-		{PullPolicy("NEVER"), PullPolicy("neveRi"), false},
 	}
 
 	for index, item := range table {

pkg/api/validation/validation.go

@@ -266,6 +266,29 @@ func validateLifecycle(lifecycle *api.Lifecycle) errs.ValidationErrorList {
 	return allErrs
 }
 
+// TODO(dchen1107): Move this along with other defaulting values
+func validatePullPolicyWithDefault(ctr *api.Container) errs.ValidationErrorList {
+	allErrors := errs.ValidationErrorList{}
+
+	// TODO(dchen1107): Move ParseImageName code to pkg/util
+	if len(ctr.ImagePullPolicy) == 0 {
+		parts := strings.Split(ctr.Image, ":")
+		// Check image tag
+		if parts[len(parts)-1] == "latest" {
+			ctr.ImagePullPolicy = api.PullAlways
+		} else {
+			ctr.ImagePullPolicy = api.PullIfNotPresent
+		}
+	}
+	if ctr.ImagePullPolicy != api.PullAlways &&
+		ctr.ImagePullPolicy != api.PullIfNotPresent &&
+		ctr.ImagePullPolicy != api.PullNever {
+		allErrors = append(allErrors, errs.NewFieldNotSupported("", ctr.ImagePullPolicy))
+	}
+
+	return allErrors
+}
+
 func validateContainers(containers []api.Container, volumes util.StringSet) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 
@@ -294,6 +317,7 @@ func validateContainers(containers []api.Container, volumes util.StringSet) errs
 		cErrs = append(cErrs, validatePorts(ctr.Ports).Prefix("ports")...)
 		cErrs = append(cErrs, validateEnv(ctr.Env).Prefix("env")...)
 		cErrs = append(cErrs, validateVolumeMounts(ctr.VolumeMounts, volumes).Prefix("volumeMounts")...)
+		cErrs = append(cErrs, validatePullPolicyWithDefault(ctr).Prefix("pullPolicy")...)
 		allErrs = append(allErrs, cErrs.PrefixIndex(i)...)
 	}
 	// Check for colliding ports across all containers.

pkg/api/validation/validation_test.go

@@ -220,6 +220,54 @@ func TestValidateVolumeMounts(t *testing.T) {
 	}
 }
 
+func TestValidatePullPolicy(t *testing.T) {
+	type T struct {
+		Container      api.Container
+		ExpectedPolicy api.PullPolicy
+	}
+	testCases := map[string]T{
+		"NotPresent1": {
+			api.Container{Name: "abc", Image: "image:latest", ImagePullPolicy: "PullIfNotPresent"},
+			api.PullIfNotPresent,
+		},
+		"NotPresent2": {
+			api.Container{Name: "abc1", Image: "image", ImagePullPolicy: "PullIfNotPresent"},
+			api.PullIfNotPresent,
+		},
+		"Always1": {
+			api.Container{Name: "123", Image: "image:latest", ImagePullPolicy: "PullAlways"},
+			api.PullAlways,
+		},
+		"Always2": {
+			api.Container{Name: "1234", Image: "image", ImagePullPolicy: "PullAlways"},
+			api.PullAlways,
+		},
+		"Never1": {
+			api.Container{Name: "abc-123", Image: "image:latest", ImagePullPolicy: "PullNever"},
+			api.PullNever,
+		},
+		"Never2": {
+			api.Container{Name: "abc-1234", Image: "image", ImagePullPolicy: "PullNever"},
+			api.PullNever,
+		},
+		"DefaultToNotPresent":  {api.Container{Name: "notPresent", Image: "image"}, api.PullIfNotPresent},
+		"DefaultToNotPresent2": {api.Container{Name: "notPresent1", Image: "image:sometag"}, api.PullIfNotPresent},
+		"DefaultToAlways1":     {api.Container{Name: "always", Image: "image:latest"}, api.PullAlways},
+		"DefaultToAlways2":     {api.Container{Name: "always", Image: "foo.bar.com:5000/my/image:latest"}, api.PullAlways},
+	}
+	for k, v := range testCases {
+		ctr := &v.Container
+		errs := validatePullPolicyWithDefault(ctr)
+		if len(errs) != 0 {
+			t.Errorf("case[%s] expected success, got %#v", k, errs)
+		}
+		if ctr.ImagePullPolicy != v.ExpectedPolicy {
+			t.Errorf("case[%s] expected policy %v, got %v", k, v.ExpectedPolicy, ctr.ImagePullPolicy)
+		}
+	}
+
+}
+
 func TestValidateContainers(t *testing.T) {
 	volumes := util.StringSet{}
 	capabilities.SetForTests(capabilities.Capabilities{

pkg/kubelet/config/http_test.go

@@ -156,7 +156,8 @@ func TestExtractFromHTTP(t *testing.T) {
 						Containers: []api.Container{{
 							Name:  "1",
 							Image: "foo",
-							TerminationMessagePath: "/dev/termination-log"}},
+							TerminationMessagePath: "/dev/termination-log",
+							ImagePullPolicy:        "PullIfNotPresent"}},
 					},
 				},
 				api.BoundPod{
@@ -169,7 +170,8 @@ func TestExtractFromHTTP(t *testing.T) {
 						Containers: []api.Container{{
 							Name:  "1",
 							Image: "foo",
-							TerminationMessagePath: "/dev/termination-log"}},
+							TerminationMessagePath: "/dev/termination-log",
+							ImagePullPolicy:        "PullIfNotPresent"}},
 					},
 				}),
 		},

pkg/kubelet/kubelet.go

@@ -1095,9 +1095,8 @@ func (kl *Kubelet) syncPod(pod *api.BoundPod, dockerContainers dockertools.Docke
 		if err != nil {
 			glog.Errorf("Couldn't make a ref to pod %v, container %v: '%v'", pod.Name, container.Name, err)
 		}
-		if !api.IsPullNever(container.ImagePullPolicy) {
+		if container.ImagePullPolicy != api.PullNever {
 			present, err := kl.dockerPuller.IsImagePresent(container.Image)
-			latest := dockertools.RequireLatestImage(container.Image)
 			if err != nil {
 				if ref != nil {
 					record.Eventf(ref, "failed", "Failed to inspect image %q", container.Image)
@@ -1105,8 +1104,8 @@ func (kl *Kubelet) syncPod(pod *api.BoundPod, dockerContainers dockertools.Docke
 				glog.Errorf("Failed to inspect image %q: %v; skipping pod %q container %q", container.Image, err, podFullName, container.Name)
 				continue
 			}
-			if api.IsPullAlways(container.ImagePullPolicy) ||
+			if container.ImagePullPolicy == api.PullAlways ||
-				(api.IsPullIfNotPresent(container.ImagePullPolicy) && (!present || latest)) {
+				(container.ImagePullPolicy == api.PullIfNotPresent && (!present)) {
 				if err := kl.pullImage(container.Image, ref); err != nil {
 					continue
 				}

pkg/kubelet/kubelet_test.go

@@ -487,7 +487,7 @@ func TestSyncPodsCreatesNetAndContainerPullsImage(t *testing.T) {
 			},
 			Spec: api.PodSpec{
 				Containers: []api.Container{
-					{Name: "bar"},
+					{Name: "bar", Image: "something", ImagePullPolicy: "PullIfNotPresent"},
 				},
 			},
 		},
@@ -502,7 +502,7 @@ func TestSyncPodsCreatesNetAndContainerPullsImage(t *testing.T) {
 
 	fakeDocker.Lock()
 
-	if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", ""}) {
+	if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "something"}) {
 		t.Errorf("Unexpected pulled containers: %v", puller.ImagesPulled)
 	}
 
@@ -1924,7 +1924,7 @@ func TestSyncPodsWithPullPolicy(t *testing.T) {
 
 	fakeDocker.Lock()
 
-	if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "pull_always_image", "pull_if_not_present_image", "want:latest"}) {
+	if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "pull_always_image", "pull_if_not_present_image"}) {
 		t.Errorf("Unexpected pulled containers: %v", puller.ImagesPulled)
 	}