From 4c39986c6105a4b8f64c7fff75287d7ab290114c Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Fri, 10 Feb 2023 00:18:56 -0600 Subject: [PATCH] [fix][function] Fix k8s runtime shallow clone bug --- .../BasicKubernetesManifestCustomizer.java | 21 ++++++-- ...BasicKubernetesManifestCustomizerTest.java | 53 +++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizer.java b/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizer.java index d82a8f2d1d671..3f02ff4849563 100644 --- a/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizer.java +++ b/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizer.java @@ -62,7 +62,7 @@ public class BasicKubernetesManifestCustomizer implements KubernetesManifestCust @Setter @NoArgsConstructor @AllArgsConstructor - @Builder(toBuilder = true) + @Builder() public static class RuntimeOpts { private String jobNamespace; private String jobName; @@ -71,6 +71,21 @@ public static class RuntimeOpts { private Map nodeSelectorLabels; private V1ResourceRequirements resourceRequirements; private List tolerations; + + /** + * A clone where the maps and lists are properly cloned. The k8s resources themselves are shallow clones. + */ + public RuntimeOpts partialDeepClone() { + return new RuntimeOpts( + jobNamespace, + jobName, + extraLabels != null ? new HashMap<>(extraLabels) : null, + extraAnnotations != null ? new HashMap<>(extraAnnotations) : null, + nodeSelectorLabels != null ? new HashMap<>(nodeSelectorLabels) : null, + resourceRequirements, + tolerations != null ? new ArrayList<>(tolerations) : null + ); + } } @Getter @@ -82,7 +97,7 @@ public void initialize(Map config) { RuntimeOpts opts = ObjectMapperFactory.getMapper().getObjectMapper().convertValue(config, RuntimeOpts.class); if (opts != null) { - runtimeOpts = opts.toBuilder().build(); + runtimeOpts = opts; } } else { log.warn("initialize with null config"); @@ -176,7 +191,7 @@ private V1ObjectMeta updateMeta(RuntimeOpts opts, V1ObjectMeta meta) { } public static RuntimeOpts mergeRuntimeOpts(RuntimeOpts oriOpts, RuntimeOpts newOpts) { - RuntimeOpts mergedOpts = oriOpts.toBuilder().build(); + RuntimeOpts mergedOpts = oriOpts.partialDeepClone(); if (mergedOpts.getExtraLabels() == null) { mergedOpts.setExtraLabels(new HashMap<>()); } diff --git a/pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizerTest.java b/pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizerTest.java index 0f42755694288..d70345cdfbdd3 100644 --- a/pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizerTest.java +++ b/pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/BasicKubernetesManifestCustomizerTest.java @@ -24,8 +24,10 @@ import io.kubernetes.client.openapi.models.V1Toleration; import org.testng.annotations.Test; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import static org.testng.Assert.assertEquals; @@ -93,4 +95,55 @@ public void TestMergeRuntimeOpts() { assertEquals(mergedOpts.getResourceRequirements().getLimits().get("cpu").getNumber().intValue(), 20); assertEquals(mergedOpts.getResourceRequirements().getLimits().get("memory").getNumber().intValue(), 10240); } + + // Note: this test creates many new objects to ensure that the tests guarantees objects are not mutated + // unexpectedly. + @Test + public void testMergeRuntimeOptsDoesNotModifyArguments() { + BasicKubernetesManifestCustomizer.RuntimeOpts opts1 = new BasicKubernetesManifestCustomizer.RuntimeOpts( + "namespace1", "job1", new HashMap<>(), new HashMap<>(), new HashMap<>(), new V1ResourceRequirements(), + new ArrayList<>()); + + HashMap testMap = new HashMap<>(); + testMap.put("testKey", "testValue"); + + List testList = new ArrayList<>(); + testList.add(new V1Toleration()); + + V1ResourceRequirements requirements = new V1ResourceRequirements(); + requirements.setLimits(new HashMap<>()); + BasicKubernetesManifestCustomizer.RuntimeOpts opts2 = new BasicKubernetesManifestCustomizer.RuntimeOpts( + "namespace2", "job2", testMap, testMap, testMap,requirements, testList); + + // Merge the runtime opts + BasicKubernetesManifestCustomizer.RuntimeOpts result = + BasicKubernetesManifestCustomizer.mergeRuntimeOpts(opts1, opts2); + + // Assert opts1 is same + assertEquals("namespace1", opts1.getJobNamespace()); + assertEquals("job1", opts1.getJobName()); + assertEquals(new HashMap<>(), opts1.getNodeSelectorLabels()); + assertEquals(new HashMap<>(), opts1.getExtraAnnotations()); + assertEquals(new HashMap<>(), opts1.getExtraLabels()); + assertEquals(new ArrayList<>(), opts1.getTolerations()); + assertEquals(new V1ResourceRequirements(), opts1.getResourceRequirements()); + + // Assert opts2 is same + HashMap expectedTestMap = new HashMap<>(); + expectedTestMap.put("testKey", "testValue"); + + List expectedTestList = new ArrayList<>(); + expectedTestList.add(new V1Toleration()); + + V1ResourceRequirements expectedRequirements = new V1ResourceRequirements(); + expectedRequirements.setLimits(new HashMap<>()); + + assertEquals("namespace2", opts2.getJobNamespace()); + assertEquals("job2", opts2.getJobName()); + assertEquals(expectedTestMap, opts2.getNodeSelectorLabels()); + assertEquals(expectedTestMap, opts2.getExtraAnnotations()); + assertEquals(expectedTestMap, opts2.getExtraLabels()); + assertEquals(expectedTestList, opts2.getTolerations()); + assertEquals(expectedRequirements, opts2.getResourceRequirements()); + } }