fix(server): update resourceVersion on Terminate retry (#25650)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
This commit is contained in:
Michael Crenshaw
2025-12-17 16:53:34 -05:00
committed by GitHub
parent 0a2ae95be8
commit df3be1cdf0
2 changed files with 74 additions and 1 deletions

View File

@@ -2455,7 +2455,7 @@ func (s *Server) TerminateOperation(ctx context.Context, termOpReq *application.
}
log.Warnf("failed to set operation for app %q due to update conflict. retrying again...", *termOpReq.Name)
time.Sleep(100 * time.Millisecond)
_, err = s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{})
a, err = s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("error getting application by name: %w", err)
}

View File

@@ -29,6 +29,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
k8sbatchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
@@ -4582,3 +4583,75 @@ func TestServerSideDiff(t *testing.T) {
assert.Contains(t, err.Error(), "application")
})
}
// TestTerminateOperationWithConflicts tests that TerminateOperation properly handles
// concurrent update conflicts by retrying with the fresh application object.
//
// This test reproduces a bug where the retry loop discards the fresh app object
// fetched from Get(), causing all retries to fail with stale resource versions.
func TestTerminateOperationWithConflicts(t *testing.T) {
testApp := newTestApp()
testApp.ResourceVersion = "1"
testApp.Operation = &v1alpha1.Operation{
Sync: &v1alpha1.SyncOperation{},
}
testApp.Status.OperationState = &v1alpha1.OperationState{
Operation: *testApp.Operation,
Phase: synccommon.OperationRunning,
}
appServer := newTestAppServer(t, testApp)
ctx := context.Background()
// Get the fake clientset from the deepCopy wrapper
fakeAppCs := appServer.appclientset.(*deepCopyAppClientset).GetUnderlyingClientSet().(*apps.Clientset)
getCallCount := 0
updateCallCount := 0
// Remove default reactors and add our custom ones
fakeAppCs.ReactionChain = nil
// Mock Get to return original version first, then fresh version
fakeAppCs.AddReactor("get", "applications", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) {
getCallCount++
freshApp := testApp.DeepCopy()
if getCallCount == 1 {
// First Get (for initialization) returns original version
freshApp.ResourceVersion = "1"
} else {
// Subsequent Gets (during retry) return fresh version
freshApp.ResourceVersion = "2"
}
return true, freshApp, nil
})
// Mock Update to return conflict on first call, success on second
fakeAppCs.AddReactor("update", "applications", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) {
updateCallCount++
updateAction := action.(kubetesting.UpdateAction)
app := updateAction.GetObject().(*v1alpha1.Application)
// First call (with original resource version): return conflict
if app.ResourceVersion == "1" {
return true, nil, apierrors.NewConflict(
schema.GroupResource{Group: "argoproj.io", Resource: "applications"},
app.Name,
stderrors.New("the object has been modified"),
)
}
// Second call (with refreshed resource version from Get): return success
updatedApp := app.DeepCopy()
return true, updatedApp, nil
})
// Attempt to terminate the operation
_, err := appServer.TerminateOperation(ctx, &application.OperationTerminateRequest{
Name: ptr.To(testApp.Name),
})
// Should succeed after retrying with the fresh app
require.NoError(t, err)
assert.GreaterOrEqual(t, updateCallCount, 2, "Update should be called at least twice (once with conflict, once with success)")
}