chore: Fix to intermittent E2E test failures in deployment_test.go #20974
Conversation
Signed-off-by: Jonathan West <jonwest@redhat.com>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20974 +/- ##
==========================================
+ Coverage 54.99% 55.04% +0.04%
==========================================
Files 324 324
Lines 55466 55466
==========================================
+ Hits 30505 30530 +25
+ Misses 22341 22322 -19
+ Partials 2620 2614 -6 ☔ View full report in Codecov by Sentry. |
| return (err == nil && token != ""), nil | ||
| }) | ||
| require.NoError(t, waitErr) | ||
| require.NotEmpty(t, token) |
There was a problem hiding this comment.
Do you know what other process can modify it? It might be a sign of some race condition in ArgoCD, which we may need to fix.
There was a problem hiding this comment.
My expectation is that its a core Kubernetes controller that happens to modify the ServiceAccount at the same time as Argo CD. Resource contention on shared resources is pretty common in cases like this. Retrying/re-reconciling on resource update error is a pretty standard controller/operator pattern.
| token, err = clusterauth.GetServiceAccountBearerToken(KubeClientset, ns.Name, serviceAccountName, time.Second*60) | ||
|
|
||
| // Success is no error and a real token, otherwise keep trying | ||
| return (err == nil && token != ""), nil |
There was a problem hiding this comment.
Shall we return err here, not nil?
There was a problem hiding this comment.
Returning non-nil error here is fatal: it will stop the loop, as per PollUntilContextCancel docs. But, in this case, if err is non-nil, this is only a temporary failure, which is likely to be resolved by subsequent iterations. Thus it is not appropriate to return non-nil error from within the loop.
However, if the loop is not able to complete in the allotted timeframe, the context is canceled, and error is returned from the function, which will be caught and fail the require
…rgoproj#20974) Signed-off-by: Jonathan West <jonwest@redhat.com> Signed-off-by: Adrian Aneci <aneci@adobe.com>
…rgoproj#20974) Signed-off-by: Jonathan West <jonwest@redhat.com>
This PR is a simple test update to fix flaky E2E tests.
A couple of E2E tests (that I originally wrote some time ago) are intermittently failing due to a race condition in acquisition of the
ServiceAccountsecret.Both
TestArgoCDSupportsMultipleServiceAccountsWithDifferingRBACOnSameClusterandTestDeployToKubernetesAPIURLWithQueryParameter, indeployment_test.go, attempt to acquire aServiceAccountusingGetServiceAccountBearerTokenfromclusterauth.go. However, within that package,createServiceAccountTokencan fail with the following error:failed to patch serviceaccount "e2e-test-user1-serviceaccount" with bearer token secret: Operation cannot be fulfilled on serviceaccounts "e2e-test-user1-serviceaccount": the object has been modified; please apply your changes to the latest version and try againAs you can probably guess, this is because another process is modifying the resource before our code can complete the patch.
The proposed fix is to retry on failure, up to a maximum of 20 seconds.
I have run these tests over and over for several hours, and did not hit the issue, so as best as I can tell the issue is resolved with this PR.
Example of failing run:
Checklist: