Bug 1192

Bug closed.

ns-3-dev changeset: 55f494c59bf2

Created attachment 1176 [details]
Another set of teardown changes

Hi there again.

I'm reopening it since, while working with this bencharking stuff, I noticed some more cases (though subtle) of improper teardown.

Especially in the case of failed tests, the simulator keeps alive (it's not destroyed), and this brought some trouble (of the segmentation fault at times) later on.

Also, many tests change NS3 configuration values, and they don't restore them to the usual defaults. Because of a lack of configuration getters, the patch attached only sets them back to the defaults documented in NS3's doxy, but the best thing would be to make backups of them and restore to their previous values.

In any case, this patch is a lot bigger, and it only fixes the tests for the case where you want to run several tests in succession, in the same process. ./waf check doesn't run tests this way, so there should be no difference there.

IF this patch is accepted, I can post the benchmarking stuff (without this patch, benchmark results aren't significant because of failed test runs).

In the past, I was as frustrated as you by this and put together similar patches but I eventually gave up because I could not see how we could make sure that all our global variables are reset to the right state after a test runs. So, I thought: if I cannot get this right 100%, what is the point of making people believe it might work and suffer when it does not ? i.e., I chose to live with test.py (even though I hate this).

I would tend to feel like making this bug as WONTFIX based on the above unless you feel that you can really make the whole attribute system be nicely reset upon test completion to make sure different tests do not interfere on each other.

(In reply to comment #2)
> Created attachment 1176 [details]
> Another set of teardown changes
> 
> Hi there again.
> 
> I'm reopening it since, while working with this bencharking stuff, I noticed
> some more cases (though subtle) of improper teardown.
> 
> Especially in the case of failed tests, the simulator keeps alive (it's not
> destroyed), and this brought some trouble (of the segmentation fault at times)
> later on.
> 
> Also, many tests change NS3 configuration values, and they don't restore them
> to the usual defaults. Because of a lack of configuration getters, the patch
> attached only sets them back to the defaults documented in NS3's doxy, but the
> best thing would be to make backups of them and restore to their previous
> values.
> 
> In any case, this patch is a lot bigger, and it only fixes the tests for the
> case where you want to run several tests in succession, in the same process.
> ./waf check doesn't run tests this way, so there should be no difference there.
> 
> IF this patch is accepted, I can post the benchmarking stuff (without this
> patch, benchmark results aren't significant because of failed test runs).

(In reply to comment #3)
> In the past, I was as frustrated as you by this and put together similar
> patches but I eventually gave up because I could not see how we could make sure
> that all our global variables are reset to the right state after a test runs.
> So, I thought: if I cannot get this right 100%, what is the point of making
> people believe it might work and suffer when it does not ? i.e., I chose to
> live with test.py (even though I hate this).
> 
> I would tend to feel like making this bug as WONTFIX based on the above unless
> you feel that you can really make the whole attribute system be nicely reset
> upon test completion to make sure different tests do not interfere on each
> other.

I was wondering whether config-store could be leveraged to save the initial state of defaults and reload it every new test.

Isn't this in general an issue of running multiple simulations from one main(); i.e. the same problem would be exhibited in programs like this one:
examples/wireless/wifi-adhoc.cc ?

Should we be advising people not to do this and to do multiple runs from outside of ns-3 main programs?

(In reply to comment #4)
> (In reply to comment #3)
> > In the past, I was as frustrated as you by this and put together similar
> > patches but I eventually gave up because I could not see how we could make sure
> > that all our global variables are reset to the right state after a test runs.
> > So, I thought: if I cannot get this right 100%, what is the point of making
> > people believe it might work and suffer when it does not ? i.e., I chose to
> > live with test.py (even though I hate this).
> > 
> > I would tend to feel like making this bug as WONTFIX based on the above unless
> > you feel that you can really make the whole attribute system be nicely reset
> > upon test completion to make sure different tests do not interfere on each
> > other.
> 
> I was wondering whether config-store could be leveraged to save the initial
> state of defaults and reload it every new test.

yes. Some of our attributes are broken though. Say, some of the wifi attributes which have hidden dependencies between them which introduces ordering constraints on how you set them which breaks config-store badly. I have already pointed this out before. Someone needs to carefully review all attributes and check that they really are setters with no side-effects.

> Isn't this in general an issue of running multiple simulations from one main();
> i.e. the same problem would be exhibited in programs like this one:
> examples/wireless/wifi-adhoc.cc ?
> 
> Should we be advising people not to do this and to do multiple runs from
> outside of ns-3 main programs?

In general, it is less of a problem because users know the simulation they are running so they know how to cleanup. Here, we do not know which tests are being run so it is hard to protect against random crazy things beeing done there.

Ok, so, there are two possible venues:

A reset() method that really resets state, or config getters, so that tests can roll back changes.

Given the mentioned side effects, I'm leaning towards reset(), which would be broader.

Hm...

(In reply to comment #6)
> Ok, so, there are two possible venues:
> 
> A reset() method that really resets state, or config getters, so that tests can
> roll back changes.
> 
> Given the mentioned side effects, I'm leaning towards reset(), which would be
> broader.
> 
> Hm...

Sure, the problem is "just" that you need to implement it. I tried a couple of times and did not get something reliable.

Ok, I did the reset thing, and it seems to work reliably.

The routing-aodv-regression test, especially the bug-772 one, seems to depend on config settings (arp timeout) set by previous test cases (which would be a problem).

I'll try to update the patch to include that.

> > 
> > I was wondering whether config-store could be leveraged to save the initial
> > state of defaults and reload it every new test.
> 
> yes. Some of our attributes are broken though. Say, some of the wifi attributes
> which have hidden dependencies between them which introduces ordering
> constraints on how you set them which breaks config-store badly. I have already
> pointed this out before. Someone needs to carefully review all attributes and
> check that they really are setters with no side-effects.
> 

I file a new bug on this aspect (1213).

Otherwise, I support what Claudio is trying to do with this patch.

Ok, I found more sources of mis-isolation in tests, which could also be a real bug (ie, not only affect tests).

1. SeedManager::SetSeed(..) does not do anything if the RNG has been initialized already. This means, in some situations SetSeed could be ignored, which I guess is not good for experiments either.

2. Mac48Address has global state that cannot be controlled (ie: a static variable controlling the MAC sequence), so MACs assigned in PCAP-checking tests make tests fail when ran in succession. If I change the static by a GlobalValue, it gets fixed (thanks to the patch that resets GlobalValue s). Thing is, PCAPs need re-generation (MACs are still different than expected in current PCAPs, but consistent and reproducible).

Anybody knows how to generate new PCAPs?

Have you looked at the Tracing section of the ns-3 manual:

    www.nsnam.org/docs/release/3.11/manual/html/tracing.html

Ok, I know that, but I was expecting something along the lines of "Run test-runner with arguments blah". Someone already does generate them from time to time, so there must be a rather easy way to do it...

...it's just not documented.

NM - I just found it.

I'm preparing a patchset.

Since it will take time (I have to test each patchset applied to the current tip), a summary first:
 * SeedManager::SetSeed/SetRun did not take effect after RNG initialization
   - incidentally, after forcing it to re-seed the RNG, 
     creating RandomVariables isn't as fast (though not that slow either), 
     so I have a patch that makes simple-odfm-wimax-phy NOT create an RNG 
     each time a packet is processed.
 * PacketMetadata: it has Enable() but no Disable(). So a Disable was added
   that the test framework can use to make sure there are no side effects 
   after test execution
 * Config::Reset() added that resets all GlobalValue s and attribute defaults
   to their initial values.
 * Mac48Address uses a static local variable to generate the MAC series,
   this results in uncontrollable side effects that make tests that depend
   on MACs not repeatable. Changing it to a GlobalValue (that gets updated
   after each call to Allocate) solves this, allowing user code control
   over the MAC series (through Config), and allowing the test framework
   to reset the series before test runs.
 * The test framework now assures repeatable conditions within test cases, by:
   - Destroying the simulator at base TestCase::DoTeardown/Setup
   - Resetting configuration (attribute defaults and global values) at
     various points (TestCase::DoTeardown, TestSuite::DoSetup)
   - Saving/Restoring the RNG state at TestSuite::DoSetup/Teardown
     (I tried per TestCase, but some test cases depend on RNG state
     set by previous test cases, sorting that out would have been
     a LOT more work)
 * I recreated the PCAP traces that now use repeatable (different)
   MACs and RNG state.

Incidentally, I found that RvBatteryModel segfaults in some situations when running tests repeatedly. I'll post a patch in a separate issue (since it's completely independent with the issue at hand here).

Created attachment 1200 [details]
Patchset with SeedManager::SetSeed/Run fixes

Import with "hg import <patchfile>"
Requires PCAP re-generation.

Created attachment 1201 [details]
Patchset with Config::Reset implementation

Import with "hg import <patchfile>"

Created attachment 1202 [details]
Patchset with complete set of test repeatability fixes

Import with "hg import <patchfile>"
Requires PCAP re-generation.

Created attachment 1203 [details]
Updated reference PCAP traces for tests

(In reply to comment #13)

>  * The test framework now assures repeatable conditions within test cases, by:
>    - Destroying the simulator at base TestCase::DoTeardown/Setup
>    - Resetting configuration (attribute defaults and global values) at
>      various points (TestCase::DoTeardown, TestSuite::DoSetup)
>    - Saving/Restoring the RNG state at TestSuite::DoSetup/Teardown
>      (I tried per TestCase, but some test cases depend on RNG state
>      set by previous test cases, sorting that out would have been
>      a LOT more work)

I like the principle of this; for how the current test framework runs tests sequentially, it seems like this is the best workaround we have.

One small concern that I have is that the values restored in TestCase::DoTeardown may be hard to maintain, in case the upstream default values change.  I'm wondering whether a safer approach would be to just cache the current defaults when in DoSetup state so that they can be restored in DoTeardown.  

Conceptually, what I mean is that you would do something like this in TestCase::DoSetup:

  SaveDefault ("ns3::WifiRemoteStationManager::FragmentationThreshold");
before doing
  Config::SetDefault (...);

then in TestCase::DoTeardown, you would do

  RestoreDefault ("ns3::WifiRemoteStationManager::FragmentationThreshold");

BTW, thanks for doing all of this cleanup; I'd like to get these patches in.

(In reply to comment #18)
> One small concern that I have is that the values restored in
> TestCase::DoTeardown may be hard to maintain, in case the upstream default
> values change.  I'm wondering whether a safer approach would be to just cache
> the current defaults when in DoSetup state so that they can be restored in
> DoTeardown.  

That's exactly what the last patch does.

> TestCase::DoSetup:
> 
>   SaveDefault ("ns3::WifiRemoteStationManager::FragmentationThreshold");
> before doing
>   Config::SetDefault (...);

It's not explicit, every GlobalValue remembers the initial default given at construction time. They have a Reset() method that can be called, or a convenient Config::Reset() that resets it all. TestCase::DoTeardown calls Config::Reset to clean all up.

> BTW, thanks for doing all of this cleanup; I'd like to get these patches in.

:-)

You're welcome.

(In reply to comment #19)
> (In reply to comment #18)
> > One small concern that I have is that the values restored in
> > TestCase::DoTeardown may be hard to maintain, in case the upstream default
> > values change.  I'm wondering whether a safer approach would be to just cache
> > the current defaults when in DoSetup state so that they can be restored in
> > DoTeardown.  
> 
> That's exactly what the last patch does.

I was looking at this part of the patch (for example):
https://www.nsnam.org/bugzilla/attachment.cgi?id=1202&action=diff#a/src/energy/test/basic-energy-model-test.cc_sec3

maybe the current patch doesn't uniformly apply Config::Reset()?  (I see it elsewhere in the patch, though)

(In reply to comment #20)
> I was looking at this part of the patch (for example):
> https://www.nsnam.org/bugzilla/attachment.cgi?id=1202&action=diff#a/src/energy/test/basic-energy-model-test.cc_sec3
> 
> maybe the current patch doesn't uniformly apply Config::Reset()?  (I see it
> elsewhere in the patch, though)

Oh... my bad...

...those SetDefault at TearDown should no longer be necessary. I though I had removed them.

TestCase::DoTeardown() calls Config::Reset() already (in a previous patch), so SetDefault should not be necessary.

Created attachment 1217 [details]
Monolithic patch

Attached a monolithic patch as per Matthew's request.

It has NOT been rebased to the development head, too many test changes there, merging could take a while.