Skip to content

refactor: isolate bulk dependent resource handling more #1530

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
merged 6 commits into from
Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
import io.javaoperatorsdk.operator.processing.event.ResourceID;

@Ignore
Expand All @@ -20,61 +21,32 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>

private final boolean creatable = this instanceof Creator;
private final boolean updatable = this instanceof Updater;
private final boolean bulk = this instanceof BulkDependentResource;

protected Creator<R, P> creator;
protected Updater<R, P> updater;
protected BulkDependentResource<R, P> bulkDependentResource;
private ResourceDiscriminator<R, P> resourceDiscriminator;
private final DependentResourceReconciler<R, P> dependentResourceReconciler;

@SuppressWarnings({"unchecked", "rawtypes"})
@SuppressWarnings({"unchecked"})
protected AbstractDependentResource() {
creator = creatable ? (Creator<R, P>) this : null;
updater = updatable ? (Updater<R, P>) this : null;

bulkDependentResource = bulk ? (BulkDependentResource) this : null;
dependentResourceReconciler = this instanceof BulkDependentResource
? new BulkDependentResourceReconciler<>((BulkDependentResource<R, P>) this)
: new SingleDependentResourceReconciler<>(this);
}


@Override
public ReconcileResult<R> reconcile(P primary, Context<P> context) {
if (bulk) {
final var targetResources = bulkDependentResource.desiredResources(primary, context);

Map<String, R> actualResources =
bulkDependentResource.getSecondaryResources(primary, context);

deleteBulkResourcesIfRequired(targetResources.keySet(), actualResources, primary, context);
final List<ReconcileResult<R>> results = new ArrayList<>(targetResources.size());

targetResources.forEach((key, resource) -> {
results.add(reconcileIndexAware(primary, actualResources.get(key), resource, key, context));
});

return ReconcileResult.aggregatedResult(results);
} else {
var actualResource = getSecondaryResource(primary, context);
return reconcileIndexAware(primary, actualResource.orElse(null), null, null, context);
}
return dependentResourceReconciler.reconcile(primary, context);
}

@SuppressWarnings({"rawtypes"})
protected void deleteBulkResourcesIfRequired(Set targetKeys, Map<String, R> actualResources,
P primary, Context<P> context) {
actualResources.forEach((key, value) -> {
if (!targetKeys.contains(key)) {
bulkDependentResource.deleteBulkResource(primary, value, key, context);
}
});
}

protected ReconcileResult<R> reconcileIndexAware(P primary, R actualResource, R desiredResource,
String key,
Context<P> context) {
protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> context) {
if (creatable || updatable) {
if (actualResource == null) {
if (creatable) {
var desired = bulkAwareDesired(primary, desiredResource, context);
var desired = desired(primary, context);
throwIfNull(desired, primary, "Desired");
logForOperation("Creating", primary, desired);
var createdResource = handleCreate(desired, primary, context);
Expand All @@ -83,15 +55,9 @@ protected ReconcileResult<R> reconcileIndexAware(P primary, R actualResource, R
} else {
if (updatable) {
final Matcher.Result<R> match;
if (bulk) {
match =
bulkDependentResource.match(actualResource, desiredResource, primary, key, context);
} else {
match = updater.match(actualResource, primary, context);
}
match = match(actualResource, primary, context);
if (!match.matched()) {
final var desired =
match.computedDesired().orElse(bulkAwareDesired(primary, desiredResource, context));
final var desired = match.computedDesired().orElse(desired(primary, context));
throwIfNull(desired, primary, "Desired");
logForOperation("Updating", primary, desired);
var updatedResource = handleUpdate(actualResource, desired, primary, context);
Expand All @@ -110,9 +76,8 @@ protected ReconcileResult<R> reconcileIndexAware(P primary, R actualResource, R
return ReconcileResult.noOperation(actualResource);
}

private R bulkAwareDesired(P primary, R alreadyComputedDesire, Context<P> context) {
return bulk ? alreadyComputedDesire
: desired(primary, context);
public Result<R> match(R resource, P primary, Context<P> context) {
return updater.match(resource, primary, context);
}

@Override
Expand Down Expand Up @@ -179,12 +144,7 @@ protected R desired(P primary, Context<P> context) {
}

public void delete(P primary, Context<P> context) {
if (bulk) {
var actualResources = bulkDependentResource.getSecondaryResources(primary, context);
deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context);
} else {
handleDelete(primary, context);
}
dependentResourceReconciler.delete(primary, context);
}

protected void handleDelete(P primary, Context<P> context) {
Expand All @@ -200,11 +160,8 @@ protected boolean isCreatable() {
return creatable;
}

@SuppressWarnings("unused")
protected boolean isUpdatable() {
return updatable;
}

protected boolean isBulk() {
return bulk;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@ public interface BulkDependentResource<R, P extends HasMetadata>
extends Creator<R, P>, Deleter<P> {

/**
* @return number of resources to create
* Retrieves a map of desired secondary resources associated with the specified primary resource,
* identified by an arbitrary key.
*
* @param primary the primary resource with which we want to identify which secondary resources
* are associated
* @param context the {@link Context} associated with the current reconciliation
* @return a Map associating bulk secondary resources with the specified primary via arbitrary
* identifiers
*/
Map<String, R> desiredResources(P primary, Context<P> context);

Expand All @@ -39,14 +46,15 @@ public interface BulkDependentResource<R, P extends HasMetadata>
* {@link Context}.
*
* @param actualResource the resource we want to determine whether it's matching the desired state
* @param desired the resource's desired state
* @param primary the primary resource from which the desired state is inferred
* @param key key of the resource
* @param context the context in which the resource is being matched
* @return a {@link Result} encapsulating whether the resource matched its desired state and this
* associated state if it was computed as part of the matching process. Use the static
* convenience methods ({@link Result#nonComputed(boolean)} and
* {@link Result#computed(boolean, Object)})
*/
Result<R> match(R actualResource, R desired, P primary, String key, Context<P> context);

default Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
return Matcher.Result.computed(desired.equals(actualResource), desired);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
package io.javaoperatorsdk.operator.processing.dependent;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
import io.javaoperatorsdk.operator.processing.event.ResourceID;

class BulkDependentResourceReconciler<R, P extends HasMetadata>
implements DependentResourceReconciler<R, P> {

private final BulkDependentResource<R, P> bulkDependentResource;

BulkDependentResourceReconciler(BulkDependentResource<R, P> bulkDependentResource) {
this.bulkDependentResource = bulkDependentResource;
}

@Override
public ReconcileResult<R> reconcile(P primary, Context<P> context) {
final var desiredResources = bulkDependentResource.desiredResources(primary, context);
Map<String, R> actualResources =
bulkDependentResource.getSecondaryResources(primary, context);

deleteBulkResourcesIfRequired(desiredResources.keySet(), actualResources, primary, context);

final List<ReconcileResult<R>> results = new ArrayList<>(desiredResources.size());
final var updatable = bulkDependentResource instanceof Updater;
desiredResources.forEach((key, value) -> {
final var instance =
updatable ? new UpdatableBulkDependentResourceInstance<>(bulkDependentResource, value)
: new BulkDependentResourceInstance<>(bulkDependentResource, value);
results.add(instance.reconcile(primary, actualResources.get(key), context));
});

return ReconcileResult.aggregatedResult(results);
}

@Override
public void delete(P primary, Context<P> context) {
var actualResources = bulkDependentResource.getSecondaryResources(primary, context);
deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context);
}

protected void deleteBulkResourcesIfRequired(Set<String> expectedKeys,
Map<String, R> actualResources, P primary, Context<P> context) {
actualResources.forEach((key, value) -> {
if (!expectedKeys.contains(key)) {
bulkDependentResource.deleteBulkResource(primary, value, key, context);
}
});
}

private static class BulkDependentResourceInstance<R, P extends HasMetadata>
extends AbstractDependentResource<R, P>
implements Creator<R, P>, Deleter<P> {
private final BulkDependentResource<R, P> bulkDependentResource;
private final R desired;

private BulkDependentResourceInstance(BulkDependentResource<R, P> bulkDependentResource,
R desired) {
this.bulkDependentResource = bulkDependentResource;
this.desired = desired;
}

@SuppressWarnings("unchecked")
private AbstractDependentResource<R, P> asAbstractDependentResource() {
return (AbstractDependentResource<R, P>) bulkDependentResource;
}

@Override
protected R desired(P primary, Context<P> context) {
return desired;
}

@SuppressWarnings("unchecked")
public R update(R actual, R desired, P primary, Context<P> context) {
return ((Updater<R, P>) bulkDependentResource).update(actual, desired, primary, context);
}

@Override
public Result<R> match(R resource, P primary, Context<P> context) {
return bulkDependentResource.match(resource, desired, primary, context);
}

@Override
protected void onCreated(ResourceID primaryResourceId, R created) {
asAbstractDependentResource().onCreated(primaryResourceId, created);
}

@Override
protected void onUpdated(ResourceID primaryResourceId, R updated, R actual) {
asAbstractDependentResource().onUpdated(primaryResourceId, updated, actual);
}

@Override
public Class<R> resourceType() {
return asAbstractDependentResource().resourceType();
}

@Override
public R create(R desired, P primary, Context<P> context) {
return bulkDependentResource.create(desired, primary, context);
}
}

private static class UpdatableBulkDependentResourceInstance<R, P extends HasMetadata>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why this is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed so that we can have the exact same interface for a "single" or bulk dependent in AbstractDependentResource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specifically, this is needed to be able to do this: https://github.com/java-operator-sdk/java-operator-sdk/pull/1530/files#diff-0d27f73ffef40826943dbbef3e9a2a1e74e2a10341627042ffa5a792d03c3638R39. In order to do it this way, the objects we iterate over need to be AbstractDependentResources.

extends BulkDependentResourceInstance<R, P> implements Updater<R, P> {

private UpdatableBulkDependentResourceInstance(
BulkDependentResource<R, P> bulkDependentResource,
R desired) {
super(bulkDependentResource, desired);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package io.javaoperatorsdk.operator.processing.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;

interface DependentResourceReconciler<R, P extends HasMetadata> {

ReconcileResult<R> reconcile(P primary, Context<P> context);

void delete(P primary, Context<P> context);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package io.javaoperatorsdk.operator.processing.dependent;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;

class SingleDependentResourceReconciler<R, P extends HasMetadata>
implements DependentResourceReconciler<R, P> {

private final AbstractDependentResource<R, P> instance;

SingleDependentResourceReconciler(AbstractDependentResource<R, P> dependentResource) {
this.instance = dependentResource;
}

@Override
public ReconcileResult<R> reconcile(P primary, Context<P> context) {
final var maybeActual = instance.getSecondaryResource(primary, context);
return instance.reconcile(primary, maybeActual.orElse(null), context);
}

@Override
public void delete(P primary, Context<P> context) {
instance.handleDelete(primary, context);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
return matcher.match(actualResource, primary, context);
}

public Result<R> match(R actualResource, R desired, P primary, String key, Context<P> context) {
@SuppressWarnings("unused")
public Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
return GenericKubernetesResourceMatcher.match(desired, actualResource, false);
}

Expand All @@ -148,6 +149,7 @@ protected void handleDelete(P primary, Context<P> context) {
resource.ifPresent(r -> client.resource(r).delete());
}

@SuppressWarnings("unused")
public void deleteBulkResource(P primary, R resource, String key, Context<P> context) {
client.resource(resource).delete();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.processing.dependent.BulkDependentResource;
import io.javaoperatorsdk.operator.processing.dependent.BulkUpdater;
import io.javaoperatorsdk.operator.processing.dependent.Matcher;
import io.javaoperatorsdk.operator.processing.dependent.external.PollingDependentResource;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
import io.javaoperatorsdk.operator.sample.bulkdependent.BulkDependentTestCustomResource;
Expand Down Expand Up @@ -92,12 +91,4 @@ public void deleteBulkResource(BulkDependentTestCustomResource primary, External
Context<BulkDependentTestCustomResource> context) {
externalServiceMock.delete(resource.getId());
}

@Override
public Matcher.Result<ExternalResource> match(ExternalResource actualResource,
ExternalResource desired,
BulkDependentTestCustomResource primary, String index,
Context<BulkDependentTestCustomResource> context) {
return Matcher.Result.computed(desired.equals(actualResource), desired);
}
}