From 1ddb22a06fde5b83156e0b56862db1f97475c836 Mon Sep 17 00:00:00 2001 From: daguimu Date: Fri, 27 Mar 2026 11:52:33 +0800 Subject: [PATCH] fix: incorrect cross-cluster call warning when clusterName is not set Move the "A cross-cluster call occurs" warning from the outer else block (which triggers when clusterName is blank) into the inner else block (which triggers when clusterName IS set but no same-cluster instances are found). Additionally, throttle the cross-cluster warning to log at most once per 5 minutes per (serviceId, clusterName) to prevent log flooding under sustained cross-cluster fallback traffic. Fixes #4020 --- .../nacos/loadbalancer/NacosLoadBalancer.java | 21 +- .../loadbalancer/NacosLoadBalancerTests.java | 229 ++++++++++++++++++ 2 files changed, 245 insertions(+), 5 deletions(-) create mode 100644 spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/test/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancerTests.java diff --git a/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/main/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancer.java b/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/main/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancer.java index c5e45e3afd..457315d246 100644 --- a/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/main/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancer.java +++ b/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/main/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancer.java @@ -60,6 +60,10 @@ public class NacosLoadBalancer implements ReactorServiceInstanceLoadBalancer { private static final String IPV4_REGEX = "((2(5[0-5]|[0-4]\\d))|[0-1]?\\d{1,2})(.((2(5[0-5]|[0-4]\\d))|[0-1]?\\d{1,2})){3}"; + private static final long CROSS_CLUSTER_WARN_INTERVAL_MS = 10_000L; + + private volatile long lastWarnLogTime; + private static final String IPV6_KEY = "IPv6"; /** * Storage local valid IPv6 address, it's a flag whether local machine support IPv6 address stack. @@ -152,11 +156,9 @@ private Response getInstanceResponse(Request request, if (!CollectionUtils.isEmpty(sameClusterInstances)) { instancesToChoose = sameClusterInstances; } - } - else { - log.warn( - "A cross-cluster call occurs，name = {}, clusterName = {}, instance = {}", - serviceId, clusterName, serviceInstances); + else { + warnCrossClusterThrottled(clusterName, serviceInstances); + } } instancesToChoose = this.filterInstanceByIpType(instancesToChoose); @@ -183,4 +185,13 @@ private Response getInstanceResponse(Request request, } } + private void warnCrossClusterThrottled(String clusterName, List serviceInstances) { + long now = System.currentTimeMillis(); + if (now - lastWarnLogTime > CROSS_CLUSTER_WARN_INTERVAL_MS) { + lastWarnLogTime = now; + log.warn("A cross-cluster call occurs，name = {}, clusterName = {}, instance = {}", + serviceId, clusterName, serviceInstances); + } + } + } diff --git a/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/test/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancerTests.java b/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/test/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancerTests.java new file mode 100644 index 0000000000..bf23a77915 --- /dev/null +++ b/spring-cloud-alibaba-starters/spring-cloud-starter-alibaba-nacos-discovery/src/test/java/com/alibaba/cloud/nacos/loadbalancer/NacosLoadBalancerTests.java @@ -0,0 +1,229 @@ +/* + * Copyright 2013-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.alibaba.cloud.nacos.loadbalancer; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import com.alibaba.cloud.nacos.NacosDiscoveryProperties; +import com.alibaba.cloud.nacos.util.InetIPv6Utils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.DefaultResponse; +import org.springframework.cloud.client.loadbalancer.EmptyResponse; +import org.springframework.cloud.client.loadbalancer.Request; +import org.springframework.cloud.client.loadbalancer.Response; +import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link NacosLoadBalancer} cross-cluster call log logic. + * + * @author daguimu + */ +@ExtendWith(MockitoExtension.class) +public class NacosLoadBalancerTests { + + @Mock + private ObjectProvider supplierProvider; + + @Mock + private InetIPv6Utils inetIPv6Utils; + + @Mock + private LoadBalancerAlgorithm defaultAlgorithm; + + private NacosDiscoveryProperties nacosDiscoveryProperties; + + private NacosLoadBalancer nacosLoadBalancer; + + @BeforeEach + void setUp() { + nacosDiscoveryProperties = new NacosDiscoveryProperties(); + + Map algorithmMap = new HashMap<>(); + algorithmMap.put(LoadBalancerAlgorithm.DEFAULT_SERVICE_ID, defaultAlgorithm); + + nacosLoadBalancer = new NacosLoadBalancer( + supplierProvider, + "test-service", + nacosDiscoveryProperties, + inetIPv6Utils, + Collections.emptyList(), + algorithmMap); + } + + @Test + public void blankClusterNameShouldNotWarnCrossCluster() { + nacosDiscoveryProperties.setClusterName(null); + List instances = createInstances("192.168.1.1", "clusterA"); + + when(defaultAlgorithm.getInstance(any(), any())).thenReturn(instances.get(0)); + + Response response = invokeGetInstanceResponse(instances); + + assertThat(response).isInstanceOf(DefaultResponse.class); + assertThat(response.hasServer()).isTrue(); + assertThat(getLastWarnLogTime()).isEqualTo(0L); + } + + @Test + public void emptyClusterNameShouldNotWarnCrossCluster() { + nacosDiscoveryProperties.setClusterName(""); + List instances = createInstances("192.168.1.1", "clusterA"); + + when(defaultAlgorithm.getInstance(any(), any())).thenReturn(instances.get(0)); + + Response response = invokeGetInstanceResponse(instances); + + assertThat(response).isInstanceOf(DefaultResponse.class); + assertThat(response.hasServer()).isTrue(); + assertThat(getLastWarnLogTime()).isEqualTo(0L); + } + + @Test + public void sameClusterShouldSelectSameClusterInstance() { + nacosDiscoveryProperties.setClusterName("clusterA"); + + List instances = new ArrayList<>(); + instances.add(createInstance("192.168.1.1", "clusterA")); + instances.add(createInstance("192.168.1.2", "clusterB")); + + ServiceInstance clusterAInstance = instances.get(0); + when(defaultAlgorithm.getInstance(any(), any())).thenReturn(clusterAInstance); + + Response response = invokeGetInstanceResponse(instances); + + assertThat(response).isInstanceOf(DefaultResponse.class); + assertThat(response.hasServer()).isTrue(); + assertThat(getLastWarnLogTime()).isEqualTo(0L); + } + + @Test + public void noSameClusterInstancesShouldStillReturnResponse() { + nacosDiscoveryProperties.setClusterName("clusterC"); + + List instances = new ArrayList<>(); + instances.add(createInstance("192.168.1.1", "clusterA")); + instances.add(createInstance("192.168.1.2", "clusterB")); + + when(defaultAlgorithm.getInstance(any(), any())).thenReturn(instances.get(0)); + + Response response = invokeGetInstanceResponse(instances); + + assertThat(response).isInstanceOf(DefaultResponse.class); + assertThat(response.hasServer()).isTrue(); + assertThat(getLastWarnLogTime()).isGreaterThan(0L); + } + + @Test + public void crossClusterWarnShouldBeThrottled() { + nacosDiscoveryProperties.setClusterName("clusterC"); + + List instances = new ArrayList<>(); + instances.add(createInstance("192.168.1.1", "clusterA")); + + when(defaultAlgorithm.getInstance(any(), any())).thenReturn(instances.get(0)); + + // Attach a ListAppender to capture log output + ch.qos.logback.classic.Logger nacosLogger = (ch.qos.logback.classic.Logger) + org.slf4j.LoggerFactory.getLogger(NacosLoadBalancer.class); + ListAppender listAppender = new ListAppender<>(); + listAppender.start(); + nacosLogger.addAppender(listAppender); + + try { + for (int i = 0; i < 10; i++) { + Response response = invokeGetInstanceResponse(instances); + assertThat(response).isInstanceOf(DefaultResponse.class); + assertThat(response.hasServer()).isTrue(); + } + + // Warning should be emitted exactly once despite 10 invocations + long crossClusterWarnCount = listAppender.list.stream() + .filter(e -> e.getLevel() == Level.WARN) + .filter(e -> e.getFormattedMessage().contains("cross-cluster call occurs")) + .count(); + assertThat(crossClusterWarnCount).isEqualTo(1); + assertThat(getLastWarnLogTime()).isGreaterThan(0L); + } + finally { + nacosLogger.detachAppender(listAppender); + } + } + + @Test + public void emptyServiceInstancesShouldReturnEmptyResponse() { + Response response = invokeGetInstanceResponse(Collections.emptyList()); + + assertThat(response).isInstanceOf(EmptyResponse.class); + } + + private Response invokeGetInstanceResponse(List instances) { + try { + java.lang.reflect.Method method = NacosLoadBalancer.class.getDeclaredMethod( + "getInstanceResponse", Request.class, List.class); + method.setAccessible(true); + @SuppressWarnings("unchecked") + Response result = (Response) method.invoke( + nacosLoadBalancer, (Request) null, instances); + return result; + } + catch (Exception e) { + throw new RuntimeException(e); + } + } + + private long getLastWarnLogTime() { + try { + java.lang.reflect.Field field = NacosLoadBalancer.class.getDeclaredField("lastWarnLogTime"); + field.setAccessible(true); + return (long) field.get(nacosLoadBalancer); + } + catch (Exception e) { + throw new RuntimeException(e); + } + } + + private List createInstances(String host, String cluster) { + List instances = new ArrayList<>(); + instances.add(createInstance(host, cluster)); + return instances; + } + + private ServiceInstance createInstance(String host, String cluster) { + Map metadata = new HashMap<>(); + metadata.put("nacos.cluster", cluster); + return new DefaultServiceInstance("instance-" + host, "test-service", host, 8080, false, metadata); + } +}