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

feat: Combine resource events created,updated,notmodified. #293

Merged
merged 1 commit into from Sep 20, 2021

Conversation

buehler
Copy link
Owner

@buehler buehler commented Sep 20, 2021

This closes #292.

BREAKING CHANGE: This removes the separat methods
for "created", "updated", and "not modified" events.
Those events are combined into one event "reconcile".
According to https://en.wikipedia.org/wiki/Control_loop,
this is considered good practice. To migrate, remove
all references to the mentioned events and replace
them with one call to "reconcileasync".

@buehler
Copy link
Owner Author

buehler commented Sep 20, 2021

@erin-allison
This removes the differentiation between created/updated/not-modified. I'm using pre-release builds until we're happy with the new API and we had the chance to implement as many breaking changes as we need. I'd like to hear your opinion about the changes that I proposed. Currently the cache is still in place (to check for finalizers / status object). But I guess, this can be simpler as you mentioned in #264.

This closes #292.

BREAKING CHANGE: This removes the separat methods
for "created", "updated", and "not modified" events.
Those events are combined into one event "reconcile".
According to https://en.wikipedia.org/wiki/Control_loop,
this is considered good practice. To migrate, remove
all references to the mentioned events and replace
them with one call to "reconcileasync".

Signed-off-by: Christoph Bühler <cbuehler@rootd.ch>
@anekdoti
Copy link

anekdoti commented Sep 20, 2021

I agree with merging created and updated events. But on the other hand, I found the NotModified handler very useful to perform tasks like, e.g., periodically updating the status without reconciling dependent resources. By removing NotModified, one has to implement some kind of own caching solution. Similarly, you now have to check for that only the status is updated to avoid infinite loops by status updates.

Would it make sense to only merge create and update (where status updates are ignored) and leave the handling of not modified resources our resources where only the status is updated to its handler methods?

Or could something like the cache be left as a utility service that can be used in ReconcileAsync and that can tell you whether a resource got changed and in which way?

@buehler
Copy link
Owner Author

buehler commented Sep 20, 2021

@anekdoti

Right now, the status-modified still exists. So there should not be an infinite loop. And not modified was only used, when one threw requeue requests at the operator or of the watcher restarted.

I guess the best practice to check on resources periodically would be a hosted service that updates the status of resources in a timed manner. The controller should only reconcile resources IMO.

@buehler
Copy link
Owner Author

buehler commented Sep 20, 2021

To clarify: I only removed "created" "updated" and "not-modified". "deleted", "statusupdated" still exist

@anekdoti
Copy link

Thank you for the clarification! So Reconcile is only called when the entity is seen for the first time or something else than the status got changed, right? Thank you for pointing me to the hosted service solution again.

@buehler
Copy link
Owner Author

buehler commented Sep 20, 2021

Yes, exactly. Reconcile is fired when:

  • the watcher is started and catches the entities for the first time
  • the watcher is restarted and "recatches" the entities again. this results in a reconcile because that was not-modified
  • an entity is newly created
  • an entity is updated (only the spec-stuff, not the status)

I just thought about it again. If you would use an IHostedService, you could check upon your entities and then update the status of the objects with the KubernetesClient. The operator will then receive a "status updated" event and possibly does nothing.

@buehler buehler merged commit acde9e8 into next Sep 20, 2021
@buehler buehler deleted the feat/combine-events branch September 20, 2021 09:37
@anekdoti
Copy link

Thank you! Having the unified event handler and a hosted service for the status update sounds like the proper solution.

@github-actions
Copy link

🎉 This PR is included in version 6.0.0-prerelease.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

buehler added a commit that referenced this pull request Oct 11, 2021
This closes #292.

BREAKING CHANGE: This removes the separat methods
for "created", "updated", and "not modified" events.
Those events are combined into one event "reconcile".
According to https://en.wikipedia.org/wiki/Control_loop,
this is considered good practice. To migrate, remove
all references to the mentioned events and replace
them with one call to "reconcileasync".
buehler added a commit that referenced this pull request Oct 11, 2021
This closes #292.

BREAKING CHANGE: This removes the separat methods
for "created", "updated", and "not modified" events.
Those events are combined into one event "reconcile".
According to https://en.wikipedia.org/wiki/Control_loop,
this is considered good practice. To migrate, remove
all references to the mentioned events and replace
them with one call to "reconcileasync".
@TheConstructor
Copy link

This doesn't seem to be reflected in src/KubeOps.Templates/Templates/Operator.CSharp/Controller/DemoController.cs and src/KubeOps.Templates/Templates/Operator.FSharp/Controller/DemoController.fs.

I just used the template to create a new C# project and was missing a reference to KubeOps, and seemingly implementing no longer used methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants