Apache Ignite Developers - Legacy Mail Archive

IP finder in tests

Classic

List

Threaded

14 messages Options

Denis Mekhanikov

IP finder in tests

Igniters,

Almost every test in Ignite project has the following pattern: shared
*TcpDiscoveryVmIpFinder
*is defined as a field of a test class, which is then used in discovery
configuration in *getConfiguration(...)* method. There are more than 700
test classes with this setup.

But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests by
default. I don't think, that it should be used in tests at all, since
external nodes may accidentally affect test results.

The only case, where it makes sense is multi JVM tests. Shared static IP
finder is not applicable there, since nodes are run in different JVMs and
cannot share the same IP finder object.

I would like to change the default IP finder to a shared
*TcpDiscoveryVmIpFinder.
*In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we could
fall back to multicast.

What do you think?

Denis

Ilya Kasnacheev

Re: IP finder in tests

Hello!

I think it's a sensible decision that is long overdue.

Regards,

--
Ilya Kasnacheev

2018-08-01 13:22 GMT+03:00 Denis Mekhanikov <[hidden email]>:

> Igniters,
>
> Almost every test in Ignite project has the following pattern: shared
> *TcpDiscoveryVmIpFinder
> *is defined as a field of a test class, which is then used in discovery
> configuration in *getConfiguration(...)* method. There are more than 700
> test classes with this setup.
>
> But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests by
> default. I don't think, that it should be used in tests at all, since
> external nodes may accidentally affect test results.
>
> The only case, where it makes sense is multi JVM tests. Shared static IP
> finder is not applicable there, since nodes are run in different JVMs and
> cannot share the same IP finder object.
>
> I would like to change the default IP finder to a shared
> *TcpDiscoveryVmIpFinder.
> *In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we could
> fall back to multicast.
>
> What do you think?
>
> Denis
>

Dmitriy Pavlov

Re: IP finder in tests

In reply to this post by Denis Mekhanikov

Hi Denis,

Thank you for bringing the question here.

I totally support this change.

Sincerely,
Dmitriy Pavlov

ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:

Pavel Kovalenko

Re: IP finder in tests

Hi Denis,

I also definitely support this change. As an engineer who tightly working
with MakeTeamCityGreenAgain activity, I notice several test suites hanging
related with MulticastIpFinder infinite loops on datagram receiving. Here
is last recorded fail -
https://ci.ignite.apache.org/viewLog.html?buildId=1544659&tab=buildResultsDiv&buildTypeId=IgniteTests24Java8_BinaryObjectsSimpleMapperBasic
.

Changing default ipFinder in tests to VM will help with TeamCity stability.

2018-08-01 14:20 GMT+03:00 Dmitriy Pavlov <[hidden email]>:

> Hi Denis,
>
> Thank you for bringing the question here.
>
> I totally support this change.
>
> Sincerely,
> Dmitriy Pavlov
>
> ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:
>
> > Igniters,
> >
> > Almost every test in Ignite project has the following pattern: shared
> > *TcpDiscoveryVmIpFinder
> > *is defined as a field of a test class, which is then used in discovery
> > configuration in *getConfiguration(...)* method. There are more than 700
> > test classes with this setup.
> >
> > But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests by
> > default. I don't think, that it should be used in tests at all, since
> > external nodes may accidentally affect test results.
> >
> > The only case, where it makes sense is multi JVM tests. Shared static IP
> > finder is not applicable there, since nodes are run in different JVMs and
> > cannot share the same IP finder object.
> >
> > I would like to change the default IP finder to a shared
> > *TcpDiscoveryVmIpFinder.
> > *In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we could
> > fall back to multicast.
> >
> > What do you think?
> >
> > Denis
> >
>

Stanislav Lukyanov

RE: IP finder in tests

In reply to this post by Dmitriy Pavlov

+1.

I don’t see why do we need to fallback to multicast for multi-JVM – let’s just set 127.0.0.1:47500..47509 by default,
it’ll be enough for most (if not all) tests.

Stan

From: Dmitriy Pavlov
Sent: 1 августа 2018 г. 14:21
To: [hidden email]
Subject: Re: IP finder in tests

Hi Denis,

Thank you for bringing the question here.

I totally support this change.

Sincerely,
Dmitriy Pavlov

ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:

Alexey Zinoviev

Re: IP finder in tests

Great, sometimes I made it manually in local tests. It will be great to
change the situation.

2018-08-01 18:22 GMT+06:00 Stanislav Lukyanov <[hidden email]>:

> +1.
>
> I don’t see why do we need to fallback to multicast for multi-JVM – let’s
> just set 127.0.0.1:47500..47509 by default,
> it’ll be enough for most (if not all) tests.
>
> Stan
>
> From: Dmitriy Pavlov
> Sent: 1 августа 2018 г. 14:21
> To: [hidden email]
> Subject: Re: IP finder in tests
>
> Hi Denis,
>
> Thank you for bringing the question here.
>
> I totally support this change.
>
> Sincerely,
> Dmitriy Pavlov
>
> ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:
>
> > Igniters,
> >
> > Almost every test in Ignite project has the following pattern: shared
> > *TcpDiscoveryVmIpFinder
> > *is defined as a field of a test class, which is then used in discovery
> > configuration in *getConfiguration(...)* method. There are more than 700
> > test classes with this setup.
> >
> > But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests by
> > default. I don't think, that it should be used in tests at all, since
> > external nodes may accidentally affect test results.
> >
> > The only case, where it makes sense is multi JVM tests. Shared static IP
> > finder is not applicable there, since nodes are run in different JVMs and
> > cannot share the same IP finder object.
> >
> > I would like to change the default IP finder to a shared
> > *TcpDiscoveryVmIpFinder.
> > *In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we could
> > fall back to multicast.
> >
> > What do you think?
> >
> > Denis
> >
>
>

Dmitrii Ryabov

Re: IP finder in tests

We have ticket [1] for this change.

[1] https://issues.apache.org/jira/browse/IGNITE-6826

2018-08-01 15:58 GMT+03:00 Alexey Zinoviev <[hidden email]>:

> Great, sometimes I made it manually in local tests. It will be great to
> change the situation.
>
> 2018-08-01 18:22 GMT+06:00 Stanislav Lukyanov <[hidden email]>:
>
> > +1.
> >
> > I don’t see why do we need to fallback to multicast for multi-JVM – let’s
> > just set 127.0.0.1:47500..47509 by default,
> > it’ll be enough for most (if not all) tests.
> >
> > Stan
> >
> > From: Dmitriy Pavlov
> > Sent: 1 августа 2018 г. 14:21
> > To: [hidden email]
> > Subject: Re: IP finder in tests
> >
> > Hi Denis,
> >
> > Thank you for bringing the question here.
> >
> > I totally support this change.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:
> >
> > > Igniters,
> > >
> > > Almost every test in Ignite project has the following pattern: shared
> > > *TcpDiscoveryVmIpFinder
> > > *is defined as a field of a test class, which is then used in discovery
> > > configuration in *getConfiguration(...)* method. There are more than
> 700
> > > test classes with this setup.
> > >
> > > But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests by
> > > default. I don't think, that it should be used in tests at all, since
> > > external nodes may accidentally affect test results.
> > >
> > > The only case, where it makes sense is multi JVM tests. Shared static
> IP
> > > finder is not applicable there, since nodes are run in different JVMs
> and
> > > cannot share the same IP finder object.
> > >
> > > I would like to change the default IP finder to a shared
> > > *TcpDiscoveryVmIpFinder.
> > > *In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we
> could
> > > fall back to multicast.
> > >
> > > What do you think?
> > >
> > > Denis
> > >
> >
> >
>

Dmitriy Pavlov

Re: IP finder in tests

Hi Dmitriy,

I guess this ticket relates to examples, but new proposal is related only
to test.

Community didn't make an agreement about examples change, so I suggest to
create new ticket (and probably link both tickets) and PR.

Sincerely,
Dmitriy Pavlov

ср, 1 авг. 2018 г. в 20:30, Dmitrii Ryabov <[hidden email]>:

> We have ticket [1] for this change.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-6826
>
> 2018-08-01 15:58 GMT+03:00 Alexey Zinoviev <[hidden email]>:
>
> > Great, sometimes I made it manually in local tests. It will be great to
> > change the situation.
> >
> > 2018-08-01 18:22 GMT+06:00 Stanislav Lukyanov <[hidden email]>:
> >
> > > +1.
> > >
> > > I don’t see why do we need to fallback to multicast for multi-JVM –
> let’s
> > > just set 127.0.0.1:47500..47509 by default,
> > > it’ll be enough for most (if not all) tests.
> > >
> > > Stan
> > >
> > > From: Dmitriy Pavlov
> > > Sent: 1 августа 2018 г. 14:21
> > > To: [hidden email]
> > > Subject: Re: IP finder in tests
> > >
> > > Hi Denis,
> > >
> > > Thank you for bringing the question here.
> > >
> > > I totally support this change.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > ср, 1 авг. 2018 г. в 13:23, Denis Mekhanikov <[hidden email]>:
> > >
> > > > Igniters,
> > > >
> > > > Almost every test in Ignite project has the following pattern: shared
> > > > *TcpDiscoveryVmIpFinder
> > > > *is defined as a field of a test class, which is then used in
> discovery
> > > > configuration in *getConfiguration(...)* method. There are more than
> > 700
> > > > test classes with this setup.
> > > >
> > > > But for some reason *TcpDiscoveryMulticastIpFinder *is used in tests
> by
> > > > default. I don't think, that it should be used in tests at all, since
> > > > external nodes may accidentally affect test results.
> > > >
> > > > The only case, where it makes sense is multi JVM tests. Shared static
> > IP
> > > > finder is not applicable there, since nodes are run in different JVMs
> > and
> > > > cannot share the same IP finder object.
> > > >
> > > > I would like to change the default IP finder to a shared
> > > > *TcpDiscoveryVmIpFinder.
> > > > *In cases, when *GridAbstractTest#isMultiJvm() *returns *true*, we
> > could
> > > > fall back to multicast.
> > > >
> > > > What do you think?
> > > >
> > > > Denis
> > > >
> > >
> > >
> >
>

yzhdanov

Re: IP finder in tests

I disagree. Probably, no change required. Each TC agent use own multicast
group so nodes do not intersect. If any of the test does not properly clean
up and leaves nodes running this dhould be flagged as test fail which is
the case.

Please provide strong reasons to start with this.

--Yakov

Dmitriy Pavlov

Re: IP finder in tests

Hi Yakov,

Regarding Each TC agent use own multicast: I'm not sure it is true, TC
admins tried to do so, but not succeded.

One more reason is speed of tests run. Why do we need to scan something if
we always will connect localhost. TC tests do not use multicast in almost
every test.

Sincerely,
Dmitriy Pavlov

чт, 2 авг. 2018 г. в 0:27, Yakov Zhdanov <[hidden email]>:

> I disagree. Probably, no change required. Each TC agent use own multicast
> group so nodes do not intersect. If any of the test does not properly clean
> up and leaves nodes running this dhould be flagged as test fail which is
> the case.
>
> Please provide strong reasons to start with this.
>
> --Yakov
>

Yakov Zhdanov-2

Re: IP finder in tests

It should be true, otherwise we would have nodes from all agents
intersecting. No?

And multicast IP finder is the defailt one, so I would not reduce its test
volume.

Yakov Zhdanov
www.gridgain.com

2018-08-02 0:32 GMT+03:00 Dmitriy Pavlov <[hidden email]>:

> Hi Yakov,
>
> Regarding Each TC agent use own multicast: I'm not sure it is true, TC
> admins tried to do so, but not succeded.
>
> One more reason is speed of tests run. Why do we need to scan something if
> we always will connect localhost. TC tests do not use multicast in almost
> every test.
>
> Sincerely,
> Dmitriy Pavlov
>
> чт, 2 авг. 2018 г. в 0:27, Yakov Zhdanov <[hidden email]>:
>
> > I disagree. Probably, no change required. Each TC agent use own multicast
> > group so nodes do not intersect. If any of the test does not properly
> clean
> > up and leaves nodes running this dhould be flagged as test fail which is
> > the case.
> >
> > Please provide strong reasons to start with this.
> >
> > --Yakov
> >
>

Pavel Kovalenko

Re: IP finder in tests

Hi Yakov,

Currently TC agents defended by Docker virtual network, that's why we don't
see intersection between several clusters, but in case of any step aside
(running several suites on one agent, running several tests on one machine
and so on) we will have problems and return back to this conversation.
I'm voting for simplifying and speeding up testing process. It will also
reduce the number of copy-paste in ton of tests, where Vm Ip Finder is used
explicitly. As developer I'm confusing when I see in a test VmIpFinder and
in other test Multicast without any reason or comment.
If you care about test coverage of MulticastIpFinder you can pick several
suites where number of starting/stopping is most frequent and leave
multicast there, but in general it's not necessary to have it everywhere.

2018-08-02 0:54 GMT+03:00 Yakov Zhdanov <[hidden email]>:

> It should be true, otherwise we would have nodes from all agents
> intersecting. No?
>
> And multicast IP finder is the defailt one, so I would not reduce its test
> volume.
>
> Yakov Zhdanov
> www.gridgain.com
>
> 2018-08-02 0:32 GMT+03:00 Dmitriy Pavlov <[hidden email]>:
>
> > Hi Yakov,
> >
> > Regarding Each TC agent use own multicast: I'm not sure it is true, TC
> > admins tried to do so, but not succeded.
> >
> > One more reason is speed of tests run. Why do we need to scan something
> if
> > we always will connect localhost. TC tests do not use multicast in almost
> > every test.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > чт, 2 авг. 2018 г. в 0:27, Yakov Zhdanov <[hidden email]>:
> >
> > > I disagree. Probably, no change required. Each TC agent use own
> multicast
> > > group so nodes do not intersect. If any of the test does not properly
> > clean
> > > up and leaves nodes running this dhould be flagged as test fail which
> is
> > > the case.
> > >
> > > Please provide strong reasons to start with this.
> > >
> > > --Yakov
> > >
> >
>

Denis Mekhanikov

Re: IP finder in tests

Yakov,

Almost every test in the project uses the Vm IP finder anyway.
It has become a convention to use it in all tests.
So, I'm trying to reduce the amount of copy-pasted code and improve test's
isolation.

Also tests may be run outside TeamCity during development process.
Multicast Ip Finder makes developers disconnect from their networks to
guarantee, that no other nodes will get in the way.
So, I don't see any advantages of multicast IP finder over Vm in context of
tests.

Denis

чт, 2 авг. 2018 г. в 1:46, Pavel Kovalenko <[hidden email]>:

> Hi Yakov,
>
> Currently TC agents defended by Docker virtual network, that's why we don't
> see intersection between several clusters, but in case of any step aside
> (running several suites on one agent, running several tests on one machine
> and so on) we will have problems and return back to this conversation.
> I'm voting for simplifying and speeding up testing process. It will also
> reduce the number of copy-paste in ton of tests, where Vm Ip Finder is used
> explicitly. As developer I'm confusing when I see in a test VmIpFinder and
> in other test Multicast without any reason or comment.
> If you care about test coverage of MulticastIpFinder you can pick several
> suites where number of starting/stopping is most frequent and leave
> multicast there, but in general it's not necessary to have it everywhere.
>
> 2018-08-02 0:54 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>
> > It should be true, otherwise we would have nodes from all agents
> > intersecting. No?
> >
> > And multicast IP finder is the defailt one, so I would not reduce its
> test
> > volume.
> >
> > Yakov Zhdanov
> > www.gridgain.com
> >
> > 2018-08-02 0:32 GMT+03:00 Dmitriy Pavlov <[hidden email]>:
> >
> > > Hi Yakov,
> > >
> > > Regarding Each TC agent use own multicast: I'm not sure it is true, TC
> > > admins tried to do so, but not succeded.
> > >
> > > One more reason is speed of tests run. Why do we need to scan something
> > if
> > > we always will connect localhost. TC tests do not use multicast in
> almost
> > > every test.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > чт, 2 авг. 2018 г. в 0:27, Yakov Zhdanov <[hidden email]>:
> > >
> > > > I disagree. Probably, no change required. Each TC agent use own
> > multicast
> > > > group so nodes do not intersect. If any of the test does not properly
> > > clean
> > > > up and leaves nodes running this dhould be flagged as test fail which
> > is
> > > > the case.
> > > >
> > > > Please provide strong reasons to start with this.
> > > >
> > > > --Yakov
> > > >
> > >
> >
>

yzhdanov

Re: IP finder in tests

Well, I tend to agree. Can you try applying it on some suite and share the
results in terms of run time decrease and decreasing number of random
failures due to improper tests stop or cleanup?

--Yakov

2018-08-02 16:37 GMT+03:00 Denis Mekhanikov <[hidden email]>:

> Yakov,
>
> Almost every test in the project uses the Vm IP finder anyway.
> It has become a convention to use it in all tests.
> So, I'm trying to reduce the amount of copy-pasted code and improve test's
> isolation.
>
> Also tests may be run outside TeamCity during development process.
> Multicast Ip Finder makes developers disconnect from their networks to
> guarantee, that no other nodes will get in the way.
> So, I don't see any advantages of multicast IP finder over Vm in context of
> tests.
>
> Denis
>
> чт, 2 авг. 2018 г. в 1:46, Pavel Kovalenko <[hidden email]>:
>
> > Hi Yakov,
> >
> > Currently TC agents defended by Docker virtual network, that's why we
> don't
> > see intersection between several clusters, but in case of any step aside
> > (running several suites on one agent, running several tests on one
> machine
> > and so on) we will have problems and return back to this conversation.
> > I'm voting for simplifying and speeding up testing process. It will also
> > reduce the number of copy-paste in ton of tests, where Vm Ip Finder is
> used
> > explicitly. As developer I'm confusing when I see in a test VmIpFinder
> and
> > in other test Multicast without any reason or comment.
> > If you care about test coverage of MulticastIpFinder you can pick several
> > suites where number of starting/stopping is most frequent and leave
> > multicast there, but in general it's not necessary to have it everywhere.
> >
> > 2018-08-02 0:54 GMT+03:00 Yakov Zhdanov <[hidden email]>:
> >
> > > It should be true, otherwise we would have nodes from all agents
> > > intersecting. No?
> > >
> > > And multicast IP finder is the defailt one, so I would not reduce its
> > test
> > > volume.
> > >
> > > Yakov Zhdanov
> > > www.gridgain.com
> > >
> > > 2018-08-02 0:32 GMT+03:00 Dmitriy Pavlov <[hidden email]>:
> > >
> > > > Hi Yakov,
> > > >
> > > > Regarding Each TC agent use own multicast: I'm not sure it is true,
> TC
> > > > admins tried to do so, but not succeded.
> > > >
> > > > One more reason is speed of tests run. Why do we need to scan
> something
> > > if
> > > > we always will connect localhost. TC tests do not use multicast in
> > almost
> > > > every test.
> > > >
> > > > Sincerely,
> > > > Dmitriy Pavlov
> > > >
> > > > чт, 2 авг. 2018 г. в 0:27, Yakov Zhdanov <[hidden email]>:
> > > >
> > > > > I disagree. Probably, no change required. Each TC agent use own
> > > multicast
> > > > > group so nodes do not intersect. If any of the test does not
> properly
> > > > clean
> > > > > up and leaves nodes running this dhould be flagged as test fail
> which
> > > is
> > > > > the case.
> > > > >
> > > > > Please provide strong reasons to start with this.
> > > > >
> > > > > --Yakov
> > > > >
> > > >
> > >
> >
>