-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ block move with annotation #8690
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |||
"fmt" | ||||
"os" | ||||
"path/filepath" | ||||
"time" | ||||
|
||||
"github.com/pkg/errors" | ||||
corev1 "k8s.io/api/core/v1" | ||||
|
@@ -32,6 +33,7 @@ import ( | |||
kerrors "k8s.io/apimachinery/pkg/util/errors" | ||||
"k8s.io/apimachinery/pkg/util/sets" | ||||
"k8s.io/apimachinery/pkg/util/version" | ||||
"k8s.io/apimachinery/pkg/util/wait" | ||||
"k8s.io/klog/v2" | ||||
"sigs.k8s.io/controller-runtime/pkg/client" | ||||
|
||||
|
@@ -333,6 +335,19 @@ func (o *objectMover) move(graph *objectGraph, toProxy Proxy, mutators ...Resour | |||
return errors.Wrap(err, "error pausing ClusterClasses") | ||||
} | ||||
|
||||
log.Info("Waiting for all resources to be ready to move") | ||||
// exponential backoff configuration which returns durations for a total time of ~2m. | ||||
// Example: 0, 5s, 8s, 11s, 17s, 26s, 38s, 57s, 86s, 128s | ||||
waitForMoveUnblockedBackoff := wait.Backoff{ | ||||
Duration: 5 * time.Second, | ||||
nojnhuh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
Factor: 1.5, | ||||
Steps: 10, | ||||
Jitter: 0.1, | ||||
} | ||||
if err := waitReadyForMove(o.fromProxy, graph.getMoveNodes(), o.dryRun, waitForMoveUnblockedBackoff); err != nil { | ||||
return errors.Wrap(err, "error waiting for resources to be ready to move") | ||||
} | ||||
|
||||
// Nb. DO NOT call ensureNamespaces at this point because: | ||||
// - namespace will be ensured to exist before creating the resource. | ||||
// - If it's done here, we might create a namespace that can end up unused on target cluster (due to mutators). | ||||
|
@@ -595,6 +610,55 @@ func setClusterClassPause(proxy Proxy, clusterclasses []*node, pause bool, dryRu | |||
return nil | ||||
} | ||||
|
||||
func waitReadyForMove(proxy Proxy, nodes []*node, dryRun bool, backoff wait.Backoff) error { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great if we can collect wait for move during discovery, then we can make this a no-op for most of the object, and implement a better UX for objects that are actually blocking move by printing "Wating for ... to unblock move" before starting to wait There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that seems better. I can take a look at that as a follow-up. |
||||
if dryRun { | ||||
return nil | ||||
} | ||||
|
||||
log := logf.Log | ||||
|
||||
c, err := proxy.NewClient() | ||||
if err != nil { | ||||
return errors.Wrap(err, "error creating client") | ||||
} | ||||
|
||||
for _, n := range nodes { | ||||
obj := &metav1.PartialObjectMetadata{ | ||||
ObjectMeta: metav1.ObjectMeta{ | ||||
Name: n.identity.Name, | ||||
Namespace: n.identity.Namespace, | ||||
}, | ||||
TypeMeta: metav1.TypeMeta{ | ||||
APIVersion: n.identity.APIVersion, | ||||
Kind: n.identity.Kind, | ||||
}, | ||||
} | ||||
key := client.ObjectKeyFromObject(obj) | ||||
log := log.WithValues("apiVersion", obj.GroupVersionKind(), "resource", klog.KObj(obj)) | ||||
|
||||
blockLogged := false | ||||
if err := retryWithExponentialBackoff(backoff, func() error { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it is implemented the backoff applies to every CR, thus assuming that we are moving 100 objects, this can result in 100*5sec, which is a lot. Is that the expected behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I had in mind for the time being. I generally wouldn't expect this to ever wait for That same parallelism could be applied to the waits here as well, but that didn't seem worth the additional complexity under the assumption that resources are going to be paused in parallel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got your point, but It is not ideal because ultimately the time the users have to wait can vary in a significant way, however, I can be ok with this approach for the first iteration if we can get a better UX as suggested in other comments. TL;DR; if (and only if) there is a block on an object, before starting the wait loop we should log "Waiting for the block to be removed on ..." or something similar that makes the users aware of where the move process is blocked at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my read of the code, it looks like the "resource is not ready to move" error returned here will get logged here which happens before any
Does that cover what you describe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, this is the debug of every retry and it is very noisy; it will also be printed for every object, not only the objects with an actual block move. So it does not comply with
It just provides debug/trace details on every step of the wait loop when you are already in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you'd like to see a single log message indicating that at least one object is blocking the move? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. obj1 no block move annotation --> no message at all (and possibly skip entirely the wait loop) NOTE: this is consistent with most of the logs in clusterctl, where we inform about the next operation before starting it, which is even more important in this case because otherwise the system will seem to be stuck waiting; in this case the caveat is that according to the spec we should wait only for objects with the annotation, not for all the objects (this will be much easier if you collect collect wait for move during discovery, as I have suggested in another comment on the same topic) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, I've updated this to do that. FWIW we need to enter the wait loop no matter what to determine if the annotation exists, but the loop is exited as soon as the annotation doesn't exist (which may be in the first iteration). You already suggested gathering whether the resource is blocked during object discovery which could make this short-circuit earlier, but I'd like to do that as a follow-up if that's still acceptable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this would also be good to track in an issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #9196 |
||||
if err := c.Get(ctx, key, obj); err != nil { | ||||
return errors.Wrapf(err, "error getting %s/%s", obj.GroupVersionKind(), key) | ||||
} | ||||
|
||||
if _, exists := obj.GetAnnotations()[clusterctlv1.BlockMoveAnnotation]; exists { | ||||
if !blockLogged { | ||||
log.Info(fmt.Sprintf("Move blocked by %s annotation, waiting for it to be removed", clusterctlv1.BlockMoveAnnotation)) | ||||
blockLogged = true | ||||
CecileRobertMichon marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
return errors.Errorf("resource is not ready to move: %s/%s", obj.GroupVersionKind(), key) | ||||
} | ||||
log.V(5).Info("Resource is ready to move") | ||||
return nil | ||||
}); err != nil { | ||||
return err | ||||
} | ||||
} | ||||
|
||||
return nil | ||||
} | ||||
|
||||
// patchCluster applies a patch to a node referring to a Cluster object. | ||||
func patchCluster(proxy Proxy, n *node, patch client.Patch, mutators ...ResourceMutatorFunc) error { | ||||
cFrom, err := proxy.NewClient() | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, are these the actual numbers it would use or did you pull this from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jitter makes this inherently random to some degree, but I threw together something to simulate what these numbers could be and this is one example.