fix: race condition when updating registry settings #4036
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(cherry picked from commit 004d72d)
cbodonnell
commented
Sep 13, 2023
Comment on lines
+213
to
+225
| skipImagePush := updateAppRegistryRequest.IsReadOnly | ||
| if foundApp.IsAirgap { | ||
| // TODO: pushing images not yet supported in airgapped instances. | ||
| skipImagePush = true | ||
| } | ||
|
|
||
| latestSequence, err := store.GetStore().GetLatestAppSequence(foundApp.ID, true) | ||
| if err != nil { | ||
| logger.Error(errors.Wrapf(err, "failed to get latest app sequence for app %s", foundApp.Slug)) | ||
| updateAppRegistryResponse.Error = err.Error() | ||
| JSON(w, http.StatusInternalServerError, updateAppRegistryResponse) | ||
| return | ||
| } |
There was a problem hiding this comment.
Moved this bit out of the goroutine since we don't want to set an initial task status of "running" if the query fails.
cbodonnell
changed the title
cbodonnell
commented
Sep 13, 2023
| @@ -22,7 +22,7 @@ metadata: | |||
| name: capacity-res-job | |||
| spec: | |||
| parallelism: 4 | |||
| backoffLimit: 0 | |||
| backoffLimit: 6 | |||
There was a problem hiding this comment.
Unrelated to the issue, but should fix flakiness we've been seeing in this test as it will allow this job, which reserves GKE autopilot capacity, to be retried.
divolgin
approved these changes
Sep 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(cherry picked from commit 004d72d)
What this PR does / why we need it:
This PR fixes a race condition where the backend would return a response from the PUT
/api/v1/app/{appSlug}/registryhandler before the image re-writing task started. This could result in a scenario where the UI polls the/api/v1/app/{appSlug}/imagerewritestatusendpoint before there is a task status, so the backend will return a response indicating that image re-writing is complete even though it has not even begun. This causes the UI to not display the spinner and status messages, as expected.Example of this in our tests: https://app.testim.io/#/project/wpYAooUimFDgQxY73r17/branch/ricardomaraschini%2Fsc-86964%2Fkots-test-workflow-for-helmvm/test/aPhWnwQiqtBxx2sn/step/wbZJmRizYrvNWuVc/viewer/screenshots?result-id=XCLtQZlJnzWmYF5k&path=w5blTCkqUInoZuUc%3AwbZJmRizYrvNWuVc
Which issue(s) this PR fixes:
See testim link above...
Special notes for your reviewer:
Changed the Registry Settings step in
validate-smoke-testto mark an error if the loading indicator does not show to add coverage for the UI (link).Steps to reproduce
It's a race condition, so only happens occasionally. To reproduce, change the registry settings and save. The UI will not always show the loading indicator and status messages.
Does this PR introduce a user-facing change?
Does this PR require documentation?
NONE