Data plane reconciler handles failed reconciliation. #568

pierDipi · 2021-01-28T09:27:42Z

We were caching resources that failed to reconcile.
This leads to not retrying to reconcile such resources.

This PR handle failed reconciled resources by avoiding
caching them, simplify our reconciler code, and add
logging.

Proposed Changes

Data plane reconciler handles failed reconciliation

Release Note

Data plane reconciler handles failed reconciliation.

Docs

/kind bug

codecov · 2021-01-28T09:39:55Z

Codecov Report

Merging #568 (a58cb30) into master (3a19f2d) will increase coverage by 0.43%.
The diff coverage is 88.35%.

@@             Coverage Diff              @@
##             master     #568      +/-   ##
============================================
+ Coverage     71.79%   72.22%   +0.43%     
- Complexity      367      391      +24     
============================================
  Files            72       72              
  Lines          2581     2657      +76     
  Branches        121      112       -9     
============================================
+ Hits           1853     1919      +66     
- Misses          584      596      +12     
+ Partials        144      142       -2

Flag	Coverage Δ	Complexity Δ
java-unittests	`79.37% <88.35%> (+0.41%)`	`0.00 <55.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
.../kafka/broker/core/eventbus/ContractPublisher.java	`80.00% <ø> (ø)`	`3.00 <0.00> (ø)`
...enting/kafka/broker/receiver/ReceiverVerticle.java	`92.85% <ø> (-0.25%)`	`3.00 <0.00> (ø)`
...ka/broker/dispatcher/ConsumerDeployerVerticle.java	`69.09% <34.78%> (-13.14%)`	`9.00 <3.00> (ø)`
.../eventing/kafka/broker/receiver/RequestMapper.java	`87.90% <76.00%> (-4.07%)`	`18.00 <5.00> (+1.00)`	⬇️
...nciler/impl/ResourcesReconcilerMessageHandler.java	`96.15% <95.45%> (+4.48%)`	`10.00 <9.00> (+5.00)`
...e/eventing/kafka/broker/core/file/FileWatcher.java	`83.33% <100.00%> (-0.39%)`	`8.00 <0.00> (+1.00)`	⬇️
.../core/reconciler/impl/ResourcesReconcilerImpl.java	`93.79% <100.00%> (+7.58%)`	`58.00 <38.00> (+17.00)`
...ting/kafka/broker/core/utils/CollectionsUtils.java	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`

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 3a19f2d...3517362. Read the comment docs.

pierDipi · 2021-01-28T10:05:36Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

slinkydeveloper

Minor comments

...ain/java/dev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerImpl.java

data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/RequestMapper.java

pierDipi · 2021-01-28T11:04:53Z

Failed to create v1 broker "broker": Internal error occurred: failed calling webhook "webhook.eventing.knative.dev": Post https://eventing-webhook.knative-eventing.svc:443/defaulting?timeout=2s: no endpoints available for service "eventing-webhook"

pierDipi · 2021-01-28T11:05:55Z

I guess HPA gets in the way with chaosduck.

pierDipi · 2021-01-28T12:11:38Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/FileWatcher.java

pierDipi · 2021-01-28T22:13:42Z

/retest

slinkydeveloper · 2021-01-29T09:04:58Z

/retest

pierDipi · 2021-01-29T11:25:56Z

/retest
/hold

pierDipi · 2021-01-29T13:08:07Z

/unhold

pierDipi · 2021-01-29T13:08:41Z

Worked, retesting
/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-01-29T14:19:16Z

/retest

pierDipi · 2021-01-29T16:30:26Z

Worked, retesting
/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-01-29T17:34:25Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

pierDipi · 2021-02-08T11:42:10Z

/test pull-knative-sandbox-eventing-kafka-broker-unit-tests

pierDipi · 2021-02-08T12:38:53Z

/retest

slinkydeveloper

Small nit, otherwise sounds good to me

...ev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerMessageHandler.java

pierDipi · 2021-02-08T17:07:26Z

/retest

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

slinkydeveloper · 2021-02-08T18:40:56Z

/lgtm
/approve

knative-prow-robot · 2021-02-08T18:41:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, 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~~ [pierDipi,slinkydeveloper]

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

pierDipi · 2021-02-08T20:09:03Z

/retest

pierDipi · 2021-02-08T20:49:34Z

/test pull-knative-sandbox-eventing-kafka-broker-integration-tests

…ons#568) * Update go.mod Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Reconciler handles failed verticles deploy Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Add more logging Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Logging: egress -> ingress Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Use CompositeFuture.join for egress reconciler Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Use computeIfAbsent Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Check Exception on delete egress Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Check only generation + explanation Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * There could be concurrent reconciliation using the event bus (?) Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * all -> join Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Refactor reconcile Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Move set new contract to help reasoning Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Delete benchmarks

…e-extensions#568) * Re-add limits Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Update remove_source_limits.patch Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Revert unrelated formatting Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Generate release artifacts Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow-robot added kind/bug Categorizes issue or PR as related to a bug. area/data-plane labels Jan 28, 2021

knative-prow-robot requested review from matzew and slinkydeveloper January 28, 2021 09:27

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

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

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

slinkydeveloper reviewed Jan 28, 2021

View reviewed changes

pierDipi force-pushed the refactor-reconciler branch from b4e5c97 to 97bb560 Compare January 28, 2021 11:21

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2021

pierDipi force-pushed the refactor-reconciler branch from ed6cb91 to d6c6418 Compare January 28, 2021 15:06

slinkydeveloper reviewed Jan 28, 2021

View reviewed changes

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/FileWatcher.java Outdated Show resolved Hide resolved

pierDipi force-pushed the refactor-reconciler branch from c985d70 to 28e2454 Compare January 28, 2021 15:28

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2021

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

pierDipi mentioned this pull request Jan 29, 2021

[WIP] Guard concurrent reconciliation #575

Closed

pierDipi requested a review from slinkydeveloper February 8, 2021 12:02

slinkydeveloper reviewed Feb 8, 2021

View reviewed changes

...ev/knative/eventing/kafka/broker/core/reconciler/impl/ResourcesReconcilerMessageHandler.java Outdated Show resolved Hide resolved

pierDipi added 13 commits February 8, 2021 19:38

Update go.mod

e8290af

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Reconciler handles failed verticles deploy

ea99a4b

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Add more logging

aa218a5

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Logging: egress -> ingress

43b7dfd

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Use CompositeFuture.join for egress reconciler

c78347d

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Use computeIfAbsent

8b1ba86

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Check Exception on delete egress

c1c12b6

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Check only generation + explanation

71c0760

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

There could be concurrent reconciliation using the event bus (?)

fd0dc48

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

all -> join

5fbc842

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Refactor reconcile

e697d24

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Move set new contract to help reasoning

381200b

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Delete benchmarks

3517362

pierDipi force-pushed the refactor-reconciler branch from cd1f7f5 to 3517362 Compare February 8, 2021 18:38

pierDipi requested a review from slinkydeveloper February 8, 2021 18:39

knative-prow-robot assigned slinkydeveloper Feb 8, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2021

knative-prow-robot merged commit 44707db into knative-extensions:master Feb 8, 2021

pierDipi deleted the refactor-reconciler branch April 14, 2021 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data plane reconciler handles failed reconciliation. #568

Data plane reconciler handles failed reconciliation. #568

pierDipi commented Jan 28, 2021

codecov bot commented Jan 28, 2021 •

edited

Loading

pierDipi commented Jan 28, 2021

slinkydeveloper left a comment

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

slinkydeveloper commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Feb 8, 2021

pierDipi commented Feb 8, 2021

slinkydeveloper left a comment

pierDipi commented Feb 8, 2021

slinkydeveloper commented Feb 8, 2021

knative-prow-robot commented Feb 8, 2021

pierDipi commented Feb 8, 2021

pierDipi commented Feb 8, 2021

Data plane reconciler handles failed reconciliation. #568

Data plane reconciler handles failed reconciliation. #568

Conversation

pierDipi commented Jan 28, 2021

Proposed Changes

codecov bot commented Jan 28, 2021 • edited Loading

Codecov Report

pierDipi commented Jan 28, 2021

slinkydeveloper left a comment

Choose a reason for hiding this comment

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

pierDipi commented Jan 28, 2021

slinkydeveloper commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Jan 29, 2021

pierDipi commented Feb 8, 2021

pierDipi commented Feb 8, 2021

slinkydeveloper left a comment

Choose a reason for hiding this comment

pierDipi commented Feb 8, 2021

slinkydeveloper commented Feb 8, 2021

knative-prow-robot commented Feb 8, 2021

pierDipi commented Feb 8, 2021

pierDipi commented Feb 8, 2021

codecov bot commented Jan 28, 2021 •

edited

Loading