-
Notifications
You must be signed in to change notification settings - Fork 220
feat: dependents use traits to specify which features they support #963
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
Conversation
We do have sort of the same idea (#958 (comment)) And I see that you were force to slip some logic in the manager. But if you do for delete why not for the reconcile ? The dependent is not really standalone anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general like this implementation. Made some comments that might be worth to revisit and discuss but in general it is good IMHO.
Just one thing I don't really get, does this solve the Read Only case for KubernetesDependetResource? since it is already implements all the trait interfaces. Eventually those will be feature flags? Or how that is meant to be solved?
Because probably something like that would hold if we would provide some generic external resource implementations.
Isn't that at the end breaks down to decision if we have nice trait methods to specify how to create and update (or delete), but then we are not able to do default implementations, or we need feature flags. In a very similar way we have in #958 for editable.
@@ -8,7 +8,5 @@ | |||
public interface DependentResource<R, P extends HasMetadata> { | |||
void reconcile(P primary, Context context); | |||
|
|||
void delete(P primary, Context context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not remove this, understand that would not nice with traits, but this is our higher level abstraction should not be changed because of the undelying implementation. We can still have delete trait, that is called only if the AbstractDependentResource
implements Deleter within this function.
Also it might look nicer in AbstractDependentResource
that there is explicit mention of Deleter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reconcile and delete are two sides of the same coin. Basically, delete is just reconcile with primary being null. So maybe
void reconcile(Optional<P> primary, Context context)
can embrace both. And then, in the default implementation, call delete if primary is not present and it implements Deleter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not work, basically the delete is only needed if there is non kubernetes objects managed or no owner reference set. But then we don't know what to delete if there is no primary as input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not work, basically the delete is only needed if there is non kubernetes objects managed or no owner reference set. But then we don't know what to delete if there is no primary as input.
That's true. Still, delete and reconcile are tight together in some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the delete is called if there is finalizer in place, and the custom resource is marked for deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of workflows there might be an actual relationship (eventually planned to manage workflows automatically), the delete is a reverse workflow maybe just calling delete instead of reconcile? I mean there is such a relationship for sure, but cannot think of other.
Yes, delete should be understood as remove desired on actual. For creatable, it means to delete the actual. For editonly, it means to undo the patch on actual
Regarding finalizers, in the future, maybe the default can be guess automatically from dependent and set if one dependent is actually implementing traits Deleter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is undo patch? Not sure that is possible in general. How it would know the previous state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some operators just need to patch a dependent resource. If you don't handle spec change, the dependent knows the content of the patch from the spec. Otherwise, yes, applied spec should be persist somewhere to be able to undo it.
Currently, in my operator, I have something like below and I'm pretty sure it can fit in the dependent model
@Override
public UpdateControl<Primary> reconcile(Primary primary,
Context context) {
var secondaryResource = context.getSecondaryResource(Dependent.class);
return secondaryResource.map((Dependent dependent) -> {
// ensure the secondary resource has still the desired patch applied
// and update the status
return ensureIsPatched(dependent, primary);
}).orElseGet(() -> {
// give a chance to update the primary resource status
// for eg. adding a False Ready Condition with reason "missing secondary"
return missingSecondary(primary);
});
}
@Override
public DeleteControl cleanup(Primary primary, Context context) {
var target = context.getSecondaryResource(Dependent.class);
target.ifPresent((Dependent dependent) -> {
ensureIsNotPatched(dependent, primary);
});
return DeleteControl.defaultDelete();
}
ensureIsPatched actually do something like
k8sclient.resources(actual).edit( (a) -> applyPatch(a));
primary.getStatus().setObservedSpec(primary.getSpec());
return UpdateControl.updateStatus(primary);
and ensureIsNotPatched
Optional.ofNullable(primary.getStatus().getObservedSpec()).map(spec -> {
this.client.resources(actual).edit(o -> undoPatch(o, spec));
});
The main issue is that while it's a dependent, the reconcile logic is a bit different and having this logic implemented in the dependent base clase itself do not allow much of reuse. I need to take time to look at it with traits aspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting use case! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sclorng I see, but this is quite an exotic case, I mean probably not makes sense to generalize the framework to support this case at this point. But certainly would be doable just overriding the appropriate methods.
...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java
Outdated
Show resolved
Hide resolved
Do I understand correctly that in a read only resource the initializer would be overriden with NOOP operations? ReadOnlyDeployment() {
init(Creator.NOOP, Updater.NOOP, Deleter.NOOP);
} One more thing that I might consider, is replacing trait methods with feature flags (so boolean values for editor, updator, deleter), and for NOOP that would mean the method is empty - so in In more detail: public abstract class AbstractDependentResource<R, P extends HasMetadata>
implements DependentResource<R, P> {
protected boolean creator;
protected boolean updater;
protected boolean deleter;
public AbstractDependentResource(boolean creator, boolean updater, boolean deleter) {
this.creator = creator;
this.updater = updater;
this.updater = updater;
}
@Override
public void reconcile(P primary, Context context) {
if (creator || updater) {
var maybeActual = getResource(primary);
var desired = desired(primary, context);
if (maybeActual.isEmpty()) {
if (creator) {
create(desired, primary, context);
}
} else {
final var actual = maybeActual.get();
if (updater && match(actual, desired, context)) {
update(actual, desired, primary, context);
}
}
}
}
boolean match(R actualResource, R desiredResource, Context context) {
Objects.equals(actualResource, desiredResource);
}
// these should be probably abstract, just in case of KubernetedDependentResource
public void update(R actual, R target, P primary, Context context) {
}
public void create(R target, P primary, Context context) {
}
public void delete(P primary, Context context) {
} The positive side would be that it's really just flags, for me it feels that there is quite a lot ceremony to allow these methods for CUP (create,update,delete), while these are anyways implemented in KubernetesDependentResource. Just in little compex way with private base classes in the background. And in the read only version, or just edit or just create version (not sure we want something like just create actually, but lets say some might need it), these flags needs to be configured anyways just in constructor just in form of methods - what is a little strange. public class OnlyEditKubernetesClass extends KubernetesDependentResource{
public JustEditClass() {
init(NOOP, new UpdateDependentOperation(), NOOP)
}
// ...
} What do you think? |
The idea is that The |
The problem with flags is that you could have an implementation that sets the flags without actually providing the implementation which makes things incoherent. The SDK should try to enforce the semantics: if a user says that their dependent resource needs to be created, then it should enforce that there is indeed logic to create the dependent, though, granted, that logic could be empty. The act of requiring to implement an interface makes things more explicit than a boolean in a constructor (which could also be passed wrong, i.e. in the wrong order, it's always tricky to have several parameters of the same type on methods). Another aspect of flags is that these should probably be made available to the annotation layer making it more complex as well. The first dependent resource implementations actually had these flags but I now think that trait interfaces are a more elegant solution because it extracts the information from the code (i.e. what you actually implement) instead of relying on an explicit configuration step, which creates the problem of having to deal with the configuration (and you've been removing the configuration layers that the non-standalone version had) and with the fact that there could be a mismatch between what the configuration says and what is actually implemented. Using traits solves the problem elegantly I think by making the code the configuration 😄. |
I understand the goal here, and the structure. I really like it, just wanted to discuss if it could be simplified by the flags. Maybe we could add a sample Read Only and Update Only implementation, how it would look like, just we can see the code together, but all I want say that at the end this is how an edit only kubernetes would look like at the end: public class OnlyUpdateKubernetesResource extends KubernetesDependentResource<Deployment, Primary> {
public JustEditClass() {
init(NOOP, new UpdateDependentOperation(), NOOP)
}
@Override
public Deployment desired(Primary primary, Context context) {
//...
}
} Where this part:
That is true that there can be a mismatch for flags and overriden methods. We could provide adapters, which overrides the only the wanna be empty methods and sets the flags. In that regard the current implementation is really good. I think we should create issues to provide abstract dependent resource for external ones, since there will be the same issue as with kubernetes ones for the getResource(). Will do that. But I'm otherwise fine to merge this, it's really good. |
Also we should add unit tests :) |
public UpdateDependentOperation() { | ||
super("Updating"); | ||
matcher = GenericKubernetesResourceMatcher.matcherFor(resourceType()); | ||
processor = KubernetesDependentResource.this instanceof ResourceUpdatePreProcessor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a setter for this still on KDR but that can be an another PR when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There actually was one but I removed it with the idea that now everything is defined in a similar fashion using traits to override the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that for standalone DRs, it wouldn't be necessary to make subclasses, but it is how it is. So we will se how it evolves, maybe if someone for standalone ones would not want to do subclass we can provide an "adaptor".
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; | ||
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; | ||
|
||
@KubernetesDependent(labelSelector = "app.kubernetes.io/managed-by=tomcat-operator") | ||
public class DeploymentDependentResource extends KubernetesDependentResource<Deployment, Tomcat> { | ||
public class DeploymentDependentResource extends KubernetesDependentResource<Deployment, Tomcat> | ||
implements Creator<Deployment, Tomcat>, Updater<Deployment, Tomcat> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot this be done by the init?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the whole point is to declare what your dependent is doing explicitly. init
is only there to set the defaults for subclasses of AbstractDependentResource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, won't in most cases actually a full CRUD resource? Should we have an adaptor for that?
Kudos, SonarCloud Quality Gate passed! |
This is meant to be a cleaner alternative to both #956 and #958.