Skip to content

Commit 0d7f026

Browse files
metacosmmetacosm
authored
feat: record desired state in Context (#3082)
Signed-off-by: Chris Laprun <metacosm@gmail.com>
1 parent cb39807 commit 0d7f026

File tree

8 files changed

+134
-41
lines changed

8 files changed

+134
-41
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,19 @@
1515
*/
1616
package io.javaoperatorsdk.operator.api.reconciler;
1717

18+
import java.util.Map;
1819
import java.util.Optional;
1920
import java.util.Set;
21+
import java.util.concurrent.ConcurrentHashMap;
2022
import java.util.concurrent.ExecutorService;
23+
import java.util.function.Function;
2124
import java.util.stream.Collectors;
2225
import java.util.stream.Stream;
2326

2427
import io.fabric8.kubernetes.api.model.HasMetadata;
2528
import io.fabric8.kubernetes.client.KubernetesClient;
2629
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
30+
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
2731
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedWorkflowAndDependentResourceContext;
2832
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedWorkflowAndDependentResourceContext;
2933
import io.javaoperatorsdk.operator.processing.Controller;
@@ -41,6 +45,7 @@ public class DefaultContext<P extends HasMetadata> implements Context<P> {
4145
defaultManagedDependentResourceContext;
4246
private final boolean primaryResourceDeleted;
4347
private final boolean primaryResourceFinalStateUnknown;
48+
private final Map<DependentResource<?, P>, Object> desiredStates = new ConcurrentHashMap<>();
4449

4550
public DefaultContext(
4651
RetryInfo retryInfo,
@@ -157,4 +162,12 @@ public DefaultContext<P> setRetryInfo(RetryInfo retryInfo) {
157162
this.retryInfo = retryInfo;
158163
return this;
159164
}
165+
166+
@SuppressWarnings("unchecked")
167+
public <R> R getOrComputeDesiredStateFor(
168+
DependentResource<R, P> dependentResource, Function<P, R> desiredStateComputer) {
169+
return (R)
170+
desiredStates.computeIfAbsent(
171+
dependentResource, ignored -> desiredStateComputer.apply(getPrimaryResource()));
172+
}
160173
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import io.fabric8.kubernetes.api.model.HasMetadata;
2525
import io.javaoperatorsdk.operator.api.reconciler.Context;
26+
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
2627
import io.javaoperatorsdk.operator.api.reconciler.Ignore;
2728
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
2829
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
@@ -85,7 +86,7 @@ protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> c
8586
if (creatable() || updatable()) {
8687
if (actualResource == null) {
8788
if (creatable) {
88-
var desired = desired(primary, context);
89+
var desired = getOrComputeDesired(context);
8990
throwIfNull(desired, primary, "Desired");
9091
logForOperation("Creating", primary, desired);
9192
var createdResource = handleCreate(desired, primary, context);
@@ -95,7 +96,8 @@ protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> c
9596
if (updatable()) {
9697
final Matcher.Result<R> match = match(actualResource, primary, context);
9798
if (!match.matched()) {
98-
final var desired = match.computedDesired().orElseGet(() -> desired(primary, context));
99+
final var desired =
100+
match.computedDesired().orElseGet(() -> getOrComputeDesired(context));
99101
throwIfNull(desired, primary, "Desired");
100102
logForOperation("Updating", primary, desired);
101103
var updatedResource = handleUpdate(actualResource, desired, primary, context);
@@ -127,7 +129,6 @@ protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> c
127129

128130
@Override
129131
public Optional<R> getSecondaryResource(P primary, Context<P> context) {
130-
131132
var secondaryResources = context.getSecondaryResources(resourceType());
132133
if (secondaryResources.isEmpty()) {
133134
return Optional.empty();
@@ -212,6 +213,27 @@ protected R desired(P primary, Context<P> context) {
212213
+ " updated");
213214
}
214215

216+
/**
217+
* Retrieves the desired state from the {@link Context} if it has already been computed or calls
218+
* {@link #desired(HasMetadata, Context)} and stores its result in the context for further use.
219+
* This ensures that {@code desired} is only called once per reconciliation to avoid unneeded
220+
* processing and supports scenarios where idempotent computation of the desired state is not
221+
* feasible.
222+
*
223+
* <p>Note that this method should normally only be called by the SDK itself and exclusively (i.e.
224+
* {@link #desired(HasMetadata, Context)} should not be called directly by the SDK) whenever the
225+
* desired state is needed to ensure it is properly cached for the current reconciliation.
226+
*
227+
* @param context the {@link Context} in scope for the current reconciliation
228+
* @return the desired state associated with this dependent resource based on the currently
229+
* in-scope primary resource as found in the context
230+
*/
231+
protected R getOrComputeDesired(Context<P> context) {
232+
assert context instanceof DefaultContext<P>;
233+
DefaultContext<P> defaultContext = (DefaultContext<P>) context;
234+
return defaultContext.getOrComputeDesiredStateFor(this, p -> desired(p, defaultContext));
235+
}
236+
215237
public void delete(P primary, Context<P> context) {
216238
dependentResourceReconciler.delete(primary, context);
217239
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ protected void handleExplicitStateCreation(P primary, R created, Context<P> cont
105105

106106
@Override
107107
public Matcher.Result<R> match(R resource, P primary, Context<P> context) {
108-
var desired = desired(primary, context);
108+
var desired = getOrComputeDesired(context);
109109
return Matcher.Result.computed(resource.equals(desired), desired);
110110
}
111111

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import io.javaoperatorsdk.operator.api.reconciler.Ignore;
2828
import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
2929
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
30-
import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
3130

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

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> m
138138
Context<P> context,
139139
boolean labelsAndAnnotationsEquality,
140140
String... ignorePaths) {
141-
final var desired = dependentResource.desired(primary, context);
141+
final var desired = dependentResource.getOrComputeDesired(context);
142142
return match(desired, actualResource, labelsAndAnnotationsEquality, context, ignorePaths);
143143
}
144144

@@ -150,7 +150,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> m
150150
boolean specEquality,
151151
boolean labelsAndAnnotationsEquality,
152152
String... ignorePaths) {
153-
final var desired = dependentResource.desired(primary, context);
153+
final var desired = dependentResource.getOrComputeDesired(context);
154154
return match(
155155
desired, actualResource, labelsAndAnnotationsEquality, specEquality, context, ignorePaths);
156156
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ public R update(R actual, R desired, P primary, Context<P> context) {
143143

144144
@Override
145145
public Result<R> match(R actualResource, P primary, Context<P> context) {
146-
final var desired = desired(primary, context);
146+
final var desired = getOrComputeDesired(context);
147147
return match(actualResource, desired, primary, context);
148148
}
149149

@@ -297,16 +297,16 @@ protected Optional<R> selectTargetSecondaryResource(
297297
* @return id of the target managed resource
298298
*/
299299
protected ResourceID targetSecondaryResourceID(P primary, Context<P> context) {
300-
return ResourceID.fromResource(desired(primary, context));
300+
return ResourceID.fromResource(getOrComputeDesired(context));
301301
}
302302

303303
protected boolean addOwnerReference() {
304304
return garbageCollected;
305305
}
306306

307307
@Override
308-
protected R desired(P primary, Context<P> context) {
309-
return super.desired(primary, context);
308+
protected R getOrComputeDesired(Context<P> context) {
309+
return super.getOrComputeDesired(context);
310310
}
311311

312312
@Override

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java‎

Lines changed: 67 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
import org.junit.jupiter.api.Test;
2222

2323
import io.fabric8.kubernetes.api.model.ConfigMap;
24+
import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
2425
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
2526
import io.javaoperatorsdk.operator.api.reconciler.Context;
27+
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
2628
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
2729

2830
import static org.junit.jupiter.api.Assertions.*;
@@ -31,15 +33,21 @@
3133

3234
class AbstractDependentResourceTest {
3335

36+
private static final TestCustomResource PRIMARY = new TestCustomResource();
37+
private static final DefaultContext<TestCustomResource> CONTEXT = createContext(PRIMARY);
38+
39+
private static DefaultContext<TestCustomResource> createContext(TestCustomResource primary) {
40+
return new DefaultContext<>(mock(), mock(), primary, false, false);
41+
}
42+
3443
@Test
3544
void throwsExceptionIfDesiredIsNullOnCreate() {
3645
TestDependentResource testDependentResource = new TestDependentResource();
3746
testDependentResource.setSecondary(null);
3847
testDependentResource.setDesired(null);
3948

4049
assertThrows(
41-
DependentResourceException.class,
42-
() -> testDependentResource.reconcile(new TestCustomResource(), null));
50+
DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT));
4351
}
4452

4553
@Test
@@ -49,8 +57,7 @@ void throwsExceptionIfDesiredIsNullOnUpdate() {
4957
testDependentResource.setDesired(null);
5058

5159
assertThrows(
52-
DependentResourceException.class,
53-
() -> testDependentResource.reconcile(new TestCustomResource(), null));
60+
DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT));
5461
}
5562

5663
@Test
@@ -60,8 +67,7 @@ void throwsExceptionIfCreateReturnsNull() {
6067
testDependentResource.setDesired(configMap());
6168

6269
assertThrows(
63-
DependentResourceException.class,
64-
() -> testDependentResource.reconcile(new TestCustomResource(), null));
70+
DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT));
6571
}
6672

6773
@Test
@@ -71,8 +77,28 @@ void throwsExceptionIfUpdateReturnsNull() {
7177
testDependentResource.setDesired(configMap());
7278

7379
assertThrows(
74-
DependentResourceException.class,
75-
() -> testDependentResource.reconcile(new TestCustomResource(), null));
80+
DependentResourceException.class, () -> testDependentResource.reconcile(PRIMARY, CONTEXT));
81+
}
82+
83+
@Test
84+
void checkThatDesiredIsOnlyCalledOnce() {
85+
final var testDependentResource = new DesiredCallCountCheckingDR();
86+
final var primary = new TestCustomResource();
87+
final var spec = primary.getSpec();
88+
spec.setConfigMapName("foo");
89+
spec.setKey("key");
90+
spec.setValue("value");
91+
final var context = createContext(primary);
92+
testDependentResource.reconcile(primary, context);
93+
94+
spec.setValue("value2");
95+
testDependentResource.reconcile(primary, context);
96+
97+
assertEquals(1, testDependentResource.desiredCallCount);
98+
99+
context.getOrComputeDesiredStateFor(
100+
testDependentResource, p -> testDependentResource.desired(p, context));
101+
assertEquals(1, testDependentResource.desiredCallCount);
76102
}
77103

78104
private ConfigMap configMap() {
@@ -130,22 +156,12 @@ protected ConfigMap desired(TestCustomResource primary, Context<TestCustomResour
130156
return desired;
131157
}
132158

133-
public ConfigMap getSecondary() {
134-
return secondary;
135-
}
136-
137-
public TestDependentResource setSecondary(ConfigMap secondary) {
159+
public void setSecondary(ConfigMap secondary) {
138160
this.secondary = secondary;
139-
return this;
140161
}
141162

142-
public ConfigMap getDesired() {
143-
return desired;
144-
}
145-
146-
public TestDependentResource setDesired(ConfigMap desired) {
163+
public void setDesired(ConfigMap desired) {
147164
this.desired = desired;
148-
return this;
149165
}
150166

151167
@Override
@@ -172,4 +188,35 @@ public Matcher.Result<ConfigMap> match(
172188
return result;
173189
}
174190
}
191+
192+
private static class DesiredCallCountCheckingDR extends TestDependentResource {
193+
private short desiredCallCount;
194+
195+
@Override
196+
public ConfigMap update(
197+
ConfigMap actual,
198+
ConfigMap desired,
199+
TestCustomResource primary,
200+
Context<TestCustomResource> context) {
201+
return desired;
202+
}
203+
204+
@Override
205+
public ConfigMap create(
206+
ConfigMap desired, TestCustomResource primary, Context<TestCustomResource> context) {
207+
return desired;
208+
}
209+
210+
@Override
211+
protected ConfigMap desired(TestCustomResource primary, Context<TestCustomResource> context) {
212+
final var spec = primary.getSpec();
213+
desiredCallCount++;
214+
return new ConfigMapBuilder()
215+
.editOrNewMetadata()
216+
.withName(spec.getConfigMapName())
217+
.endMetadata()
218+
.addToData(spec.getKey(), spec.getValue())
219+
.build();
220+
}
221+
}
175222
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,37 +18,48 @@
1818
import java.util.Map;
1919
import java.util.Optional;
2020

21-
import org.junit.jupiter.api.BeforeAll;
2221
import org.junit.jupiter.api.Test;
2322

2423
import io.fabric8.kubernetes.api.model.*;
2524
import io.fabric8.kubernetes.api.model.apps.Deployment;
2625
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
2726
import io.fabric8.kubernetes.api.model.apps.DeploymentStatusBuilder;
27+
import io.fabric8.kubernetes.client.KubernetesClient;
2828
import io.javaoperatorsdk.operator.MockKubernetesClient;
2929
import io.javaoperatorsdk.operator.ReconcilerUtils;
3030
import io.javaoperatorsdk.operator.api.reconciler.Context;
31+
import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
3132

3233
import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match;
3334
import static org.assertj.core.api.Assertions.assertThat;
3435
import static org.mockito.Mockito.mock;
35-
import static org.mockito.Mockito.when;
3636

3737
@SuppressWarnings({"unchecked"})
3838
class GenericKubernetesResourceMatcherTest {
3939

40-
private static final Context context = mock(Context.class);
40+
private static final Context context = new TestContext();
41+
42+
private static class TestContext extends DefaultContext<HasMetadata> {
43+
private final KubernetesClient client = MockKubernetesClient.client(HasMetadata.class);
44+
45+
public TestContext() {
46+
this(null);
47+
}
48+
49+
public TestContext(HasMetadata primary) {
50+
super(mock(), mock(), primary, false, false);
51+
}
52+
53+
@Override
54+
public KubernetesClient getClient() {
55+
return client;
56+
}
57+
}
4158

4259
Deployment actual = createDeployment();
4360
Deployment desired = createDeployment();
4461
TestDependentResource dependentResource = new TestDependentResource(desired);
4562

46-
@BeforeAll
47-
static void setUp() {
48-
final var client = MockKubernetesClient.client(HasMetadata.class);
49-
when(context.getClient()).thenReturn(client);
50-
}
51-
5263
@Test
5364
void matchesTrivialCases() {
5465
assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched()).isTrue();
@@ -77,9 +88,10 @@ void matchesWithStrongSpecEquality() {
7788
@Test
7889
void doesNotMatchRemovedValues() {
7990
actual = createDeployment();
91+
final var localContext = new TestContext(createPrimary("removed"));
8092
assertThat(
8193
GenericKubernetesResourceMatcher.match(
82-
dependentResource.desired(createPrimary("removed"), null), actual, context)
94+
dependentResource.getOrComputeDesired(localContext), actual, localContext)
8395
.matched())
8496
.withFailMessage("Removing values in metadata should lead to a mismatch")
8597
.isFalse();

0 commit comments

Comments
 (0)