Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change default Broker delivery spec #6011

Merged
merged 2 commits into from
Jan 18, 2022
Merged

Change default Broker delivery spec #6011

merged 2 commits into from
Jan 18, 2022

Conversation

pierDipi
Copy link
Member

Our Broker delivery spec is unset by default, which means that it has literally a
fire-and-forget delivery guarantee because with don't retry.

To make our defaults stronger, we should set a reasonable delivery spec
by default when no delivery spec is specified.

Default Delivery Spec

The following delivery spec isn't perfect for every use case by at least
prevents events loss for normal networking errors (eg connection resets, etc)
or during upgrades.

delivery:
  retry: 10
  backoffPolicy: exponential
  backoffDelay: PT0.2S

The retry period with the above configuration is ~200 seconds long

Fixes #5936

Proposed Changes

  • Change default Broker delivery spec

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

Change default Broker delivery spec

Docs

None

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Dec 22, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2021
@pierDipi pierDipi changed the title Change default Broker delivery spec [WIP] Change default Broker delivery spec Dec 22, 2021
@knative-prow-robot 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 Dec 22, 2021
@pierDipi pierDipi force-pushed the KNATIVE-5936_Change-default-delivery-spec branch from 0e7142b to b58af9b Compare December 22, 2021 16:17
@pierDipi pierDipi changed the title [WIP] Change default Broker delivery spec Change default Broker delivery spec Dec 22, 2021
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 22, 2021
@codecov
Copy link

codecov bot commented Dec 22, 2021 •
edited
Loading

Codecov Report

Merging #6011 (ee5eae0) into main (eb4c06c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6011   +/-   ##
=======================================
  Coverage   82.32%   82.32%           
=======================================
  Files         225      225           
  Lines        7636     7636           
=======================================
  Hits         6286     6286           
  Misses        910      910           
  Partials      440      440

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 eb4c06c...ee5eae0. Read the comment docs.

@pierDipi pierDipi changed the title Change default Broker delivery spec [WIP] Change default Broker delivery spec Dec 22, 2021
@knative-prow-robot 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 Dec 22, 2021
@pierDipi
Change default Broker delivery spec
c855f12 
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
@pierDipi pierDipi force-pushed the KNATIVE-5936_Change-default-delivery-spec branch 2 times, most recently from 8d15bb9 to d5d5997 Compare January 10, 2022 13:14
@pierDipi
Avoid assuming no delivery spec configured for broker.
ee5eae0 
This commit adds a delivery spec to `t2` to avoid assuming
that the Broker delivery spec is empty by default.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@pierDipi pierDipi force-pushed the KNATIVE-5936_Change-default-delivery-spec branch from d5d5997 to ee5eae0 Compare January 11, 2022 07:56
@pierDipi pierDipi changed the title [WIP] Change default Broker delivery spec Change default Broker delivery spec Jan 18, 2022
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2022
@pierDipi
Copy link
Member Author

/cc @matzew @devguyio @aliok @odacremolbap

@devguyio
Copy link
Contributor

LGTM, just need to socialize this more within the WG and our users before we merge it. @pierDipi was this already discussed during any WG meeting?
/hold

@knative-prow-robot 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 18, 2022
@pierDipi
Copy link
Member Author

Yes, this was discussed with you and @xtreme-sameer-vohra, we concluded that in addition to a better delivery guarantee by default, this also increases consistency across multiple broker implementations.

@lionelvillard
Copy link
Member

related to #4440.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022
@devguyio
Copy link
Contributor

Yes, this was discussed with you and @xtreme-sameer-vohra, we concluded that in addition to a better delivery guarantee by default, this also increases consistency across multiple broker implementations.

my volatile memory 🤦🏽‍♂️

/unhold

@knative-prow-robot 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 18, 2022
@knative-prow-robot knative-prow-robot merged commit d7b4d2c into knative:main Jan 18, 2022
pierDipi added a commit to pierDipi/eventing that referenced this pull request Jan 21, 2022
@pierDipi
Improve check reliability in TestChannelNamespaceDefaulting
453253d 
When we changed the default retry parameters in knative#6011, the check to see
if the webhook has observed the update isn't reliable anymore, this PR
checks for the expected delivery parameter to ensure that the update
was actually seen by the webhook.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
pierDipi added a commit to pierDipi/eventing that referenced this pull request Jan 21, 2022
@pierDipi
Improve check reliability in TestChannelNamespaceDefaulting
1c32c8f 
When we changed the default retry parameters in knative#6011, the check to see
if the webhook has observed the update isn't reliable anymore, this PR
checks for the expected delivery parameter to ensure that the update
was actually seen by the webhook.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliok aliok Awaiting requested review from aliok

@devguyio devguyio Awaiting requested review from devguyio

@matzew matzew Awaiting requested review from matzew

@odacremolbap odacremolbap Awaiting requested review from odacremolbap

Assignees

@lionelvillard lionelvillard

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change default Broker delivery spec
4 participants
@pierDipi @knative-prow-robot @devguyio @lionelvillard