Fix udp port check retry and check all udp ports by ryoqun · Pull Request #10385 · solana-labs/solana

ryoqun · 2020-06-03T09:29:53Z

Problem

Validator starts nevertheless some of its udp port are closed....
Also, it doesn't test all of listening ports.

Summary of Changes

Really abort as failure after the maximum number of retries.
Also, test all the ports.

Context

Found via last-minute checking of #10209

Follow-up #10181, #10291.

ryoqun · 2020-06-03T11:57:19Z

Odd, test started to fail exactly due to this change...

mvines

Thanks! Would you mind adding a new test for this around test_get_public_ip_addr() as well please? Clearly there's not enough test coverage here!

stale · 2020-06-10T16:13:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Pull request has been modified.

ryoqun · 2020-06-11T16:16:25Z


    if let Some(ref cluster_entrypoint) = cluster_entrypoint {
-        let udp_sockets = [
-            node.sockets.tpu.first(),


@mvines Well, this was broken and caused actual CI failures in this pr...
That's because we open many sockets on the same port and there is no guarantee os feeds the echo-backed packets into the first socket. We must recv from all of them.

ah, nice find

ryoqun · 2020-06-11T16:17:47Z

-        let udp_sockets = [
-            node.sockets.tpu.first(),
-            /*
-            Enable these ports when `IpEchoServerMessage` supports more than 4 UDP ports:


I've started to support these as well as a side-effect of addressing same-port-shared sockets.

For this, I only needed just chunks().

ryoqun · 2020-06-11T16:20:24Z

-                    Err(err) => warn!("udp recv failure: {}", err),
-                }
-            });
-            match receiver.recv_timeout(Duration::from_secs(5)) {


Well, I dunno why channel() is needed.. I've simplified it just by socket.set_read_timeout()....

ryoqun · 2020-06-11T16:22:24Z

-            let port = udp_socket.local_addr().unwrap().port();
-            let udp_socket = udp_socket.try_clone().expect("Unable to clone udp socket");
-            let (sender, receiver) = channel();
-            std::thread::spawn(move || {


Well, not join()-ing is a bad taste. I'm pretty sure this leaks a thread or it reads an actual data from the socket at arbitrary time after the validator really start to boot...

Oops, I'll fix this for tcp as well...

ryoqun · 2020-06-11T16:24:41Z

@mvines I wanted to solve the mysterious CI error and it turned out that the port checker is broken in another way by #10291...

I'll adjust the mentioned unit test later..

ryoqun · 2020-06-11T16:31:21Z

+
 #[derive(Serialize, Deserialize, Default)]
 pub(crate) struct IpEchoServerMessage {
-    tcp_ports: [u16; 4], // Fixed size list of ports to avoid vec serde


Let's strive for not breaking ABI. ;)

mvines

looking much better than my previous code :)

mvines · 2020-06-11T16:33:53Z


    if let Some(ref cluster_entrypoint) = cluster_entrypoint {
-        let udp_sockets = [
-            node.sockets.tpu.first(),


ah, nice find

mvines · 2020-06-11T16:34:54Z

-        let udp_sockets = [
-            node.sockets.tpu.first(),
-            /*
-            Enable these ports when `IpEchoServerMessage` supports more than 4 UDP ports:


ryoqun · 2020-06-11T17:39:17Z

+            &node.sockets.repair,
+            &node.sockets.serve_repair,
+        ];
+        udp_sockets.extend(node.sockets.tpu.iter().take(3));


codecov · 2020-06-11T19:03:51Z

Codecov Report

Merging #10385 into master will decrease coverage by 0.0%.
The diff coverage is 95.0%.

@@            Coverage Diff            @@
##           master   #10385     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         297      297             
  Lines       69981    70045     +64     
=========================================
+ Hits        57210    57261     +51     
- Misses      12771    12784     +13

ryoqun · 2020-06-12T16:25:36Z

+            &node.sockets.repair,
+            &node.sockets.serve_repair,
+        ];
+        udp_sockets.extend(node.sockets.tpu.iter());


I'm pretty sure there is more elegant way to write this?

ryoqun · 2020-06-12T17:11:31Z

@mvines I've added tests and polished the impl a bit!!

ryoqun · 2020-06-13T05:44:41Z

Adding test made diff coverage improved from 80% to 95%. :)

mvines

Thanks for all the effort here, this is so much better now

* Don't start if udp port is really closed * Fully check all udp ports * Remove test code....... * Add tests and adjust impl a bit * Add comment * Move comment a bit * Move a bit * clean ups (cherry picked from commit a39df7e) # Conflicts: # validator/src/main.rs

* Don't start if udp port is really closed * Fully check all udp ports * Remove test code....... * Add tests and adjust impl a bit * Add comment * Move comment a bit * Move a bit * clean ups (cherry picked from commit a39df7e)

automerge

ryoqun requested a review from mvines June 3, 2020 09:29

mvines previously approved these changes Jun 3, 2020

View reviewed changes

mvines added v1.1 labels Jun 3, 2020

stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 10, 2020

ryoqun added 2 commits June 12, 2020 01:08

Don't start if udp port is really closed

4ff3b72

Fully check all udp ports

8f30ba5

ryoqun force-pushed the no-start-with-closed-port branch from fdd0d05 to 8f30ba5 Compare June 11, 2020 16:13

stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 11, 2020

ryoqun commented Jun 11, 2020

View reviewed changes

mvines reviewed Jun 11, 2020

View reviewed changes

ryoqun changed the title ~~Don't start if udp port is really closed~~ Fix udp port check retry and check all udp ports Jun 11, 2020

ryoqun commented Jun 11, 2020

View reviewed changes

Remove test code.......

77d2a6f

Add tests and adjust impl a bit

f44b9c8

ryoqun commented Jun 12, 2020

View reviewed changes

ryoqun added 4 commits June 13, 2020 01:34

Add comment

89836be

Move comment a bit

9a7787a

Move a bit

7ac5ee5

clean ups

f792056

ryoqun requested a review from mvines June 12, 2020 17:11

mvines approved these changes Jun 13, 2020

View reviewed changes

ryoqun merged commit a39df7e into solana-labs:master Jun 14, 2020

mergify Bot mentioned this pull request Jun 14, 2020

Fix udp port check retry and check all udp ports (bp #10385) #10576

Merged

mergify Bot mentioned this pull request Jun 14, 2020

Fix udp port check retry and check all udp ports (bp #10385) #10577

Merged

solana-grimes pushed a commit that referenced this pull request Jun 15, 2020

Fix udp port check retry and check all udp ports (bp #10385) (#10576)

bc808d7

automerge

solana-grimes pushed a commit that referenced this pull request Jun 15, 2020

Fix udp port check retry and check all udp ports (#10385) (#10577)

133baa8

automerge

Conversation

ryoqun commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Context

Uh oh!

ryoqun commented Jun 3, 2020

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Jun 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Jun 12, 2020

Uh oh!

ryoqun commented Jun 13, 2020

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryoqun commented Jun 3, 2020 •

edited

Loading

ryoqun Jun 11, 2020 •

edited

Loading

codecov Bot commented Jun 11, 2020 •

edited

Loading