Implement instances last operation #3628
Conversation
zabanov-lab
force-pushed
the
issues/3536-managed-service-last-operation-attempt-2
branch
from
402ac26
to
6e7b8dd
Compare
Previously we were setting the ProvisioningFailedCondition to true if the broker returned error on provision, no matter the error. The problem with doing this is that the error can be recoverable, such as the network flaking, but the instance is sent to a state from which it cannot recover (When the ProvisioningFailedCondition is set the reconciler gives up early in the reconciliation loop). This change fixes the problem described above by defining a new UnrecoverableError type in the osbapi client. The controller would then set the ProvisioningFailedCondition only if the error it received is an UnrecoverableError. Also LastOperation field is added in CFServiceInstance and there is a logic implemented in the controllers about the different states and this last operation information is propagated to the presenter logic According to the OSBAPI spec unrecoverable perovision error codes are 400, 409 and 422. Co-authored-by: Danail Branekov <[email protected]>
Co-authored-by: Danail Branekov <[email protected]>
zabanov-lab
force-pushed
the
issues/3536-managed-service-last-operation-attempt-2
branch
from
6e7b8dd
to
b275ca1
Compare
| @@ -36,6 +37,11 @@ var _ = Describe("Service Instance", func() { | |||
| Labels: map[string]string{ | |||
| "foo": "bar", | |||
| }, | |||
| LastOperation: korifiv1alpha1.LastOperation{ | |||
There was a problem hiding this comment.
Presenters and handlers are not supposed to reference kubernetes types directly. We can move this type to model/services/instances.go so that both the presenter and the CRD can reference it from there.
| }) | ||
| }) | ||
|
|
||
| When("polling the initial instance last operation", func() { |
There was a problem hiding this comment.
The OSBAPI broker will never return initial state, see spec. This makes the whole When redundant.
| cfServiceInstance.Status.LastOperation = korifiv1alpha1.LastOperation{ | ||
| Type: "create", | ||
| State: "failed", | ||
| } | ||
| return ctrl.Result{}, k8s.NewNotReadyError().WithCause(err).WithReason("SecretInvalid") |
There was a problem hiding this comment.
We should not requeue this error as an invalid secret can only be fiexed by the client. We should return NewNotReadyError. ... .WithNoRequeue. As this is hard to test and the test's value is arguable we can skip the test in this case.
| return ctrl.Result{}, k8s.NewNotReadyError().WithCause(err).WithReason("SecretInvalid") | ||
| } | ||
|
|
||
| log.V(1).Info("credentials secret", "name", credentialsSecret.Name, "version", credentialsSecret.ResourceVersion) | ||
| cfServiceInstance.Status.Credentials = corev1.LocalObjectReference{Name: credentialsSecret.Name} | ||
|
|
||
| if cfServiceInstance.Status.CredentialsObservedVersion == "" { |
There was a problem hiding this comment.
We should extract these two conditionals in a helper function that returns last operation to tidy up the main loop.
| @@ -46,6 +46,13 @@ var _ = Describe("CFServiceInstance", func() { | |||
| }, | |||
| } | |||
| Expect(adminClient.Create(ctx, instance)).To(Succeed()) | |||
|
|
|||
| Expect(k8s.Patch(ctx, adminClient, instance, func() { | |||
There was a problem hiding this comment.
No need to set last operation here - the controller will do this in the reconciliation loop
Previously we were setting the ProvisioningFailedCondition to true if the broker returned error on provision, no matter the error. The problem with doing this is that the error can be recoverable, such as the network flaking, but the instance is sent to a state from which it cannot recover (When the ProvisioningFailedCondition is set the reconciler gives up early in the reconciliation loop).
This change fixes the problem described above by defining a new UnrecoverableError type in the osbapi client. The controller would then set the ProvisioningFailedCondition only if the error it received is an UnrecoverableError.
Also LastOperation field is added in CFServiceInstance and there is a logic implemented in the controllers about the different states and this last operation information is propagated to the presenter logic
According to the OSBAPI spec unrecoverable perovision error codes are 400, 409 and 422.
Is there a related GitHub Issue?
#3536
What is this change about?
Implement instances last operation
Does this PR introduce a breaking change?
No