Skip to content

Commit

Permalink
Fix bind delete (#59)
Browse files Browse the repository at this point in the history
* Update the deploying of function code to be separate from hte bind to demonstrate late binding

* Handle deletes more robustly.

* address CR comments

* Convert to grpc status and look at the code instead of string matching
  • Loading branch information
Ville Aikas authored and google-prow-robot committed May 24, 2018
1 parent e192c87 commit 44bdaa7
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 17 deletions.
37 changes: 22 additions & 15 deletions pkg/controller/bind/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,24 +293,40 @@ func (c *Controller) syncHandler(key string) error {
// Don't mutate the informer's copy of our object.
bind = bind.DeepCopy()

// See if the binding has been deleted
accessor, err := meta.Accessor(bind)
if err != nil {
log.Fatalf("Failed to get metadata: %s", err)
}
deletionTimestamp := accessor.GetDeletionTimestamp()

// Find the Route that they want.
routeName := bind.Spec.Action.RouteName
route, err := c.routesLister.Routes(namespace).Get(routeName)
if err != nil {
functionDNS := ""

// Only return an error if we're not deleting so that we can delete
// the binding even if the route has already been deleted.
if err != nil && deletionTimestamp == nil {
if errors.IsNotFound(err) {
runtime.HandleError(fmt.Errorf("Route %q in namespace %q does not exist", routeName, namespace))
}
return err
}

// Grab the route
functionDNS := route.Status.Domain
if route != nil {
functionDNS = route.Status.Domain
glog.Infof("Found route DNS as %q", functionDNS)
}
if len(functionDNS) == 0 {
runtime.HandleError(fmt.Errorf("Route %q does not have domain", routeName))
// Only return an error if we're not deleting so that we can delete
// the binding even if the route has already been deleted.
if deletionTimestamp == nil {
return fmt.Errorf("Route %q does not have domain", routeName)
}
log.Printf("Route %q does not have domain", routeName)
}

glog.Infof("Found route DNS as '%q'", functionDNS)

es, err := c.eventSourcesLister.EventSources(namespace).Get(bind.Spec.Trigger.Service)
if err != nil {
if errors.IsNotFound(err) {
Expand All @@ -333,15 +349,6 @@ func (c *Controller) syncHandler(key string) error {
return err
}

// See if the binding has been deleted
accessor, err := meta.Accessor(bind)
if err != nil {
log.Printf("Failed to get metadata: %s", err)
panic("Failed to get metadata")
}

deletionTimestamp := accessor.GetDeletionTimestamp()

// If there are conditions or a context do nothing.
if (bind.Status.Conditions != nil || bind.Status.BindContext != nil) && deletionTimestamp == nil {
glog.Infof("Already has status, skipping")
Expand Down
18 changes: 16 additions & 2 deletions pkg/sources/gcp_pubsub.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (

"github.com/golang/glog"
"github.com/google/uuid"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

apierrs "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -75,8 +77,13 @@ func (t *GCPPubSubEventSource) Unbind(trigger EventTrigger, bindContext BindCont
sub := client.Subscription(subscriptionName)
err = sub.Delete(ctx)
if err != nil {
glog.Warningf("Failed to delete subscription %q : %s", subscriptionName, err)
return err
glog.Warningf("Failed to delete subscription %q : %s : %+v", subscriptionName, err, err)
// Return error only if it's something else than NotFound
rpcStatus := status.Convert(err)
if rpcStatus.Code() != codes.NotFound {
return err
}
glog.Infof("Subscription %q already deleted", subscriptionName)
}
return nil
}
Expand Down Expand Up @@ -118,6 +125,13 @@ func (t *GCPPubSubEventSource) Bind(trigger EventTrigger, route string) (*BindCo
err = t.createWatcher("bind-system", deploymentName, t.image, projectID, subscriptionName, route)
if err != nil {
glog.Infof("Failed to create deployment: %v", err)

// delete the subscription so it's not left floating around.
errDelete := sub.Delete(ctx)
if errDelete != nil {
glog.Infof("Failed to delete subscription while trying to clean up %q : %s", subscriptionName, errDelete)
}

return nil, err
}

Expand Down

0 comments on commit 44bdaa7

Please sign in to comment.