-
-
Notifications
You must be signed in to change notification settings - Fork 82
feat!: introduce result-pattern and automatic finalizer management #980
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?
Conversation
# Conflicts: # src/KubeOps.Abstractions/KubeOps.Abstractions.csproj
…t for builder identifier
…g (hybrid cache) - Integrated FusionCache for robust caching in resource watchers. - Enhanced default configuration with extensible settings in `OperatorSettings`. - Improved concurrency handling using `SemaphoreSlim` for entity events. - Updated tests and dependencies to reflect caching changes.
…nt entity locks - Renamed `DefaultCacheConfiguration` to `DefaultResourceWatcherCacheConfiguration` for clarity. - Introduced cache key prefix to improve cache segmentation. - Removed `ConcurrentDictionary` for entity locks to simplify concurrency management. - Refactored event handling logic for "added" and "modified" events to streamline codebase.
- Updated `ConfigureResourceWatcherEntityCache` to use `IFusionCacheBuilder` for extensibility. - Moved resource watcher cache setup logic to `WithResourceWatcherCaching` extension. - Added detailed XML comments for `EntityLoggingScope` to improve documentation. - Removed redundant `DefaultResourceWatcherCacheConfiguration`.
- Renamed `WithResourceWatcherCaching` to `WithResourceWatcherEntityCaching` for clarity. - Updated `CacheExtensions` to be `internal` to limit scope. - Removed unused dependency on `ZiggyCreatures.Caching.Fusion`.
…onstants` for consistency
- Added a new `Caching` documentation page explaining resource watcher caching with FusionCache and configuration options (in-memory and distributed). - Updated sidebar positions for `Deployment`, `Utilities`, and `Testing` to accommodate the new `Caching` page.
…usionCache details - Improved explanations for in-memory and distributed caching setups. - Added example code for customizing resource watcher cache with FusionCache. - Included references to FusionCache and Redis documentation for further guidance.
# Conflicts:
# src/KubeOps.Operator/Watcher/ResourceWatcher{TEntity}.cs
# Conflicts:
# examples/Operator/Finalizer/FinalizerOne.cs
# src/KubeOps.Abstractions/KubeOps.Abstractions.csproj
# src/KubeOps.Operator/Builder/CacheExtensions.cs
# src/KubeOps.Operator/Constants/CacheConstants.cs
# src/KubeOps.Operator/KubeOps.Operator.csproj
# src/KubeOps.Operator/Watcher/ResourceWatcher{TEntity}.cs
…ependency - Removed redundant requeue logic and optimized entity cache operations during deletion in `ResourceWatcher`. - Upgraded `ZiggyCreatures.FusionCache` to version `2.4.0`.
- Introduced `RequeueType` enumeration to specify requeue operation types (`Added`, `Modified`, `Deleted`). - Implemented `RequeueTypeExtensions` for mapping `WatchEventType` to `RequeueType`. - Updated requeue mechanism to include `RequeueType` in `EntityRequeue` and related methods. - Refactored `TimedEntityQueue` and related classes to support `RequeueEntry` containing both the entity and its requeue type. - Adjusted tests to incorporate `RequeueType` into entity requeue logic.
… reconciliation logic - Created `IReconciler<TEntity>` interface and its implementation to handle entity creation, modification, and deletion. - Updated `ResourceWatcher` and `EntityRequeueBackgroundService` to use `Reconciler` for reconciliation operations. - Removed redundant FusionCache dependency from `ResourceWatcher` and related classes. - Streamlined requeue mechanics and replaced entity finalization logic with `Reconciler` integration.
- Registered `IReconciler<TEntity>` and its implementation `Reconciler<TEntity>` in the service container. - Ensured proper integration with existing requeue and entity processing workflows.
…-attach/detach options - Added `AutoAttachFinalizers` and `AutoDetachFinalizers` settings in `OperatorSettings`, enabling automatic management of entity finalizers during reconciliation. - Extended `Reconciler` to respect these settings for adding and removing finalizers. - Introduced `EntityFinalizerExtensions` for streamlined finalizer handling and identifier generation. - Updated relevant interfaces and documentation for improved clarity and usability.
…ant values - Update `KubernetesEntitySyntaxReceiver` to utilize `SemanticModel` for attribute argument resolution, ensuring accurate value retrieval.
- Updated `EntityFinalizerExtensions` to correctly append "finalizer" when missing from the name. - Added unit tests to validate finalizer identifier generation, including cases for length limits and naming consistency.
- Renamed test cases and entities for improved clarity and consistency. - Added new tests for entities with no group values and entities with varying group definitions. - Adjusted expected
…interface for improved flexibility - Extracted `ITimedEntityQueue` interface from `TimedEntityQueue` implementation. - Updated all usages, including services and tests, to rely on the interface. - Added extension methods for requeue key management. - Improved code consistency and maintainability across the queue system.
…r election - Replaced `EnableLeaderElection` with `LeaderElectionType` in `OperatorSettings` for enhanced configurability. - Added `LeaderElectionType` enum with options: None, Single, and Custom. - Updated `OperatorBuilder` to handle leader election logic based on `LeaderElectionType`. - Modified `EntityRequeueBackgroundService` to public visibility and implemented proper `Dispose` logic. - Adjusted tests to reflect new leader election mechanism. - Improved code maintainability and alignment with distributed system requirements.
…alizer tests - Marked entities (`V1OperatorIntegrationTestEntity` and its sub-classes) and test classes as `sealed` for better design and optimization. - Added null checks in finalizer integration tests to improve safety and adherence to modern C# standards. - Disabled obsolete warning in `KubernetesClient` with a TODO for clarification.
…sts and core classes - Updated object and dictionary initializations to improve readability. - Applied consistent formatting to multiline constructs and private methods. - Improved alignment with modern C# coding style.
- Adjusted formatting in `RbacGenerator` and `NamespacedOperator.Integration.Test` to enhance consistency and readability.
- Added license headers to multiple source and test files to align with .NET Foundation standards. - Adjusted method formatting in `V1TestEntityController` for better readability and consistency.
- Simplified explanation of `AutoDetachFinalizers` behavior. - Updated code documentation to clarify handling of processed messages.
- Added a note about ensuring cluster and local time synchronization to address potential leader election issues caused by time drift.
- Updated parameter and method names for clarity and better alignment with coding conventions. - Replaced `provider` with `serviceProvider` and `settings` with `operatorSettings`. - Enhanced readability and alignment with modern C# standards in reconciliation methods.
|
@ralf-cestusio I've now found the time to submit the pull request - you're welcome to review the changes and provide feedback. Thanks. |
- Added `AutoAttachFinalizers` and `AutoDetachFinalizers` configurations in integration tests. - Updated finalizer method signatures to include cancellation tokens for improved handling.
|
Hey @kimpenhaus One question to start with: The first change with the returning result pattern. This was implmeneted in a long past version of the sdk (v6 or so) and I changed it because I thought it is more extensible when you inject finalizer and requeue mechanisms instead of relying on return values. The return values are parsed by the SDK core engine and thus, feature implementations and enhancements must also touch the core. With the injection of such extensions (e.g. finalizers, requeue), you can provide those without touching the core. Or: at least, that was my intention. wdyt? |
|
Hey Christoph @buehler, Yeah - take your time. I know it's a lot of work on your side. I appreciate the time you'll be investing - thanks for that. 🙏🏼 Regarding your point on the result pattern: My intention is as follows:
Looking forward to your feedback! 😊 |
# Conflicts:
# src/KubeOps.Abstractions/Entities/KubernetesExtensions.cs
# src/KubeOps.Operator/Watcher/ResourceWatcher{TEntity}.cs
# test/KubeOps.Operator.Test/KubeOps.Operator.Test.csproj
# test/KubeOps.Transpiler.Test/KubeOps.Transpiler.Test.csproj
|
I wanted to give some qualitative feedback (completely from a user perspective) I have adapted out operator to use this pr and i really like how development feels. Normal reconcile code has also become more readable and i managed to now avoid all 409 errors, because we can harness updates more easily. |
|
Cool! Thanks for the insights. Definitely looking forward to the code :) |
buehler
left a comment
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.
Thank you for this big contribution! I really like the changes and I'm looking forward to see how people integrate those into their operators. Getting more aligned with the go implementation makes sense apparently.
I do have some minor comments/questions. Feel free to comment on them and let us have a discussion :-)
Thanks again!
| using k8s.Models; | ||
|
|
||
| using KubeOps.Abstractions.Finalizer; | ||
| using KubeOps.Abstractions.Reconciliation.Finalizer; |
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.
Same here. Since the return type of the finalizeasync method has changed, please update the template
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.
fixed
| @@ -1,4 +1,4 @@ | |||
| using KubeOps.Abstractions.Controller; | |||
| using KubeOps.Abstractions.Reconciliation.Controller; | |||
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.
same :)
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.
fixed
| using k8s.Models; | ||
|
|
||
| using KubeOps.Abstractions.Finalizer; | ||
| using KubeOps.Abstractions.Reconciliation.Finalizer; |
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.
same :)
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.
fixed
| namespace ConversionWebhookOperator.Controller; | ||
|
|
||
| [EntityRbac(typeof(V1TestEntity), Verbs = RbacVerb.All)] | ||
| public class V1TestEntityController(ILogger<V1TestEntityController> logger) : IEntityController<V1TestEntity> |
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.
for consistency: can you make this one "sealed" as well? (you made the others sealed)
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.
fixed
| @@ -1,4 +1,4 @@ | |||
| using KubeOps.Abstractions.Controller; | |||
| using KubeOps.Abstractions.Reconciliation.Controller; | |||
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.
since you changed the structure and return values, can you update the template return types as well? this will fail when templates are used. also, for consistency, I'd like to see the controller / finalizer "sealed" like you did in the example codes.
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.
wasn't aware of the templates - sorry - fixed all of them :)
| entity.Kind, | ||
| entity.Name(), | ||
| identifier); | ||
| return ReconciliationResult<TEntity>.Success(entity); |
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.
Is "not being able to finalize" a success or rather an error? Depends on the finalizer I guess. wdyt?
| namespace KubeOps.Operator.Queue; | ||
|
|
||
| internal sealed class EntityRequeueBackgroundService<TEntity>( | ||
| public class EntityRequeueBackgroundService<TEntity>( |
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.
If made public (intentionally I guess?), then there should be an XML Documentation for the class to be consistent.
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.
I made it public to have a chance to overload it, as we do. we wanted requeuing to be durable and are using azure service bus as scheduler for it (added a sample to the docs how one can use it). this also gives the opportunity to trigger reconciliation over the service bus from different scenarios we have. e.g. when having a "infrastructural" deployments with some changes which need reconciliation to be triggered.
added the xml docs
|
|
||
| namespace KubeOps.Operator.Queue; | ||
|
|
||
| public sealed record RequeueEntry<TEntity> |
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.
short xml documentation about the class/record
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.
added
|
|
||
| public RequeueType RequeueType { get; } | ||
|
|
||
| public static RequeueEntry<TEntity> CreateFor(TEntity entity, RequeueType requeueType) |
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.
why use "CreateFor" instead of the normal constructor? this seems redundant.
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.
valid point - I thought a factory method is more expressive in the code but removing it and changing the record to required init properties reduced the code a lot - changed it
…ate reconciliation method signatures - Marked entity-related classes as sealed for improved clarity and security. - Adjusted reconciliation and finalizer method return types to use `ReconciliationResult` in dotnet templates.
- Simplified condition checks by replacing `IsFailure` with `!IsSuccess`. - Updated related tests and logic to reflect the removal of `IsFailure` property.
…ialization and simplify object creation - Replaced factory method with init-only properties in `RequeueEntry`. - Enhanced instantiation of `TimedQueueEntry` with object initializer syntax. - Added XML documentation for improved code readability.
| if (scope.ServiceProvider.GetKeyedService<IEntityFinalizer<TEntity>>(identifier) is not | ||
| { } finalizer) | ||
| { | ||
| logger.LogDebug( |
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.
I don't think this should be a warning.
There are valid cases why there is a finalizer on a resource, but its not handled by the operator.
A good example is a cascading foreground delete where k8s itself sets a finalizer
Hey Christoph @buehler
This is a comprehensive PR containing changes from integrating the operator into our cluster environment.
Summary
This PR introduces breaking changes to the KubeOps SDK, implementing a result pattern inspired by the Go operator implementation. Controllers and finalizers now return
ReconciliationResult<TEntity>enabling explicit success/failure states, centralized requeuing viaRequeueAfter, and automatic finalizer lifecycle management. Additional improvements include extensible requeue mechanisms, const value support in source generators, and configurable leader election types.Breaking Changes⚠️
1. Result Pattern
Controller and finalizer interfaces now return
Task<ReconciliationResult<TEntity>>instead ofTask:Before:
After:
The
ReconciliationResult<TEntity>provides:RequeueAftertimespan for delayed reprocessingMigration Example:
2. Namespace Reorganization
Types moved to new namespaces:
IEntityController<TEntity>:KubeOps.Abstractions.Controller→KubeOps.Abstractions.Reconciliation.ControllerIEntityFinalizer<TEntity>:KubeOps.Abstractions.Finalizer→KubeOps.Abstractions.Reconciliation.FinalizerEntityRequeue:KubeOps.Abstractions.Queue→KubeOps.Abstractions.Reconciliation.QueueIEntityRequeueFactory:KubeOps.Abstractions.Queue→KubeOps.Abstractions.Reconciliation.QueueMigration: Update using statements in your controllers and finalizers.
3. Queue Interface Changes
The internal queue interface is now public and extensible:
This enables implementing durable requeue mechanisms (e.g., backed by Redis, Service Bus, database) by overriding the default in-memory implementation.
New Features
1. Automatic Finalizer Management
Two new settings provide automatic finalizer lifecycle management:
Benefits:
2. Const Value Support in Source Generator
The syntax receiver now supports constant values in Kubernetes entity attributes:
Benefits:
3. Leader Election Type Configuration
Introduction of
LeaderElectionTypeenum for explicit leader election configuration:Configuration:
Benefits:
4. Extensible Requeue Mechanism
Introduction of
RequeueTypeenum andITimedEntityQueue<TEntity>interface:Use Cases:
Example Implementation:
5. ReconciliationContext
New context object providing metadata about reconciliation triggers:
Helps distinguish between API server events and operator-initiated requeues.
Implementation Details
Core Components
ReconciliationResult (
src/KubeOps.Abstractions/Reconciliation/ReconciliationResult{TEntity}.cs)Reconciler (
src/KubeOps.Operator/Reconciliation/Reconciler.cs)ITimedEntityQueue (
src/KubeOps.Operator/Queue/ITimedEntityQueue{TEntity}.cs)Alignment with Go Implementation
This implementation draws inspiration from controller-runtime (Go):
Testing
ReconciliationResult<TEntity>ReconciliationContext<TEntity>RequeueTypeDocumentation
Additional Notes
Migration Checklist
For operators upgrading to this version:
ReconciliationResult<TEntity>ReconciliationResult<TEntity>