ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses #2041

showuon · 2023-07-31T11:53:23Z

Found 2 places we will compare the addresses to check if there's any major change, but the addresses comparison will compare hostname + IP address + port. If the DNS is slow, there might be chances that the hostname and port are identical, but the IP address has <unresolved> V.S. x.x.x.x. So the result will be the unexpected unequal.

This PR tried to recreateSocketAddresses before comparing the addresses to fix the issue.

Note: No tests added because it didn't change any logic. Just need to make sure it passes the existing tests.

sonatype-lift · 2023-07-31T11:53:26Z

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

showuon · 2023-07-31T11:55:51Z

@symat @eolivelli , similar to #2040 and #1524. Call for review. Thanks.

ppatierno

Thanks @showuon! I tried it on different environments running Kubernetes (i.e. minikube, AKS, OpenShift, ...) and it worked fine for me!

anmolnar

Thanks @showuon , lgtm.
Have you considered adding a unit test for this?

showuon · 2023-08-13T12:22:46Z

@anmolnar , thanks for the comment. Tests added.

Note: It's not easy to test the slow DNS scenario, so I tried to verify the method is invoked as expected, alternatively.

Thanks.

showuon · 2023-08-28T07:08:53Z

@anmolnar , please take a look when available. Thanks.

…KEEPER-4708_2

showuon · 2023-10-03T06:47:04Z

@anmolnar , please take a look when available. Thanks.

anmolnar · 2023-10-18T12:12:30Z

@showuon You have checkstyle validation error

Error:  /home/runner/work/zookeeper/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LeaderBeanTest.java:43:1: Extra separation in import group before 'org.apache.jute.OutputArchive' [ImportOrder]

showuon · 2023-10-18T12:44:14Z

@showuon You have checkstyle validation error

Error:  /home/runner/work/zookeeper/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LeaderBeanTest.java:43:1: Extra separation in import group before 'org.apache.jute.OutputArchive' [ImportOrder]

@anmolnar , fixed. Please also help backport to 3.8/3.9 branch if no other comments. Thank you very much.

showuon · 2023-10-19T06:09:05Z

The compatibility test failed on java 11 in 3.6.3, but passed on java 8 in 3.6.3. I think it should be fine. Thanks.

anmolnar

nit: In unit tests you don't validate that recreateSocketAddress's return object is not the same as the input. Otherwise lgtm.
Let's wait for another committer @eolivelli @symat @tisonkun @ztzg

showuon · 2023-10-20T03:49:41Z

pom.xml

+      <dependency>
+        <groupId>org.mockito</groupId>
+        <artifactId>mockito-inline</artifactId>
+        <version>${mockito.version}</version>
+      </dependency>


Adding mockito-inline module to support final method mocking (in InetAddress class). See here.

I think that's fine, but I think you should add test scope to it.

<scope>test</scope>

showuon · 2023-10-20T03:52:11Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LeaderBeanTest.java

+        // test case 3: Simulating the DNS returning different IP address for the same hostname during recreation.
+        // After recreateSocketAddresses, the MultipleAddresses should contain the updated IP address instance while other fields unchanged.
+        InetAddress spyInetAddr = Mockito.spy(loopback);
+        InetSocketAddress addr3 = new InetSocketAddress(spyInetAddr, PortAssignment.unique());
+        // Verify the address is the old IP before recreateSocketAddresses.
+        assertEquals(oldIP, addr3.getAddress().getHostAddress());
+        multipleAddresses = new MultipleAddresses(Arrays.asList(addr3));
+        // simulating the DNS returning different IP address
+        when(spyInetAddr.getHostAddress()).thenReturn(newIP);
+
+        // Verify after recreateSocketAddresses, the multipleAddresses should have different IP address result
+        MultipleAddresses newMultipleAddress = leader.recreateSocketAddresses(multipleAddresses);
+        assertNotEquals(multipleAddresses, newMultipleAddress);
+        assertEquals(1, multipleAddresses.getAllAddresses().size());
+        InetSocketAddress newAddr = multipleAddresses.getAllAddresses().iterator().next();
+        // Verify the hostName should still be the same
+        assertEquals(loopback.getHostName(), newAddr.getAddress().getHostName());
+        // Verify the IP address has changed.
+        assertEquals(newIP, newAddr.getAddress().getHostAddress());


@anmolnar , added a test case to verify recreateSocketAddress return object is not the same as the input. Thanks.

lgtm. Thanks!

… into ZOOKEEPER-4708_2

see-quick · 2023-10-23T09:13:49Z

We have encountered an issue affecting over 50% of the test cases within our testing infrastructure. We would greatly appreciate it if we could move forward with addressing this issue.

^^ @eolivelli, @symat, @tisonkun, and @ztzg

I appreciate your attention to this matter in advance.

showuon · 2023-12-05T03:03:06Z

@eolivelli @symat @tisonkun @ztzg, please take a look when available. Thanks.

showuon · 2023-12-13T11:34:38Z

@eolivelli @symat @tisonkun @ztzg, please take a look. Thanks.

showuon · 2024-01-11T12:14:57Z

@eolivelli @symat @tisonkun @ztzg, please take a look. Thanks.

karthick-ponnusamy · 2024-04-10T20:28:26Z

Will this be merged anytime soon? We would greatly appreciate if this was merged as our infrastructure is greatly affected by this issue

clemcvlcs · 2024-08-29T15:00:56Z

@eolivelli @symat @tisonkun @ztzg

This PR is approved since one year now. Any chance you can have a look at this ?

We are also affected by this issue, probably as many other people.

anmolnar · 2024-09-11T16:16:12Z

@clemcvlcs @showuon
Sure, I'm happy to merge it. Please rebase and resolve conflicts.

…KEEPER-4708_2

showuon · 2024-09-12T05:57:39Z

@anmolnar , conflicts resolved. Thanks.

anmolnar · 2024-09-12T15:14:36Z

Merged. Thanks for the contribution @showuon !
I cannot backport to branch-3.9 , because it has conflicts. Please open new PR if you'd like to backport.

ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses

52b2fbc

ppatierno approved these changes Aug 3, 2023

View reviewed changes

anmolnar reviewed Aug 8, 2023

View reviewed changes

ZOOKEEPER-4708: add test

eec320d

showuon force-pushed the ZOOKEEPER-4708_2 branch from c775218 to 8a97b79 Compare August 13, 2023 12:19

ZOOKEEPER-4708: add more test

92dfa5f

showuon force-pushed the ZOOKEEPER-4708_2 branch from 8a97b79 to 92dfa5f Compare August 13, 2023 12:39

showuon added 2 commits August 29, 2023 20:17

Merge branch 'master' of https://github.com/apache/zookeeper into ZOO…

4c2d16f

…KEEPER-4708_2

fix import

38ee164

ztzg force-pushed the master branch from 1c60545 to e2070be Compare October 3, 2023 12:57

showuon mentioned this pull request Oct 17, 2023

ZOOKEEPER-4728: force to re-resolve hostname into IP when binding. #2040

Merged

fix checkstyle

1106c8e

fix checkstyle

894642d

showuon force-pushed the ZOOKEEPER-4708_2 branch from 1106c8e to 894642d Compare October 19, 2023 03:21

anmolnar approved these changes Oct 19, 2023

View reviewed changes

test recreateSocketAddress's return object is not the same as the input

02b4206

showuon commented Oct 20, 2023

View reviewed changes

showuon added 2 commits October 20, 2023 20:17

Merge branch 'ZOOKEEPER-4708_2' of https://github.com/showuon/zookeeper…

8bf14f8

… into ZOOKEEPER-4708_2

add test scope

ff114d2

see-quick approved these changes Oct 23, 2023

View reviewed changes

showuon added 2 commits September 12, 2024 10:12

Merge branch 'master' of https://github.com/apache/zookeeper into ZOO…

925d705

…KEEPER-4708_2

ZOOKEEPER-4708_2: fix checkstyle error

3cf97b6

anmolnar merged commit 34c6d9f into apache:master Sep 12, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses #2041

ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses #2041

showuon commented Jul 31, 2023 •

edited

Loading

sonatype-lift bot commented Jul 31, 2023

showuon commented Jul 31, 2023

ppatierno left a comment

anmolnar left a comment

showuon commented Aug 13, 2023

showuon commented Aug 28, 2023

showuon commented Oct 3, 2023

anmolnar commented Oct 18, 2023

showuon commented Oct 18, 2023

showuon commented Oct 19, 2023

anmolnar left a comment

showuon Oct 20, 2023 •

edited

Loading

anmolnar Oct 20, 2023 •

edited

Loading

showuon Oct 20, 2023

anmolnar Oct 20, 2023

see-quick commented Oct 23, 2023

showuon commented Dec 5, 2023

showuon commented Dec 13, 2023

showuon commented Jan 11, 2024

karthick-ponnusamy commented Apr 10, 2024 •

edited

Loading

clemcvlcs commented Aug 29, 2024

anmolnar commented Sep 11, 2024

showuon commented Sep 12, 2024

anmolnar commented Sep 12, 2024

ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses #2041

ZOOKEEPER-4708: recreateSocketAddresses before comparing addresses #2041

Conversation

showuon commented Jul 31, 2023 • edited Loading

sonatype-lift bot commented Jul 31, 2023

Sonatype Lift is retiring

showuon commented Jul 31, 2023

ppatierno left a comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

showuon commented Aug 13, 2023

showuon commented Aug 28, 2023

showuon commented Oct 3, 2023

anmolnar commented Oct 18, 2023

showuon commented Oct 18, 2023

showuon commented Oct 19, 2023

anmolnar left a comment

Choose a reason for hiding this comment

showuon Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

anmolnar Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

showuon Oct 20, 2023

Choose a reason for hiding this comment

anmolnar Oct 20, 2023

Choose a reason for hiding this comment

see-quick commented Oct 23, 2023

showuon commented Dec 5, 2023

showuon commented Dec 13, 2023

showuon commented Jan 11, 2024

karthick-ponnusamy commented Apr 10, 2024 • edited Loading

clemcvlcs commented Aug 29, 2024

anmolnar commented Sep 11, 2024

showuon commented Sep 12, 2024

anmolnar commented Sep 12, 2024

showuon commented Jul 31, 2023 •

edited

Loading

showuon Oct 20, 2023 •

edited

Loading

anmolnar Oct 20, 2023 •

edited

Loading

karthick-ponnusamy commented Apr 10, 2024 •

edited

Loading