fix(appset): fall back to create when patch returns NotFound (#17312)#28645
fix(appset): fall back to create when patch returns NotFound (#17312)#28645rickbrouwer wants to merge 4 commits into
Conversation
❗ Preview Environment stop on Bunnyshell failedSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Bundle ReportBundle size has no change ✅ |
blakepettersson
added
cherry-pick/3.3
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #28645 +/- ##
==========================================
+ Coverage 65.22% 65.23% +0.01%
==========================================
Files 426 426
Lines 59741 59748 +7
==========================================
+ Hits 38964 38977 +13
- Misses 17191 17192 +1
+ Partials 3586 3579 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
I don't know if that can still be done that quickly, but given that you (@reggie-k) just indicated at the Contributors Office Hours closing the patch release for 3.4 today, it would be super super super nice if this could still be included. |
There was a problem hiding this comment.
Let's fix the issue at the source. We can update the cacheSyncingClient's patch command to evict the application from the cache on a not found error and return the not found error. That should (almost) immediately requeue the applicationset.
|
Thanks @rumstead ! |
| err := c.Client.Patch(ctx, obj, patch, opts...) | ||
| if err != nil && apierrors.IsNotFound(err) { | ||
| c.evictFromCache(ctx, obj) |
There was a problem hiding this comment.
i think there is still a case where the phantom application would live in the cache. eg, if DeepEqual returns true. I think we just add a comment somewhere and acknowledge that is PR is getting us in a better spot but doesn't cover every case.
There was a problem hiding this comment.
Seems good to me indeed, added
| func (c *cacheSyncingClient) evictFromCache(ctx context.Context, obj client.Object) { | ||
| if _, ok := obj.(*application.Application); !ok { | ||
| return | ||
| } |
There was a problem hiding this comment.
r.Update in removeOwnerReferencesOnDeleteAppSet and r.Delete in deleteInCluster hit the same stale cache NotFound and won't evict. moving this into execAndSyncCache's error branch covers all the verbs and avoids duplicating the guard/getStore/log here.
| func TestEvictFromCacheHandlesGetStoreError(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := runtime.NewScheme() | ||
| require.NoError(t, application.AddToScheme(scheme)) | ||
| c := &cacheSyncingClient{ | ||
| Client: fake.NewClientBuilder().WithScheme(scheme).Build(), | ||
| storesByNs: map[string]k8scache.Store{}, | ||
| getNSCache: func(_ context.Context, _ client.Object) (ctrlcache.Cache, error) { | ||
| return nil, errors.New("no cache") | ||
| }, | ||
| } |
There was a problem hiding this comment.
NIT: these literals duplicate what newClient does. should we parameterize and reuse?
There was a problem hiding this comment.
Pull request overview
Fixes a reconciliation failure mode in the ApplicationSet controller when an Application is manually deleted but the controller’s informer cache remains stale, causing Patch to hit the API server and return NotFound repeatedly.
Changes:
- On
PatchreturningNotFound, evicts the staleApplicationobject from the informer store to force a fresh cache read on the next reconcile. - Adds unit tests covering the eviction behavior and error/skip paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
applicationset/utils/client.go |
Evicts stale cached Application objects when Patch fails with NotFound, enabling self-healing reconcile behavior. |
applicationset/utils/client_test.go |
Adds tests verifying eviction on NotFound and validating safe behavior for non-Application objects and store/cache error paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
@rickbrouwer now there's a conflict |
|
I see. I need to take a look at it for a moment to process this correctly. |
Done. Just a little heads-up: I touched the same block you modified in #28663. Just for information. |
When an Application managed by an ApplicationSet is manually deleted, the applicationset-controller's informer cache can remain stale.
CreateOrUpdatethen reads the deleted Application from the cache, takes the update path, andPatchfails against the API server withapplications.argoproj.io "XXX" not found.The ApplicationSet keeps reporting
UpdateApplicationErroruntil the controller is restarted.The fix is in
cacheSyncingClient.Patch. When Patch returns NotFound, evict the stale object from the informer store. The requeued reconcile sees a fresh cache and takes the Create path, so the ApplicationSet self-heals without a controller restart.Because this is a bug fix, please cherry-pick to release-3.5, release-3.4 and release-3.3.
Fixes #17312
Checklist: