Skip to content

Architectural Decision/Discussion: Layering Architecture Regarding Dependent Resources #858

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

Closed
csviri opened this issue Jan 21, 2022 · 10 comments
Assignees
Labels
dependent-resources-epic lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@csviri
Copy link
Collaborator

csviri commented Jan 21, 2022

Introduction

We started to prototype and prepare the first iteration of Dependent Resource.
See Initial issue. And other related upcoming features / issues here
(This issue can shed a light on it what it wants to achieve)

The idea is nutshell is that the currently with v2 version released, JOSDK in core provides the standard facilities to implement K8S Operators in Java. It is "standard" it implements the practices where is consensus withing the community - is't roughly equivalent to what controller-runtime (go) and kopf (pyhton) covers. We had more discussions about the next steps and to also provide a higher level abstraction, thus in more declarative manner define resources and/or workflows. This is what dependent resources are.

@metacosm created the first design and implemented the first version/prototype. We all agreed that we want is to have a declarative way to define a set of resources (with annotations and related object model) and in case dependencies between them; where the developer just declares the building blocks and the execution is on the framework from that point. (Or course with extension points and all the features needed for flexibility)
This issues is NOT about that api design, we already discussed that and will further discuss, but agreed this is the right aim / goal.

Problem Statement / Question

One aspect to discuss if this feature should go to the core of the framework itself or we would like to have dependent resources as a isolated layer (separate maven module), what has impact on the current layers and APIs as less as possible. The first version is implemented like (1):

1. variation: Using dependent resources, Ideally would be an opt-in solution (part of the core module), thus the current API's will be accessible, there
will be at least some on top of them.

  • Pros:
    1. Nice out of the box experience. So everything would be in one dependency, somebody just reads the docs, and don't have to think about the layers and different modules.
  • Cons:
    1. The core would be even more complex, the complexity of the code is already quite high, not easy to reason about. This would make the actual implementation of core functionality more complex.
    2. Not necessarily that clear API, see other variant.
    3. This approach provides two distinct way how to implement a solution basically with one "API". So there is no clear demarcation, this makes problem also with integration tests, where we provide samples, in which way those samples should be implemented?

2. variation: An alternative to this, is to not change the current layer as released in v2.0.x just handle the lower layer. And provide a special Reconciler implementation, that handles the logic that would now go to the core. (Technically a also new maven module that depends on core).

  • Pros:
    1. Simplicity of the core module. Core will stay much simpler.
    2. Nice separation of concerns/layers. It would be both code and functionality wise visible where is the demarcation of these two layers. This might lead to easier understanding, of what and how is happening. In case easier debugging and problem solving. And would be cleaner architecture.
    3. Separation of APIs. Not there will be no mixing of APIs at all. Thus we can make a dedicated API in this version of the reconciler implementation that is tailored to this solution, AND mainly we won't introduce new constructs to the "lower layer API" in core module.
    4. Note we will provide an implementation on top of the "standard" api, but there are more ways to implement and support this - others might provide 3rd party dependent resource implementations - so putting this into the core is quite an opinionated decision.
  • Cons:
    1. We did not create a prototype yet, but might need to have special extension point to have for this feature. It might be harder or even hackier to implement on the top of current API. (On the other hand extension points might also provide possibility to have other 3rd party implementations of such feature)

Notes / Comments

  1. Both cases we want to stay with the annotation as it is designed now (and underlying object model).
  2. Note that we all agree that in both cases we want to have also a lower layer API exposed too, and have possibility write Operators with the current api. So dependent resource are not replacement, but a complementary solution.
@csviri
Copy link
Collaborator Author

csviri commented Jan 21, 2022

@lburgazzoli @andreaTP @adam-sandor @ppatierno and others. Would be happy to hear you opinion on this topic. Hope it's clear what is desrcibed here.

@metacosm feel free to add cons / pros, probably I did not listed everything we talked about.

Thank you all!

@andreaTP
Copy link
Collaborator

For the long term I would be more inclined toward the second approach, but, I think that we can incrementally tend to it e.g.:

3. variation: is to not change the current layer as released in v2.0.x, implement it as a special Reconciler(same as in variation 2) but have it in the core to have full access to the internals and shorten the release cycle of such an important feature.
Eventually extract it to a separate Maven module etc.

Pros:
i. Shorter time to have it live
ii. Can coexist with the current offered API
iii. Non need to extract/expose the API to enable it from the core until is stable

Cons:
i. Possible code duplication
ii. Two alternative approaches baked in the core for some time
iii. Need to explain clearly the distinction to new users etc.
iv. More complexity to handle into the core

A possible mitigation would be to mark the package as experimental and clearly mention that the API can break without notice.
I'm proposing this but I don't have a deep understanding on how much complexity is in the current core and how much the situation can become worst with this new feature 😇 .

@adam-sandor
Copy link
Collaborator

I really like @andreaTP's approach all the way with the experimental package name. This way of introducing new features has worked really well for Kubernetes, where a large userbase has a low-friction way of experimenting with features until they mature enough to be separated out to independent modules. Using some good package and class naming and docs we can make sure everyone understands where the "standard" functionality ends and where the dependent resource API begins. There is still a chance these would be too intertwined to be separate modules and this approach would reveal that too.

@csviri
Copy link
Collaborator Author

csviri commented Jan 25, 2022

Started to prototype this with a separate module to clearly see the boundaries. It is pretty much just a skeleton of the design, but will progress on it in coming days:
#866

@metacosm
Copy link
Collaborator

Regarding the first approach: the code is not that much more complex as the majority of what it does is just wiring existing things together. There's just one additional layer of complexity to allow InformerEventSource implementations to watch several namespaces at once but that's the extent of it: we don't touch the complex part of the logic which is the dispatcher. So I don't really agree with the statement that it makes the core too complex. Granted, we're not covering the full scope of features at the moment but we could also decide to stop here. :)

Regarding the API: the current version has made no effort at defining an API aside from the annotations. That wasn't the scope. To me, it would make more sense to define such an API if it is needed after users try it out as it is and it seems premature to start re-architecturing things before we gather such feedback because otherwise we might be "optimizing" for the wrong concerns.

The main advantage for the current approach, I think, is simplicity both for the user and the developers because you don't have to check the logic in different spots and it's easier to follow through than having to decide which reconciler implementation to use.

Another consideration is the maintenance aspect of things. Experimental packages / code duplication are a no-go for me considering the size of the core team. That works well when you can afford to have dedicated people tracking experimental features, which is not really the case here.

The core concern is to get the feature in people's hands ASAP, get feedback and then iterate as needed. So far, I don't see that feedback so it's like we're discussing changing things that people haven't really have had a chance to try yet so that seems premature.

@metacosm
Copy link
Collaborator

Another issue with a layered approach which would probably involve either an abstract Reconciler implementation or a completely separate interface is that it might make (at least in first approximation) looking up user provided implementations more complex for the Quarkus extension and the Spring Boot starter because they will have to handle the specific implementation used by the user.

If a layered implementation defines another Reconciler implementation, you also run the risk of confusing users who might, for example, use the annotations on the inappropriate Reconciler implementation, in which case things wouldn't work as expected. This could be worked around with documentation, of course, but I tend to think that the least documentation we have to provide for users to be able to use the SDK properly, the better. The current approach doesn't require the user to decide about whether to use dependent resources or not and is completely opt-in.

@csviri
Copy link
Collaborator Author

csviri commented Jan 25, 2022

Regarding the API: the current version has made no effort at defining an API aside from the annotations. That wasn't the scope. To me, it would make more sense to define such an API if it is needed after users try it out as it is and it seems premature to start re-architecturing things before we gather such feedback because otherwise we might be "optimizing" for the wrong concerns.

I agree, but it's not about the API, as mentioned I really like it. Looks nice with the annotations. It's about how to implement this. (I have only little worry that annotations might be limiting for some use cases, and parametrizations when it comes to reusablity, but that is a different topic). But yes, users should give feedback, already can do with the current implementation.

The main advantage for the current approach, I think, is simplicity both for the user and the developers because you don't have to check the logic in different spots and it's easier to follow through than having to decide which reconciler implementation to use.

While this is true it's also source of confusion, since we provide a way of implementing reconciliation both in a declarative and imperative way and mixing these together. So its now completely possible to implement a Reconcilaer with annotations (declarative way) and in a reconciler add imperative commands after. Which is also could be a source of confusion, ideally I would separate these too completely so we don't mix those APIs. Currently this looks a bit confusing to me. But this could be changed and can be improved. So it's kinda secondary topic, not the primary concern.

Another consideration is the maintenance aspect of things. Experimental packages / code duplication are a no-go for me considering the size of the core team. That works well when you can afford to have dedicated people tracking experimental features, which is not really the case here.

I agree, but probably how it's meant is not with code duplication it's just meant to move the part which are more likely a subject to change into a separate "experimental" package, so we can actually release it as part of the main branch.

The core concern is to get the feature in people's hands ASAP, get feedback and then iterate as needed. So far, I don't see that feedback so it's like we're discussing changing things that people haven't really have had a chance to try yet so that seems premature.

This is quite fundamental approach in the architecture, and the approach. Again not talking about the API change. It's better to have these decisions on the architecture asap, so we don't spend to much time to changing it later.

Another issue with a layered approach which would probably involve either an abstract Reconciler implementation or a completely separate interface is that it might make (at least in first approximation) looking up user provided implementations more complex for the Quarkus extension and the Spring Boot starter because they will have to handle the specific implementation used by the user.

In spring boot probably won't complicate things, that much, spring is quite flexible on this. But can take a look how to improve that part.

@csviri
Copy link
Collaborator Author

csviri commented Jan 25, 2022

Regarding the current approach, I'm fine proceeding it this way, just IMHO we should:

  1. The low level API is not changed and are fully available as a super-set of features, behind dependent resources. (So we continue to support it as the primary API).
  2. The integration tests as were now will be implemented on top of low level API (now some migrated), and we will add new integration tests for the dependent resources.
  3. We have end to end tests showcasing all the features of the low level API and separate end to end tests for the Dependent Resource. So there is no confusion.
  4. If we add API for example in Context (pretty sure we will have to). We make it very explicit that that API is for depndent resources. Like hiding it behind a api group:
    context.dependents().mymethodRelatedToDR()
  5. all dependent resources related classes goes to separate packages suffixed dependentresources. Like the api.dependentresources or process.dependentresource. Maybe combined with experimental.

This way we can make sure that those two approaches are clearly separated and not mixed in a confusing way.

So again I like this, just it's really important that the layers and APIs are well understood and structured in the architectures. (And as you can see it's already kinda effort to make these APIs clearly separated and maintainable/testable.)

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 27, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 14 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent-resources-epic lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

4 participants