KafkaChannel reconciler to use KafkaChannel v1beta1 API

[aliok]

Fixes #1397

Proposed Changes

• KafkaChannel reconciler to use KafkaChannel v1beta1 API

Release Note

- 🧽 Update or clean up current behavior
Reconcile KafkaChannel using v1beta1 api shape. Operate on dependent resources (Subscriptions, etc.) using their v1 shapes.

Docs

[aliok]

Did some work, there's still compile errors and crap, will do the rest after knative/eventing#3702 is done
cc @slinkydeveloper

[aliok]

Now this PR needs #1408 to get merged

[aliok]

/retest

[aliok]

I am able to reproduce the problem in the failing tests:

eventing-controller-584ccd45bb-ffkgw eventing-controller {
  "level": "warn",
  "ts": "2020-08-06T11:44:28.600Z",
  "logger": "controller.knative.dev-eventing-pkg-reconciler-subscription.Reconciler",
  "caller": "subscription/subscription.go:463",
  "msg": "Failed to patch the Channel",
  "commit": "aa13ba6",
  "knative.dev/traceid": "9c9e50e6-0087-4d08-b3f9-98cf7298075b",
  "knative.dev/key": "default/subscription",
  "error": "admission webhook \"defaulting.webhook.kafka.messaging.knative.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"subscribers\"",
  "patch": "eyJzcGVjIjp7InN1YnNjcmliZXJzIjpbeyJnZW5lcmF0aW9uIjoxLCJzdWJzY3JpYmVyVXJpIjoiaHR0cDovL2Zvby5kZWZhdWx0LnN2Yy5jbHVzdGVyLmxvY2FsLyIsInVpZCI6IjkwNTNmYWZiLTcyZWEtNDBjNS1iOTlhLTIyMmQ2ZTM3YmNmMCJ9XX19"
}
eventing-controller-584ccd45bb-ffkgw eventing-controller {
  "level": "warn",
  "ts": "2020-08-06T11:44:28.600Z",
  "logger": "controller.knative.dev-eventing-pkg-reconciler-subscription.Reconciler",
  "caller": "subscription/subscription.go:160",
  "msg": "Failed to sync physical Channel",
  "commit": "aa13ba6",
  "knative.dev/traceid": "9c9e50e6-0087-4d08-b3f9-98cf7298075b",
  "knative.dev/key": "default/subscription",
  "error": "admission webhook \"defaulting.webhook.kafka.messaging.knative.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"subscribers\""
}

• KafkaChannel is created as v1alpha1 but the annotation messaging.knative.dev/subscribable is v1
• Subscription reconciler (https://github.com/knative/eventing/blob/b93138bf0cec38e2fdf73100ee2ba12b316b49f5/pkg/reconciler/subscription/subscription.go#L511) checks if the channel supports v1alpha1 OR v1beta1/v1 duck type. It thinks v1beta1/v1, because the annotation messaging.knative.dev/subscribable is v1
• It tries to do something in the channel spec, it can't because the duck type API shape is not v1beta1/v1

To reproduce:

1. Create KafkaCluster (no instructions below) and use it in config-kafka

apiVersion: v1
kind: ConfigMap
metadata:
  name: config-kafka
  namespace: knative-eventing
data:
  # Broker URL. Replace this with the URLs for your kafka cluster,
  # which is in the format of my-cluster-kafka-bootstrap.my-kafka-namespace:9092.
  bootstrapServers: my-cluster-kafka-bootstrap.kafka:9092

2. Create KafkaChannel v1alpha1

apiVersion: messaging.knative.dev/v1alpha1
kind: KafkaChannel
metadata:
  name: kafka-channel
spec: {}

3. Create subscription for the KafkaChannel created above

apiVersion: messaging.knative.dev/v1
kind: Subscription
metadata:
  name: subscription
spec:
  channel:
    apiVersion: messaging.knative.dev/v1alpha1
    kind: KafkaChannel
    name: kafka-channel
  subscriber:
    ref:
      apiVersion: v1
      kind: Service
      name: foo

[aliok]

Hmm, the root cause of the problem looks like having the annotation as v1 on v1alpha1 resource.

When I apply the resource

apiVersion: messaging.knative.dev/v1alpha1
kind: KafkaChannel
metadata:
  name: kafka-channel
spec: {}

and then describe:

$ k describe kafkachannels.v1alpha1.messaging.knative.dev
Name:         kafka-channel
Namespace:    default
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"messaging.knative.dev/v1alpha1","kind":"KafkaChannel","metadata":{"annotations":{},"name":"kafka-channel","namespace":"defa...
              messaging.knative.dev/subscribable: v1

I don't get why the messaging.knative.dev/subscribable annotation is v1. Checking this now.

[matzew]

So it's hitting this: https://github.com/aliok/eventing-contrib/blob/kafkachannel-v1beta1-reconciler/kafka/channel/pkg/apis/messaging/v1beta1/kafka_channel_defaults.go#L37 ?

[aliok]

@matzew Yes, it looks like it does. I pushed a new commit. Let's see what happens.

[pierDipi]

These log lines from eventing-controller seem interesting as well (there are multiple of them), but I'm not sure if they are related:

{"level":"warn","ts":"2020-08-06T11:11:26.358Z","logger":"controller.knative.dev-eventing-pkg-reconciler-subscription.Reconciler","caller":"subscription/subscription.go:463","msg":"Failed to patch the Channel","commit":"aa13ba6","knative.dev/traceid":"18936795-cbc1-406d-ad62-e9270e1b369d","knative.dev/key":"test-event-transformation-for-trigger-v1-broker-v1-beta1-kbvbsd/kafkachannel-trigger1-df7ef279-b563-405b-8dff-46c39438d224","error":"admission webhook \"defaulting.webhook.kafka.messaging.knative.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"subscribers\"","patch":"eyJzcGVjIjp7InN1YnNjcmliZXJzIjpbeyJnZW5lcmF0aW9uIjoxLCJyZXBseVVyaSI6Imh0dHA6Ly9icm9rZXItaW5ncmVzcy5rbmF0aXZlLWV2ZW50aW5nLnN2Yy5jbHVzdGVyLmxvY2FsL3Rlc3QtZXZlbnQtdHJhbnNmb3JtYXRpb24tZm9yLXRyaWdnZXItdjEtYnJva2VyLXYxLWJldGExLWtidmJzZC9rYWZrYWNoYW5uZWwiLCJzdWJzY3JpYmVyVXJpIjoiaHR0cDovL2Jyb2tlci1maWx0ZXIua25hdGl2ZS1ldmVudGluZy5zdmMuY2x1c3Rlci5sb2NhbC90cmlnZ2Vycy90ZXN0LWV2ZW50LXRyYW5zZm9ybWF0aW9uLWZvci10cmlnZ2VyLXYxLWJyb2tlci12MS1iZXRhMS1rYnZic2QvdHJpZ2dlcjEvZGY3ZWYyNzktYjU2My00MDViLThkZmYtNDZjMzk0MzhkMjI0IiwidWlkIjoiNDVmZjUyZDQtZTMyMy00YWE2LTlmMmItZDk0YmNkMmNhNzMyIn1dfX0="}
{"level":"warn","ts":"2020-08-06T11:11:26.358Z","logger":"controller.knative.dev-eventing-pkg-reconciler-subscription.Reconciler","caller":"subscription/subscription.go:160","msg":"Failed to sync physical Channel","commit":"aa13ba6","knative.dev/traceid":"18936795-cbc1-406d-ad62-e9270e1b369d","knative.dev/key":"test-event-transformation-for-trigger-v1-broker-v1-beta1-kbvbsd/kafkachannel-trigger1-df7ef279-b563-405b-8dff-46c39438d224","error":"admission webhook \"defaulting.webhook.kafka.messaging.knative.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"subscribers\""}

[aliok]

Yes, they're related. That's exactly the problem. The subscriber reconciler wants to patch the channel with a subscribers field as it assumes the channel is a v1 duck supporting one but it is not.
I pushed a new commit, let's see if it fixes the problem.

[aliok]

/retest

I don't see the json: unknown field \"subscribers\""} problem anymore, but not really sure what fails now.

[matzew]

@nlopezgi does this ring a bell for you?

[nlopezgi]

@nlopezgi does this ring a bell for you?

It might be related, as the issue I'm seeing is also related to subscribers.
In my case the issue is that the duckv1alpha1.SubscribableStatus has a list of Subscribers (type duckv1beta1.SubscriberStatus) and its not clear to me how to migrate (or if there needs to be a migration) to support this with a duckv1.SubscriberStatus. The error is manifesting in that tests that run with duckv1 have problem dealing with duckv1alpha1.Channeleable.

I think there might be something missing in the types to properly do this migration (e.g., does the duckv1alpha1.SubscribableStatus need to now have a list of Subscribers that is of type duckv1.SubscriberStatus - how do we do this migration from duckv1beta1.SubscriberStatus->duckv1.SubscriberStatus?). I frankly don't know enough about how version evolution works in knative to properly define clearly the problem or how to solve it.

This issue might be related to the one I'm seeing if the reason you are seeing the json: unknown field \"subscribers\""} is because you have a duckv1alpha1 Subscription resource that is somehow getting a Subscriber list that is of type duckv1 and not duckv1beta1

Not sure if this helps, but more details of the error I'm seeing are in knative/eventing#3789.

[nlopezgi]

So I looked a bit more at the types code to try to understand what is happening. I think the core issue is related to the ChannelableCombined. This type is supposed to represent "a skeleton type wrapping Subscribable and Addressable of both v1alpha1 and v1beta1 duck types". This leads me to think that as part of the types migration we need to add something else there for the Subscribable and Addressable for v1, otherwise we might end up breaking in some form backward compatibility (but again, I know little of how these things evolve in practice, so I might be missing something)

[aliok]

@nlopezgi I think the "unknown field: subscribers" problem is now solved. The subscribable duck type support annotation was somehow set to v1 even for v1alpha1 type and that was causing the reconciler to assume the api shape is different.
I added explicit setting to v1alpha1 duck version for v1alpha1 type and now the reconciler is happy.

I don't know what's failing now. I am now trying to understand. It is hard to debug when tracing comes into play.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

[mattmoor]

Format Go code:

Suggested change

	"context"
	"context"

[slinkydeveloper]

/retest

[slinkydeveloper]

/lgtm

[matzew]

oh, was that the reason of the issues ?

[slinkydeveloper]

no, but it was causing other issues in logging. The reason of issues was the previous subscription type used by the dispatcher

[matzew]

/lgtm
/approve

[slinkydeveloper]

/retest

[slinkydeveloper]

/retest

[aliok]

/retest

I compared it with the previous failure and the same tests fail for different API versions.
Let's try once more.

[matzew]

it seems to be on TestBrokerRedelivery 🤔 - but perhaps run by hand, or manually, and see?

[slinkydeveloper]

/retest

[slinkydeveloper]

@aliok seems like that test is flaky #1433, maybe should we disable it?

[aliok]

@slinkydeveloper I am on PTO and leaving to driving to beaches :) thanks for taking care of this!

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
kafka/channel/pkg/dispatcher/dispatcher.go	61.7%	63.1%	1.5

[pierDipi]

/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [matzew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment