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.

Sign up for GitHub

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

Jump to bottom

[V2] Fix log spamming from cache misses when updating status repeatedly #679

Closed
wants to merge 2 commits into from
Closed

[V2] Fix log spamming from cache misses when updating status repeatedly #679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9b3002
fix: reduce log spam by patching status updates
rquitales Sep 21, 2024
bfdf78d
fix: always use FieldManager for Create/Patch/Update calls
rquitales Sep 21, 2024
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
2 changes: 1 addition & 1 deletion operator/internal/controller/auto/update_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (r *UpdateReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
meta.SetStatusCondition(&obj.Status.Conditions, *rs.failed)
rs.complete.ObservedGeneration = obj.Generation
meta.SetStatusCondition(&obj.Status.Conditions, *rs.complete)
return r.Status().Update(ctx, obj)
return r.Status().Update(ctx, obj, client.FieldOwner(FieldManager))
}

if rs.complete.Status == metav1.ConditionTrue {
Expand Down
6 changes: 5 additions & 1 deletion operator/internal/controller/auto/workspace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}
return ctrl.Result{}, err
}
// Store a copy of the original workspace to create patches for status updates.
originalWorkspace := w.DeepCopy()

ready := meta.FindStatusCondition(w.Status.Conditions, WorkspaceConditionTypeReady)
if ready == nil {
Expand All @@ -97,7 +99,9 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
w.Status.ObservedGeneration = w.Generation
ready.ObservedGeneration = w.Generation
meta.SetStatusCondition(&w.Status.Conditions, *ready)
return r.Status().Update(ctx, w)
// Use .Status().Patch() instead of .Update() to avoid conflicts with sequential reconcile loops hitting a
// stale version of the object from the cache.
return r.Status().Patch(ctx, w, client.MergeFrom(originalWorkspace), client.FieldOwner(FieldManager))
Copy link
Contributor Author

@rquitales rquitales Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could alternatively do something like the following if we wanted to keep using .Update and reduce log spam:

err := r.Status().Update(...)
if err != nil && isResourceVersionOld(err) {
    return ctrl.Result{Requeue: true}, nil
}
return ctrl.Result{}, err

This will allow the reconcile loop to handle and act on a Stack object that is current. In this scenario, using Patch should be safe, as the spec is up-to-date, it's just that updating a status does not trigger the informer cache to be invalidated.

Copy link
Contributor

@blampe blampe Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at UpdateStatus makes me think patching (or more specifically put'ing) the status is fairly typical, so I'm OK with this. Although it does seem like we've still got some cache issues. Total stab in the dark but I wonder if a call to mgr.GetCache().WaitForCacheSync() before we start doing work would help?

}

if w.DeletionTimestamp != nil {
Expand Down
2 changes: 1 addition & 1 deletion operator/internal/controller/pulumi/stack_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ func (r *StackReconciler) Reconcile(ctx context.Context, request ctrl.Request) (
}

saveStatus := func() error {
if err := r.Status().Update(ctx, instance); err != nil {
if err := r.Status().Update(ctx, instance, client.FieldOwner(FieldManager)); err != nil {
log.Error(err, "unable to save object status")
return err
}
Expand Down