Skip to content

Commit eae7969

Browse files
DonnerbartDonnerbart
committed
fix: Fix infinite resource updates due to canonical format conversion of resource requirements
Signed-off-by: David Sondermann <[email protected]>
1 parent 3b2a4fb commit eae7969

13 files changed

+716
-0
lines changed

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

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
2+
3+
import java.util.List;
4+
import java.util.Map;
5+
import java.util.Optional;
6+
7+
import io.fabric8.kubernetes.api.model.Container;
8+
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
9+
import io.fabric8.kubernetes.api.model.PodTemplateSpec;
10+
import io.fabric8.kubernetes.api.model.Quantity;
11+
import io.fabric8.kubernetes.api.model.ResourceRequirements;
12+
13+
/**
14+
* Sanitizes the {@link ResourceRequirements} in the containers of a pair of {@link PodTemplateSpec}
15+
* instances.
16+
* <p>
17+
* When the sanitizer finds a mismatch in the structure of the given templates, before it gets to
18+
* the nested resource limits and requests, it returns early without fixing the actual map. This is
19+
* an optimization because the given templates will anyway differ at this point. This means we do
20+
* not have to attempt to sanitize the resources for these use cases, since there will anyway be an
21+
* update of the K8s resource.
22+
* <p>
23+
* The algorithm traverses the whole template structure because we need the actual and desired
24+
* {@link Quantity} instances to compare their numerical amount. Using the
25+
* {@link GenericKubernetesResource#get(Map, Object...)} shortcut would need to create new instances
26+
* just for the sanitization check.
27+
*/
28+
class ResourceRequirementsSanitizer {
29+
30+
static void sanitizeResourceRequirements(final Map<String, Object> actualMap,
31+
final PodTemplateSpec actualTemplate, final PodTemplateSpec desiredTemplate) {
32+
if (actualTemplate == null || desiredTemplate == null) {
33+
return;
34+
}
35+
if (actualTemplate.getSpec() == null || desiredTemplate.getSpec() == null) {
36+
return;
37+
}
38+
sanitizeResourceRequirements(actualMap, actualTemplate.getSpec().getInitContainers(),
39+
desiredTemplate.getSpec().getInitContainers(), "initContainers");
40+
sanitizeResourceRequirements(actualMap, actualTemplate.getSpec().getContainers(),
41+
desiredTemplate.getSpec().getContainers(), "containers");
42+
}
43+
44+
private static void sanitizeResourceRequirements(final Map<String, Object> actualMap,
45+
final List<Container> actualContainers, final List<Container> desiredContainers,
46+
final String containerPath) {
47+
int containers = desiredContainers.size();
48+
if (containers == actualContainers.size()) {
49+
for (int containerIndex = 0; containerIndex < containers; containerIndex++) {
50+
var desiredContainer = desiredContainers.get(containerIndex);
51+
var actualContainer = actualContainers.get(containerIndex);
52+
if (!desiredContainer.getName().equals(actualContainer.getName())) {
53+
return;
54+
}
55+
sanitizeResourceRequirements(actualMap, actualContainer.getResources(),
56+
desiredContainer.getResources(),
57+
containerPath, containerIndex);
58+
}
59+
}
60+
}
61+
62+
private static void sanitizeResourceRequirements(final Map<String, Object> actualMap,
63+
final ResourceRequirements actualResource, final ResourceRequirements desiredResource,
64+
final String containerPath, final int containerIndex) {
65+
if (desiredResource == null || actualResource == null) {
66+
return;
67+
}
68+
sanitizeQuantities(actualMap, actualResource.getRequests(), desiredResource.getRequests(),
69+
containerPath, containerIndex, "requests");
70+
sanitizeQuantities(actualMap, actualResource.getLimits(), desiredResource.getLimits(),
71+
containerPath, containerIndex, "limits");
72+
}
73+
74+
@SuppressWarnings("unchecked")
75+
private static void sanitizeQuantities(final Map<String, Object> actualMap,
76+
final Map<String, Quantity> actualResource, final Map<String, Quantity> desiredResource,
77+
final String containerPath, final int containerIndex, final String quantityPath) {
78+
Optional.ofNullable(
79+
GenericKubernetesResource.get(actualMap, "spec", "template", "spec", containerPath,
80+
containerIndex, "resources", quantityPath))
81+
.map(Map.class::cast)
82+
.filter(m -> m.size() == desiredResource.size())
83+
.ifPresent(m -> actualResource.forEach((key, actualQuantity) -> {
84+
var desiredQuantity = desiredResource.get(key);
85+
if (desiredQuantity == null) {
86+
return;
87+
}
88+
// check if the string representation of the Quantity instances is equal
89+
if (actualQuantity.getAmount().equals(desiredQuantity.getAmount())
90+
&& actualQuantity.getFormat().equals(desiredQuantity.getFormat())) {
91+
return;
92+
}
93+
// check if the numerical amount of the Quantity instances is equal
94+
if (actualQuantity.equals(desiredQuantity)) {
95+
// replace the actual Quantity with the desired Quantity in the canonical format to
96+
// prevent a resource update
97+
m.replace(key, desiredQuantity.toString());
98+
}
99+
}));
100+
}
101+
}

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

+21
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
2020
import io.fabric8.kubernetes.api.model.HasMetadata;
2121
import io.fabric8.kubernetes.api.model.ManagedFieldsEntry;
22+
import io.fabric8.kubernetes.api.model.apps.DaemonSet;
23+
import io.fabric8.kubernetes.api.model.apps.Deployment;
24+
import io.fabric8.kubernetes.api.model.apps.ReplicaSet;
2225
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
2326
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
2427
import io.javaoperatorsdk.operator.OperatorException;
@@ -28,6 +31,8 @@
2831
import com.github.difflib.DiffUtils;
2932
import com.github.difflib.UnifiedDiffUtils;
3033

34+
import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements;
35+
3136
/**
3237
* Matches the actual state on the server vs the desired state. Based on the managedFields of SSA.
3338
* <p>
@@ -185,6 +190,22 @@ private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
185190
}
186191
}
187192
}
193+
sanitizeResourceRequirements(actualMap, actualSpec.getTemplate(), desiredSpec.getTemplate());
194+
} else if (actual instanceof Deployment actualDeployment
195+
&& desired instanceof Deployment desiredDeployment) {
196+
sanitizeResourceRequirements(actualMap,
197+
actualDeployment.getSpec().getTemplate(),
198+
desiredDeployment.getSpec().getTemplate());
199+
} else if (actual instanceof ReplicaSet actualReplicaSet
200+
&& desired instanceof ReplicaSet desiredReplicaSet) {
201+
sanitizeResourceRequirements(actualMap,
202+
actualReplicaSet.getSpec().getTemplate(),
203+
desiredReplicaSet.getSpec().getTemplate());
204+
} else if (actual instanceof DaemonSet actualDaemonSet
205+
&& desired instanceof DaemonSet desiredDaemonSet) {
206+
sanitizeResourceRequirements(actualMap,
207+
actualDaemonSet.getSpec().getTemplate(),
208+
desiredDaemonSet.getSpec().getTemplate());
188209
}
189210
}
190211

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

+199
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
2+
3+
import java.util.Map;
4+
5+
import org.assertj.core.api.MapAssert;
6+
import org.junit.jupiter.api.Test;
7+
8+
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
9+
import io.fabric8.kubernetes.api.model.HasMetadata;
10+
import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder;
11+
import io.fabric8.kubernetes.api.model.Quantity;
12+
import io.fabric8.kubernetes.api.model.apps.StatefulSetBuilder;
13+
import io.fabric8.kubernetes.client.KubernetesClient;
14+
import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
15+
import io.javaoperatorsdk.operator.MockKubernetesClient;
16+
17+
import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements;
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.mockito.Mockito.mock;
20+
import static org.mockito.Mockito.verifyNoInteractions;
21+
22+
class ResourceRequirementsSanitizerTest {
23+
24+
private final Map<String, Object> actualMap = mock();
25+
26+
private final KubernetesClient client = MockKubernetesClient.client(HasMetadata.class);
27+
private final KubernetesSerialization serialization = client.getKubernetesSerialization();
28+
29+
@Test
30+
void testSanitizeResourceRequirements_whenTemplateIsNull_doNothing() {
31+
final var template = new PodTemplateSpecBuilder().build();
32+
33+
sanitizeResourceRequirements(actualMap, null, template);
34+
sanitizeResourceRequirements(actualMap, template, null);
35+
verifyNoInteractions(actualMap);
36+
}
37+
38+
@Test
39+
void testSanitizeResourceRequirements_whenTemplateSpecIsNull_doNothing() {
40+
final var template = new PodTemplateSpecBuilder().withSpec(null).build();
41+
final var templateWithSpec = new PodTemplateSpecBuilder().withNewSpec().endSpec().build();
42+
43+
sanitizeResourceRequirements(actualMap, template, templateWithSpec);
44+
sanitizeResourceRequirements(actualMap, templateWithSpec, template);
45+
verifyNoInteractions(actualMap);
46+
}
47+
48+
@Test
49+
void testSanitizeResourceRequirements_whenContainerSizeMismatch_doNothing() {
50+
final var template = new PodTemplateSpecBuilder().withNewSpec()
51+
.addNewContainer().withName("test").endContainer()
52+
.endSpec().build();
53+
final var templateWithTwoContainers = new PodTemplateSpecBuilder().withNewSpec()
54+
.addNewContainer().withName("test").endContainer()
55+
.addNewContainer().withName("test-new").endContainer()
56+
.endSpec().build();
57+
58+
sanitizeResourceRequirements(actualMap, template, templateWithTwoContainers);
59+
sanitizeResourceRequirements(actualMap, templateWithTwoContainers, template);
60+
verifyNoInteractions(actualMap);
61+
}
62+
63+
@Test
64+
void testSanitizeResourceRequirements_whenContainerNameMismatch_doNothing() {
65+
final var template = new PodTemplateSpecBuilder().withNewSpec()
66+
.addNewContainer().withName("test").endContainer()
67+
.endSpec().build();
68+
final var templateWithNewContainerName = new PodTemplateSpecBuilder().withNewSpec()
69+
.addNewContainer().withName("test-new").endContainer()
70+
.endSpec().build();
71+
72+
sanitizeResourceRequirements(actualMap, template, templateWithNewContainerName);
73+
sanitizeResourceRequirements(actualMap, templateWithNewContainerName, template);
74+
verifyNoInteractions(actualMap);
75+
}
76+
77+
@Test
78+
void testSanitizeResourceRequirements_whenResourceIsNull_doNothing() {
79+
final var template = new PodTemplateSpecBuilder().withNewSpec()
80+
.addNewContainer().withName("test").endContainer()
81+
.endSpec().build();
82+
final var templateWithResource = new PodTemplateSpecBuilder().withNewSpec()
83+
.addNewContainer().withName("test").withNewResources().endResources().endContainer()
84+
.endSpec().build();
85+
86+
sanitizeResourceRequirements(actualMap, template, templateWithResource);
87+
sanitizeResourceRequirements(actualMap, templateWithResource, template);
88+
verifyNoInteractions(actualMap);
89+
}
90+
91+
@Test
92+
void testSanitizeResourceRequirements_whenResourceSizeMismatch_doNothing() {
93+
final var actualMap = sanitizeRequestsAndLimits(
94+
Map.of("cpu", new Quantity("2")),
95+
Map.of(),
96+
Map.of("cpu", new Quantity("4")),
97+
Map.of("cpu", new Quantity("4"), "memory", new Quantity("4Gi")));
98+
assertResources(actualMap, "requests")
99+
.hasSize(1)
100+
.containsEntry("cpu", "2");
101+
assertResources(actualMap, "limits")
102+
.hasSize(1)
103+
.containsEntry("cpu", "4");
104+
}
105+
106+
@Test
107+
void testSanitizeResourceRequirements_whenResourceKeyMismatch_doNothing() {
108+
final var actualMap = sanitizeRequestsAndLimits(
109+
Map.of("cpu", new Quantity("2")),
110+
Map.of("memory", new Quantity("4Gi")),
111+
Map.of(),
112+
Map.of());
113+
assertResources(actualMap, "requests")
114+
.hasSize(1)
115+
.containsEntry("cpu", "2");
116+
assertResources(actualMap, "limits").isNull();
117+
}
118+
119+
@Test
120+
void testSanitizeResourceRequirements_whenResourcesHaveSameAmountAndFormat_doNothing() {
121+
final var actualMap = sanitizeRequestsAndLimits(
122+
Map.of("memory", new Quantity("4Gi")),
123+
Map.of("memory", new Quantity("4Gi")),
124+
Map.of("cpu", new Quantity("2")),
125+
Map.of("cpu", new Quantity("2")));
126+
assertResources(actualMap, "requests")
127+
.hasSize(1)
128+
.containsEntry("memory", "4Gi");
129+
assertResources(actualMap, "limits")
130+
.hasSize(1)
131+
.containsEntry("cpu", "2");
132+
}
133+
134+
@Test
135+
void testSanitizeResourceRequirements_whenResourcesHaveNumericalAmountMismatch_doNothing() {
136+
final var actualMap = sanitizeRequestsAndLimits(
137+
Map.of("cpu", new Quantity("2"), "memory", new Quantity("4Gi")),
138+
Map.of("cpu", new Quantity("4"), "memory", new Quantity("4Ti")),
139+
Map.of("cpu", new Quantity("2")),
140+
Map.of("cpu", new Quantity("4000m")));
141+
assertResources(actualMap, "requests")
142+
.hasSize(2)
143+
.containsEntry("cpu", "2")
144+
.containsEntry("memory", "4Gi");
145+
assertResources(actualMap, "limits")
146+
.hasSize(1)
147+
.containsEntry("cpu", "2");
148+
}
149+
150+
@Test
151+
void testSanitizeResourceRequirements_whenResourcesHaveAmountAndFormatMismatchWithSameNumericalAmount_thenSanitizeActualMap() {
152+
final var actualMap = sanitizeRequestsAndLimits(
153+
Map.of("cpu", new Quantity("2"), "memory", new Quantity("4Gi")),
154+
Map.of("cpu", new Quantity("2000m"), "memory", new Quantity("4096Mi")),
155+
Map.of("cpu", new Quantity("4")),
156+
Map.of("cpu", new Quantity("4000m")));
157+
assertResources(actualMap, "requests")
158+
.hasSize(2)
159+
.containsEntry("cpu", "2000m")
160+
.containsEntry("memory", "4096Mi");
161+
assertResources(actualMap, "limits")
162+
.hasSize(1)
163+
.containsEntry("cpu", "4000m");
164+
}
165+
166+
@SuppressWarnings("unchecked")
167+
private Map<String, Object> sanitizeRequestsAndLimits(
168+
final Map<String, Quantity> actualRequests, final Map<String, Quantity> desiredRequests,
169+
final Map<String, Quantity> actualLimits, final Map<String, Quantity> desiredLimits) {
170+
final var actual = new StatefulSetBuilder().withNewSpec().withNewTemplate().withNewSpec()
171+
.addNewContainer()
172+
.withName("test")
173+
.withNewResources()
174+
.withRequests(actualRequests).withLimits(actualLimits)
175+
.endResources()
176+
.endContainer()
177+
.endSpec().endTemplate().endSpec().build();
178+
final var desired = new StatefulSetBuilder().withNewSpec().withNewTemplate().withNewSpec()
179+
.addNewContainer()
180+
.withName("test")
181+
.withNewResources()
182+
.withRequests(desiredRequests).withLimits(desiredLimits)
183+
.endResources()
184+
.endContainer()
185+
.endSpec().endTemplate().endSpec().build();
186+
187+
final var actualMap = serialization.convertValue(actual, Map.class);
188+
sanitizeResourceRequirements(actualMap,
189+
actual.getSpec().getTemplate(),
190+
desired.getSpec().getTemplate());
191+
return actualMap;
192+
}
193+
194+
private static MapAssert<String, Object> assertResources(final Map<String, Object> actualMap,
195+
final String resourceName) {
196+
return assertThat(GenericKubernetesResource.<Map<String, Object>>get(actualMap,
197+
"spec", "template", "spec", "containers", 0, "resources", resourceName));
198+
}
199+
}

0 commit comments

Comments
 (0)