Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: reduce conflicts when update configmap in k8s #89 #93

Merged
merged 2 commits into from
Jan 13, 2025
Merged

feat: reduce conflicts when update configmap in k8s #89 #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cc173e
feat: reduce conflicts when update configmap in k8s #89
24kpure Dec 9, 2024
0ce4201
changes for code review
24kpure Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Apollo Java 2.4.0
* [Fix monitor arg cause npe](https://github.com/apolloconfig/apollo-java/pull/86)
* [Fix the concurrent issue in SpringValueRegistry.scanAndClean](https://github.com/apolloconfig/apollo-java/pull/95)
* [Feature support incremental configuration synchronization client](https://github.com/apolloconfig/apollo-java/pull/90)
* [Feature reduce conflicts when update configmap in k8](https://github.com/apolloconfig/apollo-java/pull/93)

------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo-java/milestone/4?closed=1)
65 changes: 64 additions & 1 deletion apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,44 @@

import com.ctrip.framework.apollo.core.utils.StringUtils;
import com.ctrip.framework.apollo.exceptions.ApolloConfigException;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import io.kubernetes.client.openapi.ApiClient;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1ConfigMap;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodList;
import io.kubernetes.client.util.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;

import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.TimeUnit;

/**
* Manages Kubernetes ConfigMap operations.
* Required Kubernetes permissions:
* - pods: [get, list] - For pod selection and write eligibility
* - configmaps: [get, create, update] - For ConfigMap operations
*/
@Service
public class KubernetesManager {
private static final Logger logger = LoggerFactory.getLogger(KubernetesManager.class);

private static final String RUNNING_POD_FIELD_SELECTOR = "status.phase=Running";

private static final int MAX_SEARCH_NUM = 100;

private ApiClient client;
private CoreV1Api coreV1Api;
private int propertyKubernetesMaxWritePods = 3;
private String localPodName = System.getenv("HOSTNAME");

public KubernetesManager() {
try {
Expand All @@ -51,8 +68,11 @@ public KubernetesManager() {
}
}

public KubernetesManager(CoreV1Api coreV1Api) {
@VisibleForTesting
public KubernetesManager(CoreV1Api coreV1Api, String localPodName, int propertyKubernetesMaxWritePods) {
this.coreV1Api = coreV1Api;
this.localPodName = localPodName;
this.propertyKubernetesMaxWritePods = propertyKubernetesMaxWritePods;
}

private V1ConfigMap buildConfigMap(String name, String namespace, Map<String, String> data) {
Expand Down Expand Up @@ -132,6 +152,10 @@ public boolean updateConfigMap(String k8sNamespace, String name, Map<String, Str
return false;
}

if (!isWritePod(k8sNamespace)) {
return true;
}
Comment on lines +155 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reconsider return value for non-write pods

Returning true when isWritePod returns false might mask issues by indicating success when no update was attempted. Consider returning a different status or throwing an exception to make it clear that the pod is not eligible to write.


int maxRetries = 5;
int retryCount = 0;
long waitTime = 100;
Expand Down Expand Up @@ -205,4 +229,43 @@ public boolean checkConfigMapExist(String k8sNamespace, String configMapName) {
return false;
}
}

/**
* check pod whether pod can write configmap
*
* @param k8sNamespace config map namespace
* @return true if this pod can write configmap, false otherwise
*/
private boolean isWritePod(String k8sNamespace) {
try {
if (Strings.isNullOrEmpty(localPodName)) {
return true;
}
24kpure marked this conversation as resolved.
Show resolved Hide resolved
V1Pod localPod = coreV1Api.readNamespacedPod(localPodName, k8sNamespace, null);
24kpure marked this conversation as resolved.
Show resolved Hide resolved
V1ObjectMeta localMetadata = localPod.getMetadata();
if (localMetadata == null || localMetadata.getLabels() == null) {
return true;
}
String appName = localMetadata.getLabels().get("app");
String labelSelector = "app=" + appName;

V1PodList v1PodList = coreV1Api.listNamespacedPod(k8sNamespace, null, null,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the number of pod instances is large, the interface return value may be particularly large, and the limit parameter needs to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what is a large number of instances, so I set it to 1000 . How about your advice?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 50 or 100 is sufficient. 1000 clients pull 1000 pod lists from the k8s server at the same time, which may affect the performance of the k8s server or network.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 50 or 100 is sufficient. 1000 clients pull 1000 pod lists from the k8s server at the same time, which may affect the performance of the k8s server or network.

First of all, I agree with you. According to the test, there is little difference between the returned data size of 1000 and 200.Howerer, in the worst scenario, there will be 1000 pod trying to update the pod, which is not as expected.100 may be a good choice.
It's very nice of you ~

null, RUNNING_POD_FIELD_SELECTOR, labelSelector,
MAX_SEARCH_NUM, null, null
, null, null);

return v1PodList.getItems().stream()
.map(V1Pod::getMetadata)
.filter(Objects::nonNull)
24kpure marked this conversation as resolved.
Show resolved Hide resolved
//Make each node selects the same write nodes by sorting
.filter(metadata -> metadata.getCreationTimestamp() != null)
.sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp))
.map(V1ObjectMeta::getName)
.limit(propertyKubernetesMaxWritePods)
.anyMatch(localPodName::equals);
Comment on lines +257 to +265
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance pod selection criteria

The current pod selection logic only considers the creation timestamp and "Running" status. Consider additional criteria:

  1. Pod readiness status (Ready condition)
  2. Pod container statuses
  3. Pod deletion timestamp (ignore pods being terminated)

Example enhancement:

     return v1PodList.getItems().stream()
         .map(V1Pod::getMetadata)
         .filter(Objects::nonNull)
+        .filter(metadata -> metadata.getDeletionTimestamp() == null)
         .filter(metadata -> metadata.getCreationTimestamp() != null)
         .sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp))
         .map(V1ObjectMeta::getName)
         .limit(propertyKubernetesMaxWritePods)
         .anyMatch(localPodName::equals);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return v1PodList.getItems().stream()
.map(V1Pod::getMetadata)
.filter(Objects::nonNull)
//Make each node selects the same write nodes by sorting
.filter(metadata -> metadata.getCreationTimestamp() != null)
.sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp))
.map(V1ObjectMeta::getName)
.limit(propertyKubernetesMaxWritePods)
.anyMatch(localPodName::equals);
return v1PodList.getItems().stream()
.map(V1Pod::getMetadata)
.filter(Objects::nonNull)
.filter(metadata -> metadata.getDeletionTimestamp() == null)
//Make each node selects the same write nodes by sorting
.filter(metadata -> metadata.getCreationTimestamp() != null)
.sorted(Comparator.comparing(V1ObjectMeta::getCreationTimestamp))
.map(V1ObjectMeta::getName)
.limit(propertyKubernetesMaxWritePods)
.anyMatch(localPodName::equals);

} catch (Exception e) {
logger.info("Error determining write pod eligibility:{}", e.getMessage(), e);
return true;
}
24kpure marked this conversation as resolved.
Show resolved Hide resolved
}
}
2 changes: 1 addition & 1 deletion apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ private void initClientMonitorExceptionQueueSize() {
public int getMonitorExceptionQueueSize() {
return monitorExceptionQueueSize;
}

private boolean getPropertyBoolean(String propertyName, String envName, boolean defaultVal) {
String enablePropertyNamesCache = System.getProperty(propertyName);
if (Strings.isNullOrEmpty(enablePropertyNamesCache)) {
Expand Down
56 changes: 46 additions & 10 deletions apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,30 @@
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1ConfigMap;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodList;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mockito;

import java.time.OffsetDateTime;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.mockito.Mockito.*;
import static org.junit.Assert.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.isNull;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class KubernetesManagerTest {

Expand All @@ -38,7 +54,7 @@ public class KubernetesManagerTest {
@Before
public void setUp() {
coreV1Api = mock(CoreV1Api.class);
kubernetesManager = new KubernetesManager(coreV1Api);
kubernetesManager = new KubernetesManager(coreV1Api, "localPodName", 3);

MockInjector.setInstance(KubernetesManager.class, kubernetesManager);
MockInjector.setInstance(CoreV1Api.class, coreV1Api);
Expand All @@ -58,13 +74,13 @@ public void testCreateConfigMapSuccess() throws Exception {
.metadata(new V1ObjectMeta().name(name).namespace(namespace))
.data(data);

when(coreV1Api.createNamespacedConfigMap(eq(namespace), eq(configMap), isNull(), isNull(), isNull(),isNull())).thenReturn(configMap);
when(coreV1Api.createNamespacedConfigMap(eq(namespace), eq(configMap), isNull(), isNull(), isNull(), isNull())).thenReturn(configMap);

// act
String result = kubernetesManager.createConfigMap(namespace, name, data);

// assert
verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), any(V1ConfigMap.class), isNull(), isNull(), isNull(),isNull());
verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), any(V1ConfigMap.class), isNull(), isNull(), isNull(), isNull());
assert name.equals(result);
}

Expand All @@ -82,7 +98,7 @@ public void testCreateConfigMapNullData() throws Exception {
String result = kubernetesManager.createConfigMap(namespace, name, data);

// assert
verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), any(V1ConfigMap.class), isNull(), isNull(), isNull(),isNull());
verify(coreV1Api, times(1)).createNamespacedConfigMap(eq(namespace), any(V1ConfigMap.class), isNull(), isNull(), isNull(), isNull());
assert name.equals(result);
}

Expand Down Expand Up @@ -135,20 +151,40 @@ public void testUpdateConfigMapSuccess() throws Exception {
// arrange
String namespace = "default";
String name = "testConfigMap";
Map<String, String> data = new HashMap<>();
data.put("key", "value");

V1Pod pod = new V1Pod()
.metadata(
new V1ObjectMeta()
.name("localPodName")
.creationTimestamp(OffsetDateTime.now())
.labels(Collections.singletonMap("app", "app")));
V1PodList v1PodList = new V1PodList().addItemsItem(new V1Pod().metadata(pod.getMetadata()));
Comment on lines +155 to +161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add test cases for pod write eligibility scenarios.

The current test only covers the basic success case. Add test cases for:

  1. Pod without required labels
  2. Pod with deletion timestamp
  3. Multiple pods with different creation timestamps
  4. Pods in non-running states

Would you like me to generate these additional test cases?


Map<String, String> existData = new HashMap<>();
existData.put("key", "value");
V1ConfigMap configMap = new V1ConfigMap();
configMap.metadata(new V1ObjectMeta().name(name).namespace(namespace));
configMap.data(data);
configMap.data(existData);

when(coreV1Api.readNamespacedPod("localPodName", namespace, null)).thenReturn(pod);
when(coreV1Api.listNamespacedPod(namespace, null, null,
null, null, "app=app",
null, null, null
, null, null)).thenReturn(v1PodList);
Comment on lines +169 to +173
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Test case needs alignment with KubernetesManager implementation

The test case doesn't match the actual implementation in KubernetesManager.java:

  • The label selector should be dynamically constructed from pod metadata's "app" label value
  • One test case is missing the required RUNNING_POD_FIELD_SELECTOR
  • The MAX_SEARCH_NUM limit (100) should be used in the listNamespacedPod calls
🔗 Analysis chain

Verify the listNamespacedPod parameters.

The API call uses hardcoded values that might need adjustment:

  1. The label selector "app=app" seems generic
  2. The status phase selector is missing
  3. The limit parameter is not set
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other test files that might define these constants
rg -t java "app=app" .
rg -t java "status\.phase=Running" .

Length of output: 675

Script:

#!/bin/bash
# Search for the implementation and any constants
ast-grep --pattern 'private static final String $_ = "app=$_";'
cat ./apollo-client/src/main/java/com/ctrip/framework/apollo/kubernetes/KubernetesManager.java

Length of output: 11517

when(coreV1Api.readNamespacedConfigMap(name, namespace, null)).thenReturn(configMap);
when(coreV1Api.replaceNamespacedConfigMap(name, namespace, configMap, null, null, null, null)).thenReturn(configMap);

// act
Boolean success = kubernetesManager.updateConfigMap(namespace, name, data);
HashMap<String, String> updateData = new HashMap<>(existData);
updateData.put("newKey","newValue");
boolean success = kubernetesManager.updateConfigMap(namespace, name, updateData);

// assert
assertTrue(success);
Mockito.verify(coreV1Api, Mockito.times(1)).listNamespacedPod(namespace, null, null,
null, "status.phase=Running", "app=app",
100, null, null
, null, null);
Comment on lines +184 to +187
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify the pod listing parameters.

The verification uses different parameters than the actual call:

  1. Status phase selector is present in verification but missing in the actual call
  2. Limit is set to 100 in verification but missing in the actual call

Apply this diff to align the parameters:

-        when(coreV1Api.listNamespacedPod(namespace, null, null,
-                null, null, "app=app",
-                null, null, null
-                , null, null)).thenReturn(v1PodList);
+        when(coreV1Api.listNamespacedPod(namespace, null, null,
+                null, "status.phase=Running", "app=app",
+                100, null, null
+                , null, null)).thenReturn(v1PodList);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Mockito.verify(coreV1Api, Mockito.times(1)).listNamespacedPod(namespace, null, null,
null, "status.phase=Running", "app=app",
100, null, null
, null, null);
when(coreV1Api.listNamespacedPod(namespace, null, null,
null, "status.phase=Running", "app=app",
100, null, null
, null, null)).thenReturn(v1PodList);

}

/**
Expand Down
Loading