Delete bindings when deleting a service instance #2150

jeff-phillips-18 · 2017-09-22T22:42:16Z

jeff-phillips-18 · 2017-09-22T22:44:17Z

spadgett

It's still possible to delete the service instance... It just won't complete until the bindings are deleted as well. Do we want to block it completely in the UI or just warn?

@ncameronbritt

spadgett · 2017-09-23T13:40:35Z

app/scripts/services/serviceInstances.js

+      };
+
+      if (CatalogService.SERVICE_CATALOG_ENABLED) {
+        DataService.list(resource, context, function (bindings) {


I believe everywhere we call this, we already have a list of the bindings. I'd rather just pass those in if we have them.

The service instance page does not have the bindings. I can take them optionally and only retrieve if not given.

The service instance page does not have the bindings.

Hey @jeff-phillips-18 I'm OK with it in that case.

I want to look at the instance page anyway later. It looks like it's requesting service classes twice, and that's potentially a lot of data.

In general, I think it's better for the page controllers to get the data and pass them down to the components to avoid requesting things more than once.

I can take a look at that as part of this. Basically its the ResourceServiceBindings that gets the bindings for the service instance page (an all the 'application' pages. I can update that logic as well if you'd like.

It might be better to wait for #2155 since I have some changes to instance page and resource service bindings component in flight :/

Pushed changes to only get bindings when not passed. Holding off on updating all the controllers using the ResourceServiceBindings.

spadgett · 2017-09-23T13:41:33Z

app/scripts/services/serviceInstances.js

+      });
+    };
+
+    var deprovision = function (apiObject) {


I realize this isn't new, but I'd rather call the parameter serviceInstance because the function is very specific to instances. apiObject sounds like any object.

spadgett · 2017-09-23T13:42:47Z

app/views/modals/warning.html

+<div class="modal-resource-action">
+  <div class="modal-body">
+    <h1>
+      <span class="pficon pficon-warning-triangle-o"></span>


<span class="pficon pficon-warning-triangle-o" aria-hidden="true"></span> <span class="sr-only">warning</span>

spadgett · 2017-09-23T13:44:27Z

app/scripts/services/serviceInstances.js

+        // TODO - remove once this is resolved https://github.com/kubernetes-incubator/service-catalog/issues/942
+        var opts = {
+          propagationPolicy: null
+        };


Not related to this PR, but we should double-check that this isn't needed for bindings as well. Right now we're using delete-link for bindings and in some cases and not passing propogationPolicy: null

Disregard, looks like we are handling it: https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/deleteLink.js#L153

spadgett · 2017-09-23T13:45:11Z

app/scripts/services/serviceInstances.js

+        resolve: {
+          modalConfig: function() {
+            return {
+              title: "Unable to Deprovision",


We call the action "Delete" in the UI, so I'd be consistent in the messages we use (in the dialog and notifications).

spadgett · 2017-09-23T13:53:04Z

Do we want to check if binding has a deletion timestamp? Otherwise you will have to wait for all the bindings to be deleted from the system completely before deleting the service instance, which could take a while depending on the broker. I'd rather be able to to mark everything deleted once and let it finish whenever it's done.

I'm starting to think it should just be a warning and not block.

ncameronbritt · 2017-09-23T14:20:37Z

The original idea was to block the user from deleting until the bindings had been deleted, but I'm not sure we considered the possibility that deleting the bindings could take some time.

I think it would be a better experience to allow users to delete the bindings from that dialog, rather than just telling them to go do it. The bindings would be then marked for deletion and the deprovision could go ahead as @spadgett suggested. Maybe something like this?

Related but probably outside of this scope, should we even be showing things that are marked for deletion in the UI? I know it doesn't work this way in practice, but deleting seems like an action that should be able to happen more or less instantaneously. Do users need to care about things they deleted but which are just hanging around waiting for the system to delete them?

ncameronbritt · 2017-09-23T14:26:03Z

That "delete all bindings" checkbox should probably say "delete all bindings for $service"

…

On Sat, Sep 23, 2017 at 10:20 AM Cameron Britt ***@***.***> wrote: The original idea was to block the user from deleting until the bindings had been deleted, but I'm not sure we considered the possibility that deleting the bindings could take some time. I think it would be a better experience to allow users to delete the bindings from that dialog, rather than just telling them to go do it. The bindings would be then marked for deletion and the deprovision could go ahead as @spadgett <https://github.com/spadgett> suggested. Maybe something like this? [image: screen shot 2017-09-23 at 10 10 15 am] <https://user-images.githubusercontent.com/8974593/30773885-eaae6878-a047-11e7-9032-ffec12a740f3.png> Related but probably outside of this scope, should we even be showing things that are marked for deletion in the UI? I know it doesn't work this way in practice, but deleting seems like an action that should be able to happen more or less instantaneously. Do users need to care about things they deleted but which are just hanging around waiting for the system to delete them? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2150 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIjxASIzWiTUxFs8QzOLUKV3p1vhUjPZks5slRO4gaJpZM4PhUkh> .

jeff-phillips-18 · 2017-09-25T14:26:03Z

@ncameronbritt How about the title?

jeff-phillips-18 · 2017-09-25T15:47:15Z

@ncameronbritt @spadgett How about something like:

ncameronbritt · 2017-09-25T15:57:33Z

@spadgett and I were just talking about this. I think the best experience is to simply delete the bindings for the user. Deleting the service will also delete any deployments created by the service, and there's no reason for the user to think that bindings needed to be treated in a special way, and a binding doesn't have any value without the service.

Maybe it makes sense to add a sentence like we have here: "$service and its data will no longer be available to your application."

jeff-phillips-18 · 2017-09-25T17:25:16Z

Updated to automatically delete bindings when deleting a service instance (updated PR title as well).
Confirmation Dialog:

ncameronbritt · 2017-09-25T17:33:43Z

Works for me. I would probably change "It cannot be undone" to "This cannot be undone." And in the sentence before it should be "your applications".

jeff-phillips-18 · 2017-09-26T15:19:05Z

I would probably change "It cannot be undone" to "This cannot be undone."

This is in the delete utility code (been there for a while). I can change it, but it would change for most deletion confirmation dialogs. @ncameronbritt Please advise.

jeff-phillips-18 · 2017-09-26T15:29:23Z

@spadgett Also added checks to not show the "no bindable services" message when the object is either a service instance or it is marked for deletion. Removed the "Create Binding" link when the object is marked for deletion.

spadgett · 2017-09-26T16:32:06Z

app/scripts/services/serviceInstances.js

+        var resource = {
+          group: 'servicecatalog.k8s.io',
+          resource: 'serviceinstancecredentials'
+        };


We should probably start switching to

var resource = APIService.getPreferredVersion('serviceinstancecredentials');

spadgett · 2017-09-26T16:33:01Z

app/scripts/services/serviceInstances.js

+      var resource = {
+        group: 'servicecatalog.k8s.io',
+        resource: 'serviceinstances'
+      };


var resource = APIService.getPreferredVersion('serviceinstances');

spadgett · 2017-09-26T16:35:45Z

app/scripts/services/serviceInstances.js

+      var resource = {
+        group: 'servicecatalog.k8s.io',
+        resource: 'serviceinstancecredentials'
+      };


var resource = APIService.getPreferredVersion('serviceinstancecredentials');

spadgett · 2017-09-26T17:08:30Z

app/scripts/services/serviceInstances.js

+        });
+      }
+
+      return deferred.promise;


You can just return the promise from DataService.list instead of creating your own deferred.

return DataService.list(resource, context, function (serviceBindings) { bindings = serviceBindings.by('metadata.name'); return BindingService.getBindingsForResource(bindings, apiObject); });

@spadgett What comes back from the promise is not the bindings manipulated by the callback function. Maybe I'm doing something else wrong?

Sorry my JS was wrong:

return DataService.list(resource, context).then(function(serviceBindings) { bindings = serviceBindings.by('metadata.name'); return BindingService.getBindingsForResource(bindings, apiObject); });

Ah yes, that makes sense. Sorry, I should have seen that.

Fixed. Thanks @spadgett

spadgett · 2017-09-26T17:09:45Z

app/scripts/services/serviceInstances.js

+
+      getBindingsIfNecessary(apiObject, bindings).then(function(serviceBindings) {
+        _.each(serviceBindings, function (binding) {
+          if (!binding.metadata.deletionTimestamp) {


This is a style nit, but I personally find early returns easier to reason about.

if (binding.metadata.deletionTimestamp) { // The binding is already marked for deletion. return; } // <code to delete the binding>

ncameronbritt · 2017-09-26T17:20:22Z

@jeff-phillips-18 I don't feel that strongly about "this" vs. "it", so if it's going to change a bunch of other stuff, I'd say leave it as is for now, and we can address the language change taking into account all of the different contexts in which it occurs later, if we feel like we need to.

spadgett

Thanks @jeff-phillips-18 looks good

spadgett · 2017-09-26T18:23:06Z

[merge]

openshift-bot · 2017-09-26T18:23:22Z

Evaluated for origin web console merge up to 1056510

openshift-bot · 2017-09-26T18:23:30Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/272/) (Base Commit: b92abe5) (PR Branch Commit: 1056510)

jeff-phillips-18 requested review from spadgett, cdcabrera and ncameronbritt September 22, 2017 22:42

spadgett requested changes Sep 23, 2017

View reviewed changes

jeff-phillips-18 force-pushed the services branch from b5dcf95 to 5cdf111 Compare September 25, 2017 17:21

jeff-phillips-18 changed the title ~~Do not attempt service instance deletion if it has bindings~~ Delete bindings when deleting a service instance Sep 25, 2017

jeff-phillips-18 force-pushed the services branch from 5cdf111 to 4f5479b Compare September 26, 2017 15:25

spadgett reviewed Sep 26, 2017

View reviewed changes

jeff-phillips-18 force-pushed the services branch from 4f5479b to fda31b5 Compare September 26, 2017 17:51

Delete bindings when deleting a service instance

1056510

jeff-phillips-18 force-pushed the services branch from fda31b5 to 1056510 Compare September 26, 2017 18:03

spadgett approved these changes Sep 26, 2017

View reviewed changes

openshift-bot merged commit c6db605 into openshift:master Sep 26, 2017

Delete bindings when deleting a service instance #2150

Delete bindings when deleting a service instance #2150

Conversation

jeff-phillips-18 commented Sep 22, 2017

jeff-phillips-18 commented Sep 22, 2017

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Sep 23, 2017

ncameronbritt commented Sep 23, 2017

ncameronbritt commented Sep 23, 2017 via email

jeff-phillips-18 commented Sep 25, 2017

jeff-phillips-18 commented Sep 25, 2017

ncameronbritt commented Sep 25, 2017

jeff-phillips-18 commented Sep 25, 2017

ncameronbritt commented Sep 25, 2017

jeff-phillips-18 commented Sep 26, 2017

jeff-phillips-18 commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncameronbritt commented Sep 26, 2017

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Sep 26, 2017

openshift-bot commented Sep 26, 2017

openshift-bot commented Sep 26, 2017 • edited Loading

spadgett Sep 26, 2017 •

edited

Loading

openshift-bot commented Sep 26, 2017 •

edited

Loading