Merge pull request #66136 from deads2k/cli-02-delete-uid

pkg/kubectl/cmd/delete.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
@@ -229,6 +230,7 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
 		r = r.IgnoreErrors(errors.IsNotFound)
 	}
 	deletedInfos := []*resource.Info{}
+	uidMap := kubectlwait.UIDMap{}
 	err := r.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
 			return err
@@ -246,7 +248,28 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
 		}
 		options.PropagationPolicy = &policy
 
-		return o.deleteResource(info, options)
+		response, err := o.deleteResource(info, options)
+		if err != nil {
+			return err
+		}
+		resourceLocation := kubectlwait.ResourceLocation{
+			GroupResource: info.Mapping.Resource.GroupResource(),
+			Namespace:     info.Namespace,
+			Name:          info.Name,
+		}
+		if status, ok := response.(*metav1.Status); ok && status.Details != nil {
+			uidMap[resourceLocation] = status.Details.UID
+			return nil
+		}
+		responseMetadata, err := meta.Accessor(response)
+		if err != nil {
+			// we don't have UID, but we didn't fail the delete, next best thing is just skipping the UID
+			glog.V(1).Info(err)
+			return nil
+		}
+		uidMap[resourceLocation] = responseMetadata.GetUID()
+
+		return nil
 	})
 	if err != nil {
 		return err
@@ -271,6 +294,7 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
 	}
 	waitOptions := kubectlwait.WaitOptions{
 		ResourceFinder: genericclioptions.ResourceFinderForResult(resource.InfoListVisitor(deletedInfos)),
+		UIDMap:         uidMap,
 		DynamicClient:  o.DynamicClient,
 		Timeout:        effectiveTimeout,
 
@@ -281,19 +305,21 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
 	err = waitOptions.RunWait()
 	if errors.IsForbidden(err) || errors.IsMethodNotSupported(err) {
 		// if we're forbidden from waiting, we shouldn't fail.
+		// if the resource doesn't support a verb we need, we shouldn't fail.
 		glog.V(1).Info(err)
 		return nil
 	}
 	return err
 }
 
-func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav1.DeleteOptions) error {
-	if err := resource.NewHelper(info.Client, info.Mapping).DeleteWithOptions(info.Namespace, info.Name, deleteOptions); err != nil {
-		return cmdutil.AddSourceToErr("deleting", info.Source, err)
+func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav1.DeleteOptions) (runtime.Object, error) {
+	deleteResponse, err := resource.NewHelper(info.Client, info.Mapping).DeleteWithOptions(info.Namespace, info.Name, deleteOptions)
+	if err != nil {
+		return nil, cmdutil.AddSourceToErr("deleting", info.Source, err)
 	}
 
 	o.PrintObj(info)
-	return nil
+	return deleteResponse, nil
 }
 
 // deletion printing is special because we do not have an object to print.

pkg/kubectl/cmd/wait/BUILD

@@ -15,6 +15,8 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library",
         "//staging/src/k8s.io/client-go/dynamic:go_default_library",
@@ -48,6 +50,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library",
         "//staging/src/k8s.io/client-go/dynamic/fake:go_default_library",

pkg/kubectl/cmd/wait/wait.go

@@ -27,6 +27,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/dynamic"
@@ -176,12 +178,23 @@ func conditionFuncFor(condition string) (ConditionFunc, error) {
 	return nil, fmt.Errorf("unrecognized condition: %q", condition)
 }
 
+type ResourceLocation struct {
+	GroupResource schema.GroupResource
+	Namespace     string
+	Name          string
+}
+
+type UIDMap map[ResourceLocation]types.UID
+
 // WaitOptions is a set of options that allows you to wait.  This is the object reflects the runtime needs of a wait
 // command, making the logic itself easy to unit test with our existing mocks.
 type WaitOptions struct {
 	ResourceFinder genericclioptions.ResourceFinder
-	DynamicClient  dynamic.Interface
-	Timeout        time.Duration
+	// UIDMap maps a resource location to a UID.  It is optional, but ConditionFuncs may choose to use it to make the result
+	// more reliable.  For instance, delete can look for UID consistency during delegated calls.
+	UIDMap        UIDMap
+	DynamicClient dynamic.Interface
+	Timeout       time.Duration
 
 	Printer     printers.ResourcePrinter
 	ConditionFn ConditionFunc
@@ -222,6 +235,16 @@ func IsDeleted(info *resource.Info, o *WaitOptions) (runtime.Object, bool, error
 			// TODO this could do something slightly fancier if we wish
 			return info.Object, false, err
 		}
+		resourceLocation := ResourceLocation{
+			GroupResource: info.Mapping.Resource.GroupResource(),
+			Namespace:     gottenObj.GetNamespace(),
+			Name:          gottenObj.GetName(),
+		}
+		if uid, ok := o.UIDMap[resourceLocation]; ok {
+			if gottenObj.GetUID() != uid {
+				return gottenObj, true, nil
+			}
+		}
 
 		watchOptions := metav1.ListOptions{}
 		watchOptions.FieldSelector = "metadata.name=" + info.Name

pkg/kubectl/cmd/wait/wait_test.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	dynamicfakeclient "k8s.io/client-go/dynamic/fake"
@@ -46,6 +47,7 @@ func newUnstructured(apiVersion, kind, namespace, name string) *unstructured.Uns
 			"metadata": map[string]interface{}{
 				"namespace": namespace,
 				"name":      name,
+				"uid":       "some-UID-value",
 			},
 		},
 	}
@@ -69,6 +71,7 @@ func TestWaitForDeletion(t *testing.T) {
 		info       *resource.Info
 		fakeClient func() *dynamicfakeclient.FakeDynamicClient
 		timeout    time.Duration
+		uidMap     UIDMap
 
 		expectedErr     string
 		validateActions func(t *testing.T, actions []clienttesting.Action)
@@ -97,6 +100,51 @@ func TestWaitForDeletion(t *testing.T) {
 			},
 		},
 		{
+			name: "uid conflict on get",
+			info: &resource.Info{
+				Mapping: &meta.RESTMapping{
+					Resource: schema.GroupVersionResource{Group: "group", Version: "version", Resource: "theresource"},
+				},
+				Name:      "name-foo",
+				Namespace: "ns-foo",
+			},
+			fakeClient: func() *dynamicfakeclient.FakeDynamicClient {
+				fakeClient := dynamicfakeclient.NewSimpleDynamicClient(scheme)
+				fakeClient.PrependReactor("get", "theresource", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, newUnstructured("group/version", "TheKind", "ns-foo", "name-foo"), nil
+				})
+				count := 0
+				fakeClient.PrependWatchReactor("theresource", func(action clienttesting.Action) (handled bool, ret watch.Interface, err error) {
+					if count == 0 {
+						count++
+						fakeWatch := watch.NewRaceFreeFake()
+						go func() {
+							time.Sleep(100 * time.Millisecond)
+							fakeWatch.Stop()
+						}()
+						return true, fakeWatch, nil
+					}
+					fakeWatch := watch.NewRaceFreeFake()
+					return true, fakeWatch, nil
+				})
+				return fakeClient
+			},
+			timeout: 10 * time.Second,
+			uidMap: UIDMap{
+				ResourceLocation{Namespace: "ns-foo", Name: "name-foo"}:                                                                               types.UID("some-UID-value"),
+				ResourceLocation{GroupResource: schema.GroupResource{Group: "group", Resource: "theresource"}, Namespace: "ns-foo", Name: "name-foo"}: types.UID("some-nonmatching-UID-value"),
+			},
+
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				if len(actions) != 1 {
+					t.Fatal(spew.Sdump(actions))
+				}
+				if !actions[0].Matches("get", "theresource") || actions[0].(clienttesting.GetAction).GetName() != "name-foo" {
+					t.Error(spew.Sdump(actions))
+				}
+			},
+		},
+		{
 			name: "times out",
 			info: &resource.Info{
 				Mapping: &meta.RESTMapping{
@@ -220,6 +268,7 @@ func TestWaitForDeletion(t *testing.T) {
 			fakeClient := test.fakeClient()
 			o := &WaitOptions{
 				ResourceFinder: genericclioptions.NewSimpleFakeResourceFinder(test.info),
+				UIDMap:         test.uidMap,
 				DynamicClient:  fakeClient,
 				Timeout:        test.timeout,
 

pkg/kubectl/genericclioptions/resource/helper.go

@@ -94,18 +94,18 @@ func (m *Helper) WatchSingle(namespace, name, resourceVersion string) (watch.Int
 		Watch()
 }
 
-func (m *Helper) Delete(namespace, name string) error {
+func (m *Helper) Delete(namespace, name string) (runtime.Object, error) {
 	return m.DeleteWithOptions(namespace, name, nil)
 }
 
-func (m *Helper) DeleteWithOptions(namespace, name string, options *metav1.DeleteOptions) error {
+func (m *Helper) DeleteWithOptions(namespace, name string, options *metav1.DeleteOptions) (runtime.Object, error) {
 	return m.RESTClient.Delete().
 		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		Name(name).
 		Body(options).
 		Do().
-		Error()
+		Get()
 }
 
 func (m *Helper) Create(namespace string, modify bool, obj runtime.Object) (runtime.Object, error) {

pkg/kubectl/genericclioptions/resource/helper_test.go

@@ -129,7 +129,7 @@ func TestHelperDelete(t *testing.T) {
 				RESTClient:      client,
 				NamespaceScoped: true,
 			}
-			err := modifier.Delete("bar", "foo")
+			_, err := modifier.Delete("bar", "foo")
 			if (err != nil) != tt.Err {
 				t.Errorf("unexpected error: %t %v", tt.Err, err)
 			}