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

Bug 1476173 - Cleanup deleting namespaces #529

Merged
merged 2 commits into from Nov 3, 2017
Merged

Bug 1476173 - Cleanup deleting namespaces #529

merged 2 commits into from Nov 3, 2017

Conversation

cfchase
Copy link
Contributor

@cfchase cfchase commented Nov 2, 2017

Redo deprovision of apbs in terminating namespaces to work when autoescalate is false. Skip user permission verification if the namespace is being deleted and clean up our etcd without running deprovision.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2017
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.

This is what we were discussing. Looks okay to me. ACK

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.

Just one question but LGTM otherwise

if ok, status, err := h.validateUser(userInfo.Username, serviceInstance.Context.Namespace); !ok {
writeResponse(w, status, broker.ErrorResponse{Description: err.Error()})
return
if !nsDeleted {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to check and not run the pod during unbind like deprovision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't put this in for now, since I believe launch_apb_on_bind would have to be turned on. I can add it, as long as we think it's worth doing now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -105,7 +105,7 @@ func setFailedDeprovisionJob(dao *dao.Dao, dmsg *DeprovisionMsg) {
}

func cleanupDeprovision(
podName string, instance *apb.ServiceInstance, dao *dao.Dao, log *logging.Logger,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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

@shawn-hurley shawn-hurley merged commit f8028a6 into openshift:master Nov 3, 2017
@cfchase cfchase deleted the ns-delete branch November 29, 2017 14:17
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
* Bug 1476173 - Skip user permission verification on namespace if
it's terminating and clean up the data without executing the
deprovision.

* Bug 1476173 - Skipping apb launch on unbind if namespace is deleting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants