-
Notifications
You must be signed in to change notification settings - Fork 86
feat(controllers): Multi-controller/-finalizer dispatch via ShouldHandle(TEntity)
#1084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e304e28
acdbe13
1a87f17
e93746d
93314b2
5fabcd5
49f82e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,20 @@ namespace KubeOps.Abstractions.Reconciliation.Finalizer; | |
| public interface IEntityFinalizer<TEntity> | ||
| where TEntity : IKubernetesObject<V1ObjectMeta> | ||
| { | ||
| /// <summary> | ||
| /// Returns <c>true</c> when this finalizer is responsible for the given entity. | ||
| /// The default implementation returns <c>true</c>, preserving backward-compatible behaviour. | ||
| /// | ||
| /// When <see cref="KubeOps.Abstractions.Builder.OperatorSettings.AutoAttachFinalizers"/> is enabled, | ||
| /// only finalizers that return <c>true</c> from <see cref="ShouldHandle"/> are attached to the entity. | ||
| /// Once attached, the finalizer is dispatched by its identifier as usual — <see cref="ShouldHandle"/> | ||
| /// acts as a one-time responsibility claim at attach time, not an ongoing gate. | ||
| /// </summary> | ||
| /// <param name="entity">The entity the reconciler is considering for this finalizer.</param> | ||
| /// <param name="cancellationToken">The token to monitor for cancellation requests.</param> | ||
| /// <returns>A <see cref="ValueTask{Boolean}"/> that resolves to <c>true</c> if this finalizer should claim the entity.</returns> | ||
| ValueTask<bool> ShouldHandle(TEntity entity, CancellationToken cancellationToken = default) => ValueTask.FromResult(true); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here (mandatory not = default) |
||
|
|
||
| /// <summary> | ||
|
stevefan1999-personal marked this conversation as resolved.
|
||
| /// Finalize an entity that is pending for deletion. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,10 @@ public IOperatorBuilder AddController<TImplementation, TEntity>() | |
| where TImplementation : class, IEntityController<TEntity> | ||
| where TEntity : IKubernetesObject<V1ObjectMeta> | ||
| { | ||
| Services.TryAddScoped<IEntityController<TEntity>, TImplementation>(); | ||
| // TryAddEnumerable dedupes by (ServiceType, ImplementationType), so calling AddController | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure this AI comment is really needed 😄 |
||
| // with the same TImplementation twice registers it only once — while still allowing | ||
| // distinct implementations to coexist for the same TEntity. | ||
| Services.TryAddEnumerable(ServiceDescriptor.Scoped<IEntityController<TEntity>, TImplementation>()); | ||
| Services.TryAddSingleton<IReconciler<TEntity>, Reconciler<TEntity>>(); | ||
|
stevefan1999-personal marked this conversation as resolved.
|
||
|
|
||
| // Requeue | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| using k8s.Models; | ||
|
|
||
| using KubeOps.Abstractions.Builder; | ||
| using KubeOps.Abstractions.Entities; | ||
| using KubeOps.Abstractions.Reconciliation; | ||
| using KubeOps.Abstractions.Reconciliation.Controller; | ||
| using KubeOps.Abstractions.Reconciliation.Finalizer; | ||
|
|
@@ -115,8 +116,11 @@ await entityQueue | |
| cancellationToken); | ||
|
|
||
| await using var scope = serviceProvider.CreateAsyncScope(); | ||
| var controller = scope.ServiceProvider.GetRequiredService<IEntityController<TEntity>>(); | ||
| var result = await controller.DeletedAsync(reconciliationContext.Entity, cancellationToken); | ||
| var result = await DispatchToMatchingControllers( | ||
| scope.ServiceProvider, | ||
| reconciliationContext.Entity, | ||
| (ctrl, entity, ct) => ctrl.DeletedAsync(entity, ct), | ||
| cancellationToken); | ||
|
|
||
| if (result.IsSuccess) | ||
| { | ||
|
|
@@ -137,21 +141,106 @@ await entityQueue | |
|
|
||
| if (operatorSettings.AutoAttachFinalizers) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't see any benefit in this line |
||
| var finalizers = scope.ServiceProvider.GetKeyedServices<IEntityFinalizer<TEntity>>(KeyedService.AnyKey); | ||
|
|
||
| var anyFinalizerAdded = finalizers | ||
| .Aggregate( | ||
| false, | ||
| (changed, finalizer) => entity.AddFinalizer(finalizer.GetIdentifierName(entity)) || changed); | ||
| var anyFinalizerAdded = false; | ||
| foreach (var finalizer in finalizers) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| if (!await finalizer.ShouldHandle(entity, cancellationToken)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the same as with the controllers - this could lead to being > 1 finalizers being responsible for an entity which might lead to weired results. |
||
| { | ||
| continue; | ||
| } | ||
|
|
||
| anyFinalizerAdded = entity.AddFinalizer(finalizer.GetIdentifierName(entity)) || anyFinalizerAdded; | ||
| } | ||
|
|
||
| if (anyFinalizerAdded) | ||
| { | ||
| entity = await client.UpdateAsync(entity, cancellationToken); | ||
| } | ||
| } | ||
|
|
||
| var controller = scope.ServiceProvider.GetRequiredService<IEntityController<TEntity>>(); | ||
| return await controller.ReconcileAsync(entity, cancellationToken); | ||
| return await DispatchToMatchingControllers( | ||
| scope.ServiceProvider, | ||
| entity, | ||
| (ctrl, e, ct) => ctrl.ReconcileAsync(e, ct), | ||
| cancellationToken); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves all <see cref="IEntityController{TEntity}"/> registrations and, in registration order, | ||
| /// asks each controller <see cref="IEntityController{TEntity}.ShouldHandle"/> against the current | ||
| /// (possibly mutated) entity just-in-time and dispatches <paramref name="operation"/> when it claims | ||
| /// responsibility. On the first failure the chain short-circuits and that failure is returned. | ||
| /// If no controller is registered at all the result is a configuration-error failure; if controllers | ||
| /// are registered but none claim responsibility, a success result is returned and a warning is logged. | ||
| /// <para> | ||
| /// <b>RequeueAfter aggregation:</b> across successful controller results, the earliest non-null | ||
| /// <see cref="ReconciliationResult{TEntity}.RequeueAfter"/> is kept, so an auditing controller that | ||
| /// returns <c>Success(entity)</c> never erases a requeue requested by an earlier controller. | ||
| /// </para> | ||
| /// </summary> | ||
| private async Task<ReconciliationResult<TEntity>> DispatchToMatchingControllers( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be honest I am pretty unsure about the possibility to have more than 1 controller being responsible for an entity - shouldn't the pattern be more like:
|
||
| IServiceProvider services, | ||
| TEntity entity, | ||
| Func<IEntityController<TEntity>, TEntity, CancellationToken, Task<ReconciliationResult<TEntity>>> operation, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - please remove |
||
|
|
||
| var registeredControllers = services.GetServices<IEntityController<TEntity>>().ToList(); | ||
| if (registeredControllers.Count == 0) | ||
| { | ||
| return ReconciliationResult<TEntity>.Failure( | ||
| entity, | ||
| $"No IEntityController<{typeof(TEntity).Name}> registered. Did you forget to call AddController<T, TEntity>() on the operator builder?"); | ||
| } | ||
|
Comment on lines
+193
to
+199
|
||
|
|
||
| var currentEntity = entity; | ||
| TimeSpan? aggregatedRequeueAfter = null; | ||
| var anyDispatched = false; | ||
|
|
||
| foreach (var controller in registeredControllers) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| // Evaluate ShouldHandle just-in-time against the (possibly mutated) current entity — | ||
| // so a controller that would claim the *initial* state but reject the *post-mutation* | ||
| // state does not get invoked. | ||
| if (!await controller.ShouldHandle(currentEntity, cancellationToken)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| anyDispatched = true; | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var result = await operation(controller, currentEntity, cancellationToken); | ||
| if (!result.IsSuccess) | ||
| { | ||
| return result; | ||
| } | ||
|
|
||
| currentEntity = result.Entity; | ||
|
|
||
| if (result.RequeueAfter is not null && | ||
| (aggregatedRequeueAfter is null || result.RequeueAfter < aggregatedRequeueAfter)) | ||
| { | ||
| aggregatedRequeueAfter = result.RequeueAfter; | ||
| } | ||
| } | ||
|
|
||
| if (!anyDispatched) | ||
| { | ||
| logger.LogWarning( | ||
| """No responsible controller found for "{Kind}/{Name}". Skipping.""", | ||
| currentEntity.Kind, | ||
| currentEntity.Name()); | ||
| return ReconciliationResult<TEntity>.Success(currentEntity); | ||
| } | ||
|
|
||
| return ReconciliationResult<TEntity>.Success(currentEntity, aggregatedRequeueAfter); | ||
| } | ||
|
|
||
| private async Task<ReconciliationResult<TEntity>> ReconcileFinalizersSequential(TEntity entity, CancellationToken cancellationToken) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevefan1999-personal would prefer this not to be = default by default but mandatory