Merge pull request kubernetes#17547 from juanvallejo/jvallejo/handle-ds-pod-drain-local-storage

Automatic merge from submit-queue (batch tested with PRs 17547, 18151).

UPSTREAM: 56713: Allow oadm drain to continue w ds-managed pods with local storage

Fixes openshift/origin#17522

UPSTREAM: kubernetes#56713

Prevents oadm drain from failing if it encounters DaemonSet-managed pods
that have local storage, when the option to ignore DaemonSet-managed
pods has been specified.

Will add tests

cc @openshift/cli-review @dustymabe @deads2k

Origin-commit: 1c8ec8f29d1fd182c8329d57e1bd22286b8fd7c9

Author: k8s-publishing-bot

pkg/kubectl/cmd/drain.go

@@ -445,7 +445,7 @@ func (o *DrainOptions) getPodsForDeletion(nodeInfo *resource.Info) (pods []corev
 
 	for _, pod := range podList.Items {
 		podOk := true
-		for _, filt := range []podFilter{mirrorPodFilter, o.localStorageFilter, o.unreplicatedFilter, o.daemonsetFilter} {
+		for _, filt := range []podFilter{o.daemonsetFilter, mirrorPodFilter, o.localStorageFilter, o.unreplicatedFilter} {
 			filterOk, w, f := filt(pod)
 
 			podOk = podOk && filterOk
@@ -455,6 +455,13 @@ func (o *DrainOptions) getPodsForDeletion(nodeInfo *resource.Info) (pods []corev
 			if f != nil {
 				fs[f.string] = append(fs[f.string], pod.Name)
 			}
+
+			// short-circuit as soon as pod not ok
+			// at that point, there is no reason to run pod
+			// through any additional filters
+			if !podOk {
+				break
+			}
 		}
 		if podOk {
 			pods = append(pods, pod)

pkg/kubectl/cmd/drain_test.go

@@ -304,6 +304,34 @@ func TestDrain(t *testing.T) {
 		},
 	}
 
+	ds_pod_with_emptyDir := corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "bar",
+			Namespace:         "default",
+			CreationTimestamp: metav1.Time{Time: time.Now()},
+			Labels:            labels,
+			SelfLink:          testapi.Default.SelfLink("pods", "bar"),
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         "extensions/v1beta1",
+					Kind:               "DaemonSet",
+					Name:               "ds",
+					BlockOwnerDeletion: boolptr(true),
+					Controller:         boolptr(true),
+				},
+			},
+		},
+		Spec: corev1.PodSpec{
+			NodeName: "node",
+			Volumes: []corev1.Volume{
+				{
+					Name:         "scratch",
+					VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: ""}},
+				},
+			},
+		},
+	}
+
 	orphaned_ds_pod := corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:              "bar",
@@ -414,15 +442,16 @@ func TestDrain(t *testing.T) {
 	}
 
 	tests := []struct {
-		description  string
-		node         *corev1.Node
-		expected     *corev1.Node
-		pods         []corev1.Pod
-		rcs          []api.ReplicationController
-		replicaSets  []extensions.ReplicaSet
-		args         []string
-		expectFatal  bool
-		expectDelete bool
+		description   string
+		node          *corev1.Node
+		expected      *corev1.Node
+		pods          []corev1.Pod
+		rcs           []api.ReplicationController
+		replicaSets   []extensions.ReplicaSet
+		args          []string
+		expectWarning string
+		expectFatal   bool
+		expectDelete  bool
 	}{
 		{
 			description:  "RC-managed pod",
@@ -474,6 +503,17 @@ func TestDrain(t *testing.T) {
 			expectFatal:  false,
 			expectDelete: false,
 		},
+		{
+			description:   "DS-managed pod with emptyDir with --ignore-daemonsets",
+			node:          node,
+			expected:      cordoned_node,
+			pods:          []corev1.Pod{ds_pod_with_emptyDir},
+			rcs:           []api.ReplicationController{rc},
+			args:          []string{"node", "--ignore-daemonsets"},
+			expectWarning: "WARNING: Ignoring DaemonSet-managed pods: bar\n",
+			expectFatal:   false,
+			expectDelete:  false,
+		},
 		{
 			description:  "Job-managed pod",
 			node:         node,
@@ -661,21 +701,27 @@ func TestDrain(t *testing.T) {
 			cmd := NewCmdDrain(f, buf, errBuf)
 
 			saw_fatal := false
+			fatal_msg := ""
 			func() {
 				defer func() {
 					// Recover from the panic below.
 					_ = recover()
 					// Restore cmdutil behavior
 					cmdutil.DefaultBehaviorOnFatal()
 				}()
-				cmdutil.BehaviorOnFatal(func(e string, code int) { saw_fatal = true; panic(e) })
+				cmdutil.BehaviorOnFatal(func(e string, code int) { saw_fatal = true; fatal_msg = e; panic(e) })
+
 				cmd.SetArgs(test.args)
 				cmd.Execute()
 			}()
 			if test.expectFatal {
 				if !saw_fatal {
 					t.Fatalf("%s: unexpected non-error when using %s", test.description, currMethod)
 				}
+			} else {
+				if saw_fatal {
+					t.Fatalf("%s: unexpected error when using %s: %s", test.description, currMethod, fatal_msg)
+				}
 			}
 
 			if test.expectDelete {
@@ -693,6 +739,16 @@ func TestDrain(t *testing.T) {
 					t.Fatalf("%s: unexpected delete when using %s", test.description, currMethod)
 				}
 			}
+
+			if len(test.expectWarning) > 0 {
+				if len(errBuf.String()) == 0 {
+					t.Fatalf("%s: expected warning, but found no stderr output", test.description)
+				}
+
+				if errBuf.String() != test.expectWarning {
+					t.Fatalf("%s: actual warning message did not match expected warning message.\n Expecting: %s\n  Got: %s", test.description, test.expectWarning, errBuf.String())
+				}
+			}
 		}
 	}
 }