[BUG]E2E test for GCP PubSub CCP is flaky

[chizhg]

Describe the bug
E2E test cases for GCP PubSub CCP fail intermittently. More details can be found in the testgrid.

Expected behavior
All tests can pass consistently.

To Reproduce
There is no consistent way to reproduce. From the testgrid, we can see a couple of test cases for GCP PubSub CCP sometimes fail.

Additional context
The GCP PubSub CCP is installed during Knative setup (e2e-tests.sh) based on the instruction, and then the only difference between GCP PubSub CCP and other CCPs is the provisioner name.

[chizhg]

E2E tests for GCP PubSub CCP is disabled for now #1094 .
We will enable it after this issue being resolved.

[chizhg]

One example:
https://gubernator.knative.dev/build/knative-prow/logs/ci-knative-eventing-continuous/1120319055757053952#teste2e-testmaintestsinglebinaryevent-gcp-pubsub

The sender pod got 500 Internal Server Error when sending an event.

[Harwayne]

With the help of @Fredy-Z, I investigated this and believe I found the root cause. The GCP PubSub Subscription took three seconds to create:

{
    "level": "info",
    "ts": "2019-04-24T06:10:48.512Z",
    "logger": "provisioner",
    "caller": "channel/reconcile.go:123",
    "msg": "Reconciling Channel",
    "eventing.knative.dev/clusterChannelProvisioner": "gcp-pubsub",
    "eventing.knative.dev/clusterChannelProvisionerComponent": "Controller",
    "request": "test-event-transformation/e2e-eventtransformation1"
  },
  {
    "level": "info",
    "ts": "2019-04-24T06:10:51.556Z",
    "logger": "provisioner",
    "caller": "channel/reconcile.go:480",
    "msg": "Created new subscription",
    "eventing.knative.dev/clusterChannelProvisioner": "gcp-pubsub",
    "eventing.knative.dev/clusterChannelProvisionerComponent": "Controller",
    "request": "test-event-transformation/e2e-eventtransformation1",
    "channel": { ... },
    "subscription": {}
  },

This was problematic for multiple reasons:

1. This blocked the reconcile queue for three seconds.
2. The Channel and Subscription objects were both marked as Ready=true the entire time.

Problem 1 is likely something we need to be more vigilant about in general across all CCPs, namely only doing 'fast' things in the reconcile path and utilizing goroutines for slower operations, that then re-enqueue once the background operation is done.

Problem 2 is more significant. We shouldn't mark both objects as Ready, if there is no way an event would be delivered to the Subscriber. After talking with @grantr, we propose the following:

• Channel readiness represents that the Channel is ready to receive events. It makes no guarantees about which subscribers will receive those events.
• Subscription readiness represents that events sent to the Channel will be sent to the subscriber.

But this introduces something odd, the Subscription controller needs to wait for some signal from the Channel controller that the Subscription is ready. As of today we don't have a good way to send this signal. Some possibilities are:

1. Channel controller directly sets a status field/condition on the Subscription object.
   • This feels like we will run into race conditions. It also is odd to have multiple controllers set the status of a single object.
2. Channel adds a new status field that contains the status of every injected subscriber. These per subscriber statuses will need to include something like the observed generation, so the Subscription controller can differentiate between 'this is correctly synced now' vs 'this is correctly synced with an older subscriber'. Something like:

type ChannelStatus struct {
  Subscribers []Subscriber `json:"subscribers,omitempty"`
  // All the already existing fields.
}
type Subscriber struct {
  UID types.UID `json:"uid,omitempty"`
  ObservedGeneration int64 `json:"observedGeneration,omitempty"`
  Ready corev1.ConditionStatus `json:"ready,omitempty"
  Message string `json:"message,omitempty"
}

[akashrv]

I think there is no harm in two reconcilers trying to update conditions in a mutually exclusive manner.
One way could be that the subscription reconciler never calls MarkChannelReady. Instead it sets a new condition - "AddedToChannel" or "SubsrcibedToChannel". And then the channel reconciler will set MarkChannelReady when the backing channel implementation is ready, which will eventually set the Ready=True. We just have to make sure that the two reconcilers don't go in an infinite loop, which can be done the way it is done with common channel controller.

[akashrv]

/priority important-soon

[akashrv]

/assign @n3wscott
This should get fixed once the new channel starts supporting subscribableStatus

[vaikas]

moved to github.com/GoogleCloudPlatform
Let's make sure to fix this in there
@n3wscott

If it works with subscribableStatus we're good.

[n3wscott]

Fixed.

The root cause was the GCP account used to do testing was the same for each test, so if two tests ran at the same time, flakes happened.

/close