MT-Broker: return retriable status code based on the state to leverage retries by pierDipi · Pull Request #8366 · knative/eventing

pierDipi · 2024-12-03T10:59:03Z

The ingress or filter deployments were returning 400 even in the case where a given resource (like trigger, broker, subscription) wasn't found, however, this is a common case where the lister cache hasn't caught up with the latest state.

Fixes #

Proposed Changes

MT-Broker: return appropriate status code based on the state to leverage retries

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

MT-Broker: return retriable status code based on the state to leverage retries

Docs

…age retries The ingress or filter deployments were returning 400 even in the case where a given resource (like trigger, broker, subscription) wasn't found, however, this is a common case where the lister cache hasn't caught up with the latest state. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2024-12-03T11:01:11Z

-	subscription, err := h.getSubscription(features, trigger)
 	if err != nil {
-		h.logger.Info("Unable to get the Subscription of the Trigger", zap.Error(err), zap.Any("triggerRef", triggerRef))


moved behind authn feature flag as this is only needed for that

pierDipi · 2024-12-03T11:02:06Z

/cherry-pick release-1.16

pierDipi · 2024-12-03T11:02:09Z

/cherry-pick release-1.15

knative-prow-robot · 2024-12-03T11:02:09Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.16 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

knative-prow-robot · 2024-12-03T11:02:11Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pierDipi · 2024-12-03T11:02:18Z

/cc @matzew @creydr

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

codecov · 2024-12-03T11:20:11Z

Codecov Report

Attention: Patch coverage is 17.14286% with 29 lines in your changes missing coverage. Please review.

Project coverage is 64.19%. Comparing base (795e4a3) to head (0530607).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/broker/filter/filter_handler.go	17.24%	24 Missing ⚠️
pkg/broker/ingress/ingress_handler.go	16.66%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8366      +/-   ##
==========================================
- Coverage   64.27%   64.19%   -0.08%     
==========================================
  Files         389      389              
  Lines       23276    23304      +28     
==========================================
  Hits        14961    14961              
- Misses       7521     7549      +28     
  Partials      794      794

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matzew · 2024-12-03T12:26:42Z

-		writer.WriteHeader(http.StatusBadRequest)
+	if apierrors.IsNotFound(err) {
+		h.logger.Info("Unable to find the Trigger", zap.Error(err), zap.Any("triggerRef", triggerRef))
+		writer.WriteHeader(http.StatusNotFound)


matzew · 2024-12-03T12:27:32Z

 	if err != nil {
 		h.Logger.Warn("could not get channel address from broker", zap.Error(err))
-		return http.StatusBadRequest, kncloudevents.NoDuration
+		return http.StatusInternalServerError, kncloudevents.NoDuration


matzew

/lgtm
/approve

knative-prow · 2024-12-03T12:27:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew,pierDipi]

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 · 2024-12-03T12:29:42Z

@pierDipi: new pull request created: #8367

Details

In response to this:

/cherry-pick release-1.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

knative-prow-robot · 2024-12-03T12:30:18Z

@pierDipi: #8366 failed to apply on top of branch "release-1.15":

Applying: MT-Broker: return appropriate status code based on the state to leverage retries
Using index info to reconstruct a base tree...
M	pkg/broker/filter/filter_handler.go
M	pkg/broker/ingress/ingress_handler.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/broker/ingress/ingress_handler.go
Auto-merging pkg/broker/filter/filter_handler.go
CONFLICT (content): Merge conflict in pkg/broker/filter/filter_handler.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 MT-Broker: return appropriate status code based on the state to leverage retries

Details

In response to this:

/cherry-pick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…e retries (knative#8366) * MT-Broker: return appropriate status code based on the state to leverage retries The ingress or filter deployments were returning 400 even in the case where a given resource (like trigger, broker, subscription) wasn't found, however, this is a common case where the lister cache hasn't caught up with the latest state. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix unit tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

…e retries (knative#8366) (#972) * MT-Broker: return appropriate status code based on the state to leverage retries The ingress or filter deployments were returning 400 even in the case where a given resource (like trigger, broker, subscription) wasn't found, however, this is a common case where the lister cache hasn't caught up with the latest state. * Fix unit tests --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2024

knative-prow Bot requested review from Cali0707 and aliok December 3, 2024 10:59

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2024

pierDipi commented Dec 3, 2024

View reviewed changes

knative-prow Bot requested review from creydr and matzew December 3, 2024 11:02

Fix unit tests

0530607

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

matzew reviewed Dec 3, 2024

View reviewed changes

matzew approved these changes Dec 3, 2024

View reviewed changes

knative-prow Bot assigned matzew Dec 3, 2024

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024

knative-prow Bot merged commit d4357e3 into knative:main Dec 3, 2024

knative-prow-robot mentioned this pull request Dec 3, 2024

[release-1.16] MT-Broker: return retriable status code based on the state to leverage retries #8367

Merged

pierDipi mentioned this pull request Dec 3, 2024

[release-1.15] MT-Broker: return retriable status code based on the state to leverage retries #8368

Closed

pierDipi mentioned this pull request Dec 3, 2024

[release-v1.15] MT-Broker: return retriable status code based on the state to leverage retries openshift-knative/eventing#972

Merged

pierDipi mentioned this pull request Dec 4, 2024

[release-v1.16] MT-Broker: return retriable status code based on the state to leverage retries openshift-knative/eventing#975

Closed

Conversation

pierDipi commented Dec 3, 2024

Proposed Changes

Pre-review Checklist

Uh oh!

pierDipi Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

pierDipi commented Dec 3, 2024

Uh oh!

pierDipi commented Dec 3, 2024

Uh oh!

knative-prow-robot commented Dec 3, 2024

Uh oh!

knative-prow-robot commented Dec 3, 2024

Uh oh!

pierDipi commented Dec 3, 2024

Uh oh!

codecov Bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matzew Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

matzew Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Dec 3, 2024

Uh oh!

knative-prow-robot commented Dec 3, 2024

Uh oh!

knative-prow-robot commented Dec 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Dec 3, 2024 •

edited

Loading