Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Descriptive restore error on timeout due to terminating namespace #7424

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/unreleased/7424-kaovilai
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Descriptive restore error when restoring into a terminating namespace.
27 changes: 18 additions & 9 deletions pkg/util/kube/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,23 @@ 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
// - 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
kaovilai marked this conversation as resolved.
Show resolved Hide resolved
// required for keeping track of number of restored items
kaovilai marked this conversation as resolved.
Show resolved Hide resolved
var nsCreated bool
var ready bool
err := wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (bool, error) {
// if namespace is marked for deletion, and we timed out, report an error
var terminatingNamespace bool
err = wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (bool, error) {
clusterNS, err := client.Get(ctx, namespace.Name, metav1.GetOptions{})
// if namespace is marked for deletion, and we timed out, report an error

if apierrors.IsNotFound(err) {
// Namespace isn't in cluster, we're good to create.
Expand All @@ -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
}

Expand All @@ -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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as this call is used during restore as well, and if such error occur for the resource during restore, can we also add this to restore warnings list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention #7516

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is warning preferred over restore error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also per PR title, the intent is that the error are visible in restore error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changes do you see needed to be made to the PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry on the late reply, I was awk.
Yes, overall its good to have this error that you added and its visible during restore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reducing the time to address the issue #7516 we need to look at other options.

return false, nsCreated, errors.Wrapf(err, "error getting namespace %s", namespace.Name)
}

Expand Down
Loading