diff --git a/changelogs/unreleased/7424-kaovilai b/changelogs/unreleased/7424-kaovilai new file mode 100644 index 0000000000..464460386e --- /dev/null +++ b/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. \ No newline at end of file diff --git a/pkg/util/kube/utils.go b/pkg/util/kube/utils.go index e1ed48dba2..7d3a6385e4 100644 --- a/pkg/util/kube/utils.go +++ b/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,6 +103,9 @@ 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) } @@ -104,8 +113,12 @@ func EnsureNamespaceExistsAndIsReady(namespace *corev1api.Namespace, client core 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 diff --git a/pkg/util/kube/utils_test.go b/pkg/util/kube/utils_test.go index 31110dc8c0..a423aa60d9 100644 --- a/pkg/util/kube/utils_test.go +++ b/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)