Scrub namespace terminating status and deletion timestamp on restore.

Descriptive restore error on terminating namespace.

Signed-off-by: Tiger Kaovilai <[email protected]>

changelogs/unreleased/7424-kaovilai

@@ -0,0 +1,2 @@
+Scrub namespace terminating status and deletion timestamp on restore.
+Descriptive restore error when restoring into a terminating namespace.

pkg/util/kube/utils.go

@@ -62,17 +62,22 @@ func NamespaceAndName(objMeta metav1.Object) string {
 }
 
 // EnsureNamespaceExistsAndIsReady attempts to create the provided Kubernetes namespace.
-// It returns three values: a bool indicating whether or not the namespace is ready,
-// a bool indicating whether or not the namespace was created and an error if the creation failed
-// for a reason other than that the namespace already exists. Note that in the case where the
-// namespace already exists and is not ready, this function will return (false, false, nil).
-// If the namespace exists and is marked for deletion, this function will wait up to the timeout for it to fully delete.
-func EnsureNamespaceExistsAndIsReady(namespace *corev1api.Namespace, client corev1client.NamespaceInterface, timeout time.Duration) (bool, bool, error) {
+// It returns three values:
+//   - a bool indicating whether or not the namespace is ready,
+//   - a bool indicating whether or not the namespace was created and an error if the creation failed
+//     for a reason other than that the namespace already exists. Note that in the case where the
+//   - an error if one occurred.
+//
+// examples:
+//
+//	namespace already exists and is not ready, this function will return (false, false, nil).
+//	If the namespace exists and is marked for deletion, this function will wait up to the timeout for it to fully delete.
+func EnsureNamespaceExistsAndIsReady(namespace *corev1api.Namespace, client corev1client.NamespaceInterface, timeout time.Duration) (ready bool, nsCreated bool, err error) {
 	// nsCreated tells whether the namespace was created by this method
 	// required for keeping track of number of restored items
-	var nsCreated bool
-	var ready bool
-	err := wait.PollImmediate(time.Second, timeout, func() (bool, error) {
+	// if namespace is marked for deletion, and we timed out, report an error
+	var terminatingNamespace bool
+	err = wait.PollImmediate(time.Second, timeout, func() (bool, error) {
 		clusterNS, err := client.Get(context.TODO(), namespace.Name, metav1.GetOptions{})
 
 		if apierrors.IsNotFound(err) {
@@ -87,6 +92,7 @@ func EnsureNamespaceExistsAndIsReady(namespace *corev1api.Namespace, client core
 
 		if clusterNS != nil && (clusterNS.GetDeletionTimestamp() != nil || clusterNS.Status.Phase == corev1api.NamespaceTerminating) {
 			// Marked for deletion, keep waiting
+			terminatingNamespace = true
 			return false, nil
 		}
 
@@ -97,15 +103,22 @@ func EnsureNamespaceExistsAndIsReady(namespace *corev1api.Namespace, client core
 
 	// err will be set if we timed out or encountered issues retrieving the namespace,
 	if err != nil {
+		if terminatingNamespace {
+			return false, nsCreated, errors.Wrapf(err, "timed out waiting for terminating namespace %s to disappear before restoring", namespace.Name)
+		}
 		return false, nsCreated, errors.Wrapf(err, "error getting namespace %s", namespace.Name)
 	}
 
 	// In the case the namespace already exists and isn't marked for deletion, assume it's ready for use.
 	if ready {
 		return true, nsCreated, nil
 	}
-
-	clusterNS, err := client.Create(context.TODO(), namespace, metav1.CreateOptions{})
+	namespaceToCreate := namespace.DeepCopy()
+	// scrub deletionTimestamp and Phase if any, which could be coming from backup tarball
+	namespaceToCreate.SetDeletionTimestamp(nil)
+	namespaceToCreate.Status.Phase = ""
+	// Create the namespace
+	clusterNS, err := client.Create(context.TODO(), namespaceToCreate, metav1.CreateOptions{})
 	if apierrors.IsAlreadyExists(err) {
 		if clusterNS != nil && (clusterNS.GetDeletionTimestamp() != nil || clusterNS.Status.Phase == corev1api.NamespaceTerminating) {
 			// Somehow created after all our polling and marked for deletion, return an error

pkg/util/kube/utils_test.go

@@ -36,6 +36,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
+	kfake "k8s.io/client-go/kubernetes/fake"
+
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 	"github.com/vmware-tanzu/velero/pkg/uploader"
@@ -82,6 +84,14 @@ func TestEnsureNamespaceExistsAndIsReady(t *testing.T) {
 			expectedResult:        true,
 			expectedCreatedResult: true,
 		},
+		{
+			name:                  "namespace not found, namespace from backup contained deletionTimestamp, successfully created",
+			expectCreate:          true,
+			expectedResult:        true,
+			expectedCreatedResult: true,
+			nsPhase:               corev1.NamespaceTerminating,
+			nsDeleting:            true,
+		},
 		{
 			name:                  "namespace not found initially, create returns already exists error, returned namespace is ready",
 			alreadyExists:         true,
@@ -115,21 +125,56 @@ func TestEnsureNamespaceExistsAndIsReady(t *testing.T) {
 
 			timeout := time.Millisecond
 
+			var fakeClientObjs []runtime.Object
+			if test.expectNSFound || test.alreadyExists {
+				fakeClientObjs = append(fakeClientObjs, namespace)
+			}
+			clientSet := kfake.NewSimpleClientset(fakeClientObjs...)
+			// using deepcopy to avoid modifying the original namespace object for the mock client tests below
+			firstReady, firstNsCreated, _ := EnsureNamespaceExistsAndIsReady(namespace.DeepCopy(), clientSet.CoreV1().Namespaces(), timeout)
+			assert.Equal(t, test.expectedResult, firstReady)
+			assert.Equal(t, test.expectedCreatedResult, firstNsCreated)
+			// EnsureNamespaceExistsAndIsReady should not timeout when called the second time on the same namespace.
+			// This second call test ensures that the first call did not create a terminating namespace.
+			secondReady, secondNsCreated, err := EnsureNamespaceExistsAndIsReady(namespace.DeepCopy(), clientSet.CoreV1().Namespaces(), timeout)
+			if !test.nsDeleting && !(test.nsPhase == corev1.NamespaceTerminating) {
+				// assert that namespace is ready after second call
+				assert.Equal(t, true, secondReady)
+				// assert that no error such as "namespace is terminating" is returned
+				assert.Nil(t, err)
+			} else if test.alreadyExists || test.expectNSFound {
+				//  if namespace is already existing and is in terminating phase, EnsureNamespaceExistsAndIsReady should return timeout error
+				assert.NotNil(t, err)
+				assert.Contains(t, err.Error(), "timed out waiting for terminating namespace")
+			}
+			// assert that namespace is not created second time
+			assert.Equal(t, false, secondNsCreated)
+
+			// This section tests that Get Not Found and Create Already Exists errors are handled correctly
 			nsClient := &velerotest.FakeNamespaceClient{}
 			defer nsClient.AssertExpectations(t)
-
 			if test.expectNSFound {
 				nsClient.On("Get", "test", metav1.GetOptions{}).Return(namespace, nil)
 			} else {
 				nsClient.On("Get", "test", metav1.GetOptions{}).Return(&corev1.Namespace{}, k8serrors.NewNotFound(schema.GroupResource{Resource: "namespaces"}, "test"))
 			}
-
+			var namespaceToCreate *corev1.Namespace
+			if test.nsDeleting || test.nsPhase == corev1.NamespaceTerminating {
+				namespaceToCreate = namespace.DeepCopy()
+				// scrub deletionTimestamp and Phase if any, which could be coming from backup tarball
+				namespaceToCreate.SetDeletionTimestamp(nil)
+				namespaceToCreate.Status.Phase = ""
+			} else {
+				namespaceToCreate = namespace
+			}
 			if test.alreadyExists {
-				nsClient.On("Create", namespace).Return(namespace, k8serrors.NewAlreadyExists(schema.GroupResource{Resource: "namespaces"}, "test"))
+				// alreadyExists try to create namespace without deletionTimestamp and Phase
+				// and return already exists error with the original namespace which can contain deletionTimestamp and Phase
+				nsClient.On("Create", namespaceToCreate).Return(namespace, k8serrors.NewAlreadyExists(schema.GroupResource{Resource: "namespaces"}, "test"))
 			}
 
 			if test.expectCreate {
-				nsClient.On("Create", namespace).Return(namespace, nil)
+				nsClient.On("Create", namespaceToCreate).Return(namespaceToCreate, nil)
 			}
 
 			result, nsCreated, _ := EnsureNamespaceExistsAndIsReady(namespace, nsClient, timeout)