Skip to content

[WIP] improve: various simplifications #3542

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.

Sign up for GitHub

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

Closed
csviri wants to merge 1 commit into from
Closed

[WIP] improve: various simplifications #3542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Informable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
*/
package io.javaoperatorsdk.operator.api.config;

import java.util.Optional;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;

public interface Informable<R extends HasMetadata> {
Expand All @@ -29,4 +32,12 @@ default String getResourceTypeName() {
default Class<R> getResourceClass() {
return getInformerConfig().getResourceClass();
}

/**
* Optional, specific kubernetes client, typically to connect to a different cluster than the rest
* of the operator. Note that this is solely for multi cluster support.
*/
default Optional<KubernetesClient> getKubernetesClient() {
return Optional.empty();
}
}
8 changes: 0 additions & 8 deletions ...ava/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,6 @@ default String name() {
return getInformerConfig().getName();
}

/**
* Optional, specific kubernetes client, typically to connect to a different cluster than the rest
* of the operator. Note that this is solely for multi cluster support.
*/
default Optional<KubernetesClient> getKubernetesClient() {
return Optional.empty();
}

class DefaultInformerEventSourceConfiguration<R extends HasMetadata>
implements InformerEventSourceConfiguration<R> {
private final PrimaryToSecondaryMapper<?> primaryToSecondaryMapper;
Expand Down
44 changes: 4 additions & 40 deletions .../src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@
import org.slf4j.LoggerFactory;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.dsl.base.PatchContext;
import io.fabric8.kubernetes.client.dsl.base.PatchType;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.ReconcilerUtilsInternal;
import io.javaoperatorsdk.operator.processing.event.ResourceID;

import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID;
Expand Down Expand Up @@ -430,10 +430,7 @@ public static <P extends HasMetadata> P addFinalizerWithSSA(
}
try {
P resource = (P) originalResource.getClass().getConstructor().newInstance();
ObjectMeta objectMeta = new ObjectMeta();
objectMeta.setName(originalResource.getMetadata().getName());
objectMeta.setNamespace(originalResource.getMetadata().getNamespace());
resource.setMetadata(objectMeta);
resource.initNameAndNamespaceFrom(originalResource);
resource.addFinalizer(finalizerName);
return client
.resource(resource)
Expand All @@ -456,43 +453,10 @@ public static <P extends HasMetadata> P addFinalizerWithSSA(
}

public static int compareResourceVersions(HasMetadata h1, HasMetadata h2) {
return compareResourceVersions(
h1.getMetadata().getResourceVersion(), h2.getMetadata().getResourceVersion());
return ReconcilerUtilsInternal.validateAndCompareResourceVersions(h1, h2);
}

public static int compareResourceVersions(String v1, String v2) {
int v1Length = validateResourceVersion(v1);
int v2Length = validateResourceVersion(v2);
int comparison = v1Length - v2Length;
if (comparison != 0) {
return comparison;
}
for (int i = 0; i < v2Length; i++) {
int comp = v1.charAt(i) - v2.charAt(i);
if (comp != 0) {
return comp;
}
}
return 0;
}

private static int validateResourceVersion(String v1) {
int v1Length = v1.length();
if (v1Length == 0) {
throw new NonComparableResourceVersionException("Resource version is empty");
}
for (int i = 0; i < v1Length; i++) {
char char1 = v1.charAt(i);
if (char1 == '0') {
if (i == 0) {
throw new NonComparableResourceVersionException(
"Resource version cannot begin with 0: " + v1);
}
} else if (char1 < '0' || char1 > '9') {
throw new NonComparableResourceVersionException(
"Non numeric characters in resource version: " + v1);
}
}
return v1Length;
return ReconcilerUtilsInternal.validateAndCompareResourceVersions(v1, v2);
}
}
14 changes: 7 additions & 7 deletions ...ork-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ public <R extends HasMetadata> R jsonPatch(R actualResource, UnaryOperator<R> un
*/
public <R extends HasMetadata> R jsonPatch(
R actualResource, UnaryOperator<R> unaryOperator, Options options) {
R desired = desiredForJsonPatch(actualResource, unaryOperator, options);
R desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand All @@ -580,7 +580,7 @@ public <R extends HasMetadata> R jsonPatch(
UnaryOperator<R> unaryOperator,
InformerEventSource<R, P> informerEventSource,
Options options) {
R desired = desiredForJsonPatch(actualResource, unaryOperator, options);
R desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand Down Expand Up @@ -620,7 +620,7 @@ public <R extends HasMetadata> R jsonPatchStatus(
*/
public <R extends HasMetadata> R jsonPatchStatus(
R actualResource, UnaryOperator<R> unaryOperator, Options options) {
R desired = desiredForJsonPatch(actualResource, unaryOperator, options);
R desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand All @@ -645,7 +645,7 @@ public <R extends HasMetadata> R jsonPatchStatus(
UnaryOperator<R> unaryOperator,
InformerEventSource<R, P> informerEventSource,
Options options) {
R desired = desiredForJsonPatch(actualResource, unaryOperator, options);
R desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand Down Expand Up @@ -680,7 +680,7 @@ public P jsonPatchPrimary(P actualResource, UnaryOperator<P> unaryOperator) {
* @return the patched resource as returned by the API server
*/
public P jsonPatchPrimary(P actualResource, UnaryOperator<P> unaryOperator, Options options) {
P desired = desiredForJsonPatch(actualResource, unaryOperator, options);
P desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand Down Expand Up @@ -717,7 +717,7 @@ public P jsonPatchPrimaryStatus(P actualResource, UnaryOperator<P> unaryOperator
*/
public P jsonPatchPrimaryStatus(
P actualResource, UnaryOperator<P> unaryOperator, Options options) {
P desired = desiredForJsonPatch(actualResource, unaryOperator, options);
P desired = desiredForJsonPatch(actualResource, unaryOperator);
return resourcePatch(
desired,
actualResource,
Expand Down Expand Up @@ -1441,7 +1441,7 @@ public enum Mode {
}

private <T extends HasMetadata> T desiredForJsonPatch(
T actualResource, UnaryOperator<T> unaryOperator, Options options) {
T actualResource, UnaryOperator<T> unaryOperator) {
var cloned =
context
.getControllerConfiguration()
Expand Down
21 changes: 16 additions & 5 deletions ...peratorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends H
public static final String METADATA_LABELS = "/metadata/labels";
public static final String METADATA_ANNOTATIONS = "/metadata/annotations";

private static final List<String> SPEC_PREFIX = List.of(SPEC);
private static final List<String> STATUS_PREFIX = List.of(STATUS);
private static final List<String> METADATA_PREFIX = List.of(METADATA);
private static final List<String> LABELS_AND_ANNOTATIONS_PREFIX =
List.of(METADATA_LABELS, METADATA_ANNOTATIONS);

private static final String PATH = "path";
private static final String[] EMPTY_ARRAY = {};

Expand Down Expand Up @@ -182,11 +188,11 @@ public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> m
boolean matched = true;
for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) {
var node = wholeDiffJsonPatch.get(i);
if (nodeIsChildOf(node, List.of(SPEC))) {
if (nodeIsChildOf(node, SPEC_PREFIX)) {
matched = match(valuesEquality, node, ignoreList);
} else if (nodeIsChildOf(node, List.of(METADATA))) {
} else if (nodeIsChildOf(node, METADATA_PREFIX)) {
// conditionally consider labels and annotations
if (nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) {
if (nodeIsChildOf(node, LABELS_AND_ANNOTATIONS_PREFIX)) {
matched = match(labelsAndAnnotationsEquality, node, Collections.emptyList());
}
} else if (!nodeIsChildOf(node, IGNORED_FIELDS)) {
Expand Down Expand Up @@ -241,7 +247,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> m
boolean matched = true;
for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) {
var node = wholeDiffJsonPatch.get(i);
if (nodeIsChildOf(node, List.of(STATUS))) {
if (nodeIsChildOf(node, STATUS_PREFIX)) {
matched = match(valuesEquality, node, Collections.emptyList());
}
}
Expand All @@ -261,7 +267,12 @@ private static boolean match(boolean equality, JsonNode diff, final List<String>

static boolean nodeIsChildOf(JsonNode n, List<String> prefixes) {
var path = getPath(n);
return prefixes.stream().anyMatch(path::startsWith);
for (int i = 0; i < prefixes.size(); i++) {
if (path.startsWith(prefixes.get(i))) {
return true;
}
}
return false;
}

static String getPath(JsonNode n) {
Expand Down
44 changes: 25 additions & 19 deletions ...dk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.apps.ReplicaSet;
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
import io.fabric8.kubernetes.api.model.apps.StatefulSetSpec;
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.api.reconciler.Context;
Expand Down Expand Up @@ -199,25 +200,7 @@ protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap)
&& desired instanceof StatefulSet desiredStatefulSet) {
var actualSpec = actualStatefulSet.getSpec();
var desiredSpec = desiredStatefulSet.getSpec();
int claims = desiredSpec.getVolumeClaimTemplates().size();
if (claims == actualSpec.getVolumeClaimTemplates().size()) {
for (int i = 0; i < claims; i++) {
var claim = desiredSpec.getVolumeClaimTemplates().get(i);
if (claim.getSpec().getVolumeMode() == null) {
Optional.ofNullable(
GenericKubernetesResource.get(
actualMap, "spec", "volumeClaimTemplates", i, "spec"))
.map(Map.class::cast)
.ifPresent(m -> m.remove("volumeMode"));
}
if (claim.getStatus() == null) {
Optional.ofNullable(
GenericKubernetesResource.get(actualMap, "spec", "volumeClaimTemplates", i))
.map(Map.class::cast)
.ifPresent(m -> m.remove("status"));
}
}
}
sanitizeVolumeClaimTemplates(actualMap, actualSpec, desiredSpec);
sanitizePodTemplateSpec(actualMap, actualSpec.getTemplate(), desiredSpec.getTemplate());
} else if (actual instanceof Deployment actualDeployment
&& desired instanceof Deployment desiredDeployment) {
Expand All @@ -240,6 +223,29 @@ protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap)
}
}

private static void sanitizeVolumeClaimTemplates(
Map<String, Object> actualMap, StatefulSetSpec actualSpec, StatefulSetSpec desiredSpec) {
int claims = desiredSpec.getVolumeClaimTemplates().size();
if (claims != actualSpec.getVolumeClaimTemplates().size()) {
return;
}
for (int i = 0; i < claims; i++) {
var claim = desiredSpec.getVolumeClaimTemplates().get(i);
if (claim.getSpec().getVolumeMode() == null) {
Optional.ofNullable(
GenericKubernetesResource.get(actualMap, "spec", "volumeClaimTemplates", i, "spec"))
.map(Map.class::cast)
.ifPresent(m -> m.remove("volumeMode"));
}
if (claim.getStatus() == null) {
Optional.ofNullable(
GenericKubernetesResource.get(actualMap, "spec", "volumeClaimTemplates", i))
.map(Map.class::cast)
.ifPresent(m -> m.remove("status"));
}
}
}

@SuppressWarnings("unchecked")
static void keepOnlyManagedFields(
Map<String, Object> result,
Expand Down
2 changes: 1 addition & 1 deletion ...a/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ protected AbstractWorkflowExecutor(DefaultWorkflow<P> workflow, P primary, Conte
this.context = context;
this.primaryID = ResourceID.fromResource(primary);
executorService = context.getWorkflowExecutorService();
results = new HashMap<>(workflow.getDependentResourcesByName().size());
results = new HashMap<>(workflow.size());
}

protected abstract Logger logger();
Expand Down
12 changes: 7 additions & 5 deletions ...ework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public class EventProcessor<P extends HasMetadata> implements EventHandler, Life
private final Cache<P> cache;
private final EventSourceManager<P> eventSourceManager;
private final RateLimiter<? extends RateLimitState> rateLimiter;
private final ResourceStateManager resourceStateManager = new ResourceStateManager();
private final ResourceStateManager resourceStateManager;
private final Map<String, Object> metricsMetadata;
private ExecutorService executor;

Expand Down Expand Up @@ -107,6 +107,8 @@ private EventProcessor(
this.metrics = metrics != null ? metrics : Metrics.NOOP;
this.eventSourceManager = eventSourceManager;
this.rateLimiter = controllerConfiguration.getRateLimiter();
this.resourceStateManager =
new ResourceStateManager(controllerConfiguration.triggerReconcilerOnAllEvents());

metricsMetadata =
Optional.ofNullable(eventSourceManager.getController())
Expand Down Expand Up @@ -194,7 +196,7 @@ private void submitReconciliationExecution(ResourceState state) {
state.getRetry(),
state.deleteEventPresent(),
state.isDeleteFinalStateUnknown());
state.unMarkEventReceived(triggerOnAllEvents());
state.unMarkEventReceived();
metrics.reconciliationSubmitted(latest, state.getRetry(), metricsMetadata);
log.debug("Executing events for custom resource. Scope: {}", executionScope);
executor.execute(new ReconcilerExecutor(resourceID, executionScope));
Expand Down Expand Up @@ -249,10 +251,10 @@ private void handleEventMarking(Event event, ResourceState state) {
// removed, but also the informers websocket is disconnected and later reconnected. So
// meanwhile the resource could be deleted and recreated. In this case we just mark a new
// event as below.
state.markEventReceived(triggerOnAllEvents());
state.markEventReceived();
}
} else if (!state.deleteEventPresent() && !state.processedMarkForDeletionPresent()) {
state.markEventReceived(triggerOnAllEvents());
state.markEventReceived();
} else if (isTriggerOnAllEventAndDeleteEventPresent(state)) {
state.markAdditionalEventAfterDeleteEvent();
} else if (log.isDebugEnabled()) {
Expand Down Expand Up @@ -381,7 +383,7 @@ private void handleRetryOnException(
boolean eventPresent =
state.eventPresent()
|| (triggerOnAllEvents() && state.isAdditionalEventPresentAfterDeleteEvent());
state.markEventReceived(triggerOnAllEvents());
state.markEventReceived();
retryAwareErrorLogging(
state.getRetry(), eventPresent, errorHandledByReconciler, exception, executionScope);
metrics.reconciliationFailed(
Expand Down
Loading
Loading