diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go b/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go index d5fa6e8d514..7a3c3c6764c 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go +++ b/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 - the --force flag. + than the grace period. To force delete a resource, you must specify 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: - fmt.Fprintf(o.ErrOut, "warning: --force is ignored because --grace-period is not 0.\n") + case o.GracePeriod > 0 && o.ForceDeletion: + return fmt.Errorf("--force and --grace-period greater than 0 cannot be specified together") } if len(o.Raw) > 0 { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_flags.go b/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_flags.go index d34104f51d3..679d28950fc 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_flags.go +++ b/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.") diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_test.go index e9f7a04696a..f4faf6289f5 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete_test.go +++ b/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) { - cmdtesting.InitTestErrorHandler(t) +func TestGracePeriodScenarios(t *testing.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{ - NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer, - Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { - t.Logf("got request %s %s", req.Method, req.URL.Path) - switch p, m := req.URL.Path, req.Method; { - case p == "/namespaces/test/pods/nginx" && m == "GET": - count++ - switch count { - case 1, 2, 3: - return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &pods.Items[0])}, nil - default: - return &http.Response{StatusCode: http.StatusNotFound, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &metav1.Status{})}, nil + tc := []struct { + name string + cmdArgs []string + forceFlag bool + nowFlag bool + gracePeriodFlag string + expectedGracePeriod string + expectedOut string + expectedErrOut string + expectedDeleteRequestPath string + expectedExitCode int + }{ + { + 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": - return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, &corev1.Namespace{})}, nil - 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 - default: - t.Fatalf("unexpected request: %#v\n%#v", req.URL, req) - return nil, nil + if test.expectedErrOut != actualErrOut { + t.Errorf("unexpected error:\n\tExpected: %s\n\tActual: %s\n", test.expectedErrOut, actualErrOut) + } + panic(nil) + }) + defer func() { + 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() - cmd := NewCmdDelete(tf, streams) - cmd.Flags().Set("output", "name") - cmd.Flags().Set("grace-period", "0") - cmd.Run(cmd, []string{"pods/nginx"}) + // Test the command using the flags specified in the test case + streams, _, out, errOut := genericclioptions.NewTestIOStreams() + cmd := NewCmdDelete(tf, streams) + cmd.Flags().Set("output", "name") + 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 - if buf.String() != "pod/nginx\n" { - t.Errorf("unexpected output: %s\n---\n%s", buf.String(), errBuf.String()) - } - if count != 0 { - t.Errorf("unexpected calls to GET: %d", count) + // Check actual vs expected conditions + if len(test.expectedDeleteRequestPath) > 0 && !deleteOccurred { + t.Errorf("expected http delete request to %s but it did not occur", test.expectedDeleteRequestPath) + } + if test.expectedGracePeriod != actualGracePeriod { + 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()) + } + }) } }