Merge pull request #87776 from brianpursley/kubectl-813

Default grace period to 0 when --force is used to delete an object

staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go

@@ -52,8 +52,7 @@ var (
 		the --grace-period flag, or pass --now to set a grace-period of 1. Because these resources often
 		represent entities in the cluster, deletion may not be acknowledged immediately. If the node
 		hosting a pod is down or cannot reach the API server, termination may take significantly longer
-		than the grace period. To force delete a resource, you must pass a grace period of 0 and specify
+		than the grace period. To force delete a resource, you must specify the --force flag.
-		the --force flag.
 		Note: only a subset of resources support graceful deletion. In absence of the support, --grace-period is ignored.
 
 		IMPORTANT: Force deleting pods does not wait for confirmation that the pod's processes have been
@@ -90,7 +89,7 @@ var (
 		kubectl delete pod foo --now
 
 		# Force delete a pod on a dead node
-		kubectl delete pod foo --grace-period=0 --force
+		kubectl delete pod foo --force
 
 		# Delete all pods
 		kubectl delete pods --all`))
@@ -176,6 +175,9 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
 		// into --grace-period=1. Users may provide --force to bypass this conversion.
 		o.GracePeriod = 1
 	}
+	if o.ForceDeletion && o.GracePeriod < 0 {
+		o.GracePeriod = 0
+	}
 
 	o.DryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd)
 	if err != nil {
@@ -239,8 +241,8 @@ func (o *DeleteOptions) Validate() error {
 	switch {
 	case o.GracePeriod == 0 && o.ForceDeletion:
 		fmt.Fprintf(o.ErrOut, "warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.\n")
-	case o.ForceDeletion:
+	case o.GracePeriod > 0 && o.ForceDeletion:
-		fmt.Fprintf(o.ErrOut, "warning: --force is ignored because --grace-period is not 0.\n")
+		return fmt.Errorf("--force and --grace-period greater than 0 cannot be specified together")
 	}
 
 	if len(o.Raw) > 0 {

staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_flags.go

@@ -116,7 +116,7 @@ func (f *DeleteFlags) AddFlags(cmd *cobra.Command) {
 		cmd.Flags().BoolVarP(f.AllNamespaces, "all-namespaces", "A", *f.AllNamespaces, "If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.")
 	}
 	if f.Force != nil {
-		cmd.Flags().BoolVar(f.Force, "force", *f.Force, "Only used when grace-period=0. If true, immediately remove resources from API and bypass graceful deletion. Note that immediate deletion of some resources may result in inconsistency or data loss and requires confirmation.")
+		cmd.Flags().BoolVar(f.Force, "force", *f.Force, "If true, immediately remove resources from API and bypass graceful deletion. Note that immediate deletion of some resources may result in inconsistency or data loss and requires confirmation.")
 	}
 	if f.Cascade != nil {
 		cmd.Flags().BoolVar(f.Cascade, "cascade", *f.Cascade, "If true, cascade the deletion of the resources managed by this resource (e.g. Pods created by a ReplicationController).  Default true.")

staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_test.go

@@ -21,6 +21,7 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -250,52 +251,157 @@ func TestDeleteObject(t *testing.T) {
 	}
 }
 
-func TestDeleteObjectGraceZero(t *testing.T) {
+func TestGracePeriodScenarios(t *testing.T) {
-	cmdtesting.InitTestErrorHandler(t)
 	pods, _, _ := cmdtesting.TestData()
 
-	count := 0
 	tf := cmdtesting.NewTestFactory().WithNamespace("test")
 	defer tf.Cleanup()
 
 	codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...)
 
-	tf.UnstructuredClient = &fake.RESTClient{
+	tc := []struct {
-		NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
+		name                      string
-		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+		cmdArgs                   []string
-			t.Logf("got request %s %s", req.Method, req.URL.Path)
+		forceFlag                 bool
-			switch p, m := req.URL.Path, req.Method; {
+		nowFlag                   bool
-			case p == "/namespaces/test/pods/nginx" && m == "GET":
+		gracePeriodFlag           string
-				count++
+		expectedGracePeriod       string
-				switch count {
+		expectedOut               string
-				case 1, 2, 3:
+		expectedErrOut            string
-					return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &pods.Items[0])}, nil
+		expectedDeleteRequestPath string
-				default:
+		expectedExitCode          int
-					return &http.Response{StatusCode: http.StatusNotFound, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &metav1.Status{})}, nil
+	}{
+		{
+			name:                      "Deleting an object with --force should use grace period = 0",
+			cmdArgs:                   []string{"pods/foo"},
+			forceFlag:                 true,
+			expectedGracePeriod:       "0",
+			expectedOut:               "pod/foo\n",
+			expectedErrOut:            "warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.\n",
+			expectedDeleteRequestPath: "/namespaces/test/pods/foo",
+		},
+		{
+			name:                      "Deleting an object with --force and --grace-period 0 should use grade period = 0",
+			cmdArgs:                   []string{"pods/foo"},
+			forceFlag:                 true,
+			gracePeriodFlag:           "0",
+			expectedGracePeriod:       "0",
+			expectedOut:               "pod/foo\n",
+			expectedErrOut:            "warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.\n",
+			expectedDeleteRequestPath: "/namespaces/test/pods/foo",
+		},
+		{
+			name:             "Deleting an object with --force and --grace-period > 0 should fail",
+			cmdArgs:          []string{"pods/foo"},
+			forceFlag:        true,
+			gracePeriodFlag:  "10",
+			expectedErrOut:   "error: --force and --grace-period greater than 0 cannot be specified together",
+			expectedExitCode: 1,
+		},
+		{
+			name:                      "Deleting an object with --grace-period 0 should use a grace period of 1",
+			cmdArgs:                   []string{"pods/foo"},
+			gracePeriodFlag:           "0",
+			expectedGracePeriod:       "1",
+			expectedOut:               "pod/foo\n",
+			expectedDeleteRequestPath: "/namespaces/test/pods/foo",
+		},
+		{
+			name:                      "Deleting an object with --grace-period > 0 should use the specified grace period",
+			cmdArgs:                   []string{"pods/foo"},
+			gracePeriodFlag:           "10",
+			expectedGracePeriod:       "10",
+			expectedOut:               "pod/foo\n",
+			expectedDeleteRequestPath: "/namespaces/test/pods/foo",
+		},
+		{
+			name:                      "Deleting an object with the --now flag should use grace period = 1",
+			cmdArgs:                   []string{"pods/foo"},
+			nowFlag:                   true,
+			expectedGracePeriod:       "1",
+			expectedOut:               "pod/foo\n",
+			expectedDeleteRequestPath: "/namespaces/test/pods/foo",
+		},
+		{
+			name:             "Deleting an object with --now and --grace-period should fail",
+			cmdArgs:          []string{"pods/foo"},
+			nowFlag:          true,
+			gracePeriodFlag:  "10",
+			expectedErrOut:   "error: --now and --grace-period cannot be specified together",
+			expectedExitCode: 1,
+		},
+	}
+
+	for _, test := range tc {
+		t.Run(test.name, func(t *testing.T) {
+
+			// Use a custom fatal behavior with panic/recover so that we can test failure scenarios where
+			// os.Exit() would normally be called
+			cmdutil.BehaviorOnFatal(func(actualErrOut string, actualExitCode int) {
+				if test.expectedExitCode != actualExitCode {
+					t.Errorf("unexpected exit code:\n\tExpected: %d\n\tActual:   %d\n", test.expectedExitCode, actualExitCode)
 				}
-			case p == "/api/v1/namespaces/test" && m == "GET":
+				if test.expectedErrOut != actualErrOut {
-				return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &corev1.Namespace{})}, nil
+					t.Errorf("unexpected error:\n\tExpected: %s\n\tActual:   %s\n", test.expectedErrOut, actualErrOut)
-			case p == "/namespaces/test/pods/nginx" && m == "DELETE":
+				}
-				return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &pods.Items[0])}, nil
+				panic(nil)
-			default:
+			})
-				t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)
+			defer func() {
-				return nil, nil
+				if test.expectedExitCode != 0 {
+					recover()
+				}
+			}()
+
+			// Setup a fake HTTP Client to capture whether a delete request was made or not and if so,
+			// the actual grace period that was used.
+			actualGracePeriod := ""
+			deleteOccurred := false
+			tf.UnstructuredClient = &fake.RESTClient{
+				NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
+				Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+					switch p, m := req.URL.Path, req.Method; {
+					case m == "DELETE" && p == test.expectedDeleteRequestPath:
+						data := make(map[string]interface{})
+						_ = json.NewDecoder(req.Body).Decode(&data)
+						actualGracePeriod = strconv.FormatFloat(data["gracePeriodSeconds"].(float64), 'f', 0, 64)
+						deleteOccurred = true
+						return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &pods.Items[0])}, nil
+					default:
+						t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)
+						return nil, nil
+					}
+				}),
 			}
-		}),
-	}
 
-	streams, _, buf, errBuf := genericclioptions.NewTestIOStreams()
+			// Test the command using the flags specified in the test case
-	cmd := NewCmdDelete(tf, streams)
+			streams, _, out, errOut := genericclioptions.NewTestIOStreams()
-	cmd.Flags().Set("output", "name")
+			cmd := NewCmdDelete(tf, streams)
-	cmd.Flags().Set("grace-period", "0")
+			cmd.Flags().Set("output", "name")
-	cmd.Run(cmd, []string{"pods/nginx"})
+			if test.forceFlag {
+				cmd.Flags().Set("force", "true")
+			}
+			if test.nowFlag {
+				cmd.Flags().Set("now", "true")
+			}
+			if len(test.gracePeriodFlag) > 0 {
+				cmd.Flags().Set("grace-period", test.gracePeriodFlag)
+			}
+			cmd.Run(cmd, test.cmdArgs)
 
-	// uses the name from the file, not the response
+			// Check actual vs expected conditions
-	if buf.String() != "pod/nginx\n" {
+			if len(test.expectedDeleteRequestPath) > 0 && !deleteOccurred {
-		t.Errorf("unexpected output: %s\n---\n%s", buf.String(), errBuf.String())
+				t.Errorf("expected http delete request to %s but it did not occur", test.expectedDeleteRequestPath)
-	}
+			}
-	if count != 0 {
+			if test.expectedGracePeriod != actualGracePeriod {
-		t.Errorf("unexpected calls to GET: %d", count)
+				t.Errorf("unexpected grace period:\n\tExpected: %s\n\tActual:   %s\n", test.expectedGracePeriod, actualGracePeriod)
+			}
+			if out.String() != test.expectedOut {
+				t.Errorf("unexpected output:\n\tExpected: %s\n\tActual:   %s\n", test.expectedOut, out.String())
+			}
+			if errOut.String() != test.expectedErrOut {
+				t.Errorf("unexpected error output:\n\tExpected: %s\n\tActual:   %s\n", test.expectedErrOut, errOut.String())
+			}
+		})
 	}
 }