Merge pull request #47138 from smarterclayton/delete_collection

Automatic merge from submit-queue (batch tested with PRs 46979, 47078, 47138, 46916)

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.

Fixes #47137

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
 	}

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)
 

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

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) {

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())