Delete removed serviceplans when they have no instances left (#1444)

pkg/controller/controller_serviceplan.go

@@ -19,6 +19,10 @@ package controller
 import (
 	"github.com/golang/glog"
 	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -33,26 +37,66 @@ func (c *controller) servicePlanAdd(obj interface{}) {
 	c.servicePlanQueue.Add(key)
 }
 
+func (c *controller) servicePlanUpdate(oldObj, newObj interface{}) {
+	c.servicePlanAdd(newObj)
+}
+
+func (c *controller) servicePlanDelete(obj interface{}) {
+	servicePlan, ok := obj.(*v1beta1.ClusterServicePlan)
+	if servicePlan == nil || !ok {
+		return
+	}
+
+	glog.V(4).Infof("ClusterServicePlan: Received delete event for %v; no further processing will occur", servicePlan.Name)
+}
+
 // reconcileClusterServicePlanKey reconciles a ClusterServicePlan due to resync
 //  or an event on the ClusterServicePlan.  Note that this is NOT the main
 // reconciliation loop for ClusterServicePlans. ClusterServicePlans are
 // primarily reconciled in a separate flow when a ClusterServiceBroker is
 // reconciled.
 func (c *controller) reconcileClusterServicePlanKey(key string) error {
-	// currently, this is a no-op.  In the future, we'll maintain status
-	// information here.
-	return nil
+	plan, err := c.servicePlanLister.Get(key)
+	if errors.IsNotFound(err) {
+		glog.Infof("ClusterServicePlan %q: Not doing work because it has been deleted", key)
+		return nil
+	}
+	if err != nil {
+		glog.Infof("ClusterServicePlan %q: Unable to retrieve object from store: %v", key, err)
+		return err
+	}
+
+	return c.reconcileClusterServicePlan(plan)
 }
 
-func (c *controller) servicePlanUpdate(oldObj, newObj interface{}) {
-	c.servicePlanAdd(newObj)
+func (c *controller) reconcileClusterServicePlan(servicePlan *v1beta1.ClusterServicePlan) error {
+	glog.Infof("ClusterServicePlan %q (ExternalName: %q): processing", servicePlan.Name, servicePlan.Spec.ExternalName)
+
+	if !servicePlan.Status.RemovedFromBrokerCatalog {
+		return nil
+	}
+
+	glog.Infof("ClusterServicePlan %q (ExternalName: %q): has been removed from broker catalog; determining whether there are instances remaining", servicePlan.Name, servicePlan.Spec.ExternalName)
+
+	serviceInstances, err := c.findServiceInstancesOnClusterServicePlan(servicePlan)
+	if err != nil {
+		return err
+	}
+
+	if len(serviceInstances.Items) != 0 {
+		return nil
+	}
+
+	glog.Infof("ClusterServicePlan %q (ExternalName: %q): has been removed from broker catalog and has zero instances remaining; deleting", servicePlan.Name, servicePlan.Spec.ExternalName)
+	return c.serviceCatalogClient.ClusterServicePlans().Delete(servicePlan.Name, &metav1.DeleteOptions{})
 }
 
-func (c *controller) servicePlanDelete(obj interface{}) {
-	servicePlan, ok := obj.(*v1beta1.ClusterServicePlan)
-	if servicePlan == nil || !ok {
-		return
+func (c *controller) findServiceInstancesOnClusterServicePlan(servicePlan *v1beta1.ClusterServicePlan) (*v1beta1.ServiceInstanceList, error) {
+	fieldSet := fields.Set{
+		"spec.clusterServicePlanRef.name": servicePlan.Name,
 	}
+	fieldSelector := fields.SelectorFromSet(fieldSet).String()
+	listOpts := metav1.ListOptions{FieldSelector: fieldSelector}
 
-	glog.V(4).Infof("ClusterServicePlan: Received delete event for %v; no further processing will occur", servicePlan.Name)
+	return c.serviceCatalogClient.ServiceInstances(metav1.NamespaceAll).List(listOpts)
 }

pkg/controller/controller_serviceplan_test.go

@@ -0,0 +1,137 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
+	"github.com/kubernetes-incubator/service-catalog/test/fake"
+	"k8s.io/apimachinery/pkg/runtime"
+
+	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
+	clientgotesting "k8s.io/client-go/testing"
+)
+
+func TestReconcileClusterServicePlanRemovedFromCatalog(t *testing.T) {
+	getRemovedPlan := func() *v1beta1.ClusterServicePlan {
+		p := getTestClusterServicePlan()
+		p.Status.RemovedFromBrokerCatalog = true
+		return p
+	}
+
+	cases := []struct {
+		name                    string
+		plan                    *v1beta1.ClusterServicePlan
+		instances               []v1beta1.ServiceInstance
+		catalogClientPrepFunc   func(*fake.Clientset)
+		shouldError             bool
+		errText                 *string
+		catalogActionsCheckFunc func(t *testing.T, name string, actions []clientgotesting.Action)
+	}{
+		{
+			name:        "not removed from catalog",
+			plan:        getTestClusterServicePlan(),
+			shouldError: false,
+		},
+		{
+			name:        "removed from catalog, instances left",
+			plan:        getRemovedPlan(),
+			instances:   []v1beta1.ServiceInstance{*getTestServiceInstance()},
+			shouldError: false,
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServicePlanRef.name", "PGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 1)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+			},
+		},
+		{
+			name:        "removed from catalog, no instances left",
+			plan:        getRemovedPlan(),
+			instances:   nil,
+			shouldError: false,
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServicePlanRef.name", "PGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 2)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+				assertDelete(t, actions[1], getRemovedPlan())
+			},
+		},
+		{
+			name: "removed from catalog, no instances left, delete fails", plan: getRemovedPlan(),
+			instances:   nil,
+			shouldError: true,
+			catalogClientPrepFunc: func(client *fake.Clientset) {
+				client.AddReactor("delete", "clusterserviceplans", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+					return true, nil, errors.New("oops")
+				})
+			},
+			errText: strPtr("oops"),
+			catalogActionsCheckFunc: func(t *testing.T, name string, actions []clientgotesting.Action) {
+				listRestrictions := clientgotesting.ListRestrictions{
+					Labels: labels.Everything(),
+					Fields: fields.OneTermEqualSelector("spec.clusterServicePlanRef.name", "PGUID"),
+				}
+
+				expectNumberOfActions(t, name, actions, 2)
+				assertList(t, actions[0], &v1beta1.ServiceInstance{}, listRestrictions)
+				assertDelete(t, actions[1], getRemovedPlan())
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		_, fakeCatalogClient, _, testController, _ := newTestController(t, noFakeActions())
+
+		fakeCatalogClient.AddReactor("list", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) {
+			return true, &v1beta1.ServiceInstanceList{Items: tc.instances}, nil
+		})
+
+		if tc.catalogClientPrepFunc != nil {
+			tc.catalogClientPrepFunc(fakeCatalogClient)
+		}
+
+		err := testController.reconcileClusterServicePlan(tc.plan)
+		if err != nil {
+			if !tc.shouldError {
+				t.Errorf("%v: unexpected error from method under test: %v", tc.name, err)
+				continue
+			} else if tc.errText != nil && *tc.errText != err.Error() {
+				t.Errorf("%v: unexpected error text from method under test; expected %v, got %v", tc.name, tc.errText, err.Error())
+				continue
+			}
+		}
+
+		actions := fakeCatalogClient.Actions()
+
+		if tc.catalogActionsCheckFunc != nil {
+			tc.catalogActionsCheckFunc(t, tc.name, actions)
+		} else {
+			expectNumberOfActions(t, tc.name, actions, 0)
+		}
+	}
+}

pkg/controller/controller_test.go

@@ -438,6 +438,7 @@ func getTestClusterServicePlan() *v1beta1.ClusterServicePlan {
 				Name: testClusterServiceClassGUID,
 			},
 		},
+		Status: v1beta1.ClusterServicePlanStatus{},
 	}
 }
 

test/integration.sh

@@ -26,11 +26,11 @@ runTests() {
   kube::etcd::start
 
   go test -race -i github.com/kubernetes-incubator/service-catalog/test/integration/... -c \
-      && ./integration.test -test.v
+      && ./integration.test -test.v $@
 }
 
 # Run cleanup to stop etcd on interrupt or other kill signal.
 trap kube::etcd::cleanup EXIT
 
-runTests
+runTests $@