⚠ Support shutdown watches dynamically (v2)

[inteon]

⚠️ depends on #2315

PTAL at the original PR: #2099

I started making some small changes to the original PR, thinking there would be an easy fix. However, it turned out that there were still a lot of fundamental questions requiring more time and effort to get fixed.

The current state of the PR is best described by #2159 (comment).

(4/4) PR chain:

• ✨Move Source implementations to a single folder #2685
• ⚠Only allow sources to start once #2686
• ⚠ Move arguments from Watch() & Start() to Source objects #2315
• ⚠ Support shutdown watches dynamically (v2) #2159

[k8s-ci-robot]

Hi @inteon. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[FillZpp]

Thanks @inteon , nice work!

It seems you have removed the Stop() from Controller, so how should users stop one or more specific controller?

[FillZpp]

/ok-to-test

[inteon]

Thanks @inteon , nice work!

It seems you have removed the Stop() from Controller, so how should users stop one or more specific controller?

@FillZpp what do you think about a more generic solution for canceling runnables:

// NewCancelRunnable returns a new Runnable and a function to cancel that runnable.
func NewCancelRunnable(r Runnable) (Runnable, context.CancelFunc) {
	mu := sync.Mutex{}
	cancel := (func())(nil)
	canceled := false

	return RunnableFunc(func(ctx context.Context) error {
			mu.Lock()
			ctx, cancel = context.WithCancel(ctx)
			if canceled {
				cancel()
			}
			mu.Unlock()

			return r.Start(ctx)
		}), func() {
			mu.Lock()
			defer mu.Unlock()
			canceled = true
			if cancel != nil {
				cancel()
			}
		}
}

[FillZpp]

Thanks @inteon , nice work!
It seems you have removed the Stop() from Controller, so how should users stop one or more specific controller?

@FillZpp what do you think about a more generic solution for canceling runnables:

// NewCancelRunnable returns a new Runnable and a function to cancel that runnable.
func NewCancelRunnable(r Runnable) (Runnable, context.CancelFunc) {
	mu := sync.Mutex{}
	cancel := (func())(nil)
	canceled := false

	return RunnableFunc(func(ctx context.Context) error {
			mu.Lock()
			ctx, cancel = context.WithCancel(ctx)
			if canceled {
				cancel()
			}
			mu.Unlock()

			return r.Start(ctx)
		}), func() {
			mu.Lock()
			defer mu.Unlock()
			canceled = true
			if cancel != nil {
				cancel()
			}
		}
}

Emm, I think it's alright. It may not be intuitive as the Stop method, but more generic as you said.

[FillZpp]

/label tide/merge-method-squash

[FillZpp]

fixes #1884

[inteon]

@vincepri are these breaking changes ok?: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/2159/pull-controller-runtime-apidiff-master/1625461909174095872

[lou-lan]

@inteon Thank you for your contribution with this PR. I've noticed that this PR seems to have not been updated for a while. I believe this PR is very important for the enhancement of this project, and I also have use cases for this feature. If there is any way I can help, such as conducting code reviews or assisting with testing, I would be more than happy to contribute.

[inteon]

@lou-lan I'm not sure what is blocking this PR.
I resolved the remaining issues and challenges.
Looking for a final review from @vincepri @sbueringer or @FillZpp

[alvaroaleman]

Sorry, I don't understand this change. Contrary to what the title claims, it never stops any watch, it stops the source. How does that help? Please provide a real-world use-case.

If this is supposed to work towards allowing to stop watches by refcounting if they have eventhandlers or are otherwise used, I'd like to see a description somewhere as to how exactly this will work once all pieces are done.

[inteon]

Sorry, I don't understand this change. Contrary to what the title claims, it never stops any watch, it stops the source. How does that help? Please provide a real-world use-case.

If this is supposed to work towards allowing to stop watches by refcounting if they have eventhandlers or are otherwise used, I'd like to see a description somewhere as to how exactly this will work once all pieces are done.

@alvaroaleman I just added a StopWatch function to expose this functionality via the Controller API (see 6c16925).
This integration test should be a good example on how this PR can be used:
6c16925#diff-c1c23edcf40f177710028c953468fd5bf7aa0f32d67780354c609e127603bb04

[inteon]

/test pull-controller-runtime-test

[inteon]

/retest

[k8s-ci-robot]

@inteon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff-master	a169245	link	false	/test pull-controller-runtime-apidiff-master
pull-controller-runtime-apidiff	b50828a	link	false	/test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.