jmx-dev [ding] Re: [pong] Re: [ping] Re: RFR 8146015: JMXInterfaceBindingTest is failing intermittently for IPv6 addresses
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Fri Jan 15 08:50:58 UTC 2016
On 14.1.2016 17:23, Severin Gehwolf wrote:
> 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.
You can have problems also with a configuration with several loopbacks. The test considers such a setup as a machine with multiple interfaces while it is, in fact, only one interface. Also, it does not really make any sense to test binding to a loopback address - we are starting a remote JMX connector and loopback address is not accessible from outside, anyway.
>
> 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 :-/
Yep. But feel free to open a new issue, with the reproducer description, and, hopefully a proposed patch :) I will sponsor the push happily.
-JB-
>
> 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