POC - Refactor Status Updater to be resource agnostic #1787
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| package status2 | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "slices" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" | ||
| "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller" | ||
| ) | ||
|
|
||
| // K8sUpdater updates a resource from the k8s API. | ||
| // It allows us to mock the client.Reader.Status.Update method. | ||
| type K8sUpdater interface { | ||
| // Update is from client.StatusClient.SubResourceWriter. | ||
| Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error | ||
| } | ||
|
|
||
| type UpdateRequest struct { | ||
| NsName types.NamespacedName | ||
| ResourceType client.Object | ||
| Setter Setter | ||
| } | ||
|
|
||
| type Setter func(client.Object) bool | ||
|
|
||
| type Updater struct { | ||
| client client.Client | ||
| logger logr.Logger | ||
| } | ||
|
|
||
| func NewUpdater(client client.Client, logger logr.Logger) *Updater { | ||
| return &Updater{ | ||
| client: client, | ||
| logger: logger, | ||
| } | ||
| } | ||
|
|
||
| func (u *Updater) Update(ctx context.Context, reqs ...UpdateRequest) { | ||
| for _, r := range reqs { | ||
| u.writeStatuses(ctx, r.NsName, r.ResourceType, r.Setter) | ||
| } | ||
| } | ||
|
|
||
| func (u *Updater) writeStatuses( | ||
| ctx context.Context, | ||
| nsname types.NamespacedName, | ||
| obj client.Object, | ||
| statusSetter Setter, | ||
| ) { | ||
| err := wait.ExponentialBackoffWithContext( | ||
| ctx, | ||
| wait.Backoff{ | ||
| Duration: time.Millisecond * 200, | ||
| Factor: 2, | ||
| Jitter: 0.5, | ||
| Steps: 4, | ||
| Cap: time.Millisecond * 3000, | ||
| }, | ||
| // Function returns true if the condition is satisfied, or an error if the loop should be aborted. | ||
| NewRetryUpdateFunc(u.client, u.client.Status(), nsname, obj, u.logger, statusSetter), | ||
| ) | ||
| if err != nil && !errors.Is(err, context.Canceled) { | ||
| u.logger.Error( | ||
| err, | ||
| "Failed to update status", | ||
| "namespace", nsname.Namespace, | ||
| "name", nsname.Name, | ||
| "kind", obj.GetObjectKind().GroupVersionKind().Kind) | ||
| } | ||
| } | ||
|
|
||
| // NewRetryUpdateFunc returns a function which will be used in wait.ExponentialBackoffWithContext. | ||
| // The function will attempt to Update a kubernetes resource and will be retried in | ||
| // wait.ExponentialBackoffWithContext if an error occurs. Exported for testing purposes. | ||
| // | ||
| // wait.ExponentialBackoffWithContext will retry if this function returns nil as its error, | ||
| // which is what we want if we encounter an error from the functions we call. However, | ||
| // the linter will complain if we return nil if an error was found. | ||
| // | ||
| //nolint:nilerr | ||
| func NewRetryUpdateFunc( | ||
| getter controller.Getter, | ||
| updater K8sUpdater, | ||
| nsname types.NamespacedName, | ||
| obj client.Object, | ||
| logger logr.Logger, | ||
| statusSetter func(client.Object) bool, | ||
| ) func(ctx context.Context) (bool, error) { | ||
| return func(ctx context.Context) (bool, error) { | ||
| // The function handles errors by reporting them in the logs. | ||
| // We need to get the latest version of the resource. | ||
| // Otherwise, the Update status API call can fail. | ||
| // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. | ||
| // the default is configurable in the Manager options. | ||
| if err := getter.Get(ctx, nsname, obj); err != nil { | ||
| // apierrors.IsNotFound(err) can happen when the resource is deleted, | ||
| // so no need to retry or return an error. | ||
| if apierrors.IsNotFound(err) { | ||
| return true, nil | ||
| } | ||
|
|
||
| logger.V(1).Info( | ||
| "Encountered error when getting resource to update status", | ||
| "error", err, | ||
| "namespace", nsname.Namespace, | ||
| "name", nsname.Name, | ||
| "kind", obj.GetObjectKind().GroupVersionKind().Kind, | ||
| ) | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| if !statusSetter(obj) { | ||
| logger.V(1).Info( | ||
| "Skipping status update because there's no change", | ||
| "namespace", nsname.Namespace, | ||
| "name", nsname.Name, | ||
| "kind", obj.GetObjectKind().GroupVersionKind().Kind, | ||
| ) | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| if err := updater.Update(ctx, obj); err != nil { | ||
| logger.V(1).Info( | ||
| "Encountered error updating status", | ||
| "error", err, | ||
| "namespace", nsname.Namespace, | ||
| "name", nsname.Name, | ||
| "kind", obj.GetObjectKind().GroupVersionKind().Kind, | ||
| ) | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| type GroupUpdateRequest struct { | ||
| Name string | ||
| Request []UpdateRequest | ||
| } | ||
|
|
||
| type CachingGroupUpdater struct { | ||
|
There was a problem hiding this comment. Not sure this is the best name. Yes, the updater is caching, but it is not clear from the name why you would want to use this updater. We are caching so that we can replay the statuses when a leader changes. There was a problem hiding this comment. How about DelayedGroupUpdater There was a problem hiding this comment.
Is that true? Updates are made immediately if enabled... There was a problem hiding this comment. Isn't it a leader election updater? I also don't know if we need There was a problem hiding this comment.
yep. but leader election is a signal to enable it. so that's why there is Enable method. and from that perspective, since the type isn't really coupled to leader election otherwise, I didn't put Leader in its name.
If the developer wants to update statuses without leader election - Updater (like in provisioner mode) If the developer uses leader election - GroupUpdater Note that we introduce Groups because the static mode handler updates statuses separately for:
With having groups for each of those, we can replace the statuses of each group after each update by the handler. This is only needed until the Updater is enabled. We don't really need to store updates after the updater is Enabled - we can update right way. I can update the code to reflect that. There was a problem hiding this comment. This makes sense, but I still think the name should reflect the purpose of the interface. I also feel that the term group is ambiguous, but I can't think of a better name. There was a problem hiding this comment. Do you feel the same way about DelayedGroupUpdater? There was a problem hiding this comment. I think DeferredGroupUpdater is a better description of what the updater does, but I still don't think it conveys that it is safe for leader election. I think you'll still end up with a comment. Naming is hard, and I'm definitely not going to block the PR over this, but I don't see the harm in calling it what it is. I'd feel differently if there were another purpose for this Updater, but it is purpose-built for leader election. Why call it anything else? Coupling isn't always bad when two things are actually related. There was a problem hiding this comment. what about LeaderAwareGroupUpdater? |
||
| updater *Updater | ||
| lock *sync.Mutex | ||
| groups map[string]GroupUpdateRequest | ||
| enabled bool | ||
| } | ||
|
|
||
| func NewCachingGroupUpdater(updater *Updater) *CachingGroupUpdater { | ||
| return &CachingGroupUpdater{ | ||
| updater: updater, | ||
| lock: &sync.Mutex{}, | ||
| groups: make(map[string]GroupUpdateRequest), | ||
| } | ||
| } | ||
|
|
||
| func (u *CachingGroupUpdater) Update(ctx context.Context, update GroupUpdateRequest) { | ||
| u.lock.Lock() | ||
| defer u.lock.Unlock() | ||
|
|
||
| if len(update.Request) == 0 { | ||
| delete(u.groups, update.Name) | ||
| } | ||
|
|
||
| u.groups[update.Name] = update | ||
|
|
||
| if !u.enabled { | ||
| return | ||
| } | ||
|
|
||
| u.updater.Update(ctx, update.Request...) | ||
| } | ||
|
|
||
| func (u *CachingGroupUpdater) Enable(ctx context.Context) { | ||
| u.lock.Lock() | ||
| defer u.lock.Unlock() | ||
|
|
||
| u.enabled = true | ||
|
|
||
| for _, update := range u.groups { | ||
| u.updater.Update(ctx, update.Request...) | ||
| } | ||
| } | ||
|
|
||
| func ConditionsEqual(prev, cur []v1.Condition) bool { | ||
| return slices.EqualFunc(prev, cur, func(c1, c2 v1.Condition) bool { | ||
| if c1.ObservedGeneration != c2.ObservedGeneration { | ||
| return false | ||
| } | ||
|
|
||
| if c1.Type != c2.Type { | ||
| return false | ||
| } | ||
|
|
||
| if c1.Status != c2.Status { | ||
| return false | ||
| } | ||
|
|
||
| if c1.Message != c2.Message { | ||
| return false | ||
| } | ||
|
|
||
| return c1.Reason == c2.Reason | ||
| }) | ||
| } | ||
|
|
||
| func ConvertConditions( | ||
| conds []conditions.Condition, | ||
| observedGeneration int64, | ||
| transitionTime v1.Time, | ||
| ) []v1.Condition { | ||
| apiConds := make([]v1.Condition, len(conds)) | ||
|
|
||
| for i := range conds { | ||
| apiConds[i] = v1.Condition{ | ||
| Type: conds[i].Type, | ||
| Status: conds[i].Status, | ||
| ObservedGeneration: observedGeneration, | ||
| LastTransitionTime: transitionTime, | ||
| Reason: conds[i].Reason, | ||
| Message: conds[i].Message, | ||
| } | ||
| } | ||
|
|
||
| return apiConds | ||
| } | ||
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.
What are we expecting for name? Does each status update require a unique name?
update-{idx}? Or is this field used to differentiate between types of status updates (gateway API resources v/s ngf resources).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.
each group requires a unique name.
yep
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.
Why not use a typed string or an enum? I think a string is too ambiguous
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.
it is up to the client to decide how many groups to use. For this perspective, string looks reasonable to me
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.
Okay, I suggest adding the groups as consts on the client side so that they are documented somewhere. I also recommend describing the purpose of the group name as a comment in the final code. It's not clear what the group name is for.