From 9ad1f80fdcd77edcdd53abec3641c04c80fd9b1e Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 7 Jun 2017 14:08:01 -0400 Subject: [PATCH] DeleteCollection should include uninitialized resources Users who delete a collection expect all resources to be deleted, and users can also delete an uninitialized resource. To preserve this expectation, DeleteCollection selects all resources regardless of initialization. The namespace controller should list uninitialized resources in order to gate cleanup of a namespace. --- .../deletion/namespaced_resources_deleter.go | 4 +- .../pkg/apis/meta/internalversion/register.go | 3 + .../pkg/registry/generic/registry/store.go | 18 ++++++ .../registry/generic/registry/store_test.go | 56 ++++++++++++++++++- test/e2e/namespace.go | 52 ++++++++++++++++- 5 files changed, 127 insertions(+), 6 deletions(-) diff --git a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go b/pkg/controller/namespace/deletion/namespaced_resources_deleter.go index 7920173b43..e061a8ac51 100644 --- a/pkg/controller/namespace/deletion/namespaced_resources_deleter.go +++ b/pkg/controller/namespace/deletion/namespaced_resources_deleter.go @@ -379,7 +379,7 @@ func (d *namespacedResourcesDeleter) listCollection( } apiResource := metav1.APIResource{Name: gvr.Resource, Namespaced: true} - obj, err := dynamicClient.Resource(&apiResource, namespace).List(metav1.ListOptions{}) + obj, err := dynamicClient.Resource(&apiResource, namespace).List(metav1.ListOptions{IncludeUninitialized: true}) if err == nil { unstructuredList, ok := obj.(*unstructured.UnstructuredList) if !ok { @@ -553,7 +553,7 @@ func (d *namespacedResourcesDeleter) estimateGracefulTerminationForPods(ns strin if podsGetter == nil || reflect.ValueOf(podsGetter).IsNil() { return estimate, fmt.Errorf("unexpected: podsGetter is nil. Cannot estimate grace period seconds for pods") } - items, err := podsGetter.Pods(ns).List(metav1.ListOptions{}) + items, err := podsGetter.Pods(ns).List(metav1.ListOptions{IncludeUninitialized: true}) if err != nil { return estimate, err } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/register.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/register.go index bf4f4cd6b9..38ddc80dfd 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/register.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/register.go @@ -30,6 +30,9 @@ const GroupName = "meta.k8s.io" // Scheme is the registry for any type that adheres to the meta API spec. var scheme = runtime.NewScheme() +// Copier exposes copying on this scheme. +var Copier runtime.ObjectCopier = scheme + // Codecs provides access to encoding and decoding for the scheme. var Codecs = serializer.NewCodecFactory(scheme) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 001b7ba207..2dcea903a0 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -998,6 +998,18 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta return out, true, err } +// copyListOptions copies list options for mutation. +func copyListOptions(options *metainternalversion.ListOptions) *metainternalversion.ListOptions { + if options == nil { + return &metainternalversion.ListOptions{} + } + copied, err := metainternalversion.Copier.Copy(options) + if err != nil { + panic(err) + } + return copied.(*metainternalversion.ListOptions) +} + // DeleteCollection removes all items returned by List with a given ListOptions from storage. // // DeleteCollection is currently NOT atomic. It can happen that only subset of objects @@ -1009,6 +1021,12 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta // possibly with storage API, but watch is not delivered correctly then). // It will be possible to fix it with v3 etcd API. func (e *Store) DeleteCollection(ctx genericapirequest.Context, options *metav1.DeleteOptions, listOptions *metainternalversion.ListOptions) (runtime.Object, error) { + // DeleteCollection must remain backwards compatible with old clients that expect it to + // remove all resources, initialized or not, within the type. It is also consistent with + // Delete which does not require IncludeUninitialized + listOptions = copyListOptions(listOptions) + listOptions.IncludeUninitialized = true + listObj, err := e.List(ctx, listOptions) if err != nil { return nil, err diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index d0443115ef..841483e7b6 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -839,6 +839,44 @@ func TestStoreDelete(t *testing.T) { } } +func TestStoreDeleteUninitialized(t *testing.T) { + podA := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: "Testing"}}}}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + + // test failure condition + _, _, err := registry.Delete(testContext, podA.Name, nil) + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error: %v", err) + } + + // create pod + _, err = registry.Create(testContext, podA, true) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // delete object + _, wasDeleted, err := registry.Delete(testContext, podA.Name, nil) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if !wasDeleted { + t.Errorf("unexpected, pod %s should have been deleted immediately", podA.Name) + } + + // try to get a item which should be deleted + _, err = registry.Get(testContext, podA.Name, &metav1.GetOptions{}) + if !errors.IsNotFound(err) { + t.Errorf("Unexpected error: %v", err) + } +} + // TestGracefulStoreCanDeleteIfExistingGracePeriodZero tests recovery from // race condition where the graceful delete is unable to complete // in prior operation, but the pod remains with deletion timestamp @@ -1546,6 +1584,14 @@ func TestStoreDeletionPropagation(t *testing.T) { func TestStoreDeleteCollection(t *testing.T) { podA := &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} podB := &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "bar"}} + podC := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "baz", + Initializers: &metav1.Initializers{ + Pending: []metav1.Initializer{{Name: "Test"}}, + }, + }, + } testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") destroyFunc, registry := NewTestGenericStoreRegistry(t) @@ -1557,6 +1603,9 @@ func TestStoreDeleteCollection(t *testing.T) { if _, err := registry.Create(testContext, podB, false); err != nil { t.Errorf("Unexpected error: %v", err) } + if _, err := registry.Create(testContext, podC, true); err != nil { + t.Errorf("Unexpected error: %v", err) + } // Delete all pods. deleted, err := registry.DeleteCollection(testContext, nil, &metainternalversion.ListOptions{}) @@ -1564,8 +1613,8 @@ func TestStoreDeleteCollection(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } deletedPods := deleted.(*example.PodList) - if len(deletedPods.Items) != 2 { - t.Errorf("Unexpected number of pods deleted: %d, expected: 2", len(deletedPods.Items)) + if len(deletedPods.Items) != 3 { + t.Errorf("Unexpected number of pods deleted: %d, expected: 3", len(deletedPods.Items)) } if _, err := registry.Get(testContext, podA.Name, &metav1.GetOptions{}); !errors.IsNotFound(err) { @@ -1574,6 +1623,9 @@ func TestStoreDeleteCollection(t *testing.T) { if _, err := registry.Get(testContext, podB.Name, &metav1.GetOptions{}); !errors.IsNotFound(err) { t.Errorf("Unexpected error: %v", err) } + if _, err := registry.Get(testContext, podC.Name, &metav1.GetOptions{}); !errors.IsNotFound(err) { + t.Errorf("Unexpected error: %v", err) + } } func TestStoreDeleteCollectionNotFound(t *testing.T) { diff --git a/test/e2e/namespace.go b/test/e2e/namespace.go index c63c6b375d..1dd9e827f4 100644 --- a/test/e2e/namespace.go +++ b/test/e2e/namespace.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/client/clientset_generated/clientset" "k8s.io/kubernetes/test/e2e/framework" . "github.com/onsi/ginkgo" @@ -78,9 +79,24 @@ func extinguish(f *framework.Framework, totalNS int, maxAllowedAfterDel int, max })) } -func ensurePodsAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { +func waitForPodInNamespace(c clientset.Interface, ns, podName string) *v1.Pod { + var pod *v1.Pod var err error + err = wait.PollImmediate(2*time.Second, 15*time.Second, func() (bool, error) { + pod, err = c.Core().Pods(ns).Get(podName, metav1.GetOptions{IncludeUninitialized: true}) + if errors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, err + } + return true, nil + }) + Expect(err).NotTo(HaveOccurred()) + return pod +} +func ensurePodsAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { By("Creating a test namespace") namespace, err := f.CreateNamespace("nsdeletetest", nil) Expect(err).NotTo(HaveOccurred()) @@ -109,6 +125,28 @@ func ensurePodsAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { By("Waiting for the pod to have running status") framework.ExpectNoError(framework.WaitForPodRunningInNamespace(f.ClientSet, pod)) + By("Creating an uninitialized pod in the namespace") + podB := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod-uninitialized", + Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: "test.initializer.k8s.io"}}}, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "nginx", + Image: framework.GetPauseImageName(f.ClientSet), + }, + }, + }, + } + go func() { + _, err = f.ClientSet.Core().Pods(namespace.Name).Create(podB) + // This error is ok, beacuse we will delete the pod before it completes initialization + framework.Logf("error from create uninitialized namespace: %v", err) + }() + podB = waitForPodInNamespace(f.ClientSet, podB.Namespace, podB.Name) + By("Deleting the namespace") err = f.ClientSet.Core().Namespaces().Delete(namespace.Name, nil) Expect(err).NotTo(HaveOccurred()) @@ -124,9 +162,15 @@ func ensurePodsAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { return false, nil })) - By("Verifying there is no pod in the namespace") + By("Recreating the namespace") + namespace, err = f.CreateNamespace("nsdeletetest", nil) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying there are no pods in the namespace") _, err = f.ClientSet.Core().Pods(namespace.Name).Get(pod.Name, metav1.GetOptions{}) Expect(err).To(HaveOccurred()) + _, err = f.ClientSet.Core().Pods(namespace.Name).Get(podB.Name, metav1.GetOptions{IncludeUninitialized: true}) + Expect(err).To(HaveOccurred()) } func ensureServicesAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { @@ -176,6 +220,10 @@ func ensureServicesAreRemovedWhenNamespaceIsDeleted(f *framework.Framework) { return false, nil })) + By("Recreating the namespace") + namespace, err = f.CreateNamespace("nsdeletetest", nil) + Expect(err).NotTo(HaveOccurred()) + By("Verifying there is no service in the namespace") _, err = f.ClientSet.Core().Services(namespace.Name).Get(service.Name, metav1.GetOptions{}) Expect(err).To(HaveOccurred())