Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1500930 - Prevent multiple deprovision pods from spawning if a job is already in progress #488

Merged
merged 5 commits into from Oct 12, 2017
Merged

Bug 1500930 - Prevent multiple deprovision pods from spawning if a job is already in progress #488

merged 5 commits into from Oct 12, 2017

Conversation

eriknelson
Copy link
Contributor

Describe what this PR does and why we need it:
Returns the correct 202 Accepted response if a deprovision job is already in progress for a requested ServiceInstance DELETE.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 12, 2017
shawn-hurley
shawn-hurley approved these changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VISACK

nice work adding that.

dymurray
dymurray approved these changes Oct 12, 2017
View reviewed changes
Copy link
Member

@dymurray dymurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, worked for me in my testing.

@djzager
Copy link
Member

djzager commented Oct 12, 2017

Test steps:

  1. Provision postgresql
  2. Deprovision postgresql

Before fix:

$ oc get namespaces
NAME                                   STATUS    AGE
...
19c38d71-9ee2-4dc8-ac2e-cdbeb12b8f32   Active    7s
3b7fe989-92e9-458b-86de-96524df6e054   Active    7s
657a984e-e3e0-41d2-b22f-de465dbdea3a   Active    7s
ansible-service-broker                 Active    5m
b2a4833f-8dc8-4811-ad69-bd17ac14b123   Active    7s
d7d4d2ba-faf4-4038-8464-488332f0c353   Active    7s
...

After fix:

$ oc get namespaces
NAME                                   STATUS    AGE
ansible-service-broker                 Active    9m
bab62a51-b76a-4a20-aa2d-098aecf682bb   Active    2s
bar-project                            Active    51s
default                                Active    11m
foo-project                            Active    5m
kube-public                            Active    11m
kube-service-catalog                   Active    11m
kube-system                            Active    11m
myproject                              Active    10m
openshift                              Active    11m
openshift-infra                        Active    11m
openshift-template-service-broker      Active    10m

djzager
djzager approved these changes Oct 12, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknelson
Copy link
Contributor Author

Thank you for the test @djzager !

@eriknelson eriknelson added the bug label Oct 12, 2017
jmrodri
jmrodri approved these changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like it. My only nit was the JobStateAPBMethodTypeVersionAddMoreText but that's it.

pkg/apb/types.go Outdated

// JobStateAPBMethodTypeUnbind - Unbind MethodType const.
JobStateAPBMethodTypeUnbind JobStateAPBMethodType = "unbind"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the variable type a little longer? OOF! add this to tech debt :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JobMethodProvision JobMethod = "provision"

@jmrodri
Copy link
Contributor

jmrodri commented Oct 12, 2017

Merging as others have tested this.

@jmrodri jmrodri merged commit 3568cfe into openshift:master Oct 12, 2017
@eriknelson eriknelson deleted the bz1500930 branch October 13, 2017 13:12
@eriknelson eriknelson mentioned this pull request Oct 17, 2017
Closed
@rthallisey
Copy link
Contributor

#387

jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
@jmrodri
Bug 1500930 - Prevent multiple deprovision pods from spawning if a jo…
9d711ae 
…b is already in progress (openshift#488)

* Add APBMethodType to JobStates

* Add dao support for job retrievals

* Return correct reponse for depro in progress

* Make linter happy

* Use saner names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@djzager djzager djzager approved these changes

@shawn-hurley shawn-hurley shawn-hurley approved these changes

@dymurray dymurray dymurray approved these changes

@rthallisey rthallisey Awaiting requested review from rthallisey

Assignees
No one assigned
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@eriknelson @djzager @jmrodri @rthallisey @shawn-hurley @dymurray @openshift-ci-robot