GEODE-8772: Make tests assign ports #5935
Conversation
Some tests in ManagementRequestLoggingDistributedTest and RestAPIsAndInterOpsDUnitTest were using the default JMX port (1099). Some tests in TombstoneDUnitTest were using the default cache server port (40404). When these tests run in parallel outside of docker, they can fail to bind these ports if another test has already bound them. These tests now assign ports via AvailablePortHelper. Authored-by: Dale Emery <demery@vmware.com>
demery-pivotal
requested review from
bschuchardt,
dschneider-pivotal,
gesterzhou and
rhoughton-pivot
as code owners
| Properties props = new Properties(); | ||
| props.setProperty(MCAST_PORT, String.valueOf(0)); | ||
| props.setProperty(LOCATORS, locators); | ||
|
|
||
| props.setProperty(JMX_MANAGER, "true"); | ||
| props.setProperty(JMX_MANAGER_START, "true"); | ||
| props.setProperty(JMX_MANAGER_PORT, "0"); | ||
| props.setProperty(JMX_MANAGER_PORT, String.valueOf(jmxManagerPort)); |
There was a problem hiding this comment.
In this case you changed from "0" to using getRandomAvailableTCPPorts. I would have thought "0" is even better because using getRandomAvailableTCPPorts has a small race in which the port it hands back could end up being used by another before this thread can bind to it. I thought "0" was better because we are telling the operating system to give us an available port at the time we bind to it so we get rid of the race window. It can be hard to do in some tests when they need to know ahead of time what the actual port is. Do you know using "0" causes test issues?
There was a problem hiding this comment.
That edit might be a mistake. My intention in this PR was to fix only uses of default ports.
Note: For this property (according to the docs) 0 means don't allow remote client connections. I'll have to look closer at what happens when 0 is specified here.
Other note: Using ephemeral ports can reduce the frequency of the race, but it doesn't eliminate it, because Geode's membership port range overlaps the ephemeral port range. So here's a troubling sequence:
- Component A "reserves" a port in the membership port range.
- Component B (perhaps running in a separate test, a separate member, or even a non-Geode process) binds an ephemeral port, and gets the "reserved" port.
- Component A tries to bind the port it "reserved" and fails.
I'm not sure how often this troubling sequence happens, but it does happen. Requesting a non-membership port (via AvailablePortHelper) helps by assigning ports outside the ephemeral port range.
There was a problem hiding this comment.
I think this change is unnecessary and I've reverted it. Thanks for catching that.
Fixing an inadvertent change of a non-default port.
| @@ -168,6 +168,8 @@ private CacheServer createRegionAndStartCacheServer(String[] regions, Cache cach | |||
| } | |||
|
|
|||
| private int startManager(final String locators, final String[] regions) throws IOException { | |||
| int httpPort = getRandomAvailableTCPPort(); | |||
There was a problem hiding this comment.
this seems like it could be final...
Some tests in ManagementRequestLoggingDistributedTest and
RestAPIsAndInterOpsDUnitTest were using the default JMX port (1099).
Some tests in TombstoneDUnitTest were using the default cache server
port (40404). When these tests run in parallel outside of docker, they
can fail to bind these ports if another test has already bound them.
These tests now assign ports via AvailablePortHelper.
Authored-by: Dale Emery demery@vmware.com