poppy-project / pypot

Python library for controlling dynamixel motors. Documentation available here:
https://docs.poppy-project.org/en/software-libraries/pypot.html
GNU General Public License v3.0
249 stars 137 forks source link

Report correct port even when dynamically allocated #297

Open davidjsherman opened 3 years ago

davidjsherman commented 3 years ago

The services launcher reports the requested port regardless of which port was actually allocated by the operating system.

When launching several service instances on the same machine, it is better to allow the OS to dynamically choose a port from the IANA ephemeral range, than to guess at what port numbers haven't been used. Tornado will happily bind an ephemeral port if port 0 is requested.

To verify that this works with poppy-services, run

$ poppy-services --poppy-simu --http --http-port 0 poppy-ergo-jr &
$ sleep 10 ; ss -tlp
State  Recv-Q Send-Q Local Address:Port   Peer Address:Port                                         
LISTEN 0      128          0.0.0.0:42961       0.0.0.0:*     users:(("poppy-services",pid=70,fd=7)) 
LISTEN 0      128             [::]:42961          [::]:*     users:(("poppy-services",pid=70,fd=8)) 

The services launcher should retrieve the list of bound sockets from Tornado, instead of reporting the port that was requested https://github.com/poppy-project/pypot/blob/3cab4f46fb09ece9fbdd663b8bdce6e251a145e5/pypot/creatures/services_launcher.py#L218

davidjsherman commented 3 years ago

I'm not sure where to put the regression test for this. I suppose ci/run_tests.sh?

So, as a shell fragment:

bash -c 'stdbuf -o0 poppy-services --poppy-simu --http --http-port 0 poppy-ergo-jr & pid=$! ; \
    sleep 20 ; \
    echo real=$(ss -tlp | grep pid=$pid | perl -ne '\''print $p if (($p)=($_=~m/0\.0\.0\.0:(\d+)/g))'\'') ; \
    kill $pid' | 
perl -ne 'END{exit($X{port}!=$X{real})} $X{$k}=$v if (($k,$v)=($_=~m/(port|real)=(\d+)/g))' && 
echo PORTS AGREE || echo PORTS MISMATCH

🙄

davidjsherman commented 3 years ago

This would be easier if you had a clirunner to capture the program output!

ymollard commented 3 years ago

Is it a recommendation or have you met a situation in which this port display was problematic? When using 0 to open many robot instances, maybe?

In order to implement a test, I would go for a dedicated test framework instead of custom bash code in the CI script. Then the CI script would invoke that framework. Maybe a good occasion to migrate to the clirunner and then implement the tests with the unittests framework from Python? 0:)

davidjsherman commented 3 years ago

There are two cases I have run into.

  1. When running independent tests in parallel on the same machine, each with a simulator as a test fixture. It would be easier to rely on the fixture for the port number, than to discover the allocated port through an OS-specific mechanism.
  2. When running a pool of simulated robots on a computer that has other services running in the 8080 range. Allowing dynamic port allocation makes it unnecessary to globally negotiate a port range with the other users.

Dynamic allocation does work, so reporting should be correct. If I start ten simulators with port 0, each reports the requested port as “0”

for i in $(yes|head -10); do poppy-services --poppy-simu --http --http-port 0 poppy-ergo-jr & sleep 2; done
[1] 75783
Attempt 1 to start the robot...
Simulated robot created! He is running on: ip=10.0.0.109
Server started on: http_port=0.
[2] 75785
Attempt 1 to start the robot...
Simulated robot created! He is running on: ip=10.0.0.109
Server started on: http_port=0.
[3] 75788
Attempt 1 to start the robot...
Simulated robot created! He is running on: ip=10.0.0.109
Server started on: http_port=0.
[...]

but each process is listening on a real port allocated from the ephemeral range

$ lsof -i4 -sTCP:LISTEN -n -P -R | grep ^Python | cat -n
     1  Python    75783 75505 sherman   10u  IPv4 0x8087a0bd505de48b      0t0  TCP *:52978 (LISTEN)
     2  Python    75785 75505 sherman    8u  IPv4 0x8087a0bd50fef123      0t0  TCP *:52979 (LISTEN)
     3  Python    75788 75505 sherman    8u  IPv4 0x8087a0bd510e5aab      0t0  TCP *:52981 (LISTEN)
     4  Python    75790 75505 sherman   10u  IPv4 0x8087a0bd3d1c8aab      0t0  TCP *:52982 (LISTEN)
     5  Python    75792 75505 sherman    8u  IPv4 0x8087a0bd50f7548b      0t0  TCP *:52984 (LISTEN)
     6  Python    75794 75505 sherman    8u  IPv4 0x8087a0bd5056a79b      0t0  TCP *:52986 (LISTEN)
     7  Python    75796 75505 sherman    8u  IPv4 0x8087a0bd510ea79b      0t0  TCP *:52987 (LISTEN)
     8  Python    75798 75505 sherman   11u  IPv4 0x8087a0bd510eb123      0t0  TCP *:52988 (LISTEN)
     9  Python    75801 75505 sherman   10u  IPv4 0x8087a0bd4ba7d79b      0t0  TCP *:52989 (LISTEN)
    10  Python    75803 75505 sherman    8u  IPv4 0x8087a0bd504d8aab      0t0  TCP *:52990 (LISTEN)

so it is wrong to report the requested port rather than the allocated port.

davidjsherman commented 3 years ago

We have cookiecutter templates that set up tox environments for testing new Python projects. Out of the box our configurations use pytest / pytest-bdd rather than unittest and discover.

I can see whether our environment could be adapted. Theoretically, tox can use unittest2 and discover, but I have no experience with that.