Dependent resource hierarchy #958
Conversation
|
This doesn't appear to address the read-only use-case for Kubernetes dependent resources. Also, if we're going to have configuration for this, I'd rather do it via interfaces to express traits than making it flags in the configuration. Using traits, we would only add the methods when needed (and to implement the support for create/update you would need these methods anyway) rather than do it with sub-classes (having trait interfaces would bypass the need for sub-classes since the base classes wouldn't need to provide abstract/default implementations). Another issue is that this implementation duplicates code in different branches of the hierarchy (notably Having trait interfaces would also help for more precise RBAC creation in extensions of the SDK that do so in a generic way instead of having to know about all the possible sub-classes which would lead to fragile code base and implementation leakage. |
There is the KubernetesReadOnlyDependentResource:
The downside of this is that, it's not very explicit. So what is the behavior if a trait object is missing. Also not sure how do you mean it. The
The point is that for different behaviors like the generic case or the read only behavior we already provide abstract implementations, if somebody overrides it should know what is doing. I guess this is generally problem with anything else, like overriding a trait object and braking the behavior.
I don't understand this, where do you mean it? Could you put here links to the code pls. |
It is not read-only though: the
There will only be one default implementation that covers most cases. If people have a case that isn't covered, they could still create their own Having lots of implementations for special cases doesn't help because it makes things more complex to understand, to maintain and to use. And it still wouldn't cover all the use cases.
There should only be one default implementation that covers 80% of the cases in a nice way. Not 10 different implementations that cover 90% in an awkward and error-prone way. :)
|
Pls check it again, I think you misread it, it does not extend the KubernetesDependentResource, and has no access to any of the methods you mention. |
What use case we don't cover with this PR? There is basically one base implementation that is extended with one generic. That's it, and if somebody want to implement it's own for external resources it is now much more easy and straight-forward now , see the Polling for read only. With traits what would be quite confusing to do. For me again the whole point of the domain modelling is have these notions explicit, and not hack it with a seemingly simple way, where are the cases are not explicitly captured, so it's quite confusing. But again happy take a look with traits if you add a prototype. |
The prototype was the implementation that was written 3 months ago… :) |
|
fixed this. Moved to base class. But needs different case for generic see, other PR. |
Indeed. However I can still override |
you don't have |
|
That was quite a different situation we did not have the |
Since the class is now empty, why do we even need it? It doesn't enforce anything, doesn't provide any new feature, it simply increases the cognitive load on the user that needs to figure out why they would use this implementation as opposed to another one.
More precisely, how is relying on empty implementation of methods any better than enforcing a behavior via a return type? For the latter, you can actually enforce it in code. |
It implements the read only interface. And that is not desired for the generic one.
You can override reocncile in any case, this is not an argument at all. If somebody want's to break it can. It's not about that, it's defining with an interface a common behavior abstraction. The optional way woudl be pretty ugly on the other hand, since one would have to override delete and update and create. While those won't be used. Think about how something like |
|
@metacosm what you are refering is the open-closed principle that is a good argument for the traits (not for the optional IMHO). And if we take a step back basically the originally why we moved from the traits, is that it is open to extension, but there is a problem how it describes the domain/use cases explicitly, and might be problems with generality, like to cover use cases which we cannot see, and even what we can see are there all trait combinations make sense? And things like that. So again don't know how to put a dot at the end of this discussion. But having this class hierarchy make much more sense to me since, we can prepare quite nice abstractions and generic toolkit with a relatively simple class hierarchy. That users just can simply extend and implement custom dependent resources. I'm up to experiment and move forward with traits also if it implements the |
Then just implement the interface. No need for an empty class that doesn't add anything useful.
But that behavior doesn't exist. It's only materialised by a specific implementation and a naming convention. There's no "read-only" concept that you can enforce in the code of the SDK with this implementation.
There's no The value of the SDK should be to make the common use cases very easy to implement, define a pattern that can be extended to all implementations, provide support for that pattern in the code, instead of trying to exhaustively support every use case we can think of. Having a class hierarchy doesn't address the generality problem, it just keeps adding special cases that make the code base more confused, more difficult to maintain and harder to learn. It also forces extensions of the SDK to know about theses implementations instead of relying on a simpler, generic pattern thus making them more fragile with respect to changes in the implementation. |
The problem with this, that the
With this just wanted to show the extensibility. Or how easy is to do it. We don't have anything for this in previous PR. But the point is that we have to think about it eventually, and have a vision for it's design.
With this PR basically we do this, with a transparent way. So again don't understand why is having an Optional in desired makes this more clean, it makes the whole api very confusing. We just separating the concerns that should be separated, and as @scrocquesel mentioned not use fat classes, that does everything. In this way also is nicely extensible.
IMHO We add special cases with the "optional desired", for an implementation that is already targets the generic use cases. That is hard to understand. Much harder than having a dedicated abstraction for a sub-problem. |
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
|
Kudos, SonarCloud Quality Gate passed! |
Extending an empty class is a bigger smell than implementing a marker interface because it forces you into a class hierarchy, which is a bad pattern for frameworks/SDK. If you think that's a smell, then that's a problem with the design. The bigger problem is whether you extend the implementation or implement the interface, the SDK still cannot enforce the read-only functionality, which is the bigger smell of all, imo. :)
It's very simple, conceptually, though: if, for any reason, your implementation doesn't know what to return at a specific time, then you return |
Hmm, but how is possible to do that? I mean even with traits, or with the desired, it's basically you can always do that. So not sure how you mean it exactly.
I understand what you mean, and I agree that it does not bring too much complexity in that regard. The problem is it breaks the abstraction. So the GenericDependentResource meant to describe the cases when the reconciliation looks exactly like this: var actual = getResource(primary);
var desired = desired(primary, context);
if (actual.isEmpty()) {
create(desired, primary, context);
} else {
if (!match(actual.get(), desired, context)) {
update(actual.get(), desired, primary, context);
}
}What is the most common case, when an controller owns the resources, and reconciles it. Whit optinal desired, we cover that and some additional more exotic case, what could desire a completely different abstraction, since we don't just kubernetes but also other external resources that will implement that. In this case this is the read only. So I don't understand this part: So what I try to say, is the good abstractions of the problems/cases should have the highest priority. And here the generic and the read-only cases deserves it's own abstraction under dependent resource. At least for me this is much more important than having done the current scope (that will probably be extended) with some adjustments to abstractions that has well defined scopes.
It's not that natural, basically then it's not a dependent resource, it's an informer. Not sure if we should actually even solve this. I mean we have to just because we want to have it nicely done with annotations, without that actually we don't even need this. We could just reference the informer from the dependent resource what is using it as an input. |
If
I'm not sure how returning The editable-only case would be supported as follows: var maybeDesired = desired(primary, context);
maybeDesired.ifPresent(desired -> {
var maybeActual = getResource(primary);
if (maybeActual.isEmpty()) {
create(desired, primary, context);
} else {
final var actual = maybeActual.get();
if (!match(actual, desired, context)) {
update(actual, desired, primary, context);
}
}
});This is, indeed, a little awkward because this would mean that However, in the current implementation, the abstraction is ill-defined, because why should editable-only be configured via annotation while read-only is implemented by a subclass / marker interface. So there's an asymmetry here that shouldn't be there. If one feature is configured by annotation, then the other should be as well. If the feature is implemented by a subclass then both should be as well, and in this case, this means adding more classes to support the combinations (i.e. you would need an
Except that the abstraction is not good or at least doesn't work for all cases. Why should read-only be more important than editable-only or creatable-only? The current implementation doesn't address this problem. I do agree, though, that returning
This just isn't true. It often happens when you reconcile dependents that there just isn't any valid state to create/update at some point in time. Granted, though, that enters the domain of managing dependencies between dependents. |
|
Trying to keep pace. Readonly(informer only) resource is something a reconciler needs to know the state. So in this sense, it's a dependent but it can't be reconciled. It's not depending on its state, it is intrisics. And to enforce that, its public contract should not expose reconcile/delete. Having a base class that expose reconcile/delete from the ground up tell that every subclass can be reconciled which is not true. And as such, whatever that used them must reconcile them and delete. I understand the need for being standalone but can some logic be put in the manager ? This way, only traits can be on the dependent ? Trying to put the managing code at the same place as the behavior itself made things not SRP and force to have a base class with all the complexity. I agree that clean separation of concern will add more code and classes. There is a tradeoff between less code and clean object modeling. public interface DependentResource<R, P extends HasMetadata> {
Optional<R> getResource(P primaryResource);
}
public interface Desiring { R desired(...); }
public interface Create implements Desiring { void create(...); }
public interface Update implements Desiring { void update(...); bool match(...); }
public interface Delete{ void delete(...); }
public abstract class KubernetesDependentResource<R, P extends HasMetadata> implements
DependentResource, EventSourceProvider, Create, Update, Delete {
}
public DependentResourceManager {
@Override
public UpdateControl<P> reconcile(P resource, Context context) {
initContextIfNeeded(resource, context);
dependents.forEach(dependent -> {
if (dependent instanceof Create/Update) {
var maybeActual = getResource(primary);
var desired = desired(primary, context);
if (maybeActual.isEmpty()) {
if (dependent instanceof Create) {
creator.create(desired, primary, context);
}
} else if (dependent instanceof Update ) {
final var actual = maybeActual.get();
if (!matcher.match(actual, desired, context)) {
updater.update(actual, desired, primary, context);
}
}
}
});
return UpdateControl.noUpdate();
}
@Override
public DeleteControl cleanup(P resource, Context context) {
initContextIfNeeded(resource, context);
dependents.stream()
.filter(dependent -> dependent instanceof Delete)
.forEach(Delete::delete);
return Reconciler.super.cleanup(resource, context);
}In this case, there is no need for default/noop behavior in the class hierarchy, if needed, another manager can handle different use case of mixins. I can even enrich the logic and reuse the existing class. For exemple, adding else case (#957). But then, what is standalone is not the dependent itself but it's manager. |
Featuring also ReadOnly, PollingReadOnly resources. Edit feature for Kubernetes