-
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
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
eb5e5de
feat: dependents use traits to specify which features they support
metacosm 17c383c
fix: remove unneeded parameter on AbstractDependentResource
metacosm 1ceaeeb
fix: actually support read-only KubernetesDependentResource
metacosm b42efb8
fix: re-add delete method on DependentResource
metacosm 862a1b3
feat: put match method on Updater as well
metacosm 30b07a9
feat: unify Deleter management
metacosm 848c355
feat: configure resource pre-processing via trait, similar to Matcher
metacosm e9e19e6
feat: make no-code read-only possible
metacosm 62d4539
fix: isCreatable/isUpdatable wrong implementations, added isDeletable
metacosm 01745cc
feat: add logging
metacosm 6138e24
fix: Service/Deployment dependents need to implement Creator/Updater
metacosm 45cdabc
fix?: make sure we use the overridden matcher implementation
metacosm 3b4a8a5
fix: avoid infinite loop
metacosm 76c71ef
fix: dependent must implement Creator trait if resources are created
metacosm 7a6c64f
refactor: clean-up
metacosm 32074df
fix: dependent spec creation was wrong
metacosm 6939f16
fix: add missing class
metacosm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
82 changes: 69 additions & 13 deletions
82
.../java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,87 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
public abstract class AbstractDependentResource<R, P extends HasMetadata, C> | ||
public abstract class AbstractDependentResource<R, P extends HasMetadata> | ||
implements DependentResource<R, P> { | ||
private static final Logger log = LoggerFactory.getLogger(AbstractDependentResource.class); | ||
|
||
private final boolean creatable = this instanceof Creator; | ||
private final boolean updatable = this instanceof Updater; | ||
private final boolean deletable = this instanceof Deleter; | ||
protected Creator<R, P> creator; | ||
protected Updater<R, P> updater; | ||
protected Deleter<P> deleter; | ||
|
||
@SuppressWarnings("unchecked") | ||
public AbstractDependentResource() { | ||
init(Creator.NOOP, Updater.NOOP, Deleter.NOOP); | ||
} | ||
|
||
@SuppressWarnings({"unchecked"}) | ||
protected void init(Creator<R, P> defaultCreator, Updater<R, P> defaultUpdater, | ||
Deleter<P> defaultDeleter) { | ||
creator = creatable ? (Creator<R, P>) this : defaultCreator; | ||
updater = updatable ? (Updater<R, P>) this : defaultUpdater; | ||
deleter = deletable ? (Deleter<P>) this : defaultDeleter; | ||
} | ||
|
||
@Override | ||
public void reconcile(P primary, Context context) { | ||
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); | ||
final var creatable = isCreatable(primary, context); | ||
final var updatable = isUpdatable(primary, context); | ||
if (creatable || updatable) { | ||
var maybeActual = getResource(primary); | ||
var desired = desired(primary, context); | ||
if (maybeActual.isEmpty()) { | ||
if (creatable) { | ||
log.debug("Creating dependent {} for primary {}", desired, primary); | ||
creator.create(desired, primary, context); | ||
} | ||
} else { | ||
final var actual = maybeActual.get(); | ||
if (updatable && !updater.match(actual, desired, context)) { | ||
log.debug("Updating dependent {} for primary {}", desired, primary); | ||
updater.update(actual, desired, primary, context); | ||
} else { | ||
log.debug("Update skipped for dependent {} as it matched the existing one", desired); | ||
} | ||
} | ||
} else { | ||
log.debug( | ||
"Dependent {} is read-only, implement Creator and/or Updater interfaces to modify it", | ||
getClass().getSimpleName()); | ||
} | ||
} | ||
|
||
protected abstract R desired(P primary, Context context); | ||
@Override | ||
public void delete(P primary, Context context) { | ||
if (isDeletable(primary, context)) { | ||
deleter.delete(primary, context); | ||
} | ||
} | ||
|
||
protected abstract boolean match(R actual, R target, Context context); | ||
protected R desired(P primary, Context context) { | ||
throw new IllegalStateException( | ||
"desired method must be implemented if this DependentResource can be created and/or updated"); | ||
} | ||
|
||
protected abstract R create(R target, P primary, Context context); | ||
@SuppressWarnings("unused") | ||
protected boolean isCreatable(P primary, Context context) { | ||
return creatable; | ||
} | ||
|
||
// the actual needed to copy/preserve new labels or annotations | ||
protected abstract R update(R actual, R target, P primary, Context context); | ||
@SuppressWarnings("unused") | ||
protected boolean isUpdatable(P primary, Context context) { | ||
return updatable; | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
protected boolean isDeletable(P primary, Context context) { | ||
return deletable; | ||
} | ||
} |
12 changes: 12 additions & 0 deletions
12
...work-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Creator.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} |
12 changes: 12 additions & 0 deletions
12
...work-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Deleter.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
...work-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Matcher.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
public interface Matcher<R> { | ||
boolean match(R actualResource, R desiredResource, Context context); | ||
} |
9 changes: 9 additions & 0 deletions
9
...java/io/javaoperatorsdk/operator/api/reconciler/dependent/ResourceUpdatePreProcessor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
|
||
public interface ResourceUpdatePreProcessor<R extends HasMetadata> { | ||
|
||
R replaceSpecOnActual(R actual, R desired, Context context); | ||
} |
18 changes: 18 additions & 0 deletions
18
...work-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Updater.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import java.util.Objects; | ||
|
||
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); | ||
|
||
default boolean match(R actualResource, R desiredResource, Context context) { | ||
return Objects.equals(actualResource, desiredResource); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53 changes: 53 additions & 0 deletions
53
...eratorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessor.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package io.javaoperatorsdk.operator.processing.dependent.kubernetes; | ||
|
||
import io.fabric8.kubernetes.api.model.ConfigMap; | ||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.fabric8.kubernetes.api.model.Secret; | ||
import io.javaoperatorsdk.operator.ReconcilerUtils; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
import io.javaoperatorsdk.operator.api.reconciler.dependent.ResourceUpdatePreProcessor; | ||
|
||
public abstract class GenericResourceUpdatePreProcessor<R extends HasMetadata> implements | ||
ResourceUpdatePreProcessor<R> { | ||
|
||
private GenericResourceUpdatePreProcessor() {} | ||
|
||
@SuppressWarnings("unchecked") | ||
public static <R extends HasMetadata> ResourceUpdatePreProcessor<R> processorFor( | ||
Class<R> resourceType) { | ||
if (Secret.class.isAssignableFrom(resourceType)) { | ||
return (ResourceUpdatePreProcessor<R>) new GenericResourceUpdatePreProcessor<Secret>() { | ||
@Override | ||
protected void updateClonedActual(Secret actual, Secret desired) { | ||
actual.setData(desired.getData()); | ||
actual.setStringData(desired.getStringData()); | ||
} | ||
}; | ||
} else if (ConfigMap.class.isAssignableFrom(resourceType)) { | ||
return (ResourceUpdatePreProcessor<R>) new GenericResourceUpdatePreProcessor<ConfigMap>() { | ||
|
||
@Override | ||
protected void updateClonedActual(ConfigMap actual, ConfigMap desired) { | ||
actual.setData(desired.getData()); | ||
actual.setBinaryData((desired.getBinaryData())); | ||
} | ||
}; | ||
} else { | ||
return new GenericResourceUpdatePreProcessor<>() { | ||
@Override | ||
protected void updateClonedActual(R actual, R desired) { | ||
var desiredSpec = ReconcilerUtils.getSpec(desired); | ||
ReconcilerUtils.setSpec(actual, desiredSpec); | ||
} | ||
}; | ||
} | ||
} | ||
|
||
public R replaceSpecOnActual(R actual, R desired, Context context) { | ||
var clonedActual = context.getConfigurationService().getResourceCloner().clone(actual); | ||
updateClonedActual(clonedActual, desired); | ||
return clonedActual; | ||
} | ||
|
||
protected abstract void updateClonedActual(R actual, R desired); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ofDeleter
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 implementsDeleter
.Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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
ensureIsPatched actually do something like
and ensureIsNotPatched
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.