delete ra deployment when sink is not found #1533

itsmurugappan · 2020-09-06T15:31:18Z

Proposed Changes

When sink is not found delete the RA deployment, this will ensure that events are not dropped
Add e2e for checking various kafka source reconciler states

Release Note

🐛 Fix bug - when kafkasource goes into "sink not found" status, receiver adapter will be deleted. The receiver adapter will be created again when the sink is available and ready to receive events.

kafka/source/pkg/reconciler/source/kafkasource.go

aliok · 2020-09-07T08:37:58Z

/assign

aliok

I will try verifying it on the cluster later, but I added some comments first.

aliok

/approve
/lgtm
/hold

The changes look good, thanks for addressing the comments.

However, I had hard time reproducing the original problem, so, added a /hold for other eyes.

cc @slinkydeveloper mind having a look?

slinkydeveloper · 2020-09-08T08:54:56Z

test/e2e/helpers/kafka_helper.go

 )

 var (
-	topicGVR = schema.GroupVersionResource{Group: strimziApiGroup, Version: strimziApiVersion, Resource: strimziTopicResource}
+	topicGVR        = schema.GroupVersionResource{Group: strimziApiGroup, Version: strimziApiVersion, Resource: strimziTopicResource}
+	KafkaChannelGVR = schema.GroupVersionResource{Group: "messaging.knative.dev", Version: "v1beta1", Resource: "kafkachannels"}


added KafkaCannelGVR for deleting it

slinkydeveloper · 2020-09-08T08:57:18Z

test/e2e/kafka_source_reconciler_test.go

+		0,
+	}, {
+		"create_sink",
+		createChannel,


I don't get why you're using the kafka channel here 😄, if you need to create a "sink", can you use a simpler one (like the test images to receive events)?

I wanted to use the simplest one like you have mentioned, but if am not wrong, if the sink is "v1/Service", it is not validated to see if its available or not. Reconciles to true always. Next option was to use ksvc, but i dint see any ksvc used as sink in any of the eventing tests nor the knative serving components getting installed. Thats why used a channel. Willing to change to simpler ones if there are other options.

i think you need an addressable, so maybe using a kube service with a pod behind the hood is fine? I would love as much as possible to have this "dependency" between the kafka source tests and kafka channel

but with kube service as i mentioned above , its not reporting the right status on the kafks source. It always reconciles to true whether its there are not. I initially tried with kube service only.

ok so then, more than creating a kafka channel, can you create an imc channel and clearly state you need it for that purpose? Because IMC is part of the eventing "core release", it's installed in the test env for sure. You cannot make the same assumption with kafka channel

if the sink is "v1/Service", it is not validated to see if its available or not.

Oh, ok, this is the reason that I wasn't able to reproduce the issue. I was trying with a Kube service

used imc initially, the tests failed, then switched to kafka channel. Tried one more time with imc, got the below logs in tests

=== RUN TestKafkaSourceReconciler/create_sink === CONT TestKafkaSourceReconciler creation.go:77: Creating channel &TypeMeta{Kind:InMemoryChannel,APIVersion:messaging.knative.dev/v1beta1,}-e2e-rt-channel creation.go:85: Failed to create "InMemoryChannel" "e2e-rt-channel": the server could not find the requested resource creation.go:90: Failed to create "InMemoryChannel" "e2e-rt-channel": the server could not find the requested resource === CONT TestKafkaSourceReconciler/create_sink testing.go:964: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test === CONT TestKafkaSourceReconciler test_runner.go:195: EVENT: {{ } {e2e-rt-kafka-source.163333e31993bfcc test-kafka-source-reconciler-wbvf2 /api/v1/namespaces/test-kafka-source-reconciler-wbvf2/events/e2e-rt-kafka-source.163333e31993bfcc 278249f1-af24-4582-93f9-4811b4e4e239 1172 0 2020-09-09 19:22:23 +0000 UTC <nil> <nil> map[] map[] [] [] []} {KafkaSource test-kafka-source-reconciler-wbvf2 e2e-rt-kafka-source be8ceb0b-957b-4e18-813c-5c40244dbdd3 sources.knative.dev/v1beta1 19196 } InternalError getting sink URI: failed to get lister for messaging.knative.dev/v1beta1, Resource=inmemorychannels: inmemorychannels.messaging.knative.dev is forbidden: User "system:serviceaccount:knative-sources:kafka-controller-manager" cannot list resource "inmemorychannels" in API group "messaging.knative.dev" at the cluster scope {kafkasource-controller } 2020-09-09 19:22:23 +0000 UTC 2020-09-09 19:22:25 +0000 UTC 6 Warning 0001-01-01 00:00:00 +0000 UTC nil nil

The first 2 errors (failed to create) are weird, i don't get what's the problem there tbh, double check all the code you use to start the imc. in fact, in testlib.Client there should be a method to create a imc channel (it's from eventing/test/lib), you don't need to develop it by yourself.

The last error is clearly an issue of cluster roles, seems like you need some specific cluster role to check if the addressable is ready. @matzew any thoughts on that?

imc was not set up as part of e2e set up, added those in the e2e scripts and test passes. @slinkydeveloper @matzew

itsmurugappan · 2020-09-08T15:40:24Z

However, I had hard time reproducing the original problem, so, added a /hold for other eyes.

@aliok

Create a kafkasource with ksvc as sink, check if kafkasource is in true status
Delete sink, which is the ksvc
kafkasource will go to false, but receiver adapter will still be up.

slinkydeveloper · 2020-09-11T06:54:29Z

/lgtm
/approve

Thanks for your contribution!

knative-prow-robot · 2020-09-11T06:54:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, itsmurugappan, slinkydeveloper

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:

~~kafka/OWNERS~~ [slinkydeveloper]
~~test/OWNERS~~ [slinkydeveloper]

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

slinkydeveloper · 2020-09-11T07:08:33Z

/unhold

slinkydeveloper · 2020-09-14T07:21:31Z

Hey @itsmurugappan seems like the tests you made are a little bit flaky, can you please check them out? #1561 #1562 #1563 #1564

itsmurugappan · 2020-09-14T16:48:56Z

sure , i will take a look.

itsmurugappan added 2 commits September 6, 2020 08:17

delete ra deployment when sink is not found

1dbefe9

delete ra deployment when sink is not found

50d7030

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release labels Sep 6, 2020

knative-prow-robot requested review from coryrc and matzew September 6, 2020 15:31

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 6, 2020

itsmurugappan added 4 commits September 6, 2020 08:44

delete ra deployment when sink is not found

9028264

delete ra deployment when sink is not found

3b30043

delete ra deployment when sink is not found

af6b53b

change sink t0 channel

631578b

aliok reviewed Sep 7, 2020

View reviewed changes

kafka/source/pkg/reconciler/source/kafkasource.go Outdated Show resolved Hide resolved

aliok reviewed Sep 7, 2020

View reviewed changes

kafka/source/pkg/reconciler/source/kafkasource.go Outdated Show resolved Hide resolved

knative-prow-robot assigned aliok Sep 7, 2020

aliok reviewed Sep 7, 2020

View reviewed changes

itsmurugappan added 2 commits September 7, 2020 12:26

handle errors

d0afc27

handle errors

7e099de

aliok approved these changes Sep 8, 2020

View reviewed changes

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 8, 2020

slinkydeveloper reviewed Sep 8, 2020

View reviewed changes

use imc instead of kafka channel

32e7c35

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020

itsmurugappan added 4 commits September 9, 2020 11:56

use imc instead of kafka channel

4d0bd75

use imc instead of kafka channel

f0abe98

install imc in e2e

6347e52

install imc in e2e

46a84cb

knative-prow-robot assigned slinkydeveloper Sep 11, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2020

knative-prow-robot merged commit 1c59d4d into knative:master Sep 11, 2020

itsmurugappan deleted the adapter branch September 11, 2020 21:21

itsmurugappan restored the adapter branch October 8, 2020 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete ra deployment when sink is not found #1533

delete ra deployment when sink is not found #1533

itsmurugappan commented Sep 6, 2020

aliok commented Sep 7, 2020

aliok left a comment

aliok left a comment

slinkydeveloper Sep 8, 2020

itsmurugappan Sep 8, 2020

slinkydeveloper Sep 8, 2020

itsmurugappan Sep 8, 2020

slinkydeveloper Sep 8, 2020

itsmurugappan Sep 8, 2020

slinkydeveloper Sep 9, 2020 •

edited

Loading

aliok Sep 9, 2020

itsmurugappan Sep 9, 2020

slinkydeveloper Sep 10, 2020 •

edited

Loading

itsmurugappan Sep 10, 2020

itsmurugappan commented Sep 8, 2020

slinkydeveloper commented Sep 11, 2020

knative-prow-robot commented Sep 11, 2020

slinkydeveloper commented Sep 11, 2020

slinkydeveloper commented Sep 14, 2020

itsmurugappan commented Sep 14, 2020

delete ra deployment when sink is not found #1533

delete ra deployment when sink is not found #1533

Conversation

itsmurugappan commented Sep 6, 2020

Proposed Changes

aliok commented Sep 7, 2020

aliok left a comment

Choose a reason for hiding this comment

aliok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsmurugappan commented Sep 8, 2020

slinkydeveloper commented Sep 11, 2020

knative-prow-robot commented Sep 11, 2020

slinkydeveloper commented Sep 11, 2020

slinkydeveloper commented Sep 14, 2020

itsmurugappan commented Sep 14, 2020

slinkydeveloper Sep 9, 2020 •

edited

Loading

slinkydeveloper Sep 10, 2020 •

edited

Loading