Bug 1499622 - Return 202 if provisioning job is in progress #498
Conversation
openshift-ci-robot
added
the
size/S
dymurray
changed the title
pkg/handler/handler.go
Outdated
| @@ -268,6 +268,8 @@ func (h handler) provision(w http.ResponseWriter, r *http.Request, params map[st | |||
| switch err { | |||
| case broker.ErrorDuplicate: | |||
| writeResponse(w, http.StatusConflict, broker.ProvisionResponse{}) | |||
| case broker.ErrorProvisionInProgress: | |||
| writeResponse(w, http.StatusAccepted, broker.ProvisionResponse{}) | |||
There was a problem hiding this comment.
I would change broker.ProvisionResponse{} to resp.
pkg/broker/broker.go
Outdated
|
|
||
| if alreadyInProgress { | ||
| a.log.Infof("Provision requested for instance %s, but job is already in progress", serviceInstance.ID) | ||
| return nil, ErrorProvisionInProgress |
There was a problem hiding this comment.
I would change nil to &ProvisionResponse{Operation: token} somehow we'll need the token not sure yet.
There was a problem hiding this comment.
👍 deleted my comment that was essentially the same as this.
|
@eriknelson thoughts on my comments about returning a ProvisionResponse with Operation when we are still in progress? |
pkg/broker/broker.go
Outdated
| @@ -619,6 +621,16 @@ func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest, | |||
| // | |||
| // if err is not nil, we will just bubble that up | |||
|
|
|||
| alreadyInProgress, err := a.isProvisionInProgress(serviceInstance) | |||
There was a problem hiding this comment.
I think this call needs to be within the if uuid.Equal.... block.
Reason being if it is a duplicate we want to return duplicate and not give the job token to the other service instance
|
Will review this first thing tomorrow morning. |
dymurray
changed the title
|
Please look at the actual branch... for some reason the PR is showing out of date diffs. |
openshift-ci-robot
added
size/M
|
@jmrodri @shawn-hurley Updated with your feedback. |
|
@jmrodri Does the spec call for the operation token on an InProgress response? |
|
@eriknelson I believe it does here: https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#body-3 |
| if err != nil { | ||
| return nil, fmt.Errorf("An error occurred while trying to determine if a deprovision job is already in progress for instance: %s", instance.ID) | ||
| } | ||
|
|
||
| if alreadyInProgress { | ||
| a.log.Infof("Deprovision requested for instance %s, but job is already in progress", instance.ID) | ||
| return nil, ErrorDeprovisionInProgress | ||
| return &DeprovisionResponse{Operation: jobToken}, ErrorDeprovisionInProgress |
|
@jmrodri good catch re: the Op! |
pkg/broker/broker.go
Outdated
| return len(proJobs) > 0, token, nil | ||
| } | ||
|
|
||
| func (a AnsibleBroker) isDeprovisionInProgress(instance *apb.ServiceInstance) (bool, string, error) { |
There was a problem hiding this comment.
This may be more of a nitpick but I am not a big fan of 2 functions that are so identical to each other. Suggest collapsing the two functions into 1 with a new function argument.
func (a AnsibleBroker) isJobInProgress(instance *apb.ServiceInstance, string method) (bool, string, error) {
...
jobs := dao.MapJobStatesWithMethod(allJobs, method)
...
}
Then update provision/deprovision like:
// example for deprovision
alreadyInProgress, jobToken, err := a.isJobInProgress(&instance, apb.JobMethodDeprovision)
This PR fixes the scenario when the catalog sends two provisioning requests and we return a 200 without checking for jobs in progress.
Changes proposed in this pull request