Implement lookup controller #150
Conversation
|
@MrLarssonJr Thanks a lot for working on this PR. |
|
Happy to help @AdheipSingh! I must admit I've only tested in a kind cluster, but in there updates work! Sure, I'll take a look at integrating status. I also saw the notification that some test failed, I'll look into that as well. |
|
@AdheipSingh, I just wanted to check if you've had some time to look over my new commits? |
|
@itamar-marom @cyril-corbon if you can help with the reviews. |
There was a problem hiding this comment.
Hi @MrLarssonJr sorry for the VERY late review.
I left a lot of comments, I think we should first address those and then move to another iteration because it will be easier. I'll be more active from now on :)
|
Hi @itamar-marom, thanks for your feedback! I'll incorporate it and come back with some new commits. |
|
I've marked conversations I feel like I've addressed as resolved to help me keep track of what items I've still got to address. @itamar-marom, if you feel anyone of the conversations I marked as resolved, in fact are not resolved, please do not hesitate to re-open them! |
| "time" | ||
| ) | ||
|
|
||
| type DruidClient struct { |
There was a problem hiding this comment.
Non-blocking - I think that what you did here might be a good interface for all reconcilers we might create that works with the Druid API.
Do you think we can have this as an interface and maybe create an abstraction for other reconcilers to use?
You don't have to implement it here, or at all :) would appreciate your opinion
There was a problem hiding this comment.
Thank you, happy to you like it! My goal with this struct were to collect the parts that interact with the Druid API into one place. I can definitely see how that API interaction being a common problem for all reconcilers working with Druid state.
Do you think we can have …
I see two ways of interpret this question. If you're asking for my permission to build on top of the ideas within the code, 1. how kind of you to ask(!), 2. please do! If you're asking if it would be feasible to build an abstraction on top of it, then here's a bit too long ramble of how one could do that!
Most of the methods of this type (e.g. delete, or upsert) are pretty low-level in the sense that they are just wrappers around the HTTP API. I think having one type/package that exposes this functionality as methods/functions to abstract away the HTTPness of the API would probably be helpful.
Some of the other methods that DruidClient expose, such as Reconcile or GetStatus, are a lot more logic oriented. Perhaps there are common patterns across the different reconcilers that might be worth abstracted cleanly. Perhaps the reconciliation logic is one such place. Given some interface that exposes functionality can list current instances of a resources, as well as create, replace and delete instance of that resource, one could probably create a function that consumes such an interface as well as a list of desired instances of that resource. There of course is the problem of slight variations in different applications of that pattern. The lookup resource has an example of such a slight variation of the base pattern, namely: the resource space does not start initialised, and does not support retrieving or listing instances before it is initialised. Of course, that specific example is easily solved by initialising the lookup space before handing it of to the hypothetical generic reconciliation function. However, that complication was likely easy to fix with in that abstraction since I drafted it within the context of lookup reconciliation. While I still lack the much wider context of the other reconcilers in the operator, I would be more than happy to contribute to the discussion!
|
We're getting close to closing all code-related issues! |
|
Hi @itamar-marom! My turn to say sorry for a long delay in replying, thank you for your patience 🙏 Some time has finally opened up and I'm starting to dig into your latest comments. I expect I'll have some new commits in the coming days addressing them! |
MrLarssonJr
force-pushed
the
lookup-controller
branch
from
7288f20
to
764aab3
Compare
|
Alright, became a bit of rewrite once more. Highlights:
|
|
(I tried to fix the e2e test failing due to a name for a minio tenant pool were missing. It did get further, but there still seems to be something wrong. I've had issues running it locally also due to arm64/amd64 issues inside of kind.) |
|
@AdheipSingh code looks good. I'm not sure why the e2e fails. Can you help troubleshoot this? |
Fixes #139.
Description
I'd like to be able to provision and manage my druid clusters with GitOps techniques. This druid operator allows me to declaratively configure the nodes of a druid cluster. Another part is being able do declaratively specify ingestions, which others are working on. This PR brings another piece of the puzzle by allowing users to declare druid lookups as K8s resources.
This PR implements a reconciliation strategy of getting a comprehensive understanding of both current and desired lookups, calculating a diff and based on that diff taking corrective actions. This provides a simple and resilient reconciliation loop. Another strategy that were explored during the development of this PR were to only reconcile one druid lookup K8s resource at time as events are received by the controller. While this approach could prove more efficient due to not requesting the complete state in every iteration, it proved to require fragile bookkeeping to handle certain cases, e.g. editing a lookup K8s resource to rename a lookup.
This PR has:
Key changed/added files in this PR
controllers/lookup/*