From 7cca47335f53af16adc1f0b43eb1cd389fbc2407 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 12 Jul 2018 13:41:48 -0400 Subject: [PATCH] UPSTREAM: 66136: make delete waits match on UID --- .../kubernetes/pkg/kubectl/cmd/delete.go | 36 +++++++++++--- .../kubernetes/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 +- 5 files changed, 108 insertions(+), 12 deletions(-) diff --git a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/delete.go b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/delete.go index 7ae71e76bf2d..5c921b6503bb 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/delete.go +++ b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/delete.go @@ -235,6 +235,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 @@ -252,7 +253,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 @@ -277,6 +299,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, @@ -294,23 +317,24 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error { return err } -func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav1.DeleteOptions) error { +func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav1.DeleteOptions) (runtime.Object, error) { // TODO: Remove this in or after 1.12 release. // Server version >= 1.11 no longer needs this hack. mapping := info.ResourceMapping() if mapping.Resource.GroupResource() == (schema.GroupResource{Group: "extensions", Resource: "daemonsets"}) || mapping.Resource.GroupResource() == (schema.GroupResource{Group: "apps", Resource: "daemonsets"}) { if err := updateDaemonSet(info.Namespace, info.Name, o.DynamicClient); err != nil { - return err + return nil, err } } - if err := resource.NewHelper(info.Client, info.Mapping).DeleteWithOptions(info.Namespace, info.Name, deleteOptions); err != nil { - return cmdutil.AddSourceToErr("deleting", info.Source, err) + 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 } // TODO: Remove this in or after 1.12 release. diff --git a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait.go b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait.go index 503a4bf08c68..1c6e886b858d 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait.go +++ b/vendor/k8s.io/kubernetes/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" @@ -151,12 +153,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 @@ -197,6 +210,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/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait_test.go b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait_test.go index 27446e840f6a..06fbc859b15b 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/wait/wait_test.go +++ b/vendor/k8s.io/kubernetes/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/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper.go b/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper.go index 97f26fa415c1..52a4057e08ac 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper.go +++ b/vendor/k8s.io/kubernetes/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/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper_test.go b/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper_test.go index e9cf7b1c9680..5f8a6e3e8031 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper_test.go +++ b/vendor/k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource/helper_test.go @@ -124,7 +124,7 @@ func TestHelperDelete(t *testing.T) { RESTClient: client, NamespaceScoped: true, } - err := modifier.Delete("bar", "foo") + _, err := modifier.Delete("bar", "foo") if (err != nil) != test.Err { t.Errorf("unexpected error: %t %v", test.Err, err) }