Implement PATCH /v3/service_instances for managed services #4167
Conversation
danail-branekov
left a comment
danail-branekov
left a comment
There was a problem hiding this comment.
Looks promising. The most important bits of my comments are on the managed instance controller
| type PatchManagedSIMessage struct { | ||
| GUID string | ||
| SpaceGUID string | ||
| PlanGUID string |
There was a problem hiding this comment.
I wonder whether PlanGUID should not be a pointer? A patch may want to update the metadata, but not the plan
| p.MetadataPatch.Apply(cfServiceInstance) | ||
| } | ||
|
|
||
| func (p PatchManagedSIMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) { |
There was a problem hiding this comment.
I guess this function should set the plan on the instance spec, right?
| return cfServiceInstanceToRecord(*cfServiceInstance), nil | ||
| } | ||
|
|
||
| func (r *ServiceInstanceRepo) PatchManagedServiceInstance(ctx context.Context, authInfo authorization.Info, message PatchManagedSIMessage) (ServiceInstanceRecord, error) { |
There was a problem hiding this comment.
If I recall correctly, credentials make only sense for UPSIs, therefore dealing with the credentials secret makes no sense for managed services. Speaking of that, the patch message for managed services probably does not need the Credentials field
| log.Error(err, "failed to provision service instance") | ||
| return ctrl.Result{}, fmt.Errorf("failed to provision service instance: %w", err) | ||
| } | ||
| if serviceInstance.Status.ObservedGeneration == 0 { |
There was a problem hiding this comment.
If I get the intention of this check correctly, the idea here is to perform provision when the CFServiceInstance has been just created, otherwise proceed to update.
The generation might drift because of whatever updates (maybe a new label appeared) that may occur while provisioning is already running.
Instead of relying on the observed generation, I would suggest to add PlanGUID to CFServiceInstance.Status. Then the plan guid on the spec would have the semantics of "desired plan", while the plan guid on the status would mean "actual plan". Then if both differ, the controller should perform a plan update. Once the update succeed, the controller should set the new plan guid on the status, thus "writing down" that this is already the actual plan.
Also, it is worth mentioning that once the instance is ready (isReady function returns true), the controller would stop listening for updates. Therefore you should change the function to also check whether the "desired plan" matches the "actual plan"
| lastOpResponse osbapi.LastOperationResponse, | ||
| ) (ctrl.Result, error) { | ||
| if lastOpResponse.State == "succeeded" { | ||
| setObservedGeneration(serviceInstance) |
There was a problem hiding this comment.
setting the observed generation is only committed to the etcd if the reconcile method succeeds. This is why the controller sets it on the very beginning of the reconcile method. If it succeeds, it is committed, if it fails, or needs requeue - it is discarded. Setting it here violates that pattern.
I believe you set it here to figure out whether to provision or update in the reconcile method, but I think my suggestion for the plan guid in the status would provide you with a more reliable criteria
| if osbapi.IsUnrecoveralbeError(err) { | ||
| serviceInstance.Status.LastOperation.State = "failed" | ||
| meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ | ||
| Type: korifiv1alpha1.UpdateFailedCondition, |
There was a problem hiding this comment.
I guess that if the update failed with unrecoverable error, there is no point for the controller to retry update, correct?
If so, you could ammend the isFailed function to return true if the UpdateFailedCondition is true in order to prevent further update attempts with the broker
|
|
||
| if lastOpResponse.State == "failed" { | ||
| meta.SetStatusCondition(&serviceInstance.Status.Conditions, metav1.Condition{ | ||
| Type: korifiv1alpha1.ProvisioningFailedCondition, |
There was a problem hiding this comment.
I think you should set UpdateFailedCondition here
| if err != nil { | ||
| return UpdateResponse{}, fmt.Errorf("update request failed: %w", err) | ||
| } | ||
| if statusCode == http.StatusBadRequest || statusCode == http.StatusConflict || statusCode == http.StatusUnprocessableEntity { |
There was a problem hiding this comment.
According to the osbapi spec, an update operation cannot result into StatusConflict
2 participants
Is there a related GitHub Issue?
The PR is aimed to solve this Issue 4163
What is this change about?
Implementing PATCH /v3/service_instances for managed services
Does this PR introduce a breaking change?
It shouldn't be a breaking change
Acceptance Steps
The PR is still WIP
Tag your pair, your PM, and/or team
danail-branekov