RedisCache - ensure FailedProvisioning=false when Provisioned=true #1395
Conversation
babbageclunk
requested review from
davefellows,
jananivMS,
matthchr,
Porges,
theunrepentantgeek and
WilliamMortlMicrosoft
as code owners
babbageclunk
changed the title
| @@ -50,16 +50,14 @@ func (rc *AzureRedisCacheManager) Ensure(ctx context.Context, obj runtime.Object | |||
| instance.Status.Message = err.Error() | |||
| return false, err | |||
| } | |||
| instance.Status.Message = resourcemanager.SuccessMsg | |||
| instance.Status.State = string(newRc.ProvisioningState) | |||
There was a problem hiding this comment.
minor: I would put the state setting next to the message/provisioned/provisioning setting
| return false, nil | ||
| } else if helpers.ContainsString(catchKnownError, azerr.Type) { | ||
| // TODO: Not sure why we want this to be provisioning=false? |
There was a problem hiding this comment.
It comes down to a question of what we want provisioning to mean... I think what the original ASO authors were trying to convey with it was: "I am currently waiting for Azure to actually make the thing", and so they didn't count "waiting in Kubernetes for some reason" as provisioning = true.
I don't think that's really a good interpretation though because it's not stable: We set provisioning = false here but also return false which means "try again" - causing the reconcile loop to come back through and it the SetProvisioning on line 71... so the resource is going to blip between provisioning/not until whatever knownError is resolved. I think it's clearer to just say that it's provisioning the whole time and also include the message if needed explaining what is going on.
Since we already set the message to include the error details on line 74 I think you can just delete this line.
A user reported a case where a RedisCache was provisioned and working but was marked as FailedProvisioning=true. This seems to have happened because it initially failed (possibly a temporary error that we aren't checking for), and then on a subsequent periodic reconciliation the ARM query reported that it was deployed successfully. Use the SetProvisioned helper method to prevent this state.
babbageclunk
force-pushed
the
rediscache-provisioning-flags
branch
from
49977b0 to
9544c53
Compare
What this PR does / why we need it:
A user reported a case where a RedisCache was provisioned and working but was marked as FailedProvisioning=true. This seems to have happened because it initially failed (possibly a temporary error that we aren't checking for), and then on a subsequent periodic reconciliation the ARM query reported that it was deployed successfully. Use the SetProvisioned helper method (and friends) to prevent this state.
Fixes #1394
Special notes for your reviewer:
If applicable: