Skip to content
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

[controller] State-of-the-world reconciliation #17

Merged
merged 19 commits into from
Aug 16, 2024
Merged

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Aug 6, 2024

Add support for state-of-the-world reconciliation, based on controller-runtime.

Closes #15.


  • 1. Replace cache with a watchable map
  • 2. Define a Runnable interface: { Run(stopCh <-chan struct{}) }
  • 3. Modify the list of informers of the controller to be a list of generic runnables
  • 4. Modify the controller builder option WithInformer to WithRunnable
  • 5. Add an informer builder option InformerBuilder(builder)
    • IncrementalInformer → returns a cache.SharedInformer that adds incremental changes directly to the cache (default informer builder)
    • StateReconciler → returns a runnable that simply adds a ListFunc to a slice of state reconciler ListFuncs of the controller (default informer builder)
  • 6. Modify informer builder func controller.For to controller.Watch
    • Build the watcher using the builder specified in the options
  • 7. Add a controller builder option ManagedBy(manager)
  • 8. Implement controller-runtime’s Reconciler interface
    • List from the cluster all resources whose kinds the controller is watching, by triggering all the state reconciler ListFuncs registered with the controller
    • Update the cache atomically with the newly fetched list of resources
  • 9. Modify controller.Start():
    • Run all the runnables
    • If a manager was provided, create a controller-runtime controller, passing the manager and the PM controller as the reconciler:
      controller.New("gatewayapi", c.manager, controller.Options{Reconciler: c})
      c.manager.Start()
  • 10. Rename CallbackFunc as ReconcileFunc
  • 11. Modify the ReconcileFunc to receive a list of ResourceEvents
  • 12. Modify controller builder option WithCallback to WithReconciler(ReconcileFunc)
    • Subscribe to the cache map
    • On messages published to the subscription channel:
      if snapshot, ok := <-subscription; ok {
        events := lo.Map(snapshot.Updates, …)
        topology := c.topology.Build(snapshot.State)
        reconcileFunc(ctx, events, topology)
      }

Out of scope of the PR but still addressed here (maybe extract to a separate PR)

  • Fix go workspaces for resolving dependencies

TODOs

  • Support for CreateEvent UpdateEvent in the state-of-the-world reconciler
  • Old object supplied with the UpdateEvents generated by the state-of-the-world reconciler
  • Field selector predicate for state-of-the-world reconciler
  • Bugfix: topology not thread-safe for read access - crashing occasionally when accessed concurrently by multiple goroutines
  • Bugfix: cannot compare Istio AuthorizationPolicies with github.com/google/go-cmp/cmp.Equal:
    cannot handle unexported field at {*v1.AuthorizationPolicy}.Spec.state:
            "istio.io/api/security/v1".AuthorizationPolicy
    consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported
    

@guicassolato guicassolato self-assigned this Aug 6, 2024
Code will still build and run without it, but this will fix resolving the dependencies by the IDE, Visual Studio Code in particular.

Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Adds support for state-of-the world reconciliation.

- Resource events grouped by GroupKind instead of GroupVersionResource - changes kind filters (e.g. subscribers) to singular form (e.g. `.Kind == "Gateway") instead of plural ("e.g. `.Resource == "gateways"`)
- List of multiple events now supplied to the reconcile funcs instead of single event – this is due to snapshots of the cache updates now accumulating multiple multiple changes
- Added Zap logger
- New command-line flag `--reconciliation-mode state|delta` to switch between state-of-the-world vs incremental (delta) reconciliation mode – default: state

Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
@Boomatang
Copy link
Contributor

I was looking over this and there was something that came to mind which I am not sure how we would stand on the issue. Being able to run the controller with or with out the need for the controller runtime is nice and I like the concept. But if I choice not use the controller runtime, it and all it's dependence's are included. It seems wrong to force this dependency, but for now I am not sure how we would approach the issue. Or even if it is an issue.

…s to restructure a listed object

Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
@guicassolato
Copy link
Contributor Author

I was looking over this and there was something that came to mind which I am not sure how we would stand on the issue. Being able to run the controller with or with out the need for the controller runtime is nice and I like the concept. But if I choice not use the controller runtime, it and all it's dependence's are included. It seems wrong to force this dependency, but for now I am not sure how we would approach the issue. Or even if it is an issue.

Maybe I'm not seeing it all, but it doesn't seem an issue to me, I guess. As long as the controller-runtime layer is not loaded, it's "only" unreachable code, no?

@guicassolato guicassolato marked this pull request as ready for review August 13, 2024 19:44
…ng further below is cohesive

Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
- RuntimeObject -> Object
- Object -> RuntimeObject
- RuntimeLinkFunc -> LinkFunc

+ object and event-related types moved to separate files
+ types.go -> resources.go

Signed-off-by: Guilherme Cassolato <[email protected]>
Copy link
Contributor

@KevFan KevFan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified envoy-gateway.md and multiple-gateway-providers.md for both delta and state reconciliation mode, and both are working as expected 💯

Changes looks good to me! 🥇

@guicassolato guicassolato merged commit 5fffd8e into main Aug 16, 2024
6 checks passed
@guicassolato guicassolato deleted the controller-runtime branch August 16, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State-of-world reconciliation
3 participants