From 9fe20cfd46bcab48e389f3a52e9da07c031dca79 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 12 Jul 2018 14:35:59 -0400 Subject: [PATCH] make delete waits match on UID --- pkg/kubectl/cmd/delete.go | 36 ++++++++++++-- pkg/kubectl/cmd/wait/BUILD | 3 ++ pkg/kubectl/cmd/wait/wait.go | 27 +++++++++- pkg/kubectl/cmd/wait/wait_test.go | 49 +++++++++++++++++++ .../genericclioptions/resource/helper.go | 6 +-- .../genericclioptions/resource/helper_test.go | 2 +- 6 files changed, 112 insertions(+), 11 deletions(-) diff --git a/pkg/kubectl/cmd/delete.go b/pkg/kubectl/cmd/delete.go index 81c8e067f2..8c6df9943f 100644 --- a/pkg/kubectl/cmd/delete.go +++ b/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. diff --git a/pkg/kubectl/cmd/wait/BUILD b/pkg/kubectl/cmd/wait/BUILD index b6c897b66f..dcadf17ee6 100644 --- a/pkg/kubectl/cmd/wait/BUILD +++ b/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", diff --git a/pkg/kubectl/cmd/wait/wait.go b/pkg/kubectl/cmd/wait/wait.go index 80a563aea6..9590c07e18 100644 --- a/pkg/kubectl/cmd/wait/wait.go +++ b/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 diff --git a/pkg/kubectl/cmd/wait/wait_test.go b/pkg/kubectl/cmd/wait/wait_test.go index 27446e840f..06fbc859b1 100644 --- a/pkg/kubectl/cmd/wait/wait_test.go +++ b/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) @@ -96,6 +99,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{ @@ -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, diff --git a/pkg/kubectl/genericclioptions/resource/helper.go b/pkg/kubectl/genericclioptions/resource/helper.go index 97f26fa415..52a4057e08 100644 --- a/pkg/kubectl/genericclioptions/resource/helper.go +++ b/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) { diff --git a/pkg/kubectl/genericclioptions/resource/helper_test.go b/pkg/kubectl/genericclioptions/resource/helper_test.go index 67ce7f5fe2..eb9b868b9e 100644 --- a/pkg/kubectl/genericclioptions/resource/helper_test.go +++ b/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) }