Delete non used branch resources at Parallel #5775

odacremolbap · 2021-10-01T16:11:30Z

Parallel reconciliation now checks for owned and not deleting channels and subscriptions:

If a channel is found that is not part of the current used ones, it is deleted.
If a subscription is found that is not linked to any of the used subscriptions, it is deleted.

See #5718 for the already merged Sequence counterpart

I also removed what seemed to be some not needed calls at the existing code. If reviewers ask to remove them from clarity I will happily remove them from here and take them to a new PR.

Proposed Changes

🐛 Fix leaking resources when updating Parallel.

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

Fix issue with Parallel updates leaked resources. In order for the fix to work the provided Channel must include the `delete` verb at the ClusterRole labeled `duck.knative.dev/channelable`.

knative-prow-robot · 2021-10-01T16:11:42Z

Hi @odacremolbap. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

odacremolbap · 2021-10-01T16:12:26Z

/assign @devguyio

codecov · 2021-10-01T16:21:57Z

Codecov Report

Merging #5775 (c005004) into main (609741f) will decrease coverage by 0.13%.
The diff coverage is 48.00%.

@@            Coverage Diff             @@
##             main    #5775      +/-   ##
==========================================
- Coverage   82.52%   82.39%   -0.14%     
==========================================
  Files         203      204       +1     
  Lines        6376     6452      +76     
==========================================
+ Hits         5262     5316      +54     
- Misses        768      778      +10     
- Partials      346      358      +12

Impacted Files	Coverage Δ
pkg/reconciler/parallel/parallel.go	`60.00% <48.00%> (+1.17%)`	⬆️
...nnelfanout/multi_channel_fanout_message_handler.go	`83.78% <0.00%> (-7.40%)`	⬇️
...reconciler/inmemorychannel/dispatcher/readiness.go	`71.42% <0.00%> (ø)`
pkg/broker/filter/filter_handler.go	`80.66% <0.00%> (+0.12%)`	⬆️
pkg/apis/messaging/v1/subscription_validation.go	`86.04% <0.00%> (+1.04%)`	⬆️
pkg/apis/messaging/v1/channel_validation.go	`84.21% <0.00%> (+1.35%)`	⬆️
...econciler/inmemorychannel/dispatcher/controller.go	`88.88% <0.00%> (+1.38%)`	⬆️
pkg/inmemorychannel/message_dispatcher.go	`92.30% <0.00%> (+2.30%)`	⬆️

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 609741f...c005004. Read the comment docs.

odacremolbap · 2021-10-01T17:22:30Z

pkg/reconciler/parallel/parallel.go

@@ -41,15 +42,14 @@ import (
 	parallelreconciler "knative.dev/eventing/pkg/client/injection/reconciler/flows/v1/parallel"
 	listers "knative.dev/eventing/pkg/client/listers/flows/v1"
 	messaginglisters "knative.dev/eventing/pkg/client/listers/messaging/v1"
-	"knative.dev/eventing/pkg/duck"


This import was duple of

ducklib "knative.dev/pkg/duck"

odacremolbap · 2021-10-01T17:23:43Z

pkg/reconciler/parallel/parallel.go

-	if p.DeletionTimestamp != nil {
-		// Everything is cleaned up by the garbage collector.
-		return nil
-	}


No need for this. Reconciler generated code already checks for the object status and do not call ReconcileKind when it is being deleted.

odacremolbap · 2021-10-01T17:25:39Z

pkg/reconciler/parallel/parallel.go

-	filterSubName := resources.ParallelFilterSubscriptionName(p.Name, branchNumber)
-


filterSubName generates the same name that was used for filterExpected. No need for that second call.

odacremolbap · 2021-10-01T17:26:30Z

pkg/reconciler/parallel/parallel.go

 	if err != nil {
 		return nil, nil, err
 	}

 	expected := resources.NewSubscription(branchNumber, p)
-	subName := resources.ParallelSubscriptionName(p.Name, branchNumber)


subName generates the same name that was used at expected. No need for that second call.

odacremolbap · 2021-10-01T17:27:33Z

pkg/reconciler/parallel/parallel.go

+func (r *Reconciler) reconcileSubscription(ctx context.Context, branchNumber int, expected *messagingv1.Subscription) (*messagingv1.Subscription, error) {
+	sub, err := r.subscriptionLister.Subscriptions(expected.Namespace).Get(expected.Name)


Removed subName and ns from the parameter list.
That data can be retrieved from the incoming subscription.

antoineco · 2021-10-01T20:16:31Z

/ok-to-test

antoineco

Looks sensible to me 👍
I'm a bit uncertain about these disconnected calls to the logger when we could wrap the entire error chain instead, but if that's the style in this package I'm happy to approve.

antoineco · 2021-10-01T20:32:06Z

pkg/reconciler/parallel/parallel.go

+		logging.FromContext(ctx).Errorw("Error getting lister for Channel", zap.Any("channelRef", channelObjRef), zap.Error(err))
+		return err


Instead of these calls to the logger inside this function, I think it would be helpful to wrap the error, e.g. with

return fmt.Errorf("error getting lister for Channel: %w", err)

and likewise wrap that inside ReconcileKind() with

return fmt.Errorf("error removing unwanted channels: %w", err)

This way, when Knative finally logs the error, it contains the whole chain of events that occurred, instead of multiple logger calls which might be hard to relate to one another:

Failed reconciliation: error removing unwanted channels: error getting lister for Channel: <the actual error>

Unless channelRef really needs its own field in the structured message. In that case, please ignore my suggestion.

antoineco · 2021-10-01T20:37:16Z

pkg/reconciler/parallel/parallel.go

+		return err
+	}
+
+	exists, err := l.ByNamespace(p.GetNamespace()).List(labels.Everything())


nit: When I see exists I immediately imagine a boolean value. Maybe that's just me 😄
Still, I think it would be more natural to call that variable channels, allChannels, or something in that direction. which IMO reads better in the range just below:

for _, c := range channels { /* ... */ }

antoineco · 2021-10-05T16:47:04Z

pkg/reconciler/parallel/parallel.go

+	for _, c := range exists {
+		ch, err := kmeta.DeletionHandlingAccessor(c)
+		if err != nil {
+			logging.FromContext(ctx).Errorw("Failed to get channel", zap.Any("channel", c), zap.Error(err))
+			return err
+		}
+
+		if !ch.GetDeletionTimestamp().IsZero() ||
+			!metav1.IsControlledBy(ch, p) {
+			continue
+		}
+
+		used := false
+		for _, cw := range wanted {
+			if cw.Name == ch.GetName() {
+				used = true
+				break
+			}
+		}


A general observation about this method:

It might be more efficient and clearer to just initialize two sets of Channel names:

one with all the "wanted" ones

one with the ones we "own"

Then delete all channels returned by owned.Difference(wanted).

odacremolbap · 2021-10-07T11:06:53Z

@antoineco thanks for the review.

All comments should have been taken care of here.

antoineco

Looking good!

nit: You use capitalized Subscription in some errors, lowercase in others. Same for Channels.

Approving with a hold in case you want to address, just unhold if you think it's not important.

/lgtm
/hold

knative-prow-robot · 2021-10-07T11:20:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, odacremolbap

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~~ [antoineco]

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

knative-metrics-robot · 2021-10-07T13:19:27Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/parallel/parallel.go	68.9%	75.2%	6.3

antoineco · 2021-10-07T13:21:23Z

/lgtm

odacremolbap · 2021-10-07T15:05:36Z

/unhold

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2021

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 1, 2021

knative-prow-robot assigned devguyio Oct 1, 2021

Delete non used branch resources at Parallel

26d72a3

odacremolbap force-pushed the parallel-deletes-non-used-branch-resources branch from bfa98e9 to 26d72a3 Compare October 1, 2021 16:32

odacremolbap commented Oct 1, 2021

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2021

antoineco reviewed Oct 5, 2021

View reviewed changes

better logger pattern

f4e0c0e

antoineco approved these changes Oct 7, 2021

View reviewed changes

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

knative-prow-robot assigned antoineco Oct 7, 2021

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

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

capitalize objects at error text

c005004

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 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 Oct 7, 2021

knative-prow-robot merged commit 830373d into knative:main Oct 7, 2021

odacremolbap deleted the parallel-deletes-non-used-branch-resources branch October 7, 2021 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete non used branch resources at Parallel #5775

Delete non used branch resources at Parallel #5775

odacremolbap commented Oct 1, 2021

knative-prow-robot commented Oct 1, 2021

odacremolbap commented Oct 1, 2021

codecov bot commented Oct 1, 2021 •

edited

Loading

odacremolbap Oct 1, 2021 •

edited

Loading

odacremolbap Oct 1, 2021

odacremolbap Oct 1, 2021

odacremolbap Oct 1, 2021

odacremolbap Oct 1, 2021

antoineco commented Oct 1, 2021

antoineco left a comment

antoineco Oct 1, 2021

odacremolbap Oct 7, 2021

antoineco Oct 1, 2021

odacremolbap Oct 7, 2021

antoineco Oct 5, 2021

odacremolbap Oct 7, 2021

odacremolbap commented Oct 7, 2021

antoineco left a comment

knative-prow-robot commented Oct 7, 2021

knative-metrics-robot commented Oct 7, 2021

antoineco commented Oct 7, 2021

odacremolbap commented Oct 7, 2021

		filterSubName := resources.ParallelFilterSubscriptionName(p.Name, branchNumber)

		func (r Reconciler) reconcileSubscription(ctx context.Context, branchNumber int, expected messagingv1.Subscription) (*messagingv1.Subscription, error) {
		sub, err := r.subscriptionLister.Subscriptions(expected.Namespace).Get(expected.Name)

		logging.FromContext(ctx).Errorw("Error getting lister for Channel", zap.Any("channelRef", channelObjRef), zap.Error(err))
		return err

Delete non used branch resources at Parallel #5775

Delete non used branch resources at Parallel #5775

Conversation

odacremolbap commented Oct 1, 2021

Proposed Changes

Pre-review Checklist

knative-prow-robot commented Oct 1, 2021

odacremolbap commented Oct 1, 2021

codecov bot commented Oct 1, 2021 • edited Loading

Codecov Report

odacremolbap Oct 1, 2021 • edited Loading

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

antoineco commented Oct 1, 2021

antoineco 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

odacremolbap commented Oct 7, 2021

antoineco left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 7, 2021

knative-metrics-robot commented Oct 7, 2021

antoineco commented Oct 7, 2021

odacremolbap commented Oct 7, 2021

codecov bot commented Oct 1, 2021 •

edited

Loading

odacremolbap Oct 1, 2021 •

edited

Loading