Skip to content

Commit 21978a4

Browse files
csviricsviri
committed
delete related improvements and unit tests
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
1 parent a8ea0a1 commit 21978a4

4 files changed

Lines changed: 266 additions & 5 deletions

File tree

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventFilterDetails.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,11 @@ public Optional<GenericResourceEvent> prepareSummaryEventIfNotOwnEventsPresent()
7070
if (allOwnResourceVersions.containsAll(relatedEventResourceVersions())) {
7171
return Optional.empty();
7272
}
73-
var deleteEvent =
74-
relatedEvents.stream().filter(e -> e.getAction() == ResourceAction.DELETED).findFirst();
75-
if (deleteEvent.isPresent()) {
76-
return deleteEvent;
73+
// we propagate delete event only if it is the last, if there are newer events
74+
// means the resource was re-created (not necessarily by our controller)
75+
var lastEvent = relatedEvents.get(relatedEvents.size() - 1);
76+
if (lastEvent.getAction() == ResourceAction.DELETED) {
77+
return Optional.of(lastEvent);
7778
}
7879
if (relatedEvents.size() == 1) {
7980
return Optional.of(relatedEvents.get(0));
@@ -97,6 +98,7 @@ private Set<String> relatedEventResourceVersions() {
9798
}
9899

99100
public boolean newerOrEqualEventReceivedForOwnLastUpdate() {
101+
// this means our update was not successful
100102
if (allOwnResourceVersions.isEmpty()) {
101103
return true;
102104
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,4 +277,9 @@ synchronized boolean isEmpty() {
277277
synchronized Map<ResourceID, T> getResources() {
278278
return Collections.unmodifiableMap(cache);
279279
}
280+
281+
// for testing purposes
282+
synchronized Map<ResourceID, EventFilterDetails> getActiveUpdates() {
283+
return Collections.unmodifiableMap(activeUpdates);
284+
}
280285
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventFilterDetailsTest.java‎

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
/*
2+
* Copyright Java Operator SDK Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package io.javaoperatorsdk.operator.processing.event.source.informer;
17+
18+
import org.junit.jupiter.api.BeforeEach;
19+
import org.junit.jupiter.api.Test;
20+
21+
import io.fabric8.kubernetes.api.model.ConfigMap;
22+
import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
23+
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
24+
import io.javaoperatorsdk.operator.processing.event.source.ResourceAction;
25+
26+
import static org.assertj.core.api.Assertions.assertThat;
27+
28+
class EventFilterDetailsTest {
29+
30+
private EventFilterDetails details;
31+
32+
@BeforeEach
33+
void setup() {
34+
details = new EventFilterDetails();
35+
}
36+
37+
@Test
38+
void activeUpdatesCounter() {
39+
assertThat(details.isNoActiveUpdate()).isTrue();
40+
assertThat(details.getActiveUpdates()).isZero();
41+
42+
details.increaseActiveUpdates();
43+
details.increaseActiveUpdates();
44+
assertThat(details.getActiveUpdates()).isEqualTo(2);
45+
assertThat(details.isNoActiveUpdate()).isFalse();
46+
47+
assertThat(details.decreaseActiveUpdates()).isFalse();
48+
assertThat(details.getActiveUpdates()).isEqualTo(1);
49+
50+
assertThat(details.decreaseActiveUpdates()).isTrue();
51+
assertThat(details.isNoActiveUpdate()).isTrue();
52+
}
53+
54+
@Test
55+
void summaryEmptyWhenAllRelatedEventsAreOwn() {
56+
details.addToOwnResourceVersions("2");
57+
details.addToOwnResourceVersions("3");
58+
details.addRelatedEvent(updatedEvent("2", null));
59+
details.addRelatedEvent(updatedEvent("3", "2"));
60+
61+
assertThat(details.prepareSummaryEventIfNotOwnEventsPresent()).isEmpty();
62+
}
63+
64+
@Test
65+
void summaryReturnsSingleNonOwnEvent() {
66+
var thirdParty = updatedEvent("4", "3");
67+
details.addToOwnResourceVersions("2");
68+
details.addRelatedEvent(thirdParty);
69+
70+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent();
71+
72+
assertThat(summary).contains(thirdParty);
73+
}
74+
75+
@Test
76+
void summaryReturnsLastEventWhenItIsDelete() {
77+
var firstUpdate = updatedEvent("3", "2");
78+
var deleteAtEnd = deleteEvent("4");
79+
details.addRelatedEvent(firstUpdate);
80+
details.addRelatedEvent(deleteAtEnd);
81+
82+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent();
83+
84+
assertThat(summary).contains(deleteAtEnd);
85+
}
86+
87+
@Test
88+
void summaryDoesNotReturnDeleteWhenItIsNotLast() {
89+
// simulates a delete-then-recreate sequence inside the filter window:
90+
// returning the DELETE would mask the fact that the resource exists again.
91+
var deleteEvent = deleteEvent("3");
92+
var recreate = addedEvent("4");
93+
details.addRelatedEvent(deleteEvent);
94+
details.addRelatedEvent(recreate);
95+
96+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent();
97+
98+
assertThat(summary).isPresent();
99+
assertThat(summary.get().getAction()).isEqualTo(ResourceAction.UPDATED);
100+
assertThat(summary.get().getResource().orElseThrow()).isEqualTo(recreate.getResource().get());
101+
}
102+
103+
@Test
104+
void summarySynthesizesUpdatedFromFirstPreviousToLastResource() {
105+
var first = updatedEvent("3", "2");
106+
var middle = updatedEvent("4", "3");
107+
var last = updatedEvent("5", "4");
108+
details.addRelatedEvent(first);
109+
details.addRelatedEvent(middle);
110+
details.addRelatedEvent(last);
111+
112+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent().orElseThrow();
113+
114+
assertThat(summary.getAction()).isEqualTo(ResourceAction.UPDATED);
115+
assertThat(summary.getResource().orElseThrow()).isEqualTo(last.getResource().get());
116+
assertThat(summary.getPreviousResource().orElseThrow())
117+
.isEqualTo(first.getPreviousResource().get());
118+
assertThat(summary.getLastStateUnknow()).isNull();
119+
}
120+
121+
@Test
122+
void summaryUsesFirstResourceAsPreviousWhenFirstEventHasNoPrevious() {
123+
// first event is ADD (no previous resource); synthesis must fall back to the resource itself.
124+
var added = addedEvent("3");
125+
var updated = updatedEvent("4", "3");
126+
details.addRelatedEvent(added);
127+
details.addRelatedEvent(updated);
128+
129+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent().orElseThrow();
130+
131+
assertThat(summary.getAction()).isEqualTo(ResourceAction.UPDATED);
132+
assertThat(summary.getResource().orElseThrow()).isEqualTo(updated.getResource().get());
133+
assertThat(summary.getPreviousResource().orElseThrow()).isEqualTo(added.getResource().get());
134+
}
135+
136+
@Test
137+
void summarySkipsOwnFilterWhenAtLeastOneEventIsForeign() {
138+
// even with own rvs in the mix, presence of a non-own event must surface a summary.
139+
details.addToOwnResourceVersions("3");
140+
var ownEvent = updatedEvent("3", "2");
141+
var foreign = updatedEvent("4", "3");
142+
details.addRelatedEvent(ownEvent);
143+
details.addRelatedEvent(foreign);
144+
145+
var summary = details.prepareSummaryEventIfNotOwnEventsPresent().orElseThrow();
146+
147+
assertThat(summary.getAction()).isEqualTo(ResourceAction.UPDATED);
148+
assertThat(summary.getResource().orElseThrow()).isEqualTo(foreign.getResource().get());
149+
assertThat(summary.getPreviousResource().orElseThrow())
150+
.isEqualTo(ownEvent.getPreviousResource().get());
151+
}
152+
153+
@Test
154+
void newerOrEqualReturnsTrueWhenNoOwnVersions() {
155+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isTrue();
156+
details.addRelatedEvent(updatedEvent("2", null));
157+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isTrue();
158+
}
159+
160+
@Test
161+
void newerOrEqualReturnsFalseWhenNoRelatedEventsYet() {
162+
details.addToOwnResourceVersions("3");
163+
164+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isFalse();
165+
}
166+
167+
@Test
168+
void newerOrEqualReturnsFalseWhenAllRelatedAreOlderThanLastOwn() {
169+
details.addToOwnResourceVersions("5");
170+
details.addRelatedEvent(updatedEvent("3", "2"));
171+
details.addRelatedEvent(updatedEvent("4", "3"));
172+
173+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isFalse();
174+
}
175+
176+
@Test
177+
void newerOrEqualReturnsTrueWhenRelatedMatchesLastOwn() {
178+
details.addToOwnResourceVersions("3");
179+
details.addToOwnResourceVersions("5");
180+
details.addRelatedEvent(updatedEvent("5", "4"));
181+
182+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isTrue();
183+
}
184+
185+
@Test
186+
void newerOrEqualReturnsTrueWhenRelatedNewerThanLastOwn() {
187+
details.addToOwnResourceVersions("3");
188+
details.addRelatedEvent(updatedEvent("7", "3"));
189+
190+
assertThat(details.newerOrEqualEventReceivedForOwnLastUpdate()).isTrue();
191+
}
192+
193+
private static GenericResourceEvent addedEvent(String resourceVersion) {
194+
return new GenericResourceEvent(ResourceAction.ADDED, resource(resourceVersion), null, null);
195+
}
196+
197+
private static GenericResourceEvent updatedEvent(
198+
String resourceVersion, String previousResourceVersion) {
199+
var prev = previousResourceVersion == null ? null : resource(previousResourceVersion);
200+
return new GenericResourceEvent(ResourceAction.UPDATED, resource(resourceVersion), prev, null);
201+
}
202+
203+
private static GenericResourceEvent deleteEvent(String resourceVersion) {
204+
return new GenericResourceEvent(ResourceAction.DELETED, resource(resourceVersion), null, null);
205+
}
206+
207+
private static ConfigMap resource(String resourceVersion) {
208+
return new ConfigMapBuilder()
209+
.withMetadata(
210+
new ObjectMetaBuilder()
211+
.withName("test")
212+
.withNamespace("default")
213+
.withUid("test-uid")
214+
.withResourceVersion(resourceVersion)
215+
.build())
216+
.build();
217+
}
218+
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class InformerEventSourceTest {
7171

7272
private static final String PREV_RESOURCE_VERSION = "0";
7373
private static final String DEFAULT_RESOURCE_VERSION = "2";
74-
public static final int REPEAT_COUNT = 10;
74+
public static final int REPEAT_COUNT = 5;
7575

7676
private InformerEventSource<Deployment, TestCustomResource> informerEventSource;
7777
private final KubernetesClient clientMock = MockKubernetesClient.client(Deployment.class);
@@ -466,6 +466,23 @@ void doesNotPropagateIntermediateEventForOurOwnIntermediateUpdate() {
466466
latch3.countDown();
467467
}
468468

469+
@RepeatedTest(REPEAT_COUNT)
470+
void deleteEventPropagatedIfItWasTheLastEvent() {
471+
// Within an open filter window, an external UPDATE arrives followed by a DELETE.
472+
// The summary must surface the DELETE since it represents the final state.
473+
withRealTemporaryResourceCache();
474+
475+
var latch = sendForEventFilteringUpdate(3);
476+
477+
informerEventSource.onUpdate(
478+
deploymentWithResourceVersion(3), deploymentWithResourceVersion(4));
479+
informerEventSource.onDelete(deploymentWithResourceVersion(5), false);
480+
481+
latch.countDown();
482+
483+
expectHandleDeleteEvent(5);
484+
}
485+
469486
private void awaitCachedResourceVersion(ResourceID resourceId, String resourceVersion) {
470487
await()
471488
.untilAsserted(
@@ -527,6 +544,25 @@ private void expectHandleAddEvent(int newResourceVersion, int oldResourceVersion
527544
});
528545
}
529546

547+
private void expectHandleDeleteEvent(int resourceVersion) {
548+
await()
549+
.atMost(Duration.ofSeconds(1))
550+
.untilAsserted(
551+
() -> {
552+
verify(informerEventSource, times(1))
553+
.handleEvent(
554+
eq(ResourceAction.DELETED),
555+
argThat(
556+
newResource -> {
557+
assertThat(newResource.getMetadata().getResourceVersion())
558+
.isEqualTo("" + resourceVersion);
559+
return true;
560+
}),
561+
isNull(),
562+
any());
563+
});
564+
}
565+
530566
private CountDownLatch sendForEventFilteringUpdate(int resourceVersion) {
531567
return sendForEventFilteringUpdate(testDeployment(), resourceVersion);
532568
}

0 commit comments

Comments
 (0)