Merge pull request #54708 from joelsmith/master

Automatic merge from submit-queue (batch tested with PRs 53962, 54708). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Prevent successful containers from restarting with OnFailure restart policy **What this PR does / why we need it**: This is a follow-on to #54597 which makes sure that its validation also applies to pods with a restart policy of OnFailure. This deficiency was pointed out by @smarterclayton here: https://github.com/kubernetes/kubernetes/pull/54530#discussion_r147226458 **Which issue this PR fixes** This is another fix to address #54499 **Release note**: ```release-note NONE ```

Merge pull request #54708 from joelsmith/master
444a161d · Kubernetes Submit Queue · GitHub · f5b6dd87 · 5f6c0227 · 444a161d
Unverified Commit 444a161d authored Nov 01, 2017 by Kubernetes Submit Queue Committed by GitHub Nov 01, 2017
Hide whitespace changes
Inline Side-by-side

Showing with 23 additions and 16 deletions

status_manager.go pkg/kubelet/status/status_manager.go +23 -16

No files found.
--- a/pkg/kubelet/status/status_manager.go
+++ b/pkg/kubelet/status/status_manager.go
@@ -268,13 +268,22 @@ func (m *manager) TerminatePod(pod *v1.Pod) {
 // checkContainerStateTransition ensures that no container is trying to transition
 // from a terminated to non-terminated state, which is illegal and indicates a
 // logical error in the kubelet.
-func checkContainerStateTransition(oldStatuses, newStatuses []v1.ContainerStatus) error {
+func checkContainerStateTransition(oldStatuses, newStatuses []v1.ContainerStatus, restartPolicy v1.RestartPolicy) error {
-	for _, newStatus := range newStatuses {
+	// If we should always restart, containers are allowed to leave the terminated state
-		for _, oldStatus := range oldStatuses {
+	if restartPolicy == v1.RestartPolicyAlways {
-			if newStatus.Name != oldStatus.Name {
+		return nil
-				continue
+	}
-			}
+	for _, oldStatus := range oldStatuses {
-			if oldStatus.State.Terminated != nil && newStatus.State.Terminated == nil {
+		// Skip any container that wasn't terminated
+		if oldStatus.State.Terminated == nil {
+			continue
+		}
+		// Skip any container that failed but is allowed to restart
+		if oldStatus.State.Terminated.ExitCode != 0 && restartPolicy == v1.RestartPolicyOnFailure {
+			continue
+		}
+		for _, newStatus := range newStatuses {
+			if oldStatus.Name == newStatus.Name && newStatus.State.Terminated == nil {
 				return fmt.Errorf("terminated container %v attempted illegal transition to non-terminated state", newStatus.Name)
 			}
 		}
@@ -297,15 +306,13 @@ func (m *manager) updateStatusInternal(pod *v1.Pod, status v1.PodStatus, forceUp
 	}
 	// Check for illegal state transition in containers
-	if pod.Spec.RestartPolicy == v1.RestartPolicyNever {
+	if err := checkContainerStateTransition(oldStatus.ContainerStatuses, status.ContainerStatuses, pod.Spec.RestartPolicy); err != nil {
-		if err := checkContainerStateTransition(oldStatus.ContainerStatuses, status.ContainerStatuses); err != nil {
+		glog.Errorf("Status update on pod %v/%v aborted: %v", pod.Namespace, pod.Name, err)
-			glog.Errorf("Status update on pod %v/%v aborted: %v", pod.Namespace, pod.Name, err)
+		return false
-			return false
+	}
-		}
+	if err := checkContainerStateTransition(oldStatus.InitContainerStatuses, status.InitContainerStatuses, pod.Spec.RestartPolicy); err != nil {
-		if err := checkContainerStateTransition(oldStatus.InitContainerStatuses, status.InitContainerStatuses); err != nil {
+		glog.Errorf("Status update on pod %v/%v aborted: %v", pod.Namespace, pod.Name, err)
-			glog.Errorf("Status update on pod %v/%v aborted: %v", pod.Namespace, pod.Name, err)
+		return false
-			return false
-		}
 	}
 	// Set ReadyCondition.LastTransitionTime.