Copilot commented on code in PR #10487: URL: https://github.com/apache/ozone/pull/10487#discussion_r3417440000
########## hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/scm/proxy/TestSCMFailoverProxyProviderRefresh.java: ########## @@ -0,0 +1,129 @@ +/* + * 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 regarding copyright ownership. + * The ASF licenses this file to You 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 + * + * http://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 org.apache.hadoop.hdds.scm.proxy; + +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_NAMES; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.net.NetUtils; +import org.junit.jupiter.api.Test; + +/** + * Verifies that {@link SCMFailoverProxyProviderBase#refreshProxyAddressIfChanged} + * correctly detects DNS changes and swaps in a fresh {@link SCMProxyInfo} + * when the SCM peer's IP has shifted under a stable hostname (the + * Kubernetes pod-IP-change recovery scenario). + */ +public class TestSCMFailoverProxyProviderRefresh { + + /** + * Build a provider whose only SCM entry deliberately points at a + * stale IP (127.0.0.99). Re-resolving the preserved hostname + * "localhost" yields a different IP (typically 127.0.0.1), so the + * refresh helper must swap in a fresh SCMProxyInfo. + */ + @Test + public void testRefreshSwapsAddressOnIpChange() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + // Single SCM, no service id, hostname "localhost". + conf.set(OZONE_SCM_NAMES, "localhost"); + conf.set(OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY, "localhost:9863"); + + SCMBlockLocationFailoverProxyProvider provider = + new SCMBlockLocationFailoverProxyProvider(conf); + + // Replace the cached entry with a deliberately-stale IP. This + // simulates the state we'd be in if the SCM pod had been + // rescheduled to a new IP after the provider was constructed. + SCMProxyInfo cached = provider.getSCMProxyInfoList().iterator().next(); + String nodeId = cached.getNodeId(); + InetSocketAddress staleAddr = new InetSocketAddress( + InetAddress.getByAddress(new byte[] {127, 0, 0, 99}), + cached.getAddress().getPort()); + provider.replaceProxyInfoForTest(nodeId, + new SCMProxyInfo(cached.getServiceId(), nodeId, staleAddr, + cached.getHostAndPort())); + + boolean swapped = provider.refreshProxyAddressIfChanged(nodeId); + assertTrue(swapped, "refresh must report a swap when DNS now " + + "resolves localhost to an IP different from the stale 127.0.0.99"); + + SCMProxyInfo updated = provider.getSCMProxyInfoList().iterator().next(); + assertNotEquals(staleAddr.getAddress(), updated.getAddress().getAddress(), + "after refresh, cached entry must hold a fresh IP"); + assertEquals(staleAddr.getPort(), updated.getAddress().getPort(), + "port must be preserved across the swap"); + assertEquals("localhost:9863", updated.getHostAndPort(), + "host:port string must survive the swap so future refreshes work"); + } + + /** + * When DNS still returns the cached IP, refreshProxyAddressIfChanged + * is a no-op. This guards against tearing down a healthy proxy on + * every transient blip when the IP is genuinely unchanged. + */ + @Test + public void testRefreshNoopWhenIpUnchanged() throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(OZONE_SCM_NAMES, "localhost"); + conf.set(OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY, "localhost:9863"); Review Comment: `testRefreshNoopWhenIpUnchanged` can be flaky on environments where `localhost` resolves to different addresses across calls (eg IPv4 vs IPv6, or multi-A/AAAA ordering), causing a spurious "swap". Using a numeric loopback address here makes the "unchanged" case deterministic while still validating the no-op behavior. ########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMFailoverProxyProviderBase.java: ########## @@ -260,6 +289,86 @@ public synchronized void close() throws IOException { } } + /** + * Re-resolve the configured hostname for the given SCM nodeId. If DNS + * now returns a different IP, swap in a fresh {@link SCMProxyInfo} + * (with the new resolved address) and discard any cached proxy so the + * next {@link #getProxy()} call dials the new IP. + * + * @return true when a swap occurred; false when the hostname was not + * preserved, the IP is unchanged, the lookup failed, or the + * nodeId is unknown. + */ + @VisibleForTesting + boolean refreshProxyAddressIfChanged(String nodeId) { Review Comment: `refreshProxyAddressIfChanged` is part of the production retry path (invoked from `getRetryPolicy().shouldRetry`), so marking it `@VisibleForTesting` is misleading. Keeping the method package-private is fine, but the annotation should be removed to avoid suggesting it is test-only. ########## hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/scm/proxy/TestSCMFailoverProxyProviderRefreshWired.java: ########## @@ -0,0 +1,147 @@ +/* + * 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 regarding copyright ownership. + * The ASF licenses this file to You 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 + * + * http://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 org.apache.hadoop.hdds.scm.proxy; + +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_NAMES; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.io.IOException; +import java.net.ConnectException; +import java.net.SocketTimeoutException; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.ratis.ServerNotLeaderException; +import org.apache.hadoop.io.retry.RetryPolicy; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Wired-path tests for {@link SCMFailoverProxyProviderBase#getRetryPolicy}'s + * interaction with the connection-class filter and + * {@link SCMFailoverProxyProviderBase#refreshProxyAddressIfChanged}. + * Complements {@code TestConnectionFailureUtils} (helper-in-isolation) + * and {@code TestSCMFailoverProxyProviderRefresh} (per-instance refresh) + * by exercising the actual retry policy whose return value drives the + * RetryInvocationHandler in production. + */ +public class TestSCMFailoverProxyProviderRefreshWired { + + private OzoneConfiguration conf; + + @BeforeEach + public void setUp() { + conf = new OzoneConfiguration(); + conf.set(OZONE_SCM_NAMES, "localhost"); + conf.set(OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY, "localhost:9863"); + } Review Comment: These wired retry tests currently only configure the non-HA SCM keys, which means `SCMNodeInfo.buildNodeInfo(conf)` produces a single dummy SCM entry. With a 1-node "ring", `performFailover` cannot advance, so the tests (especially the pinning test) don't actually validate failover-index behavior. Configure an HA-style SCM serviceId + 2 nodeIds so the ring can advance. ########## hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/scm/proxy/TestSCMFailoverProxyProviderRefreshWired.java: ########## @@ -0,0 +1,147 @@ +/* + * 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 regarding copyright ownership. + * The ASF licenses this file to You 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 + * + * http://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 org.apache.hadoop.hdds.scm.proxy; + +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY; +import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_NAMES; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.io.IOException; +import java.net.ConnectException; +import java.net.SocketTimeoutException; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.ratis.ServerNotLeaderException; +import org.apache.hadoop.io.retry.RetryPolicy; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Wired-path tests for {@link SCMFailoverProxyProviderBase#getRetryPolicy}'s + * interaction with the connection-class filter and + * {@link SCMFailoverProxyProviderBase#refreshProxyAddressIfChanged}. + * Complements {@code TestConnectionFailureUtils} (helper-in-isolation) + * and {@code TestSCMFailoverProxyProviderRefresh} (per-instance refresh) + * by exercising the actual retry policy whose return value drives the + * RetryInvocationHandler in production. + */ +public class TestSCMFailoverProxyProviderRefreshWired { + + private OzoneConfiguration conf; + + @BeforeEach + public void setUp() { + conf = new OzoneConfiguration(); + conf.set(OZONE_SCM_NAMES, "localhost"); + conf.set(OZONE_SCM_BLOCK_CLIENT_ADDRESS_KEY, "localhost:9863"); + } + + /** + * A counting subclass that records each call to + * {@code refreshProxyAddressIfChanged} so the test can assert exactly + * when the wiring fires. + */ + private static final class CountingProvider + extends SCMBlockLocationFailoverProxyProvider { + private int refreshCalls; + + CountingProvider(OzoneConfiguration c) { + super(c); + } + + @Override + boolean refreshProxyAddressIfChanged(String nodeId) { + refreshCalls++; + return false; + } + } + + @Test + public void testSocketTimeoutTriggersRefreshHook() throws Exception { + conf.setBoolean(OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY, true); + CountingProvider provider = new CountingProvider(conf); + RetryPolicy policy = provider.getRetryPolicy(); + policy.shouldRetry(new SocketTimeoutException("EC2 silent drop"), + 0, 0, false); + assertEquals(1, provider.refreshCalls, + "SocketTimeoutException must invoke the refresh hook exactly once"); + } + + @Test + public void testConnectExceptionTriggersRefreshHook() throws Exception { + conf.setBoolean(OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY, true); + CountingProvider provider = new CountingProvider(conf); + RetryPolicy policy = provider.getRetryPolicy(); + policy.shouldRetry( + new IOException("connection refused", new ConnectException()), + 0, 0, false); + assertEquals(1, provider.refreshCalls); + } + + @Test + public void testApplicationLevelErrorDoesNotTriggerRefresh() throws Exception { + conf.setBoolean(OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY, true); + CountingProvider provider = new CountingProvider(conf); + RetryPolicy policy = provider.getRetryPolicy(); + policy.shouldRetry(new ServerNotLeaderException("not the leader"), + 0, 0, false); + assertEquals(0, provider.refreshCalls, + "ServerNotLeaderException is application-level; refresh must NOT fire"); + } + + @Test + public void testFlagDisabledSuppressesRefresh() throws Exception { + conf.setBoolean(OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY, false); + CountingProvider provider = new CountingProvider(conf); + RetryPolicy policy = provider.getRetryPolicy(); + policy.shouldRetry(new ConnectException("refused"), 0, 0, false); + assertEquals(0, provider.refreshCalls, + "with the flag off the refresh hook must never fire"); + } + + /** + * When refresh succeeds, performFailover must stay on the current + * nodeId (via updatedLeaderNodeID) rather than advancing the ring. + */ + @Test + public void testRefreshSuccessPinsCurrentNodeId() throws Exception { + conf.setBoolean(OZONE_CLIENT_FAILOVER_RESOLVE_NEEDED_KEY, true); + SCMBlockLocationFailoverProxyProvider provider = + new SCMBlockLocationFailoverProxyProvider(conf) { + @Override + boolean refreshProxyAddressIfChanged(String nodeId) { + return true; + } + }; + + String beforeNode = provider.getCurrentProxySCMNodeId(); + RetryPolicy policy = provider.getRetryPolicy(); + + // Pre-advance the failover ring with a non-connection-class error. + policy.shouldRetry(new IOException("not-a-connection-failure"), + 0, 0, false); + + policy.shouldRetry(new ConnectException("refused"), 0, 1, false); + provider.performFailover(null); + assertEquals(beforeNode, provider.getCurrentProxySCMNodeId(), + "after a successful refresh, performFailover must stay on the " + + "original nodeId"); + assertNotNull(beforeNode); Review Comment: `testRefreshSuccessPinsCurrentNodeId` currently doesn't advance provider state (it never calls `performFailover` after the first `shouldRetry`), and with the current non-HA config it would only ever have a single SCM entry anyway. After switching the test to a 2-node HA config, update this method to explicitly move to the second node first, then assert that a successful refresh pins the provider on that node rather than advancing again. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
