Pullsubscription not being deleted with Trigger deletion

[v0id3r]

Describe the bug PullSubscription created for Trigger is not removed when trigger is deleted.

Expected behavior PullSubscription should be removed if related Trigger is removed.

To Reproduce I have default broker in default namespace. Create simple trigger -> trigger, subscription, pullsubscription created, everything ready and works. delete this trigger -> trigger and subscription removed, but pullsubscription left, cannot be deleted manually (only if I edit broker channel and clear Spec.Subscribable.Subscribers fiield)

Knative-GCP release version Knative Eventing release v0.13.6 Knative Knative Gcp release v0.13.1

Additional context Hope someone can verify bug - to confirm it is not my local configuration problem. For me it is quite urgent as PullSubscriptions are left and pods keep running.

[v0id3r]

Checked with inmemory channel. After trigger deleted, channel spec.subscribers is not updated.

❯ kubectl get triggers
No resources found in default namespace.
❯ kubectl get subscriptions
No resources found in default namespace.
❯ kubectl describe inmemorychannels
Name:         default-kne-trigger
Namespace:    default
Labels:       eventing.knative.dev/broker=default
              eventing.knative.dev/brokerEverything=true
Annotations:  messaging.knative.dev/creator: system:serviceaccount:knative-eventing:eventing-controller
              messaging.knative.dev/lastModifier: system:serviceaccount:knative-eventing:eventing-controller
              messaging.knative.dev/subscribable: v1beta1
API Version:  messaging.knative.dev/v1beta1
Kind:         InMemoryChannel
Metadata:
  Creation Timestamp:  2020-04-10T14:18:47Z
  Generation:          3
  Owner References:
    API Version:           eventing.knative.dev/v1alpha1
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  Broker
    Name:                  default
    UID:                   7aec1335-b86f-4f04-b9b1-1d7302b197b3
  Resource Version:        21866565
  Self Link:               /apis/messaging.knative.dev/v1beta1/namespaces/default/inmemorychannels/default-kne-trigger
  UID:                     c445d0b9-b7ad-40b6-bf21-0626c48ed7d8
Spec:
  Subscribers:
    Delivery:
      Dead Letter Sink:
    Generation:      2
    Reply Uri:       http://default-broker.default.svc.cluster.local
    Subscriber Uri:  http://default-broker-filter.default.svc.cluster.local/triggers/default/test/c9661ab0-44f1-4c74-b424-428a0a3aaef1
    UID:             d654cc3e-fabb-4909-87e7-325492e3f8bb

[grantr]

Looks like this might be a bug in eventing. @v0id3r how high priority is this for you? Are you prototyping or using this in production? Trying to triage.

[v0id3r]

Looks like this might be a bug in eventing. @v0id3r how high priority is this for you? Are you prototyping or using this in production? Trying to triage.

We are only starting to use eventing component, so it is not running live right now. But behavior is bad because pull subscription pods keep running until you resolve manually.

Also I think I found and fixed it. I will submit PR in short time.

[v0id3r]

I have feeling that this part is not correct in 0.13.6 release: https://github.com/knative/eventing/blob/e581ecaec7fa86b2005811a875b9a7bb39679218/pkg/reconciler/subscription/subscription.go#L475

updateChannelAddSubscriptionV1Alpha1 is used instead of updateChannelRemoveSubscriptionV1Alpha1.

And later unused remove handler was removed as dead code: https://github.com/knative/eventing/commit/54cee987710e6d3ca3d1ea3af1e7dedd138be341

https://github.com/knative/eventing/blob/3634f0894abbf58ef19e00f6ca1773d0f79d3c96/pkg/reconciler/subscription/subscription.go#L476

[grantr]

Oh nice work tracking that down! Looks like the bug was introduced in https://github.com/knative/eventing/pull/2682 and released in 0.13.0. Looking forward to your PR! It might deserve a backport.

[grantr]

Fixed by https://github.com/knative/eventing/pull/2986