Add Subscription prober #433

devguyio · 2021-03-05T05:39:02Z

Proposed Changes

Adds a prober for the dispatcher subscriptions status

TODO

Proper manual testing
Lots of cleanup
Adding tests
Add hash support

pkg/channel/consolidated/reconciler/controller/controller.go

matzew · 2021-03-05T07:19:00Z

pkg/channel/consolidated/reconciler/controller/controller.go

+
+ // Get the Dispatcher Service Endpoints and propagate the status to the Channel
+ // endpoints has the same name as the service, so not a bug.
+ eps, err := endpoints.Get(ctx).Lister().Endpoints(dispatcherNamespace).Get(dispatcherName)


pkg/channel/consolidated/reconciler/controller/controller.go

matzew · 2021-03-05T07:23:01Z

pkg/channel/consolidated/reconciler/controller/controller.go

+ return []status.ProbeTarget{
+ {
+ PodIPs: sets.NewString(readyIPs...),
+ PodPort: "8080", Port: "8080", URLs: uls,


PodPorts vs Port ?

pkg/channel/consolidated/reconciler/controller/controller.go

pkg/channel/consolidated/reconciler/controller/kafkachannel.go

pkg/channel/consolidated/status/status.go

devguyio · 2021-03-05T10:12:59Z

@matzew thanks for the review, I added a TODO section in the PR description to show that this still needs proper cleanup (comments, naming, tests etc) , mainly would like a feedback on the approach.

codecov · 2021-03-08T11:24:07Z

Codecov Report

Merging #433 (1836b97) into main (63d04bd) will decrease coverage by 0.67%.
The diff coverage is 62.37%.

@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
- Coverage   73.80%   73.13%   -0.68%     
==========================================
  Files         138      138              
  Lines        5326     5483     +157     
==========================================
+ Hits         3931     4010      +79     
- Misses       1148     1225      +77     
- Partials      247      248       +1

Impacted Files	Coverage Δ
...onsolidated/dispatcher/consumer_message_handler.go	`0.00% <0.00%> (ø)`
...l/consolidated/reconciler/controller/controller.go	`0.00% <0.00%> (ø)`
...annel/consolidated/reconciler/controller/lister.go	`0.00% <0.00%> (ø)`
pkg/source/adapter/adapter.go	`61.29% <0.00%> (-1.01%)`	⬇️
...consolidated/reconciler/controller/kafkachannel.go	`47.24% <46.15%> (+3.90%)`	⬆️
pkg/common/consumer/consumer_handler.go	`73.07% <57.14%> (-10.26%)`	⬇️
pkg/channel/consolidated/status/status.go	`74.50% <74.50%> (ø)`
pkg/channel/consolidated/dispatcher/dispatcher.go	`65.14% <100.00%> (+1.88%)`	⬆️
...nnel/consolidated/dispatcher/kafka_subscription.go	`100.00% <100.00%> (ø)`
...kg/channel/consolidated/dispatcher/subscription.go	`0.00% <0.00%> (-53.85%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63d04bd...1836b97. Read the comment docs.

slinkydeveloper

This is going in the right direction :)

matzew · 2021-03-11T14:30:10Z

Looking at TLS failures. Is the prober able to access a secured instance of apache kafka ?

matzew · 2021-03-11T14:32:36Z

@maschmid is it worth to add your test case for this scenario to this PR, or a follow up PR ?

eg. link to your testcase for that?

matzew · 2021-03-20T06:59:51Z

/lgtm
/approve

@devguyio can we get backport for 0.20 and 0.21?

knative-prow-robot · 2021-03-20T06:59:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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:

~~OWNERS~~ [matzew,slinkydeveloper]

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

devguyio · 2021-03-20T19:45:50Z

Yep. Will work on it.
/unhold

* Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

) * Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

* Add http endpoint to communicate dispatcher readySubscriber status (#344) * Groundwork for dispatcher subscriber status w/ consumers * Add functionality to serve http of subscribers * Drop callback functions * Fix failing unit test, add unsub check for chanref * Rework http handler to be dispatcher local (not kafkasubscription) * Variable typo fix * Fix copyright years * Change header name to constant * Move subscription handler to its own ServeHTTP func * Remove channelRef in KafkaSubscription * Change bad channelref request to http.StatusNotFound * Add namespace to subscriptions http output * Add Unit tests for servehttp & setready * Split uriSplit into channelRefName{,space} vars * Expose dispatcher http-sub-status port in disatcher svc * Add servehttp diagnostic messages * One more uriSplit -> channelRefName variable rename * Change how we write the http response * Add empty SetReady() method to source RA * Fix consumer_handler_test * more linting * Add back ObserveKind until controller implements substatus scraper * Add more ServeHTTP unit tests * slightly alter where we mark a handler as ready or not * Add Subscription prober (#433) * Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Do not test controller name from generated source which can change. (#320) * Update codegen Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> Co-authored-by: Lukas Berk <lberk@redhat.com> Co-authored-by: Travis Minke <travis.minke@sap.com>

* Add http endpoint to communicate dispatcher readySubscriber status (knative-extensions#344) * Groundwork for dispatcher subscriber status w/ consumers * Add functionality to serve http of subscribers * Drop callback functions * Fix failing unit test, add unsub check for chanref * Rework http handler to be dispatcher local (not kafkasubscription) * Variable typo fix * Fix copyright years * Change header name to constant * Move subscription handler to its own ServeHTTP func * Remove channelRef in KafkaSubscription * Change bad channelref request to http.StatusNotFound * Add namespace to subscriptions http output * Add Unit tests for servehttp & setready * Split uriSplit into channelRefName{,space} vars * Expose dispatcher http-sub-status port in disatcher svc * Add servehttp diagnostic messages * One more uriSplit -> channelRefName variable rename * Change how we write the http response * Add empty SetReady() method to source RA * Fix consumer_handler_test * more linting * Add back ObserveKind until controller implements substatus scraper * Add more ServeHTTP unit tests * slightly alter where we mark a handler as ready or not * Add Subscription prober (knative-extensions#433) * Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Do not test controller name from generated source which can change. (knative-extensions#320) * Update codegen Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> Co-authored-by: Lukas Berk <lberk@redhat.com> Co-authored-by: Travis Minke <travis.minke@sap.com>

* Add http endpoint to communicate dispatcher readySubscriber status (#344) * Groundwork for dispatcher subscriber status w/ consumers * Add functionality to serve http of subscribers * Drop callback functions * Fix failing unit test, add unsub check for chanref * Rework http handler to be dispatcher local (not kafkasubscription) * Variable typo fix * Fix copyright years * Change header name to constant * Move subscription handler to its own ServeHTTP func * Remove channelRef in KafkaSubscription * Change bad channelref request to http.StatusNotFound * Add namespace to subscriptions http output * Add Unit tests for servehttp & setready * Split uriSplit into channelRefName{,space} vars * Expose dispatcher http-sub-status port in disatcher svc * Add servehttp diagnostic messages * One more uriSplit -> channelRefName variable rename * Change how we write the http response * Add empty SetReady() method to source RA * Fix consumer_handler_test * more linting * Add back ObserveKind until controller implements substatus scraper * Add more ServeHTTP unit tests * slightly alter where we mark a handler as ready or not * Add Subscription prober (#433) * Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Do not test controller name from generated source which can change. (#320) * Update codegen Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> Co-authored-by: Lukas Berk <lberk@redhat.com> Co-authored-by: Travis Minke <travis.minke@sap.com> Co-authored-by: Ahmed Abdalla Abdelrehim <aabdelre@redhat.com> Co-authored-by: Lukas Berk <lberk@redhat.com> Co-authored-by: Travis Minke <travis.minke@sap.com>

* Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

* Add Subscription prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix endpoints informer in cons. KafkaChannel controller * Fix unittests after adding status prober Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Format and order go imports in cons. channel controller Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Rename import alias and remove unused variable Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Add dispatcher prober test for tesitng a single pod Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Support probing dispatchers for multiple partitions kafka channels Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Update deps Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix conumer handler test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove unused hashes from status probing test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Apply review comments and add a prober test Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove old comment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Fix fake status manager Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Return error if IsReady returns an error Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Change probing to be partition based and fix some corner cases of channel deletion * Change cleanup logic to clean ready subscriptions only Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove cleanup to avaid consumers race Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> Co-authored-by: Ahmed Abdalla Abdelrehim <aabdelre@redhat.com>

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

…sions#435) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

[release-v1.0] Golang 1.17 container image (knative-extensions#433)

devguyio requested review from a team as code owners March 5, 2021 05:39

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 5, 2021

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 5, 2021