-
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
Changes from 2 commits
eb5e5de
17c383c
1ceaeeb
b42efb8
862a1b3
30b07a9
848c355
e9e19e6
62d4539
01745cc
6138e24
45cdabc
3b4a8a5
76c71ef
7a6c64f
32074df
6939f16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
@SuppressWarnings("rawtypes") | ||
public interface Creator<R, P extends HasMetadata> { | ||
Creator NOOP = (desired, primary, context) -> { | ||
}; | ||
|
||
void create(R desired, P primary, Context context); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
@SuppressWarnings("rawtypes") | ||
public interface Deleter<P extends HasMetadata> { | ||
Deleter NOOP = (primary, context) -> { | ||
}; | ||
|
||
void delete(P primary, Context context); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 Also it might look nicer in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
Optional<R> getResource(P primaryResource); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import java.util.Objects; | ||
|
||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
@SuppressWarnings("rawtypes") | ||
public interface Matcher<R> { | ||
Matcher DEFAULT = | ||
(actualResource, desiredResource, context) -> Objects.equals(actualResource, desiredResource); | ||
|
||
boolean match(R actualResource, R desiredResource, Context context); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
@SuppressWarnings("rawtypes") | ||
public interface Updater<R, P extends HasMetadata> { | ||
Updater NOOP = (actual, desired, primary, context) -> { | ||
}; | ||
|
||
void update(R actual, R desired, P primary, Context context); | ||
} |
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.