Wget: add skip_exists option and align the Windows signature

[mcgov (mcgov)]

Part 5 of 9 of a stacked series that reworks the DPDK SRIOV hot plug tests. Stacked on #4657, review only the last commit.

Downloading large source tarballs again on every run is slow and can fail on flaky mirrors. Wget.get gains skip_exists so callers can reuse a file that is already present on the node instead of removing and redownloading it. WindowsWget.get takes the same argument so the override keeps matching the base class.

Key Test Cases:
verify_dpdk_build_netvsc|verify_dpdk_build_failsafe

Impacted LISA Features:
Sriov, NetworkInterface

Tested Azure Marketplace Images:

• canonical 0001-com-ubuntu-server-jammy 22_04-lts latest
• microsoftcblmariner azure-linux-3 azure-linux-3 latest

The merge-base changed after approval.

[Copilot]

Pull request overview

This PR extends the Wget.get API to support reusing already-downloaded files via a new skip_exists flag, and updates WindowsWget.get to keep its override signature aligned with the base class. This is intended to avoid repeated downloads of large tarballs in the DPDK SRIOV hot plug test rework series.

Changes:

• Add skip_exists: bool = False to Wget.get and short-circuit when the target path already exists.
• Add skip_exists: bool = False to WindowsWget.get to match the base signature.

Suppressed comments (1)

lisa/base_tools/wget.py:183

• Major: WindowsWget.get now accepts skip_exists, but it isn’t used, so callers will still re-download. Also, the existing overwrite=False branch checks file_path (directory) and only logs without returning, so it doesn’t actually prevent overwriting. Add an early return when the target download_path already exists and either skip_exists=True or overwrite=False.

        skip_exists: bool = False,
    ) -> str:
        cached_filename = self._url_file_cache.get(url, None)
        if cached_filename:
            if force_run:

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:74

• When skip_exists=True and the target file already exists, the method returns early (line 72) without updating _url_file_cache. That means subsequent get(url, ...) calls in the same run won’t benefit from the cache and may re-download unless skip_exists is repeatedly passed. Consider caching the existing download_path (or its canonical ls-resolved path) before returning.

        download_pure_path = self.node.get_pure_path(download_path)
        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

lisa/base_tools/wget.py:180

• WindowsWget.get adds the skip_exists parameter for signature alignment, but the implementation never reads it, so callers can’t actually reuse an existing downloaded file on Windows. Also, the current “return if file exists and not overwrite” block doesn’t return, so overwrite=False is effectively ignored. Please honor skip_exists (and overwrite=False) with an early return when download_path already exists.

        executable: bool = False,
        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,
    ) -> str:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:180

• WindowsWget.get adds the skip_exists parameter to match Wget.get, but it is never used. As a result, the Windows implementation will still remove and re-download even when skip_exists=True, so callers can’t rely on the cross-platform behavior implied by the base class signature. Implement an early return when the target file already exists (and optionally populate _url_file_cache).

        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,
    ) -> str:

lisa/base_tools/wget.py:74

• When skip_exists=True and the target path already exists, this returns early without updating _url_file_cache and without applying executable=True (no chmod +x). Callers using skip_exists with executable can get a non-executable file and future calls won’t benefit from the URL cache. Consider resolving the existing file path (same ls logic as the download path), caching it, and still applying chmod when requested before returning.

This issue also appears on line 176 of the same file.

        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:179

• Major: WindowsWget.get() now accepts skip_exists, but the parameter is currently unused. As a result, callers cannot actually reuse an already-downloaded file on Windows (the method will still proceed to remove and re-download later). Please add an early check for ls.path_exists(download_path) and return the existing path (and optionally update _url_file_cache) when skip_exists=True (and also when overwrite=False, per the existing comment).

        executable: bool = False,
        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,

lisa/base_tools/wget.py:73

• Major: When skip_exists is true and the target file already exists, this early return returns download_path without updating _url_file_cache and without normalizing the path the same way the download path does (later ls output is cached/returned). This can lead to inconsistent return values (e.g., ~/relative paths) and repeated downloads on subsequent calls. Consider resolving the existing path with the same ls logic used later, cache it (self._url_file_cache[url] = actual_file_path), and return the normalized path.

        download_pure_path = self.node.get_pure_path(download_path)
        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:72

• Major: overwrite=False still triggers a re-download when the target path already exists (the code only returns early for skip_exists). Callers that set overwrite=False to avoid re-downloading large artifacts will still hit the network and potentially overwrite the existing file. Consider either treating overwrite=False as an early-return when the file exists (similar to skip_exists), or clarifying/deprecating overwrite in favor of skip_exists to avoid ambiguous API semantics.

        # remove existing file and dir to download again.
        download_pure_path = self.node.get_pure_path(download_path)
        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path

lisa/base_tools/wget.py:180

• Major: WindowsWget.get() now accepts skip_exists, but the parameter is unused in the method body, so Windows callers cannot actually reuse an existing download (the method always removes and re-downloads). Either implement the same early-return behavior as Wget.get() when the destination exists, or remove skip_exists from the override if it is intentionally unsupported on Windows.

        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,
    ) -> str:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:74

• When skip_exists=True and the target path already exists, this returns early without updating _url_file_cache[url] and without applying executable=True (the later chmod +x). This can lead to repeated exists() checks and surprising behavior where callers request an executable but the existing file keeps non-exec permissions. Consider resolving the existing path with ls, caching it, and applying chmod before returning.

        # remove existing file and dir to download again.
        download_pure_path = self.node.get_pure_path(download_path)
        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

lisa/base_tools/wget.py:183

• WindowsWget.get now accepts skip_exists, but the method doesn’t use it. Also, the existing “not overwrite” branch checks ls.path_exists(file_path) (directory) and only logs, so it still removes and re-downloads the file. This makes both skip_exists and overwrite=False ineffective on Windows; consider checking download_path and returning early when reuse is requested.

        skip_exists: bool = False,
    ) -> str:
        cached_filename = self._url_file_cache.get(url, None)
        if cached_filename:
            if force_run:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:74

• overwrite=False is not currently honored when the target file already exists: the code only skips pre-removal, but still runs wget ... -O {download_path} which will clobber the file. Since callers rely on overwrite=False to avoid re-downloading (e.g. DPDK tarball downloads), consider treating not overwrite the same as skip_exists (early-return when the path exists), or pass a no-clobber flag instead.

        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

lisa/base_tools/wget.py:180

• WindowsWget.get() adds the skip_exists parameter to match the base signature, but it is never used. As a result Windows always removes and re-downloads even when skip_exists=True (and also when overwrite=False). Add an early-return when download_path already exists to keep semantics aligned with Wget.get().

        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,
    ) -> str:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:183

• Major: WindowsWget.get adds the skip_exists parameter but the implementation never references it, so callers cannot actually reuse an existing download on Windows (it will still remove and re-download). Consider checking whether download_path already exists (e.g., via ls.path_exists(download_path, sudo=sudo)) and returning early when skip_exists=True.

        skip_exists: bool = False,
    ) -> str:
        cached_filename = self._url_file_cache.get(url, None)
        if cached_filename:
            if force_run:

lisa/base_tools/wget.py:74

• Major: skip_exists=True returns early without updating _url_file_cache and without applying the executable=True chmod behavior. This can cause repeated existence checks on later calls and can return a non-executable file even though the caller requested executable=True. Consider caching the returned path and, if executable=True, ensuring chmod is applied before returning.

This issue also appears on line 179 of the same file.

        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lisa/base_tools/wget.py:180

• skip_exists was added to the WindowsWget.get signature, but the implementation never uses it (and the current “exists + not overwrite” branch doesn’t return and checks file_path (directory) rather than download_path). As a result, Windows still removes/redownloads even when callers request reuse. Consider mirroring the base behavior: if download_path exists and (skip_exists is True or overwrite is False), return download_path without deleting/redownloading.

    def get(
        self,
        url: str,
        file_path: str = "",
        filename: str = "",
        overwrite: bool = True,
        executable: bool = False,
        sudo: bool = False,
        force_run: bool = False,
        timeout: int = 600,
        skip_exists: bool = False,
    ) -> str:

lisa/base_tools/wget.py:74

• In Wget.get, the new skip_exists early-return bypasses the rest of the method, so it won’t update _url_file_cache for the URL and won’t apply executable=True (chmod) when the file already exists. It would be more consistent to still populate the cache and apply the executable bit before returning when skip_exists is set.

        if self.node.shell.exists(download_pure_path):
            if skip_exists:
                return download_path
            elif overwrite:
                self.node.shell.remove(download_pure_path, recursive=True)

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lisa/base_tools/wget.py:75

• When returning early due to an existing download (skip_exists or overwrite=False), the method bypasses the executable handling, so callers requesting an executable may get a non-executable file. Consider applying chmod +x (when executable=True) before returning, similar to the post-download path.

        if self.node.shell.exists(download_pure_path):
            if skip_exists or not overwrite:
                self._url_file_cache[url] = download_path
                return download_path

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lisa/base_tools/wget.py:73

• When returning early because the target path already exists (skip_exists or overwrite is False), the executable flag is ignored. This can leave an existing/non-executable file without +x even though the caller requested it. Consider applying chmod +x (respecting sudo) before returning when executable=True, or otherwise documenting that executable is only enforced on fresh downloads.

        if self.node.shell.exists(download_pure_path):
            if skip_exists or not overwrite:
                self._url_file_cache[url] = download_path
                return download_path