Skip to content
Open
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
20 changes: 6 additions & 14 deletions pkg/migrator/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,19 +209,11 @@ func (m *migrator) migrateOneItem(ctx context.Context, item *unstructured.Unstru
return nil
}
if canRetry(err) {
seconds, delay := errors.SuggestsClientDelay(err)
switch {
case delay && len(namespace) > 0:
klog.Warningf("migration of %s, in the %s namespace, will be retried after a %ds delay: %v", name, namespace, seconds, err)
time.Sleep(time.Duration(seconds) * time.Second)
case delay:
klog.Warningf("migration of %s will be retried after a %ds delay: %v", name, seconds, err)
time.Sleep(time.Duration(seconds) * time.Second)
case !delay && len(namespace) > 0:
klog.Warningf("migration of %s, in the %s namespace, will be retried: %v", name, namespace, err)
default:
klog.Warningf("migration of %s will be retried: %v", name, err)
}
delaySeconds, _ := errors.SuggestsClientDelay(err)
delay := time.Duration(delaySeconds) * time.Second
klog.V(logLevelForTryError(err)).InfoS("Retrying migration",
"object", klog.KRef(namespace, name), "delay", delay, "err", err)
time.Sleep(delay)
continue
}
// error is not retriable
Expand All @@ -244,7 +236,7 @@ func (m *migrator) try(ctx context.Context, namespace, name string, item *unstru
if err == nil {
return false, nil
}
return errors.IsConflict(err), err
return isConflictError(err), err

// TODO: The oc admin uses a defer function to do bandwidth limiting
// after doing all operations. The rate limiter is marked as an alpha
Expand Down
24 changes: 23 additions & 1 deletion pkg/migrator/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/util/net"
"k8s.io/klog/v2"
)

// ErrRetriable is a wrapper for an error that a migrator may use to indicate the
Expand Down Expand Up @@ -52,6 +53,18 @@ func isConnectionRefusedError(err error) bool {
return strings.Contains(err.Error(), "connection refused")
}

// isUIDPreconditionError checks whether the error contains "Precondition failed: UID in precondition".
// This error message is typically present when the resource is deleted before it can be written back.
func isUIDPreconditionError(err error) bool {
return strings.Contains(err.Error(), "Precondition failed: UID in precondition")
}

// isConflictError checks whether the given error is a conflict error.
// This is true when errors.IsConflict or isUIDPreconditionError returns true.
func isConflictError(err error) bool {
return errors.IsConflict(err) || isUIDPreconditionError(err)
}
Comment on lines +56 to +66
Copy link

Choose a reason for hiding this comment

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

If the UID precondition fails, doesn't that imply that the current iteration of the resource was created after the migration started? And if so, is it redundant to "migrate" it?

Copy link
Author

Choose a reason for hiding this comment

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

I think that this issue was originally raised because the migrator was trying to migrate resources that were deleted during migration, so there was a race. So the precondition failed with the stored UID being empty.

Choose a reason for hiding this comment

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

Don't API servers generally return HTTP 404 when that happens? How can it enforce a UID precondition at all if the named object does not exist?

Choose a reason for hiding this comment

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

Is the log spam this PR is trying to address caused by kubernetes/kubernetes#124347?

Copy link
Author

Choose a reason for hiding this comment

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

It seems so, the full error message is the same, just for a different object.


// interpret adds retry information to the provided error. And it might change
// the error to nil.
func interpret(err error) error {
Expand All @@ -63,7 +76,7 @@ func interpret(err error) error {
return nil
case errors.IsMethodNotSupported(err):
return ErrNotRetriable{err}
case errors.IsConflict(err):
case isConflictError(err):
return ErrRetriable{err}
case errors.IsServerTimeout(err):
return ErrRetriable{err}
Expand Down Expand Up @@ -91,3 +104,12 @@ func canRetry(err error) bool {
}
return true
}

func logLevelForTryError(err error) klog.Level {
switch {
case errors.IsNotFound(err), isConflictError(err):
return 2
default:
return 0
}
}