You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by mm...@apache.org on 2023/02/10 22:42:16 UTC
[pulsar] branch branch-2.11 updated (d7686f1d4ce -> 73b9240271c)
This is an automated email from the ASF dual-hosted git repository.
mmarshall pushed a change to branch branch-2.11
in repository https://gitbox.apache.org/repos/asf/pulsar.git
from d7686f1d4ce [improve][txn] Allow superusers to abort transactions (#19467)
new 1ef33072078 [fix][fn] Fix k8s merge runtime opts bug (#19481)
new f0b5b538136 Remove unused import from ServerCnxTest
new 73b9240271c Add * to license header
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../pulsar/broker/service/ServerCnxTest.java | 1 -
...thenticatedTransactionProducerConsumerTest.java | 2 +-
.../BasicKubernetesManifestCustomizer.java | 21 +++++++--
.../BasicKubernetesManifestCustomizerTest.java | 53 ++++++++++++++++++++++
4 files changed, 72 insertions(+), 5 deletions(-)
[pulsar] 01/03: [fix][fn] Fix k8s merge runtime opts bug (#19481)
Posted by mm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mmarshall pushed a commit to branch branch-2.11
in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit 1ef33072078d8efd92b044a8f91cad8e493a2cfe
Author: Michael Marshall <mm...@apache.org>
AuthorDate: Fri Feb 10 16:26:01 2023 -0600
[fix][fn] Fix k8s merge runtime opts bug (#19481)
Fixes: https://github.com/apache/pulsar/issues/19478
### Motivation
See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.
### Modifications
* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.
### Verifying this change
Added a test that fails before the change and passes afterwards.
### Documentation
- [x] `doc-not-needed`
This is an internal bug fix. No docs needed.
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/27
(cherry picked from commit 0205148ca84af194c42535c16d103a6f7851607f)
---
.../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 6fabb359b34..6a9b89dd354 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 class BasicKubernetesManifestCustomizer implements KubernetesManifestCust
private Map<String, String> nodeSelectorLabels;
private V1ResourceRequirements resourceRequirements;
private List<V1Toleration> 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
@@ -81,7 +96,7 @@ public class BasicKubernetesManifestCustomizer implements KubernetesManifestCust
if (config != null) {
RuntimeOpts opts = ObjectMapperFactory.getThreadLocal().convertValue(config, RuntimeOpts.class);
if (opts != null) {
- runtimeOpts = opts.toBuilder().build();
+ runtimeOpts = opts;
}
} else {
log.warn("initialize with null config");
@@ -175,7 +190,7 @@ public class BasicKubernetesManifestCustomizer implements KubernetesManifestCust
}
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 1b383061bda..b8299a8e917 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.V1ResourceRequirements;
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 class BasicKubernetesManifestCustomizerTest {
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<String, String> testMap = new HashMap<>();
+ testMap.put("testKey", "testValue");
+
+ List<V1Toleration> 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<String, String> expectedTestMap = new HashMap<>();
+ expectedTestMap.put("testKey", "testValue");
+
+ List<V1Toleration> 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());
+ }
}
[pulsar] 02/03: Remove unused import from ServerCnxTest
Posted by mm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mmarshall pushed a commit to branch branch-2.11
in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit f0b5b5381369e5ec54e1242cd3c3b8114328c1bd
Author: Michael Marshall <mm...@apache.org>
AuthorDate: Fri Feb 10 16:37:12 2023 -0600
Remove unused import from ServerCnxTest
---
.../src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java | 1 -
1 file changed, 1 deletion(-)
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
index 629ef3de892..0ea58d75c03 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
@@ -25,7 +25,6 @@ import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
-import static org.mockito.ArgumentMatchers.anyObject;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.doAnswer;
[pulsar] 03/03: Add * to license header
Posted by mm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
mmarshall pushed a commit to branch branch-2.11
in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit 73b9240271c407fa55a43cf6c14cbc8e4d8a90cc
Author: Michael Marshall <mm...@apache.org>
AuthorDate: Fri Feb 10 16:39:43 2023 -0600
Add * to license header
---
.../transaction/AuthenticatedTransactionProducerConsumerTest.java | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/AuthenticatedTransactionProducerConsumerTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/AuthenticatedTransactionProducerConsumerTest.java
index aa9102189d4..b79c3f22a67 100644
--- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/AuthenticatedTransactionProducerConsumerTest.java
+++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/AuthenticatedTransactionProducerConsumerTest.java
@@ -1,4 +1,4 @@
-/*
+/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information