Prevent delete temporary file in test_bgp_update_timer case

[weiguo-nvidia]

Description of PR

Summary: By default, tempfile.NamedTemporaryFile() creates a temporary file that will be deleted once it is closed. When the function fetch_and_delete_pcap_file is exited, the temporary file will be deleted. In the following code, the file cannot be found, so the test case fails.

Fixes # Set delete=False, it will prevent the file from being deleted

Type of change

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

Back port request

• [ ] 202012
• [ ] 202205
• [ ] 202305
• [ ] 202311
• [x] 202405

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

[bingwang-ms]

Hi @weiguo-nvidia, I didn't see test failure in test_bgp_update_timer. Can you share what's the error you saw?

[weiguo-nvidia]

Hi @bingwang-ms,

In the test_bgp_update_timer_single_route, it execute these codes

local_pcap_filename = fetch_and_delete_pcap_file(bgp_pcap, constants.log_dir, duthost, request)
bgp_updates = bgp_update_packets(local_pcap_filename)

In the fetch_and_delete_pcap_file function, it executes local_pcap_file = tempfile.NamedTemporaryFile(). The tempfile.NamedTemporaryFile() creates a temporary file. But when the function exit, the temporary file will be deleted.

def fetch_and_delete_pcap_file(bgp_pcap, log_dir, duthost, request):
    if log_dir:
        local_pcap_filename = os.path.join(
            log_dir, LOCAL_PCAP_FILE_TEMPLATE % request.node.name
        )
    else:
        local_pcap_file = tempfile.NamedTemporaryFile()      <<< HERE
        local_pcap_filename = local_pcap_file.name
    duthost.fetch(src=bgp_pcap, dest=local_pcap_filename, flat=True)
    duthost.file(path=bgp_pcap, state="absent")
    return local_pcap_filename

This is the error info in allure report before the fix.

FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp_jn4qhb6'

fname = '/tmp/tmp_jn4qhb6'

    @staticmethod
    def open(fname  # type: Union[IO[bytes], str]
             ):
        # type: (...) -> Tuple[str, _ByteStream, bytes]
        """Open (if necessary) filename, and read the magic."""
        if isinstance(fname, str):
            filename = fname
            try:
>               fdesc = gzip.open(filename, "rb")  # type: _ByteStream

filename   = '/tmp/tmp_jn4qhb6'
fname      = '/tmp/tmp_jn4qhb6'

/usr/local/lib/python3.8/dist-packages/scapy/utils.py:1209: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib/python3.8/gzip.py:58: in open
    binary_file = GzipFile(filename, gz_mode, compresslevel)
        compresslevel = 9
        encoding   = None
        errors     = None
        filename   = '/tmp/tmp_jn4qhb6'
        gz_mode    = 'rb'
        mode       = 'rb'
        newline    = None
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[AttributeError("'GzipFile' object has no attribute 'fileobj'") raised in repr()] GzipFile object at 0x7fc51a3ce0d0>
filename = '/tmp/tmp_jn4qhb6', mode = 'rb', compresslevel = 9, fileobj = None, mtime = None

    def __init__(self, filename=None, mode=None,
                 compresslevel=_COMPRESS_LEVEL_BEST, fileobj=None, mtime=None):
        """Constructor for the GzipFile class.
    
        At least one of fileobj and filename must be given a
        non-trivial value.
    
        The new class instance is based on fileobj, which can be a regular
        file, an io.BytesIO object, or any other object which simulates a file.
        It defaults to None, in which case filename is opened to provide
        a file object.
    
        When fileobj is not None, the filename argument is only used to be
        included in the gzip file header, which may include the original
        filename of the uncompressed file.  It defaults to the filename of
        fileobj, if discernible; otherwise, it defaults to the empty string,
        and in this case the original filename is not included in the header.
    
        The mode argument can be any of 'r', 'rb', 'a', 'ab', 'w', 'wb', 'x', or
        'xb' depending on whether the file will be read or written.  The default
        is the mode of fileobj if discernible; otherwise, the default is 'rb'.
        A mode of 'r' is equivalent to one of 'rb', and similarly for 'w' and
        'wb', 'a' and 'ab', and 'x' and 'xb'.
    
        The compresslevel argument is an integer from 0 to 9 controlling the
        level of compression; 1 is fastest and produces the least compression,
        and 9 is slowest and produces the most compression. 0 is no compression
        at all. The default is 9.
    
        The mtime argument is an optional numeric timestamp to be written
        to the last modification time field in the stream when compressing.
        If omitted or None, the current time is used.
    
        """
    
        if mode and ('t' in mode or 'U' in mode):
            raise ValueError("Invalid mode: {!r}".format(mode))
        if mode and 'b' not in mode:
            mode += 'b'
        if fileobj is None:
>           fileobj = self.myfileobj = builtins.open(filename, mode or 'rb')
E           FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp_jn4qhb6'

compresslevel = 9
filename   = '/tmp/tmp_jn4qhb6'
fileobj    = None
mode       = 'rb'
mtime      = None
self       = <[AttributeError("'GzipFile' object has no attribute 'fileobj'") raised in repr()] GzipFile object at 0x7fc51a3ce0d0>

/usr/lib/python3.8/gzip.py:173: FileNotFoundError

During handling of the above exception, another exception occurred:

common_setup_teardown = (<tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52de80>, <tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52dd60>)
constants = <test_bgp_update_timer.constants.<locals>._C object at 0x7fc51b432580>, duthosts = [<MultiAsicSonicHost r-panther-40>]
enum_rand_one_per_hwsku_frontend_hostname = 'r-panther-40'
request = <FixtureRequest for <Function test_bgp_update_timer_single_route[r-panther-40-default]>>
toggle_all_simulator_ports_to_enum_rand_one_per_hwsku_frontend_host_m = None, validate_active_active_dualtor_setup = None

    def test_bgp_update_timer_single_route(
        common_setup_teardown,
        constants,
        duthosts,
        enum_rand_one_per_hwsku_frontend_hostname,
        request,
        toggle_all_simulator_ports_to_enum_rand_one_per_hwsku_frontend_host_m,  # noqa F811
        validate_active_active_dualtor_setup,  # noqa F811
    ):
        duthost = duthosts[enum_rand_one_per_hwsku_frontend_hostname]
    
        n0, n1 = common_setup_teardown
        try:
            n0.start_session()
            n1.start_session()
    
            # ensure new sessions are ready
            if not wait_until(
                WAIT_TIMEOUT,
                5,
                20,
                lambda: is_neighbor_sessions_established(duthost, (n0, n1)),
            ):
                pytest.fail("Could not establish bgp sessions")
    
            announce_intervals = []
            withdraw_intervals = []
            for i, route in enumerate(constants.routes):
                bgp_pcap = BGP_LOG_TMPL % i
                with capture_bgp_packages_to_file(duthost, "any", bgp_pcap, n0.namespace):
                    n0.announce_route(route)
                    time.sleep(constants.sleep_interval)
                    duthost.shell(
                        "vtysh -c 'show ip bgp neighbors {} received-routes' | grep '{}'".format(
                            n0.ip, route["prefix"]
                        ),
                        module_ignore_errors=True,
                    )
                    duthost.shell(
                        "vtysh -c 'show ip bgp neighbors {} advertised-routes' | grep '{}'".format(
                            n1.ip, route["prefix"]
                        ),
                        module_ignore_errors=True,
                    )
                    n0.withdraw_route(route)
                    duthost.shell(
                        "vtysh -c 'show ip bgp neighbors {} received-routes' | grep '{}'".format(
                            n0.ip, route["prefix"]
                        ),
                        module_ignore_errors=True,
                    )
                    duthost.shell(
                        "vtysh -c 'show ip bgp neighbors {} advertised-routes' | grep '{}'".format(
                            n1.ip, route["prefix"]
                        ),
                        module_ignore_errors=True,
                    )
                    time.sleep(constants.sleep_interval)
    
                import pdb; pdb.set_trace()
                local_pcap_filename = fetch_and_delete_pcap_file(bgp_pcap, constants.log_dir, duthost, request)
>               bgp_updates = bgp_update_packets(local_pcap_filename)

announce_intervals = []
bgp_pcap   = '/tmp/bgp0.pcap'
common_setup_teardown = (<tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52de80>, <tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52dd60>)
constants  = <test_bgp_update_timer.constants.<locals>._C object at 0x7fc51b432580>
duthost    = <MultiAsicSonicHost r-panther-40>
duthosts   = [<MultiAsicSonicHost r-panther-40>]
enum_rand_one_per_hwsku_frontend_hostname = 'r-panther-40'
i          = 0
local_pcap_filename = '/tmp/tmp_jn4qhb6'
n0         = <tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52de80>
n1         = <tests.common.helpers.bgp.BGPNeighbor object at 0x7fc51a52dd60>
pdb        = <module 'pdb' from '/usr/lib/python3.8/pdb.py'>
request    = <FixtureRequest for <Function test_bgp_update_timer_single_route[r-panther-40-default]>>
route      = {'nexthop': '192.168.0.2', 'prefix': '10.10.100.128/27'}
toggle_all_simulator_ports_to_enum_rand_one_per_hwsku_frontend_host_m = None
validate_active_active_dualtor_setup = None
withdraw_intervals = []

bgp/test_bgp_update_timer.py:324: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
bgp/test_bgp_update_timer.py:196: in bgp_update_packets
    packets = sniff(
        pcap_file  = '/tmp/tmp_jn4qhb6'
/usr/local/lib/python3.8/dist-packages/scapy/sendrecv.py:1311: in sniff
    sniffer._run(*args, **kwargs)
        args       = ()
        kwargs     = {'lfilter': <function bgp_update_packets.<locals>.<lambda> at 0x7fc519b59550>, 'offline': '/tmp/tmp_jn4qhb6'}
        sniffer    = <scapy.sendrecv.AsyncSniffer object at 0x7fc51bd604c0>
/usr/local/lib/python3.8/dist-packages/scapy/sendrecv.py:1116: in _run
    sniff_sockets.update((PcapReader(
        L2socket   = None
        count      = 0
        flt        = None
        iface      = None
        karg       = {}
        lfilter    = <function bgp_update_packets.<locals>.<lambda> at 0x7fc519b59550>
        offline    = ['/tmp/tmp_jn4qhb6']
        opened_socket = None
        prn        = None
        quiet      = False
        self       = <scapy.sendrecv.AsyncSniffer object at 0x7fc51bd604c0>
        session    = <scapy.sessions.DefaultSession object at 0x7fc51a3ce0a0>
        session_kwargs = {}
        sniff_sockets = {}
        started_callback = None
        stop_filter = None
        store      = True
        timeout    = None
/usr/local/lib/python3.8/dist-packages/scapy/sendrecv.py:1116: in <genexpr>
    sniff_sockets.update((PcapReader(
        .0         = <list_iterator object at 0x7fc51a3ce820>
        flt        = None
        fname      = '/tmp/tmp_jn4qhb6'
        quiet      = False
/usr/local/lib/python3.8/dist-packages/scapy/utils.py:1179: in __call__
    filename, fdesc, magic = cls.open(filename)
        cls        = <class 'scapy.utils.PcapReader'>
        filename   = '/tmp/tmp_jn4qhb6'
        i          = <scapy.utils.PcapReader object at 0x7fc51a3ce9a0>
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

fname = '/tmp/tmp_jn4qhb6'

    @staticmethod
    def open(fname  # type: Union[IO[bytes], str]
             ):
        # type: (...) -> Tuple[str, _ByteStream, bytes]
        """Open (if necessary) filename, and read the magic."""
        if isinstance(fname, str):
            filename = fname
            try:
                fdesc = gzip.open(filename, "rb")  # type: _ByteStream
                magic = fdesc.read(4)
            except IOError:
>               fdesc = open(filename, "rb")
E               FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp_jn4qhb6'

filename   = '/tmp/tmp_jn4qhb6'
fname      = '/tmp/tmp_jn4qhb6'

/usr/local/lib/python3.8/dist-packages/scapy/utils.py:1212: FileNotFoundError

[bingwang-ms]

Thanks @echuawu. I didn't see same test failure. Can you delete the file after fetching it?

[echuawu]

Thanks @echuawu. I didn't see same test failure. Can you delete the file after fetching it?

@bingwang-ms , I think you intend to @weiguo-nvidia :)

[weiguo-nvidia]

Hi @bingwang-ms ,

I do more debug for this change

Case test_bgp_update_timer_session_down executes this code to call function fetch_and_delete_pcap_file. The input parameter constants.log_dir is None

local_pcap_filename = fetch_and_delete_pcap_file(bgp_pcap, constants.log_dir, duthost, request)

In the function fetch_and_delete_pcap_file, the parameter log_dir is None, so the code will execute the else branch and create a temporary file to store the pcap file. The temporary file will be deleted after the function returns.

def fetch_and_delete_pcap_file(bgp_pcap, log_dir, duthost, request):
    if log_dir:
        local_pcap_filename = os.path.join(
            log_dir, LOCAL_PCAP_FILE_TEMPLATE % request.node.name
        )
    else:
        local_pcap_file = tempfile.NamedTemporaryFile()     <<< HERE
        local_pcap_filename = local_pcap_file.name
    duthost.fetch(src=bgp_pcap, dest=local_pcap_filename, flat=True)
    duthost.file(path=bgp_pcap, state="absent")
    return local_pcap_filename

Where is the parameter constants.log_dir from? It defines in constants fixture. Because the pytest command does not pass the log_file option, so the _constants.log_dir is None. Your test pass, maybe you pass the log_file option in the pytest command, so it uses os.path.dirname(os.path.abspath(log_file))

@pytest.fixture
def constants(is_quagga, setup_interfaces, has_suppress_feature, pytestconfig):
    ...

    log_file = pytestconfig.getoption("log_file", None)
    if log_file:
        _constants.log_dir = os.path.dirname(os.path.abspath(log_file))
    else:
        _constants.log_dir = None

    return _constants

[liat-grozovik]

@bingwang-ms ok to merge?

[mssonicbld]

Cherry-pick PR to 202405: https://github.com/sonic-net/sonic-mgmt/pull/16017