Skip to content

Commit e3c828f

Browse files
csvirimetacosm
csviri
and
metacosm
authored
improve: blocklist of problematic resources for previous version annotation (#2774)
* feat: blacklist of problematic resources for previous version annotation Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * rename to blocklist, added javadocs Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * wip Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * remove deamonSet since was not able to reproduce the behavior Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * Add integration test Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * explanation and using Set instead of list Signed-off-by: Attila Mészáros <a_meszaros@apple.com> * fix: improve javadoc Signed-off-by: Chris Laprun <claprun@redhat.com> * refactor: rename so that relation between methods is more explicit Signed-off-by: Chris Laprun <claprun@redhat.com> * naming Signed-off-by: Attila Mészáros <a_meszaros@apple.com> --------- Signed-off-by: Attila Mészáros <a_meszaros@apple.com> Signed-off-by: Chris Laprun <claprun@redhat.com> Co-authored-by: Chris Laprun <claprun@redhat.com>
1 parent 5520c14 commit e3c828f

File tree

9 files changed

+268
-6
lines changed

9 files changed

+268
-6
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
import io.fabric8.kubernetes.api.model.ConfigMap;
1414
import io.fabric8.kubernetes.api.model.HasMetadata;
1515
import io.fabric8.kubernetes.api.model.Secret;
16+
import io.fabric8.kubernetes.api.model.apps.Deployment;
17+
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
1618
import io.fabric8.kubernetes.client.Config;
1719
import io.fabric8.kubernetes.client.ConfigBuilder;
1820
import io.fabric8.kubernetes.client.CustomResource;
@@ -442,12 +444,40 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
442444
*
443445
* @return if special annotation should be used for dependent resource to filter events
444446
* @since 4.5.0
445-
* @return if special annotation should be used for dependent resource to filter events
446447
*/
447448
default boolean previousAnnotationForDependentResourcesEventFiltering() {
448449
return true;
449450
}
450451

452+
/**
453+
* For dependent resources, the framework can add an annotation to filter out events resulting
454+
* directly from the framework's operation. There are, however, some resources that do not follow
455+
* the Kubernetes API conventions that changes in metadata should not increase the generation of
456+
* the resource (as recorded in the {@code generation} field of the resource's {@code metadata}).
457+
* For these resources, this convention is not respected and results in a new event for the
458+
* framework to process. If that particular case is not handled correctly in the resource matcher,
459+
* the framework will consider that the resource doesn't match the desired state and therefore
460+
* triggers an update, which in turn, will re-add the annotation, thus starting the loop again,
461+
* infinitely.
462+
*
463+
* <p>As a workaround, we automatically skip adding previous annotation for those well-known
464+
* resources. Note that if you are sure that the matcher works for your use case, and it should in
465+
* most instances, you can remove the resource type from the blocklist.
466+
*
467+
* <p>The consequence of adding a resource type to the set is that the framework will not use
468+
* event filtering to prevent events, initiated by changes made by the framework itself as a
469+
* result of its processing of dependent resources, to trigger the associated reconciler again.
470+
*
471+
* <p>Note that this method only takes effect if annotating dependent resources to prevent
472+
* dependent resources events from triggering the associated reconciler again is activated as
473+
* controlled by {@link #previousAnnotationForDependentResourcesEventFiltering()}
474+
*
475+
* @return a Set of resource classes where the previous version annotation won't be used.
476+
*/
477+
default Set<Class<? extends HasMetadata>> withPreviousAnnotationForDependentResourcesBlocklist() {
478+
return Set.of(Deployment.class, StatefulSet.class);
479+
}
480+
451481
/**
452482
* If the event logic should parse the resourceVersion to determine the ordering of dependent
453483
* resource events. This is typically not needed.

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ public class ConfigurationServiceOverrider {
4040
private Boolean parseResourceVersions;
4141
private Boolean useSSAToPatchPrimaryResource;
4242
private Boolean cloneSecondaryResourcesWhenGettingFromCache;
43+
private Set<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist;
4344

4445
@SuppressWarnings("rawtypes")
4546
private DependentResourceFactory dependentResourceFactory;
@@ -188,6 +189,12 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC
188189
return this;
189190
}
190191

192+
public ConfigurationServiceOverrider withPreviousAnnotationForDependentResourcesBlocklist(
193+
Set<Class<? extends HasMetadata>> blocklist) {
194+
this.previousAnnotationUsageBlocklist = blocklist;
195+
return this;
196+
}
197+
191198
public ConfigurationService build() {
192199
return new BaseConfigurationService(original.getVersion(), cloner, client) {
193200
@Override
@@ -328,6 +335,14 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() {
328335
cloneSecondaryResourcesWhenGettingFromCache,
329336
ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache);
330337
}
338+
339+
@Override
340+
public Set<Class<? extends HasMetadata>>
341+
withPreviousAnnotationForDependentResourcesBlocklist() {
342+
return overriddenValueOrDefault(
343+
previousAnnotationUsageBlocklist,
344+
ConfigurationService::withPreviousAnnotationForDependentResourcesBlocklist);
345+
}
331346
};
332347
}
333348
}

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
4141
private final boolean garbageCollected = this instanceof GarbageCollected;
4242
private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
4343
private volatile Boolean useSSA;
44+
private volatile Boolean usePreviousAnnotationForEventFiltering;
4445

4546
public KubernetesDependentResource() {}
4647

@@ -165,10 +166,19 @@ protected boolean useSSA(Context<P> context) {
165166
}
166167

167168
private boolean usePreviousAnnotation(Context<P> context) {
168-
return context
169-
.getControllerConfiguration()
170-
.getConfigurationService()
171-
.previousAnnotationForDependentResourcesEventFiltering();
169+
if (usePreviousAnnotationForEventFiltering == null) {
170+
usePreviousAnnotationForEventFiltering =
171+
context
172+
.getControllerConfiguration()
173+
.getConfigurationService()
174+
.previousAnnotationForDependentResourcesEventFiltering()
175+
&& !context
176+
.getControllerConfiguration()
177+
.getConfigurationService()
178+
.withPreviousAnnotationForDependentResourcesBlocklist()
179+
.contains(this.resourceType());
180+
}
181+
return usePreviousAnnotationForEventFiltering;
172182
}
173183

174184
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ private Optional<ManagedFieldsEntry> checkIfFieldManagerExists(R actual, String
179179
}
180180

181181
/** Correct for known issue with SSA */
182-
private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
182+
protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
183183
if (actual instanceof StatefulSet actualStatefulSet
184184
&& desired instanceof StatefulSet desiredStatefulSet) {
185185
var actualSpec = actualStatefulSet.getSpec();

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package io.javaoperatorsdk.operator.dependent.prevblocklist;
2+
3+
import java.util.Map;
4+
5+
import io.fabric8.kubernetes.api.model.ContainerBuilder;
6+
import io.fabric8.kubernetes.api.model.HasMetadata;
7+
import io.fabric8.kubernetes.api.model.LabelSelectorBuilder;
8+
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
9+
import io.fabric8.kubernetes.api.model.PodSpecBuilder;
10+
import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder;
11+
import io.fabric8.kubernetes.api.model.Quantity;
12+
import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder;
13+
import io.fabric8.kubernetes.api.model.apps.Deployment;
14+
import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
15+
import io.fabric8.kubernetes.api.model.apps.DeploymentSpecBuilder;
16+
import io.javaoperatorsdk.operator.api.reconciler.Context;
17+
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource;
18+
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher;
19+
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
20+
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.SSABasedGenericKubernetesResourceMatcher;
21+
22+
@KubernetesDependent
23+
public class DeploymentDependent
24+
extends CRUDKubernetesDependentResource<Deployment, PrevAnnotationBlockCustomResource> {
25+
26+
public static final String RESOURCE_NAME = "test1";
27+
28+
public DeploymentDependent() {
29+
super(Deployment.class);
30+
}
31+
32+
@Override
33+
protected Deployment desired(
34+
PrevAnnotationBlockCustomResource primary,
35+
Context<PrevAnnotationBlockCustomResource> context) {
36+
37+
return new DeploymentBuilder()
38+
.withMetadata(
39+
new ObjectMetaBuilder()
40+
.withName(primary.getMetadata().getName())
41+
.withNamespace(primary.getMetadata().getNamespace())
42+
.build())
43+
.withSpec(
44+
new DeploymentSpecBuilder()
45+
.withReplicas(1)
46+
.withSelector(
47+
new LabelSelectorBuilder().withMatchLabels(Map.of("app", "nginx")).build())
48+
.withTemplate(
49+
new PodTemplateSpecBuilder()
50+
.withMetadata(
51+
new ObjectMetaBuilder().withLabels(Map.of("app", "nginx")).build())
52+
.withSpec(
53+
new PodSpecBuilder()
54+
.withContainers(
55+
new ContainerBuilder()
56+
.withName("nginx")
57+
.withImage("nginx:1.14.2")
58+
.withResources(
59+
new ResourceRequirementsBuilder()
60+
.withLimits(Map.of("cpu", new Quantity("2000m")))
61+
.build())
62+
.build())
63+
.build())
64+
.build())
65+
.build())
66+
.build();
67+
}
68+
69+
// for testing purposes replicating the matching logic but with the special matcher
70+
@Override
71+
public Result<Deployment> match(
72+
Deployment actualResource,
73+
Deployment desired,
74+
PrevAnnotationBlockCustomResource primary,
75+
Context<PrevAnnotationBlockCustomResource> context) {
76+
final boolean matches;
77+
addMetadata(true, actualResource, desired, primary, context);
78+
if (useSSA(context)) {
79+
matches = new SSAMatcherWithoutSanitization().matches(actualResource, desired, context);
80+
} else {
81+
matches =
82+
GenericKubernetesResourceMatcher.match(desired, actualResource, false, false, context)
83+
.matched();
84+
}
85+
return Result.computed(matches, desired);
86+
}
87+
88+
// using this matcher, so we are able to reproduce issue with resource limits
89+
static class SSAMatcherWithoutSanitization<R extends HasMetadata>
90+
extends SSABasedGenericKubernetesResourceMatcher<R> {
91+
92+
@Override
93+
protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {}
94+
}
95+
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package io.javaoperatorsdk.operator.dependent.prevblocklist;
2+
3+
import io.fabric8.kubernetes.api.model.Namespaced;
4+
import io.fabric8.kubernetes.client.CustomResource;
5+
import io.fabric8.kubernetes.model.annotation.Group;
6+
import io.fabric8.kubernetes.model.annotation.ShortNames;
7+
import io.fabric8.kubernetes.model.annotation.Version;
8+
9+
@Group("sample.javaoperatorsdk")
10+
@Version("v1")
11+
@ShortNames("pabc")
12+
public class PrevAnnotationBlockCustomResource extends CustomResource<PrevAnnotationBlockSpec, Void>
13+
implements Namespaced {}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package io.javaoperatorsdk.operator.dependent.prevblocklist;
2+
3+
import java.util.concurrent.atomic.AtomicInteger;
4+
5+
import io.javaoperatorsdk.operator.api.reconciler.Context;
6+
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
7+
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
8+
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
9+
import io.javaoperatorsdk.operator.api.reconciler.Workflow;
10+
import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent;
11+
import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider;
12+
13+
@Workflow(dependents = {@Dependent(type = DeploymentDependent.class)})
14+
@ControllerConfiguration()
15+
public class PrevAnnotationBlockReconciler
16+
implements Reconciler<PrevAnnotationBlockCustomResource>, TestExecutionInfoProvider {
17+
18+
private final AtomicInteger numberOfExecutions = new AtomicInteger(0);
19+
20+
public PrevAnnotationBlockReconciler() {}
21+
22+
@Override
23+
public UpdateControl<PrevAnnotationBlockCustomResource> reconcile(
24+
PrevAnnotationBlockCustomResource resource,
25+
Context<PrevAnnotationBlockCustomResource> context) {
26+
numberOfExecutions.getAndIncrement();
27+
28+
return UpdateControl.noUpdate();
29+
}
30+
31+
public int getNumberOfExecutions() {
32+
return numberOfExecutions.get();
33+
}
34+
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package io.javaoperatorsdk.operator.dependent.prevblocklist;
2+
3+
import java.time.Duration;
4+
5+
import org.junit.jupiter.api.Test;
6+
import org.junit.jupiter.api.extension.RegisterExtension;
7+
8+
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
9+
import io.fabric8.kubernetes.api.model.apps.Deployment;
10+
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
11+
12+
import static org.assertj.core.api.Assertions.assertThat;
13+
import static org.awaitility.Awaitility.await;
14+
15+
class PrevAnnotationBlockReconcilerIT {
16+
17+
public static final String TEST_1 = "test1";
18+
19+
@RegisterExtension
20+
LocallyRunOperatorExtension extension =
21+
LocallyRunOperatorExtension.builder()
22+
// Removing resource from blocklist List would result in test failure
23+
// .withConfigurationService(
24+
// o -> o.previousAnnotationUsageBlocklist(Collections.emptyList()))
25+
.withReconciler(PrevAnnotationBlockReconciler.class)
26+
.build();
27+
28+
@Test
29+
void doNotUsePrevAnnotationForDeploymentDependent() {
30+
extension.create(testResource(TEST_1));
31+
32+
var reconciler = extension.getReconcilerOfType(PrevAnnotationBlockReconciler.class);
33+
await()
34+
.pollDelay(Duration.ofMillis(200))
35+
.untilAsserted(
36+
() -> {
37+
var deployment = extension.get(Deployment.class, TEST_1);
38+
assertThat(deployment).isNotNull();
39+
assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0).isLessThan(10);
40+
});
41+
}
42+
43+
PrevAnnotationBlockCustomResource testResource(String name) {
44+
var res = new PrevAnnotationBlockCustomResource();
45+
res.setMetadata(new ObjectMetaBuilder().withName(name).build());
46+
res.setSpec(new PrevAnnotationBlockSpec());
47+
res.getSpec().setValue("value");
48+
return res;
49+
}
50+
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package io.javaoperatorsdk.operator.dependent.prevblocklist;
2+
3+
public class PrevAnnotationBlockSpec {
4+
5+
private String value;
6+
7+
public String getValue() {
8+
return value;
9+
}
10+
11+
public PrevAnnotationBlockSpec setValue(String value) {
12+
this.value = value;
13+
return this;
14+
}
15+
}

0 commit comments

Comments
 (0)