Deletion of subscriptions occurs orphaned subscriber in channel implementaiton's spec

[seongjae-min]

Describe the bug

1. Create a channel and some subscriptions that subscribe data from the same channel.
2. Delete the subscriptions
3. The subscriptions are deleted, but the subscription information in the physical channel manifest randomly remains.

eventing/pkg/reconciler/subscription/subscription.go

Lines 439 to 470 in e63b13c

	func (r *Reconciler) patchSubscription(ctx context.Context, namespace string, channel *eventingduckv1.Channelable, sub *v1.Subscription) (bool, error) {
	after := channel.DeepCopy()
	if sub.DeletionTimestamp.IsZero() {
	r.updateChannelAddSubscription(after, sub)
	} else {
	r.updateChannelRemoveSubscription(after, sub)
	}
	patch, err := duck.CreateMergePatch(channel, after)
	if err != nil {
	return false, err
	}
	// If there is nothing to patch, we are good, just return.
	// Empty patch is {}, hence we check for that.
	if len(patch) <= 2 {
	return false, nil
	}
	resourceClient, err := eventingduck.ResourceInterface(r.dynamicClientSet, namespace, channel.GroupVersionKind())
	if err != nil {
	logging.FromContext(ctx).Warnw("Failed to create dynamic resource client", zap.Error(err))
	return false, err
	}
	patched, err := resourceClient.Patch(ctx, channel.GetName(), types.MergePatchType, patch, metav1.PatchOptions{})
	if err != nil {
	logging.FromContext(ctx).Warnw("Failed to patch the Channel", zap.Error(err), zap.Any("patch", patch))
	return false, err
	}
	logging.FromContext(ctx).Debugw("Patched resource", zap.Any("patch", patch), zap.Any("patched", patched))
	return true, nil
	}

It seems like:

1. Deletion of several subscriptions makes eventing-controller handle finalizers for the subscriptions.
2. eventing-controller handle finalizers simultaneously.
3. patchSubscription use PATCH API, so it cannot ensure the resource version.

Expected behavior
If a subscription is deleted, an element in subscriber in physical channel's spec will be deleted if the uid is the same.

To Reproduce

https://gist.github.com/WoWsj/3630deaa315fbc70043449231a4eaa1d

Knative release version

v1.8.2

Additional context

None

Questions

Is there any specific reason to use Patch, not Update?

I think Using Update with retryOnConflict is more proper way to ensure the resource versions in this case...

[pierDipi]

Hi @WoWsj, thanks for reporting, I think, we should move to use Update to avoid this problem.

Do you have capacity for a contribution?

/triage accepted

[pierDipi]

/area channel

[knative-prow]

@pierDipi: The label(s) area/channel cannot be applied, because the repository doesn't have them.

In response to this:

/area channel

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.

[seongjae-min]

@pierDipi I see. I will make a PR to change patch logic to Update. 😄

[l-qing]

Great!
I also faced a similar problem recently.

I temporarily set the Concurrency of subscription to 1, the default is 2.😁
Of course, if it is update, it supports more concurrency.

eventing/pkg/client/injection/reconciler/messaging/v1/subscription/controller.go

Line 104 in 271c9bb

impl := controller.NewContext(ctx, rec, controller.ControllerOptions{WorkQueueName: ctrTypeName, Logger: logger})

eventing/vendor/knative.dev/pkg/controller/controller.go

Lines 246 to 248 in 271c9bb

	if options.Concurrency == 0 {
	options.Concurrency = DefaultThreadsPerController
	}

eventing/vendor/knative.dev/pkg/controller/controller.go

Lines 57 to 64 in 271c9bb

	var (
	// DefaultThreadsPerController is the number of threads to use
	// when processing the controller's workqueue. Controller binaries
	// may adjust this process-wide default. For finer control, invoke
	// Run on the controller directly.
	// TODO rename the const to Concurrency and deprecated this
	DefaultThreadsPerController = 2
	)

[l-qing]

@pierDipi I see. I will make a PR to change patch logic to Update. 😄

Do you still plan to do this PR?

[seongjae-min]

@l-qing I think I can make a PR this week. I had no time to handle this issue recently 😭 . Is it an urgent thing for you?

[l-qing]

@l-qing I think I can make a PR this week. I had no time to handle this issue recently 😭 . Is it an urgent thing for you?

It's great! Looking forward to your good news.👍
I'm just asking. I didn't have time to deal with this for the next two weeks either. 😂