reconciler/managed: avoid temporary data loss to managed on annotation update #526
Conversation
sttts
changed the title
pkg/reconciler/managed/api.go
Outdated
| @@ -172,12 +172,18 @@ func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnno | |||
| // reset to their current state according to the API server. | |||
There was a problem hiding this comment.
Perhaps change this comment from "pending changes are reset" to "pending changes may be reset".
|
Do you think it's worth "E2E" testing this? It would have to be manual at the moment, unfortunately. (i.e. Build a provider using the updated reconciler.) |
That would be a tricky e2e. Certainly doable, but on the borderline of a test belonging into e2e. |
|
Actually, we should only retry with a Get on conflict. Let me change that before merging. On any other error we can just keep the old object and retry. |
sttts
force-pushed
the
sttts-managed-reconcile-avoid-temporary-data-loss
branch
from
f6a2f41 to
e2684ab
Compare
|
Have extended the unit tests and only get on conflict. |
pkg/reconciler/managed/api_test.go
Outdated
| want error | ||
| wantGetCalled bool |
There was a problem hiding this comment.
Nit: Typically once we want more than one thing we nest them into a want struct (similar to args).
pkg/reconciler/managed/api_test.go
Outdated
| getCalled := tc.args.o.GetAnnotations()["getcalled"] == "true" | ||
| if getCalled != tc.wantGetCalled { | ||
| t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...) calling get: -want %v, +got %v", tc.reason, tc.wantGetCalled, getCalled) | ||
| } |
There was a problem hiding this comment.
// tc.want.o replaces tc.wantGetCalled
if diff := cmp.Diff(tc.want.o, tc.args.o,); diff != "" {
t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff)
}Nit: Typically in this case where we're testing for a mutation of the supplied object we'd use cmp.Diff on the whole object as above. A little overkill here, but more extensible if we ever want to test more things about the object we're mutating.
There was a problem hiding this comment.
Also, I like this pattern of adding an annotation to indicate that the fake client operated on the object. Could be worth generalizing and adopting elsewhere.
sttts
force-pushed
the
sttts-managed-reconcile-avoid-temporary-data-loss
branch
from
e2684ab to
50c75a1
Compare
…n update Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
sttts
force-pushed
the
sttts-managed-reconcile-avoid-temporary-data-loss
branch
from
50c75a1 to
5b4ebc1
Compare
Description of your changes
When storing critical annotations, the retry loop always dropped changes to the
managedresource object, i.e.part of the reconcile loop's effects on
managedwere just lost, always. This PR changes this to try one updateof the resource without reset (aka client get call), and only after that fall back into best-effort code path. In effect, this
might save a number of (expensive) reconciles.
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested