Fix python crash in case data plane never stop on fast-reboot

[pyadvichuk]

If for some reason data plane never stop on fast reboot the script will
fail bc of some undefined variables.

Description of PR

• Do not crash in case data plane never stop on fast-reboot;
• Insert 1ms interpacket gap as on some platforms have seen that swss was not fast enough to insert all the FDB entries.

Have tested the CT with ReloadTest::max_nr_vl_pkts == 1000 and verified that it passed.

Summary:
Fixes # (issue)

Type of change

• Bug fix
• [] Testbed and Framework(new/improvement)
• [] Test case(new/improvement)

Approach

How did you do it?

Changed Python script

How did you verify/test it?

Run fast-reboot CT and verify it will pass even in case ReloadTest::max_nr_vl_pkts == 1000

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@yxieca please review and merge this

[qiluo-msft]

Insert 1ms interpacket gap as on some platforms have seen that swss was not fast enough to insert all the FDB entries.

Should we treat this as a bug on 'some' platform?

[pyadvichuk]

Insert 1ms interpacket gap as on some platforms have seen that swss was not fast enough to insert all the FDB entries.

Should we treat this as a bug on 'some' platform?

The problem appeared to be present even on platforms having rich CPU ( >= 4cores) and from my understanding has a bit more relation to the swss performance. Actually, this mentioned in the comments and that's why I pointed that have tested even using ReloadTest::max_nr_vl_pkts == 1000

        self.max_nr_vl_pkts = 500 # FIXME: should be 1000.
                                  # But ptf is not fast enough + swss is slow for FDB and ARP entries insertions

also, the issue I've faced time to time is very similar to that one mentioned in the testcase code

    def wait_dut_to_warm_up(self):
        # When the DUT is freshly rebooted, it appears that it needs to warm
        # up towards PTF docker. In practice, I've seen this warm up taking
        # up to ~70 seconds.

In my investigation, it seems that packets sent in a loop will be stacked inside one of the system buffers due to huge virtualization (python-vm) and then are pushed out to the network as a big burst. Inserting 1ms delay we just re-schedule processes queue and push packets to the network one by one.

[yxieca]

Can you retry with latest master branch with #890? This change addressed some IO conflicts by setting proper filters. Please see if you still need to add the sleep with this change?

If you still have to add the sleep in the end, please add a configuration entry to disable sleep by default. On your testbed, you can pass in parameter to enable the sleep.

[pyadvichuk]

Can you retry with latest master branch with #890? This change addressed some IO conflicts by setting proper filters. Please see if you still need to add the sleep with this change?

If you still have to add the sleep in the end, please add a configuration entry to disable sleep by default. On your testbed, you can pass in parameter to enable the sleep.

on our setup reachability_watcher thread crashed doing fast-reboot during the first iteration with #890.
by the way - send_in_background thread also defines the interpacket gap but with no any conditions

            for entry in packets_list:
                time.sleep(interval)
                testutils.send_packet(self, *entry)

[pavel-shirshov]

I'd put here: Failed to record reboot time.

[pyadvichuk]

I'm afraid that Failed keyword would confuse a watcher. Another cause to keep the message - it is possible to use some external regexp to parse CT results. In this case we will not break the regex.

Any way - the situation just show a particular datapline feature, not the issue.
Let me know if you insist on the change.

[pavel-shirshov]

I think it's impossible for us to measure data plane disruption time with such good granularity.
Also you're using datetime.min here as a marker of timeout while waiting of dataplane start/stop.
So I would suggest don't use this marker value for anything.

[pyadvichuk]

just added one more variable indicating the fact that dataplane never stopped

[pyadvichuk]

I'd suggest to improve this PR:

1. Remove time.sleep. We want to send packets as fast as possible
2. If a goal of this PR to fix the crash, let's do that: we need to define upper_replies in case of timeout exception.
   Also I think as soon as we have timeout exception, we should fail the test

1. I have removed time.sleep
2. I introduced additional logic (variable 'routing_always') to be able to print "Reboot time was" message correctly
3. I don't agree to fail the test in case 'timeout exception' as it just indicates the fact that the dataplane never went down. Nothing criminal from the fast-reboot case perspective

[pavel-shirshov]

1. Thanks
3. The dataplane must gone done in case of fast-reboot.

[pyadvichuk]

Could you please propose a way how to detect small periods using PTF? We need this because traffic disruption in our case is ~70ms. So, dataplane goes down but PTF can't detect such small interval. The current test implementation is able to detect such small interval once per 4..5 tries (but Ixia, for example, shows disruption reliably). In this PR I've tried to set intervals less than some minimum period to "0:00:00"

[pavel-shirshov]

Please address my comments.

[pavel-shirshov]

I'd suggest to improve this PR:

1. Remove time.sleep. We want to send packets as fast as possible
2. If a goal of this PR to fix the crash, let's do that: we need to define upper_replies in case of timeout exception.
   Also I think as soon as we have timeout exception, we should fail the test

[pyadvichuk]

@yxieca @pavel-shirshov is there anything blocks us to get this merged?