`CancellationToken` support and a lot of async improvements #725

nachtjasmin · 2024-02-27T08:25:09Z

This PR adds the ability to cancel (almost) all asynchronous operations with the built-in CancellationToken. Due to the nature of interfaces, almost all of those changes are breaking the existing API.

It also adds an WatchAsync method to the IKubernetesClient, which is making use of the newer asynchronous enumerables.

I tried to keep it as backwards-compatible as possible, however, this was not possible due to different problems. Most notably, the interfaces no longer have a default implementation to actually break existing code instead of failing silently. If I would've just changed the existing interface (with Task.CompletedTask as the return , current implementations of ReconcileAsync would have not been called anymore. Therefore I removed the default implementations to ensure that all remaining controllers get updated.

Note about the one failing test

During local development with a kind cluster, I could not get the KubeOps.Operator.Test.Events.EventPublisherIntegrationTest.Should_Increase_Count_On_Existing_Event to run successfully. It always broke with some certification error, other tests work fine tho. I could not determine whether it is an issue with the kind cluster or some other error.

buehler · 2024-03-04T08:11:00Z

Hey @nachtjasmin
Thank you for your contribution!

I don't mind breaking changes at all. While I'm reviewing the stuff, the linter is ranting about some things it seems. dotnet format should be able to help as far as I know.

Another general comment: Why do we need factories? Personally I'm not really a fan of factories. Is there a specific use-case / reason for them?

src/KubeOps.Abstractions/Events/IEventPublisherFactory.cs

src/KubeOps.Abstractions/Finalizer/IEventFinalizerAttacherFactory.cs

src/KubeOps.KubernetesClient/IKubernetesClient.cs

src/KubeOps.Operator/Builder/OperatorBuilder.cs

test/KubeOps.Operator.Test/Controller/CancelEntityRequeue.Integration.Test.cs

test/KubeOps.Operator.Test/Events/EventPublisher.Integration.Test.cs

buehler · 2024-03-04T12:47:02Z

The linter seems to be angry with you...

BREAKING: This changes the interface signature of IKubernetesClient.

BREAKING: The signature changes for derived classes.

nachtjasmin · 2024-03-05T08:20:32Z

The joy of line endings. Running dotnet format was not enough, cause Git would've changed the line endings back to LF on git add. Added a .gitattributes file in order to enforce the correct line endings for the .cs files, now it should work as intended.

buehler · 2024-03-05T08:42:38Z

all fingers crossed.

examples/ConversionWebhookOperator/Controller/V1TestEntityController.cs

+{
+    public Task ReconcileAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Reconciling entity {Entity}.", entity);


examples/ConversionWebhookOperator/Controller/V1TestEntityController.cs

+
+    public Task DeletedAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Deleted entity {Entity}.", entity);


examples/Operator/Controller/V1TestEntityController.cs

+{
+    public async Task ReconcileAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Reconciling entity {Entity}.", entity);


examples/Operator/Controller/V1TestEntityController.cs

+
+    public Task DeletedAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Deleting entity {Entity}.", entity);


examples/WebhookOperator/Controller/V1TestEntityController.cs

+{
+    public Task ReconcileAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Reconciling entity {Entity}.", entity);


examples/WebhookOperator/Controller/V1TestEntityController.cs

+
+    public Task DeletedAsync(V1TestEntity entity, CancellationToken cancellationToken)
+    {
+        logger.LogInformation("Deleted entity {Entity}.", entity);


buehler · 2024-03-05T09:23:11Z

Hey @nachtjasmin

Do you have any idea, why the tests won't start?

It seems, that the host does not start in test mode.

The debug output of the github actions:

KubeOps.Operator.Test -> /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0/KubeOps.Operator.Test.dll
Test run for /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0/KubeOps.Operator.Test.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.9.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
<7>Microsoft.Extensions.Hosting.Internal.Host[1] Hosting starting
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Starting resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Started resource watcher for V1OperatorIntegrationTestEntity.
<6>Microsoft.Hosting.Lifetime[0] Application started. Press Ctrl+C to shut down.
<6>Microsoft.Hosting.Lifetime[0] Hosting environment: Production
<6>Microsoft.Hosting.Lifetime[0] Content root path: /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0
<7>Microsoft.Extensions.Hosting.Internal.Host[2] Hosting started
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Received watch event "Added" for "OperatorIntegrationTest/test-entity", last observed resource version: 601.
<7>Microsoft.Extensions.Hosting.Internal.Host[3] Hosting stopping
<6>Microsoft.Hosting.Lifetime[0] Application is shutting down...
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Stopping resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Stopped resource watcher for V1OperatorIntegrationTestEntity.
<7>Microsoft.Extensions.Hosting.Internal.Host[4] Hosting stopped
<7>Microsoft.Extensions.Hosting.Internal.Host[1] Hosting starting
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Starting resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Started resource watcher for V1OperatorIntegrationTestEntity.
<6>Microsoft.Hosting.Lifetime[0] Application started. Press Ctrl+C to shut down.
<6>Microsoft.Hosting.Lifetime[0] Hosting environment: Production
<6>Microsoft.Hosting.Lifetime[0] Content root path: /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0
<7>Microsoft.Extensions.Hosting.Internal.Host[2] Hosting started
<7>Microsoft.Extensions.Hosting.Internal.Host[3] Hosting stopping
<6>Microsoft.Hosting.Lifetime[0] Application is shutting down...
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Stopping resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Stopped resource watcher for V1OperatorIntegrationTestEntity.
<7>Microsoft.Extensions.Hosting.Internal.Host[4] Hosting stopped
<7>Microsoft.Extensions.Hosting.Internal.Host[1] Hosting starting
<7>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Subscribe for leadership updates.
<6>Microsoft.Hosting.Lifetime[0] Application started. Press Ctrl+C to shut down.
<6>Microsoft.Hosting.Lifetime[0] Hosting environment: Production
<6>Microsoft.Hosting.Lifetime[0] Content root path: /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0
<7>Microsoft.Extensions.Hosting.Internal.Host[2] Hosting started
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] This instance started leading, starting watcher.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Starting resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Started resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Added" for "OperatorIntegrationTest/test-entity", last observed resource version: 600.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Added" for "OperatorIntegrationTest/test-entity", last observed resource version: 617.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Added" for "OperatorIntegrationTest/test-entity2", last observed resource version: 610.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Added" for "OperatorIntegrationTest/test-entity", last observed resource version: 601.
<7>Microsoft.Extensions.Hosting.Internal.Host[3] Hosting stopping
<6>Microsoft.Hosting.Lifetime[0] Application is shutting down...
<7>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Unsubscribe from leadership updates.
<7>Microsoft.Extensions.Hosting.Internal.Host[4] Hosting stopped
<7>Microsoft.Extensions.Hosting.Internal.Host[1] Hosting starting
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Starting resource watcher for V1OperatorIntegrationTestEntity.
<6>KubeOps.Operator.Watcher.ResourceWatcher[0] Started resource watcher for V1OperatorIntegrationTestEntity.
<6>Microsoft.Hosting.Lifetime[0] Application started. Press Ctrl+C to shut down.
<6>Microsoft.Hosting.Lifetime[0] Hosting environment: Production
<6>Microsoft.Hosting.Lifetime[0] Content root path: /home/runner/work/dotnet-operator-sdk/dotnet-operator-sdk/test/KubeOps.Operator.Test/bin/Debug/net8.0
<7>Microsoft.Extensions.Hosting.Internal.Host[2] Hosting started
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Deleted" for "OperatorIntegrationTest/test-entity", last observed resource version: 628.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Deleted" for "OperatorIntegrationTest/test-entity", last observed resource version: 631.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Deleted" for "OperatorIntegrationTest/test-entity2", last observed resource version: 640.
<6>KubeOps.Operator.Watcher.LeaderAwareResourceWatcher[0] Received watch event "Deleted" for "OperatorIntegrationTest/test-entity", last observed resource version: 649.

nachtjasmin · 2024-03-05T12:29:33Z

Do you have any idea, why the tests won't start?

Not at all, but I'll try to fix it. ^^

buehler · 2024-03-05T13:35:33Z

If I can be of any help, just say the word. I try to fetch your fork and run the tests locally.

nachtjasmin · 2024-03-07T09:53:12Z

At least with the local KinD cluster and with act, the tests are working again. The KubeOps.Operator.Test.Events.EventPublisherIntegrationTest.Should_Increase_Count_On_Existing_Event seems to be flaky for me, I'll try to fix it as well.

After that, I plan to rewrite the commit history to ensure a clean history. Gonna let you know when I'm done with that, just so you don't have to accept my WIP commits all the time. 😄

buehler · 2024-03-07T09:56:25Z

Nice work, thank you :-)

Don't worry about the commits, they're gonna be squashed anyway.

buehler · 2024-03-07T09:56:46Z

And thank you for your effort!

The previous implementation effectively enforced that a Kubernetes cluster is present. While this is acceptable for the production environment, it made integration testing much harder. By adding several factories for publishers, attachers and the requeue delegates, we can now mock those tasks as well. Furthermore, the leader election is no longer done during the creation phase, instead it's now implemented as one additional background service.

The synchronous event-based implementations were replaced by two separate, now asynchronous IHostedService services. One is responsible for watching the resources while the other is watching the queue for manual requeues. The queue for requeueing entities is now asynchronous as well and instead of creating multiple timers, it leverages Task.Delay and the CancellationTokenSource. Also, finalizer registrations are taking advantage of the ability to store keyed services in the DI container.

…ken.None

Also thanks to Stephen Cleary: https://blog.stephencleary.com/2020/05/backgroundservice-gotcha-startup.html

We don't wanna let run the tests forever, a generous timeout of 30 seconds should be suitable for most cases.

This reverts commit 6078151.

nachtjasmin · 2024-03-07T11:17:21Z

Well, if the commits are squashed anyway, I think I'm done now. :D
Although I'm not happy with the big "change all line endings" commit. That should've been avoided in the first place. But with the new .gitattributes file it hopefully shouldn't happen anymore for upcoming PRs.

(note to self: make smaller PRs next time 🙈)

buehler · 2024-03-07T13:13:38Z

The linter is still yapping, but I'll fix the issues right away.

nachtjasmin · 2024-03-07T14:19:00Z

The linter is still yapping, but I'll fix the issues right away.

aaah, sorry ><
totally forgot to push that one little commit which would've avoided that.

nachtjasmin force-pushed the feature/better-asynchronous-api-usage branch from c5ca283 to c9082a8 Compare February 27, 2024 08:26

nachtjasmin marked this pull request as ready for review March 1, 2024 15:46

nachtjasmin force-pushed the feature/better-asynchronous-api-usage branch from c9082a8 to b12f14b Compare March 1, 2024 15:46

nachtjasmin changed the title ~~feature: add CancellationToken support~~ CancellationToken support and a lot of async improvements Mar 1, 2024

nachtjasmin force-pushed the feature/better-asynchronous-api-usage branch from b12f14b to becd43c Compare March 1, 2024 16:26

buehler requested changes Mar 4, 2024

View reviewed changes

nachtjasmin added 5 commits March 5, 2024 09:11

feat(client)!: add cancellation support

3887f7b

feat(client)!: add WatchAsync

256c3e4

BREAKING: This changes the interface signature of IKubernetesClient.

feat(operator)!: add cancellation for controllers and finalizers

f8eab10

feat(operator)!: add cancellation to webhooks

7bd3179

BREAKING: The signature changes for derived classes.

feat(operator): add cancellation support to delegates

d52d25d

nachtjasmin force-pushed the feature/better-asynchronous-api-usage branch 2 times, most recently from c72dd5e to 32a34c5 Compare March 5, 2024 08:20

github-advanced-security bot found potential problems Mar 5, 2024

View reviewed changes

buehler previously approved these changes Mar 7, 2024

View reviewed changes

buehler enabled auto-merge (squash) March 7, 2024 09:13

auto-merge was automatically disabled March 7, 2024 09:31
Head branch was pushed to by a user without write access

nachtjasmin dismissed buehler’s stale review via 90074f9 March 7, 2024 09:31

nachtjasmin added 2 commits March 7, 2024 11:55

docs(abstractions): smaller documentation improvements

d405617

nachtjasmin added 12 commits March 7, 2024 11:55

test(operator): skip two flaky tests

2fb9ee6

fix(client): use a shared cancellationToken instead of CancellationTo…

7cf0b85

…ken.None

chore: enforce crlf line endings for .cs

05532ff

fix(operator): use Task.Run for background services

c33c70e

Also thanks to Stephen Cleary: https://blog.stephencleary.com/2020/05/backgroundservice-gotcha-startup.html

test: add timeout for InvocationCounter

83d0be8

We don't wanna let run the tests forever, a generous timeout of 30 seconds should be suitable for most cases.

fix(operator): use singleton as the lifetime

89071f5

revert(test): enable the two tests again

36d3e05

This reverts commit 6078151.

fix(client): add dispose check

a18f36c

test: ensure thread-safety of InvocationCounter

8f8f219

fixup! feat(client)!: add WatchAsync

606f520

fixup! chore: enforce crlf line endings for .cs

81a74fd

nachtjasmin force-pushed the feature/better-asynchronous-api-usage branch from 90074f9 to 81a74fd Compare March 7, 2024 11:16

buehler merged commit 2d17bff into buehler:main Mar 7, 2024
1 of 3 checks passed

nachtjasmin deleted the feature/better-asynchronous-api-usage branch March 7, 2024 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`CancellationToken` support and a lot of async improvements #725

`CancellationToken` support and a lot of async improvements #725

nachtjasmin commented Feb 27, 2024 •

edited

buehler commented Mar 4, 2024

buehler commented Mar 4, 2024

nachtjasmin commented Mar 5, 2024

buehler commented Mar 5, 2024

buehler commented Mar 5, 2024

nachtjasmin commented Mar 5, 2024

buehler commented Mar 5, 2024

nachtjasmin commented Mar 7, 2024

buehler commented Mar 7, 2024

buehler commented Mar 7, 2024

nachtjasmin commented Mar 7, 2024 •

edited

buehler commented Mar 7, 2024

nachtjasmin commented Mar 7, 2024 •

edited

CancellationToken support and a lot of async improvements #725

CancellationToken support and a lot of async improvements #725

Conversation

nachtjasmin commented Feb 27, 2024 • edited

Note about the one failing test

buehler commented Mar 4, 2024

buehler commented Mar 4, 2024

nachtjasmin commented Mar 5, 2024

buehler commented Mar 5, 2024

buehler commented Mar 5, 2024

nachtjasmin commented Mar 5, 2024

buehler commented Mar 5, 2024

nachtjasmin commented Mar 7, 2024

buehler commented Mar 7, 2024

buehler commented Mar 7, 2024

nachtjasmin commented Mar 7, 2024 • edited

buehler commented Mar 7, 2024

nachtjasmin commented Mar 7, 2024 • edited

`CancellationToken` support and a lot of async improvements #725

`CancellationToken` support and a lot of async improvements #725

nachtjasmin commented Feb 27, 2024 •

edited

nachtjasmin commented Mar 7, 2024 •

edited

nachtjasmin commented Mar 7, 2024 •

edited