jmx-dev [ding] Re: [pong] Re: [ping] Re: RFR 8146015: JMXInterfaceBindingTest is failing intermittently for IPv6 addresses
Severin Gehwolf
sgehwolf at redhat.com
Thu Jan 14 16:23:07 UTC 2016
Hi,
On Wed, 2016-01-13 at 10:51 -0800, serguei.spitsyn at oracle.com wrote:
> On 1/13/16 03:16, Jaroslav Bachorik wrote:
> > On 12.1.2016 12:55, serguei.spitsyn at oracle.com wrote:
> > > On 1/12/16 03:49, Jaroslav Bachorik wrote:
> > > > On 12.1.2016 11:47, serguei.spitsyn at oracle.com wrote:
> > > > > On 1/7/16 08:40, Daniel Fuchs wrote:
> > > > > > Hi,
> > > > > >
> > > > > > This looks OK to me.
> > > > > > I'm not sure I understand the full impact of the changes
> > > > > > in getAddressesForLocalHost() though - so hopefully someone
> > > > > > else will jump in to review that part...
> > > > >
> > > > > Hi Jaroslav,
> > > > >
> > > > > I do not understand the full impact of the getAddressesForLocalHost()
> > > > > change either
> > > > > so that, please, do not count me as a reviewer.
> > > >
> > > > Looks like
> > > >
> > > > >
> > > > > However, I have a question on the fragment:
> > > > >
> > > > > + private static boolean isLocalhost(InetAddress i) {
> > > > > + if (!i.isLoopbackAddress()) {
> > > > > + return i.getHostName().toLowerCase().equals("localhost");
> > > > > + }
> > > > > + return false;
> > > > > }
> > > > >
> > > > >
> > > > > Should true be returned instead of false if the i.isLoopbackAddress()
> > > > > returns true?
> > > > > Do we normally treat the loopbackAddress case as a localhost variant?
> > > >
> > > > Ok, maybe the method name is missing a bit - the idea is to get all
> > > > 'true' localhosts - a localhost defined on a real non-loopback adapter
> > > > (as it doesn't make sense to bind JMX remote connector to a loopback
> > > > address).
> > >
> > > Got it.
> > > Thank you for the explanation.
> > >
> > >
> > > >
> > > > I just couldn't come up with more describing name not being
> > > > excessively long :( I'm open to any suggestions.
> > >
> > > The name looks Ok to me.
> > > I'd suggest to add a short comment with your explanation above.
> >
> > Thanks. I've added the explanation and also disambiguated the method
> > name a bit.
> >
> > http://cr.openjdk.java.net/~jbachorik/8146015/webrev.01
> >
> > It looks like no one else is going to jump in and verify the
> > getAddressesForLocalHost() change (basically, excluding the loopback
> > localhost addresses as they are not suitable for binding the remote
> > JMX connector to) :/ Maybe Severing could comment, as the orignal author?
Sorry for being late in the game with this.
+ private static List<InetAddress> getAddressesForLocalHost() {
+
try {
- addrs = InetAddress.getAllByName("localhost");
- } catch (UnknownHostException e) {
+ return NetworkInterface.networkInterfaces()
+ .flatMap(NetworkInterface::inetAddresses)
+ .filter(JMXInterfaceBindingTest::isNonloopbackLocalhost)
+ .collect(Collectors.toList());
I wonder if this does what you claim it does. It looks like it's
getting all the addresses per adapter (IPv4 or IPv6) and then filters
addresses out which have "localhost" as name and aren't loopback. It's
not really what you said it does:
""""
The fix adds the requested wrapping for IPv6 addresses and adjusts the
IP selection logic to iterate over distinct adapters first and prevent
IPv4 and IPv6 address of the same adapter being treated as two
addresses
""""
How is it preventing IPv4 and IPv6 addresses *both* being used for one
adapter? Could it be that this fix works because loopback was the only
one with IPv4 and IPv6 address config on the test server?
Say one has the following /etc/hosts config:
192.168.1.14 localhost
fe80::56ee:75ff:fe35:d1d4 localhost
with the following adapter info (from ifconfig):
enp0s25: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
inet 192.168.1.14 netmask 255.255.255.0 broadcast 192.168.1.255
inet6 fe80::56ee:75ff:fe35:d1d4
I.e. for adapter enp0s25 I specify the IP addresses 192.168.1.14 and
fe80::56ee:75ff:fe35:d1d4 to be "localhost". I'd expect the test to
still fail after this patch.
+ // we need 'real' localhost addresses only (eg. not loopback ones)
+ // so we can bind the remote JMX connector to them
+ private static boolean isNonloopbackLocalhost(InetAddress i) {
+ if (!i.isLoopbackAddress()) {
+ return i.getHostName().toLowerCase().equals("localhost");
+ }
+ return false;
TBH, I don't understand why this comment is there. Particularly, the
loopback exclusion. Yet, this may be platform specific. It worked for
me under linux. It was fine with binding to IPv4 loopback (127.0.0.1)
and some interface address (e.g. 192.168.1.x). Looking at the bug it
seems the main issue was a conflict of binding to IPv4 loopback + IPv6
loopback.
So the gist would be to not use both IPv4 *and* IPv6 addresses of the
same adapter for the binding. How about using something like this for a
filter:
private static boolean isIpV4Address(InetAddress i) {
return i instanceof Inet4Address;
}
Thoughts?
Anyway, looks like it's too late now :-/
Cheers,
Severin
> > I would like to integrate this change ASAP since the test is failing
> > 100% in nightly runs.
>
> Ok. I looked to the getAddressesForLocalHost() part and think this fix
> is fine.
> Thumbs up from me.
>
> Thanks,
> Serguei
>
> >
> > -JB-
> >
> > >
> > >
> > > Thanks,
> > > Serguei
> > >
> > >
> > > >
> > > > -JB-
> > > >
> > > > >
> > > > > Thanks,
> > > > > Serguei
> > > > >
> > > > >
> > > > > >
> > > > > > best regards,
> > > > > >
> > > > > > -- daniel
> > > > > >
> > > > > > On 07/01/16 16:02, Jaroslav Bachorik wrote:
> > > > > > > On 5.1.2016 15:30, Jaroslav Bachorik wrote:
> > > > > > > > On 4.1.2016 10:05, Jaroslav Bachorik wrote:
> > > > > > > > > Gentle reminder ...
> > > > > > > > >
> > > > > > > > > On 23.12.2015 11:26, Jaroslav Bachorik wrote:
> > > > > > > > > > Please, review the following test change
> > > > > > > > > >
> > > > > > > > > > Issue : https://bugs.openjdk.java.net/browse/JDK-8146015
> > > > > > > > > > Webrev: http://cr.openjdk.java.net/~jbachorik/8146015/webrev.00
> > > > > > > > > >
> > > > > > > > > > The test fails for IPv6 addresses since the RMI expects an IPv6
> > > > > > > > > > address
> > > > > > > > > > to be properly wrapped in '[]'. In addition to that the logic for
> > > > > > > > > > selecting IP addresses to bind is flawed - it does not check
> > > > > > > > > > for IP
> > > > > > > > > > addresses of multiple adapters but for multiple IP addresses
> > > > > > > > > > assigned to
> > > > > > > > > > 'localhost'. In combination with IPv4 & IPv6 this will cause the
> > > > > > > > > > test to
> > > > > > > > > > attempt binding to IPv4 and IPv6 address of the same adapter
> > > > > > > > > > simultaneously and the test will fail.
> > > > > > > > > >
> > > > > > > > > > The fix adds the requested wrapping for IPv6 addresses and
> > > > > > > > > > adjusts
> > > > > > > > > > the
> > > > > > > > > > IP selection logic to iterate over distinct adapters first and
> > > > > > > > > > prevent
> > > > > > > > > > IPv4 and IPv6 address of the same adapter being treated as two
> > > > > > > > > > addresses
> > > > > > > > > > (for the purposes of the test).
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > > -JB-
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
More information about the serviceability-dev
mailing list