diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 3bd87102e7..f2ce548ae8 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -1485,17 +1485,56 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) field.ErrorList { allErrs = append(allErrs, field.Forbidden(specPath.Child("containers"), "pod updates may not add or remove containers")) return allErrs } - pod := *newPod - // Tricky, we need to copy the container list so that we don't overwrite the update + + // validate updateable fields: + // 1. containers[*].image + // 2. spec.activeDeadlineSeconds + + // validate updated container images + for i, ctr := range newPod.Spec.Containers { + if len(ctr.Image) == 0 { + allErrs = append(allErrs, field.Required(specPath.Child("containers").Index(i).Child("image"), "")) + } + } + + // validate updated spec.activeDeadlineSeconds. two types of updates are allowed: + // 1. from nil to a positive value + // 2. from a positive value to a lesser, non-negative value + if newPod.Spec.ActiveDeadlineSeconds != nil { + newActiveDeadlineSeconds := *newPod.Spec.ActiveDeadlineSeconds + if newActiveDeadlineSeconds < 0 { + allErrs = append(allErrs, field.Invalid(specPath.Child("activeDeadlineSeconds"), newActiveDeadlineSeconds, isNegativeErrorMsg)) + return allErrs + } + if oldPod.Spec.ActiveDeadlineSeconds != nil { + oldActiveDeadlineSeconds := *oldPod.Spec.ActiveDeadlineSeconds + if oldActiveDeadlineSeconds < newActiveDeadlineSeconds { + allErrs = append(allErrs, field.Invalid(specPath.Child("activeDeadlineSeconds"), newActiveDeadlineSeconds, "must be less than or equal to previous value")) + return allErrs + } + } + } else if oldPod.Spec.ActiveDeadlineSeconds != nil { + allErrs = append(allErrs, field.Invalid(specPath.Child("activeDeadlineSeconds"), newPod.Spec.ActiveDeadlineSeconds, "must not update from a positive integer to nil value")) + } + + // handle updateable fields by munging those fields prior to deep equal comparison. + mungedPod := *newPod + // munge containers[*].image var newContainers []api.Container - for ix, container := range pod.Spec.Containers { + for ix, container := range mungedPod.Spec.Containers { container.Image = oldPod.Spec.Containers[ix].Image newContainers = append(newContainers, container) } - pod.Spec.Containers = newContainers - if !api.Semantic.DeepEqual(pod.Spec, oldPod.Spec) { + mungedPod.Spec.Containers = newContainers + // munge spec.activeDeadlineSeconds + mungedPod.Spec.ActiveDeadlineSeconds = nil + if oldPod.Spec.ActiveDeadlineSeconds != nil { + activeDeadlineSeconds := *oldPod.Spec.ActiveDeadlineSeconds + mungedPod.Spec.ActiveDeadlineSeconds = &activeDeadlineSeconds + } + if !api.Semantic.DeepEqual(mungedPod.Spec, oldPod.Spec) { //TODO: Pinpoint the specific field that causes the invalid error after we have strategic merge diff - allErrs = append(allErrs, field.Forbidden(specPath, "pod updates may not change fields other than `containers[*].image`")) + allErrs = append(allErrs, field.Forbidden(specPath, "pod updates may not change fields other than `containers[*].image` or `spec.activeDeadlineSeconds`")) } return allErrs diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 579e333476..cfba35f0d0 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -1924,9 +1924,17 @@ func TestValidatePod(t *testing.T) { } func TestValidatePodUpdate(t *testing.T) { - now := unversioned.Now() - grace := int64(30) - grace2 := int64(31) + var ( + activeDeadlineSecondsZero = int64(0) + activeDeadlineSecondsNegative = int64(-30) + activeDeadlineSecondsPositive = int64(30) + activeDeadlineSecondsLarger = int64(31) + + now = unversioned.Now() + grace = int64(30) + grace2 = int64(31) + ) + tests := []struct { a api.Pod b api.Pod @@ -2061,6 +2069,150 @@ func TestValidatePodUpdate(t *testing.T) { true, "image change", }, + { + api.Pod{ + ObjectMeta: api.ObjectMeta{Name: "foo"}, + Spec: api.PodSpec{ + Containers: []api.Container{ + {}, + }, + }, + }, + api.Pod{ + ObjectMeta: api.ObjectMeta{Name: "foo"}, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Image: "foo:V2", + }, + }, + }, + }, + false, + "image change to empty", + }, + { + api.Pod{ + Spec: api.PodSpec{}, + }, + api.Pod{ + Spec: api.PodSpec{}, + }, + true, + "activeDeadlineSeconds no change, nil", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + true, + "activeDeadlineSeconds no change, set", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + api.Pod{}, + true, + "activeDeadlineSeconds change to positive from nil", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsLarger, + }, + }, + true, + "activeDeadlineSeconds change to smaller positive", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsLarger, + }, + }, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + false, + "activeDeadlineSeconds change to larger positive", + }, + + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsNegative, + }, + }, + api.Pod{}, + false, + "activeDeadlineSeconds change to negative from nil", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsNegative, + }, + }, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + false, + "activeDeadlineSeconds change to negative from positive", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsZero, + }, + }, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + true, + "activeDeadlineSeconds change to zero from positive", + }, + { + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsZero, + }, + }, + api.Pod{}, + true, + "activeDeadlineSeconds change to zero from nil", + }, + { + api.Pod{}, + api.Pod{ + Spec: api.PodSpec{ + ActiveDeadlineSeconds: &activeDeadlineSecondsPositive, + }, + }, + false, + "activeDeadlineSeconds change to nil from positive", + }, + { api.Pod{ ObjectMeta: api.ObjectMeta{Name: "foo"}, @@ -2149,11 +2301,11 @@ func TestValidatePodUpdate(t *testing.T) { errs := ValidatePodUpdate(&test.a, &test.b) if test.isValid { if len(errs) != 0 { - t.Errorf("unexpected invalid: %s %v, %v", test.test, test.a, test.b) + t.Errorf("unexpected invalid: %s (%+v)\nA: %+v\nB: %+v", test.test, errs, test.a, test.b) } } else { if len(errs) == 0 { - t.Errorf("unexpected valid: %s %v, %v", test.test, test.a, test.b) + t.Errorf("unexpected valid: %s\nA: %+v\nB: %+v", test.test, test.a, test.b) } } } diff --git a/test/e2e/framework.go b/test/e2e/framework.go index 39c4b7eeba..444a086792 100644 --- a/test/e2e/framework.go +++ b/test/e2e/framework.go @@ -200,6 +200,11 @@ func (f *Framework) afterEach() { f.Client = nil } +// WaitForPodTerminated waits for the pod to be terminated with the given reason. +func (f *Framework) WaitForPodTerminated(podName, reason string) error { + return waitForPodTerminatedInNamespace(f.Client, podName, reason, f.Namespace.Name) +} + // WaitForPodRunning waits for the pod to run in the namespace. func (f *Framework) WaitForPodRunning(podName string) error { return waitForPodRunningInNamespace(f.Client, podName, f.Namespace.Name) diff --git a/test/e2e/pods.go b/test/e2e/pods.go index 0f0fbee9d0..84cc3a151c 100644 --- a/test/e2e/pods.go +++ b/test/e2e/pods.go @@ -470,6 +470,86 @@ var _ = Describe("Pods", func() { Logf("Pod update OK") }) + It("should allow activeDeadlineSeconds to be updated [Conformance]", func() { + podClient := framework.Client.Pods(framework.Namespace.Name) + + By("creating the pod") + name := "pod-update-activedeadlineseconds-" + string(util.NewUUID()) + value := strconv.Itoa(time.Now().Nanosecond()) + pod := &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Labels: map[string]string{ + "name": "foo", + "time": value, + }, + }, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "nginx", + Image: "gcr.io/google_containers/nginx:1.7.9", + Ports: []api.ContainerPort{{ContainerPort: 80}}, + LivenessProbe: &api.Probe{ + Handler: api.Handler{ + HTTPGet: &api.HTTPGetAction{ + Path: "/index.html", + Port: intstr.FromInt(8080), + }, + }, + InitialDelaySeconds: 30, + }, + }, + }, + }, + } + + By("submitting the pod to kubernetes") + defer func() { + By("deleting the pod") + podClient.Delete(pod.Name, api.NewDeleteOptions(0)) + }() + pod, err := podClient.Create(pod) + if err != nil { + Failf("Failed to create pod: %v", err) + } + + expectNoError(framework.WaitForPodRunning(pod.Name)) + + By("verifying the pod is in kubernetes") + selector := labels.SelectorFromSet(labels.Set(map[string]string{"time": value})) + options := api.ListOptions{LabelSelector: selector} + pods, err := podClient.List(options) + Expect(len(pods.Items)).To(Equal(1)) + + // Standard get, update retry loop + expectNoError(wait.Poll(time.Millisecond*500, time.Second*30, func() (bool, error) { + By("updating the pod") + value = strconv.Itoa(time.Now().Nanosecond()) + if pod == nil { // on retries we need to re-get + pod, err = podClient.Get(name) + if err != nil { + return false, fmt.Errorf("failed to get pod: %v", err) + } + } + newDeadline := int64(5) + pod.Spec.ActiveDeadlineSeconds = &newDeadline + pod, err = podClient.Update(pod) + if err == nil { + Logf("Successfully updated pod") + return true, nil + } + if errors.IsConflict(err) { + Logf("Conflicting update to pod, re-get and re-update: %v", err) + pod = nil // re-get it when we retry + return false, nil + } + return false, fmt.Errorf("failed to update pod: %v", err) + })) + + expectNoError(framework.WaitForPodTerminated(pod.Name, "DeadlineExceeded")) + }) + It("should contain environment variables for services [Conformance]", func() { // Make a pod that will be a service. // This pod serves its hostname via HTTP. diff --git a/test/e2e/util.go b/test/e2e/util.go index 2166ada6f7..9485ae3ff4 100644 --- a/test/e2e/util.go +++ b/test/e2e/util.go @@ -788,6 +788,22 @@ func waitForPodNotPending(c *client.Client, ns, podName string) error { }) } +// waitForPodTerminatedInNamespace returns an error if it took too long for the pod +// to terminate or if the pod terminated with an unexpected reason. +func waitForPodTerminatedInNamespace(c *client.Client, podName, reason, namespace string) error { + return waitForPodCondition(c, namespace, podName, "terminated due to deadline exceeded", podStartTimeout, func(pod *api.Pod) (bool, error) { + if pod.Status.Phase == api.PodFailed { + if pod.Status.Reason == reason { + return true, nil + } else { + return true, fmt.Errorf("Expected pod %n/%n to be terminated with reason %v, got reason: ", namespace, podName, reason, pod.Status.Reason) + } + } + + return false, nil + }) +} + // waitForPodSuccessInNamespace returns nil if the pod reached state success, or an error if it reached failure or ran too long. func waitForPodSuccessInNamespace(c *client.Client, podName string, contName string, namespace string) error { return waitForPodCondition(c, namespace, podName, "success or failure", podStartTimeout, func(pod *api.Pod) (bool, error) { diff --git a/test/integration/pods.go b/test/integration/pods.go new file mode 100644 index 0000000000..0407d62741 --- /dev/null +++ b/test/integration/pods.go @@ -0,0 +1,163 @@ +// +build integration,!no-etcd + +/* +Copyright 2015 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package integration + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/testapi" + client "k8s.io/kubernetes/pkg/client/unversioned" + "k8s.io/kubernetes/pkg/master" + "k8s.io/kubernetes/test/integration/framework" +) + +func TestPodUpdateActiveDeadlineSeconds(t *testing.T) { + var m *master.Master + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + m.Handler.ServeHTTP(w, req) + })) + // TODO: Uncomment when fix #19254 + // defer s.Close() + + ns := "pod-activedeadline-update" + masterConfig := framework.NewIntegrationTestMasterConfig() + m, err := master.New(masterConfig) + if err != nil { + t.Fatalf("Error in bringing up the master: %v", err) + } + + framework.DeleteAllEtcdKeys() + client := client.NewOrDie(&client.Config{Host: s.URL, ContentConfig: client.ContentConfig{GroupVersion: testapi.Default.GroupVersion()}}) + + var ( + iZero = int64(0) + i30 = int64(30) + i60 = int64(60) + iNeg = int64(-1) + ) + + prototypePod := func() *api.Pod { + return &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "XXX", + }, + Spec: api.PodSpec{ + Containers: []api.Container{ + { + Name: "fake-name", + Image: "fakeimage", + }, + }, + }, + } + } + + cases := []struct { + name string + original *int64 + update *int64 + valid bool + }{ + { + name: "no change, nil", + original: nil, + update: nil, + valid: true, + }, + { + name: "no change, set", + original: &i30, + update: &i60, + valid: true, + }, + { + name: "change to positive from nil", + original: nil, + update: &i60, + valid: true, + }, + { + name: "change to smaller positive", + original: &i60, + update: &i30, + valid: true, + }, + { + name: "change to larger positive", + original: &i30, + update: &i60, + valid: false, + }, + { + name: "change to negative from positive", + original: &i30, + update: &iNeg, + valid: false, + }, + { + name: "change to negative from nil", + original: nil, + update: &iNeg, + valid: false, + }, + { + name: "change to zero from positive", + original: &i30, + update: &iZero, + valid: true, + }, + { + name: "change to zero from nil", + original: nil, + update: &iZero, + valid: true, + }, + { + name: "change to nil from positive", + original: &i30, + update: nil, + valid: false, + }, + } + + for i, tc := range cases { + pod := prototypePod() + pod.Spec.ActiveDeadlineSeconds = tc.original + pod.ObjectMeta.Name = fmt.Sprintf("activedeadlineseconds-test-%v", i) + + if _, err := client.Pods(ns).Create(pod); err != nil { + t.Errorf("Failed to create pod: %v", err) + } + + pod.Spec.ActiveDeadlineSeconds = tc.update + + _, err := client.Pods(ns).Update(pod) + if tc.valid && err != nil { + t.Errorf("%v: failed to update pod: %v", tc.name, err) + } else if !tc.valid && err == nil { + t.Errorf("%v: unexpected allowed update to pod", tc.name) + } + + deletePodOrErrorf(t, client, ns, pod.Name) + } +}