From af95c66f33dd618586cf2f2a32155dcb8686a79d Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 30 Jun 2026 16:38:27 -0400 Subject: [PATCH 01/14] Add issu and nssu support flags --- changes/mm.added | 1 + pyntc/devices/jnpr_device.py | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 changes/mm.added diff --git a/changes/mm.added b/changes/mm.added new file mode 100644 index 00000000..89f03afc --- /dev/null +++ b/changes/mm.added @@ -0,0 +1 @@ +Added issu and nssu upgrade choices for install on juniper devices. \ No newline at end of file diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index b6431204..bb916293 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -507,7 +507,7 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5"): + def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False): """Install OS on device and reboot. Args: @@ -515,7 +515,8 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5") reboot (bool): Whether to reboot the device after setting the boot options. Defaults to true. checksum (str): The checksum of the file. hashing_algorithm (str): The hashing algorithm to use. Valid values are 'md5', 'sha1', and 'sha256'. Defaults to 'md5'. - + issu (bool): Whether to perform an In-Service Software Upgrade (ISSU). Defaults to false. + nssu (bool): Whether to perform a Non-Stop Software Upgrade (NSSU). Defaults to false. """ install_ok = self.sw.install( package=image_name, @@ -524,6 +525,8 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5") progress=True, validate=True, no_copy=True, + issu=issu, + nssu=nssu, timeout=3600, ) From a4321e8e8d2123678d6e332879ded4e11a2ffeee Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 6 Jul 2026 10:33:20 -0400 Subject: [PATCH 02/14] Add request_system_snapshot and wait_for_system_snapshot --- pyntc/devices/jnpr_device.py | 296 +++++++++++++++++++++++++++++++++-- 1 file changed, 279 insertions(+), 17 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index bb916293..7d9a5373 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -9,12 +9,13 @@ from urllib.parse import urlparse from jnpr.junos import Device as JunosNativeDevice -from jnpr.junos.exception import ConfigLoadError +from jnpr.junos.exception import ConfigLoadError, ConnectClosedError, RpcError, RpcTimeoutError from jnpr.junos.op.ethport import EthPortTable # pylint: disable=import-error,no-name-in-module from jnpr.junos.utils.config import Config as JunosNativeConfig from jnpr.junos.utils.fs import FS as JunosNativeFS from jnpr.junos.utils.scp import SCP from jnpr.junos.utils.sw import SW as JunosNativeSW +from lxml import etree from pyntc import log from pyntc.devices.base_device import BaseDevice, fix_docs @@ -73,7 +74,7 @@ class JunosDevice(BaseDevice): """Juniper JunOS Device Implementation.""" vendor = "juniper" - DEFAULT_TIMEOUT = 120 + DEFAULT_TIMEOUT = 180 def __init__(self, host, username, password, *args, **kwargs): # noqa: D403 """PyNTC device implementation for Juniper JunOS. @@ -279,6 +280,57 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200): raise RebootTimeoutError(hostname=self.hostname, wait_time=timeout) + def _wait_for_system_snapshot(self, timeout=1800, interval=180): + """Poll device to verify system snapshot completion. + + Periodically checks ``show system snapshot media internal`` to verify the snapshot + was successfully taken. Used when the ``request system snapshot`` RPC times out. + Attempts XML parsing for structured verification; falls back to string matching. + + Args: + timeout (int, optional): Max seconds to wait for snapshot verification. Defaults to 1800 (30 minutes). + interval (int, optional): Seconds between verification polls. Defaults to 180 (3 minutes). + + Raises: + OSInstallError: When the snapshot verification indicates a failure. + """ + start = time.time() + + while time.time() - start < timeout: + try: + # Try structured XML approach first + try: + xml_output = self.native.rpc.request_shell_execute( + command="show system snapshot media internal | display xml" + ) + if xml_output: + root = etree.fromstring(xml_output.encode()) + # Check if any snapshot-medium elements exist (indicates snapshots present) + snapshots = root.findall(".//snapshot-medium") + if snapshots: + log.info("Host %s: System snapshot verified via XML parsing.", self.host) + return + log.debug( + "Host %s: Snapshot verification in progress (no snapshots found yet); will retry.", + self.host, + ) + except Exception as xml_exc: # pylint: disable=broad-exception-caught + # XML parsing failed, try fallback text matching + log.debug("Host %s: XML parsing failed (%s); trying text fallback.", self.host, xml_exc) + output = self.native.cli("show system snapshot media internal") + if "snapshot" in output.lower(): + log.info("Host %s: System snapshot verified via text output.", self.host) + return + log.debug("Host %s: Snapshot verification in progress; will retry.", self.host) + + except Exception as exc: # pylint: disable=broad-exception-caught + log.debug("Host %s: Snapshot verification poll failed (%s); will retry.", self.host, exc) + + time.sleep(interval) + + log.error("Host %s: System snapshot did not complete within %s seconds.", self.host, timeout) + raise OSInstallError(hostname=self.hostname, desired_boot="system snapshot") + def backup_running_config(self, filename): """Backup current running configuration. @@ -288,6 +340,45 @@ def backup_running_config(self, filename): with open(filename, "w", encoding="utf-8") as file_name: file_name.write(self.running_config) + def request_system_snapshot(self, parameters=None): + """Request a system snapshot and verify completion. + + Issues the snapshot RPC call and polls device to verify completion. + Handles RPC timeouts by switching to polling mode. + + Parameters include: + - all-members + - local + - media + - member + - partition + - slice + + Args: + parameters (str, optional): Parameters to pass to the RPC call. Defaults to None. + + Raises: + OSInstallError: If snapshot verification fails or times out. + """ + command = "request system snapshot" + if parameters is not None: + command = f"{command} {parameters}" + + try: + log.debug("Host %s: Issuing RPC: %s", self.host, command) + response = self.native.rpc.cli(command) + log.debug("Host %s: snapshot RPC completed successfully.", self.host) + + except RpcTimeoutError: + log.debug("Host %s: snapshot RPC timed out; polling device to verify.", self.host) + + except Exception as rpc_error: + log.error("Host %s: Failed to request system snapshot (%s).", self.host, rpc_error) + raise + + # Verify snapshot completed on device (handles both RPC timeout and normal completion) + self._wait_for_system_snapshot() + @property def boot_options(self): """Get os version on device. @@ -512,23 +603,120 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", Args: image_name (str): Name of image. - reboot (bool): Whether to reboot the device after setting the boot options. Defaults to true. + reboot (bool): Whether to reboot the device after setting the boot options. Defaults to true. Ignored if nssu is true. checksum (str): The checksum of the file. hashing_algorithm (str): The hashing algorithm to use. Valid values are 'md5', 'sha1', and 'sha256'. Defaults to 'md5'. issu (bool): Whether to perform an In-Service Software Upgrade (ISSU). Defaults to false. - nssu (bool): Whether to perform a Non-Stop Software Upgrade (NSSU). Defaults to false. - """ - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - issu=issu, - nssu=nssu, - timeout=3600, - ) + nssu (bool): Whether to perform a Non-Stop Software Upgrade (NSSU). Defaults to false. When true, reboot is performed automatically. + """ + # Capture pre-install state for validation after reboot + self._uptime = None + pre_install_uptime = self.uptime + pre_install_version = self.os_version + + log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_install_version, pre_install_uptime) + + try: + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + issu=issu, + nssu=nssu, + timeout=3600, + ) + except RpcError as rpc_error: + # Check if error is due to pending reboot state from previous incomplete operation + if "reboot pending for software rollback" in str(rpc_error).lower(): + log.warning( + "Host %s: Device has pending reboot state; clearing with reboot.", + self.host, + ) + # Capture uptime before reboot for verification + self._uptime = None + pre_reboot_uptime = self.uptime + + try: + # Clear pending reboot state + self.native.rpc.request_reboot() + log.info("Host %s: Reboot issued to clear pending state.", self.host) + + # Wait for device to come back up + self._wait_for_device_reboot(pre_reboot_uptime) + log.info("Host %s: Device rebooted and reconnected.", self.host) + + # Verify image file still exists + if not self.check_file_exists(image_name): + raise FileTransferError(message=f"Image file {image_name} missing after reboot") + log.info("Host %s: Image file verified after reboot.", self.host) + + # Retry install after clearing pending state + log.info("Host %s: Retrying install after clearing pending reboot state.", self.host) + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + issu=issu, + nssu=nssu, + timeout=3600, + ) + except Exception as retry_error: + log.error( + "Host %s: Failed to clear pending reboot state: %s", + self.host, + retry_error, + ) + raise + else: + # Re-raise if it's a different RPC error + raise + except ConnectClosedError: + # Connection loss during install is expected (device reboots) + log.info( + "Host %s: Connection closed during install (expected for device reboot). " + "Waiting for device to come back online.", + self.host, + ) + + try: + # Wait for device to come back up + self._wait_for_device_reboot(pre_install_uptime) + log.info("Host %s: Device reconnected after reboot.", self.host) + + # Validate OS version changed + self._uptime = None # Force refresh + post_install_version = self.os_version + + if post_install_version == pre_install_version: + log.error( + "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", + self.host, + pre_install_version, + post_install_version, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + log.info( + "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", + self.host, + pre_install_version, + post_install_version, + ) + install_ok = True + + except Exception as reboot_error: + log.error( + "Host %s: Failed during post-reboot verification: %s", + self.host, + reboot_error, + ) + raise # Sometimes install() returns a tuple of (ok, msg). Other times it returns a single bool if isinstance(install_ok, tuple): @@ -537,11 +725,15 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", if not install_ok: raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + if nssu: + self.request_system_snapshot("slice alternate all-members") + if not reboot: log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) return True - self.reboot(wait_for_reload=True) + if not nssu: + self.reboot(wait_for_reload=True) def open(self): """Open connection to device.""" @@ -719,6 +911,76 @@ def compare_file_checksum(self, checksum, filename, hashing_algorithm="md5"): """ return checksum == self.get_remote_checksum(filename, hashing_algorithm) + @staticmethod + def _netloc(src: FileCopyModel) -> str: + """Return host:port or just host from a FileCopyModel.""" + return f"{src.hostname}:{src.port}" if src.port else src.hostname + + @staticmethod + def _source_path(src: FileCopyModel) -> str: + """Return the file path from URL, using file_name if path is empty.""" + return src.path if src.path and src.path != "/" else f"/{src.file_name}" + + def remote_file_copy_rpc(self, src: FileCopyModel, dest, timeout=900): + """Copy file via raw RPC call for detailed error messages. + + Uses native.rpc.file_copy() instead of fs.cp() to capture device error details. + Embeds credentials in URL if username/token provided. Verifies file with retries. + + Args: + src (FileCopyModel): Source file model with URL and credentials. + dest (str): Destination path on device. + timeout (int): RPC timeout in seconds. Defaults to 900. + + Raises: + FileTransferError: If RPC call fails or file verification fails. + + Returns: + bool: True if file copied and verified successfully. + """ + # Build URL with embedded credentials if provided + netloc = self._netloc(src) + path = self._source_path(src) + + if src.username and src.token: + source_url = f"{src.scheme}://{src.username}:{src.token}@{netloc}{path}" + else: + source_url = f"{src.scheme}://{netloc}{path}" + + log.debug("Host %s: Attempting RPC file copy from %s to %s", self.host, source_url, dest) + + try: + # Issue the RPC file-copy command + response = self.native.rpc.file_copy( + source=source_url, + destination=dest, + dev_timeout=timeout, + ) + log.debug("Host %s: file_copy RPC response: %s", self.host, response) + + except Exception as rpc_error: + log.error("Host %s: File copy RPC failed: %s", self.host, rpc_error) + raise FileTransferError(message=f"File copy RPC failed: {str(rpc_error)}") + + # Verify file exists and matches checksum (with retries) + for attempt in range(1, 6): + try: + if self.verify_file(src.checksum, dest, hashing_algorithm=src.hashing_algorithm): + log.info("Host %s: File copy verified on attempt %d", self.host, attempt) + return True + + if attempt < 5: + log.debug("Host %s: File verification failed on attempt %d; retrying in 30s", self.host, attempt) + time.sleep(30) + + except Exception as verify_error: + log.debug("Host %s: File verification check failed (%s); retry %d/5", self.host, verify_error, attempt) + if attempt < 5: + time.sleep(30) + + log.error("Host %s: File copy verification failed after 5 attempts", self.host) + raise FileTransferError(message="File copy verification failed after 5 attempts") + def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: str | None = None, **kwargs): """Copy a file to a remote device. From ff4f8a5a5e1b6c1b4fd2989bdea09d80b43e77d4 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 7 Jul 2026 13:44:48 -0400 Subject: [PATCH 03/14] Update to install os on multiple devices without nssu --- pyntc/devices/jnpr_device.py | 285 ++++++++++++++++++++++++----------- 1 file changed, 194 insertions(+), 91 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 7d9a5373..623f8619 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -15,7 +15,6 @@ from jnpr.junos.utils.fs import FS as JunosNativeFS from jnpr.junos.utils.scp import SCP from jnpr.junos.utils.sw import SW as JunosNativeSW -from lxml import etree from pyntc import log from pyntc.devices.base_device import BaseDevice, fix_docs @@ -285,7 +284,6 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=180): Periodically checks ``show system snapshot media internal`` to verify the snapshot was successfully taken. Used when the ``request system snapshot`` RPC times out. - Attempts XML parsing for structured verification; falls back to string matching. Args: timeout (int, optional): Max seconds to wait for snapshot verification. Defaults to 1800 (30 minutes). @@ -298,30 +296,11 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=180): while time.time() - start < timeout: try: - # Try structured XML approach first - try: - xml_output = self.native.rpc.request_shell_execute( - command="show system snapshot media internal | display xml" - ) - if xml_output: - root = etree.fromstring(xml_output.encode()) - # Check if any snapshot-medium elements exist (indicates snapshots present) - snapshots = root.findall(".//snapshot-medium") - if snapshots: - log.info("Host %s: System snapshot verified via XML parsing.", self.host) - return - log.debug( - "Host %s: Snapshot verification in progress (no snapshots found yet); will retry.", - self.host, - ) - except Exception as xml_exc: # pylint: disable=broad-exception-caught - # XML parsing failed, try fallback text matching - log.debug("Host %s: XML parsing failed (%s); trying text fallback.", self.host, xml_exc) - output = self.native.cli("show system snapshot media internal") - if "snapshot" in output.lower(): - log.info("Host %s: System snapshot verified via text output.", self.host) - return - log.debug("Host %s: Snapshot verification in progress; will retry.", self.host) + output = self.native.cli("show system snapshot media internal") + if "snapshot" in output.lower(): + log.info("Host %s: System snapshot verified.", self.host) + return + log.debug("Host %s: Snapshot verification in progress; will retry.", self.host) except Exception as exc: # pylint: disable=broad-exception-caught log.debug("Host %s: Snapshot verification poll failed (%s); will retry.", self.host, exc) @@ -364,19 +343,30 @@ def request_system_snapshot(self, parameters=None): if parameters is not None: command = f"{command} {parameters}" + response = None + rpc_timed_out = False + try: log.debug("Host %s: Issuing RPC: %s", self.host, command) - response = self.native.rpc.cli(command) - log.debug("Host %s: snapshot RPC completed successfully.", self.host) + response = self.native.rpc.cli(command=command, format="text") + log.debug("Host %s: snapshot RPC completed.", self.host) except RpcTimeoutError: - log.debug("Host %s: snapshot RPC timed out; polling device to verify.", self.host) + log.debug("Host %s: snapshot RPC timed out; will poll device to verify.", self.host) + rpc_timed_out = True except Exception as rpc_error: log.error("Host %s: Failed to request system snapshot (%s).", self.host, rpc_error) raise - # Verify snapshot completed on device (handles both RPC timeout and normal completion) + # Check if snapshot completed in the response (fast path) + if response is not None and "filesystems were archived" in str(response): + log.info("Host %s: Snapshot completed successfully (fast path).", self.host) + return + + # If RPC timed out or response didn't indicate completion, poll to verify (slow path) + if rpc_timed_out or response is None: + log.debug("Host %s: Snapshot response unclear or timed out; polling device to verify.", self.host) self._wait_for_system_snapshot() @property @@ -598,7 +588,9 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False): + def install_os( # pylint: disable=too-many-positional-arguments,too-many-branches,too-many-statements + self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False + ): """Install OS on device and reboot. Args: @@ -735,6 +727,177 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", if not nssu: self.reboot(wait_for_reload=True) + def install_os_multi( # pylint: disable=too-many-branches,too-many-statements + self, + image_name: str, + checksum: str, + hashing_algorithm: str = "md5", + reboot=True, + ) -> bool: + """Install OS on Juniper virtual-chassis devices. + + Uses sw.install(reboot=True) which applies to all members simultaneously, + followed by explicit reboot of all members and snapshot of alternate partitions. + + Flow: + 1. Install all members (sw.install with reboot) and wait for reconnection with version verification + 2. Explicit reboot of all members and wait for reconnection + 3. Snapshot alternate partitions on all members + + Args: + image_name (str): Path to OS image file on device (/var/tmp/image.tgz). + checksum (str): Checksum of the image file. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). Defaults to 'md5'. + reboot (bool): Whether to perform the upgrade sequence. Defaults to True. + + Returns: + bool: True if upgrade successful. + + Raises: + FileTransferError: If image file not found or checksum mismatch. + OSInstallError: If upgrade fails. + """ + # Pre-install checks + log.info("Host %s: Starting virtual-chassis OS upgrade", self.host) + + if not self.check_file_exists(image_name): + raise FileTransferError(message=f"Image {image_name} not found") + + if not self.compare_file_checksum(checksum, image_name, hashing_algorithm): + raise FileTransferError(message=f"Checksum mismatch for {image_name}") + + log.info("Host %s: Image verified - %s", self.host, image_name) + + # Capture pre-install state + self._uptime = None + pre_install_uptime = self.uptime + pre_install_version = self.os_version + + log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_install_version, pre_install_uptime) + + # 1: Install with automatic first reboot + try: + log.info("Host %s: Calling sw.install(reboot=True) for all members", self.host) + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + reboot=reboot, + timeout=3600, + ) + except RpcError as rpc_error: + if "reboot pending for software rollback" in str(rpc_error).lower(): + log.warning("Host %s: Device has pending reboot state; clearing with reboot.", self.host) + self._uptime = None + pre_reboot_uptime = self.uptime + try: + self.native.rpc.request_reboot() + log.info("Host %s: Reboot issued to clear pending state.", self.host) + self._wait_for_device_reboot(pre_reboot_uptime) + log.info("Host %s: Device rebooted and reconnected.", self.host) + + if not self.check_file_exists(image_name): + raise FileTransferError(message=f"Image file {image_name} missing after reboot") + log.info("Host %s: Image file verified after reboot.", self.host) + + log.info("Host %s: Retrying install after clearing pending state.", self.host) + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + reboot=reboot, + timeout=3600, + ) + except Exception as retry_error: + log.error("Host %s: Failed to clear pending state: %s", self.host, retry_error) + raise + else: + raise + except ConnectClosedError: + log.info( + "Host %s: Connection closed during install (expected for device reboot). " + "Waiting for device to come back online.", + self.host, + ) + + try: + self._wait_for_device_reboot(pre_install_uptime) + log.info("Host %s: Device reconnected after reboot.", self.host) + + self._uptime = None + post_install_version = self.os_version + + if post_install_version == pre_install_version: + log.error( + "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", + self.host, + pre_install_version, + post_install_version, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + log.info( + "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", + self.host, + pre_install_version, + post_install_version, + ) + install_ok = True + + except Exception as reboot_error: + log.error("Host %s: Failed during post-reboot verification: %s", self.host, reboot_error) + raise + + if isinstance(install_ok, tuple): + install_ok = install_ok[0] + + if not install_ok: + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + if not reboot: + log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) + return True + + # 2: Issue explicit reboot of all members + log.info("Host %s: Issuing explicit reboot of all members", self.host) + self._uptime = None + pre_reboot_uptime = self.uptime + + try: + self.native.rpc.cli(command="request system reboot all-members", format="text") + log.info("Host %s: Reboot all-members command issued", self.host) + except ConnectClosedError: + log.debug("Host %s: Connection closed during reboot command (expected)", self.host) + except Exception as reboot_error: + log.error("Host %s: Failed to issue reboot command: %s", self.host, reboot_error) + raise + + # Wait for device to come back after second reboot + try: + self._wait_for_device_reboot(pre_reboot_uptime) + log.info("Host %s: Device reconnected after all-members reboot", self.host) + except Exception as wait_error: + log.error("Host %s: Failed waiting for device after reboot: %s", self.host, wait_error) + raise + + # 3: Snapshot alternate partitions on all members + log.info("Host %s: Taking snapshot of alternate partitions on all members", self.host) + try: + self.request_system_snapshot("slice alternate all-members") + log.info("Host %s: Snapshot completed successfully", self.host) + except Exception as snap_error: + log.error("Host %s: Snapshot failed: %s", self.host, snap_error) + raise + + log.info("Host %s: Virtual-chassis OS upgrade completed successfully", self.host) + return True + def open(self): """Open connection to device.""" if not self.connected: @@ -921,66 +1084,6 @@ def _source_path(src: FileCopyModel) -> str: """Return the file path from URL, using file_name if path is empty.""" return src.path if src.path and src.path != "/" else f"/{src.file_name}" - def remote_file_copy_rpc(self, src: FileCopyModel, dest, timeout=900): - """Copy file via raw RPC call for detailed error messages. - - Uses native.rpc.file_copy() instead of fs.cp() to capture device error details. - Embeds credentials in URL if username/token provided. Verifies file with retries. - - Args: - src (FileCopyModel): Source file model with URL and credentials. - dest (str): Destination path on device. - timeout (int): RPC timeout in seconds. Defaults to 900. - - Raises: - FileTransferError: If RPC call fails or file verification fails. - - Returns: - bool: True if file copied and verified successfully. - """ - # Build URL with embedded credentials if provided - netloc = self._netloc(src) - path = self._source_path(src) - - if src.username and src.token: - source_url = f"{src.scheme}://{src.username}:{src.token}@{netloc}{path}" - else: - source_url = f"{src.scheme}://{netloc}{path}" - - log.debug("Host %s: Attempting RPC file copy from %s to %s", self.host, source_url, dest) - - try: - # Issue the RPC file-copy command - response = self.native.rpc.file_copy( - source=source_url, - destination=dest, - dev_timeout=timeout, - ) - log.debug("Host %s: file_copy RPC response: %s", self.host, response) - - except Exception as rpc_error: - log.error("Host %s: File copy RPC failed: %s", self.host, rpc_error) - raise FileTransferError(message=f"File copy RPC failed: {str(rpc_error)}") - - # Verify file exists and matches checksum (with retries) - for attempt in range(1, 6): - try: - if self.verify_file(src.checksum, dest, hashing_algorithm=src.hashing_algorithm): - log.info("Host %s: File copy verified on attempt %d", self.host, attempt) - return True - - if attempt < 5: - log.debug("Host %s: File verification failed on attempt %d; retrying in 30s", self.host, attempt) - time.sleep(30) - - except Exception as verify_error: - log.debug("Host %s: File verification check failed (%s); retry %d/5", self.host, verify_error, attempt) - if attempt < 5: - time.sleep(30) - - log.error("Host %s: File copy verification failed after 5 attempts", self.host) - raise FileTransferError(message="File copy verification failed after 5 attempts") - def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: str | None = None, **kwargs): """Copy a file to a remote device. From c1db014e6a55c9da84919e1f16911ce98c2ea2d1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 7 Jul 2026 19:22:39 -0400 Subject: [PATCH 04/14] Add post-install re_info validation for OS upgrades --- pyntc/devices/jnpr_device.py | 600 +++++++++++++------- tests/unit/test_devices/test_jnpr_device.py | 235 +++++++- 2 files changed, 616 insertions(+), 219 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 623f8619..a1b8585d 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -9,8 +9,10 @@ from urllib.parse import urlparse from jnpr.junos import Device as JunosNativeDevice -from jnpr.junos.exception import ConfigLoadError, ConnectClosedError, RpcError, RpcTimeoutError -from jnpr.junos.op.ethport import EthPortTable # pylint: disable=import-error,no-name-in-module +from jnpr.junos.exception import (ConfigLoadError, ConnectClosedError, + RpcError, RpcTimeoutError) +from jnpr.junos.op.ethport import \ + EthPortTable # pylint: disable=import-error,no-name-in-module from jnpr.junos.utils.config import Config as JunosNativeConfig from jnpr.junos.utils.fs import FS as JunosNativeFS from jnpr.junos.utils.scp import SCP @@ -18,15 +20,11 @@ from pyntc import log from pyntc.devices.base_device import BaseDevice, fix_docs -from pyntc.devices.tables.jnpr.loopback import LoopbackTable # pylint: disable=no-name-in-module -from pyntc.errors import ( - CommandError, - CommandListError, - FileSystemNotFoundError, - FileTransferError, - OSInstallError, - RebootTimeoutError, -) +from pyntc.devices.tables.jnpr.loopback import \ + LoopbackTable # pylint: disable=no-name-in-module +from pyntc.errors import (CommandError, CommandListError, + FileSystemNotFoundError, FileTransferError, + OSInstallError, RebootTimeoutError) from pyntc.utils.models import FileCopyModel # Multipliers for Junos ``df``-style size suffixes. Junos formats available @@ -94,6 +92,7 @@ def __init__(self, host, username, password, *args, **kwargs): # noqa: D403 self.cu = JunosNativeConfig(self.native) # pylint: disable=invalid-name self.fs = JunosNativeFS(self.native) # pylint: disable=invalid-name self.sw = JunosNativeSW(self.native) # pylint: disable=invalid-name + self._is_virtual_chassis = None def _file_copy_local_file_exists(self, filepath): return os.path.isfile(filepath) @@ -540,6 +539,14 @@ def serial_number(self): return self._serial_number + @property + def is_virtual_chassis(self) -> bool: + """Check if device is in virtual-chassis mode.""" + if self._is_virtual_chassis is None: + self._is_virtual_chassis = self.native.facts.get("vc_capable", False) + + return self._is_virtual_chassis + def file_copy(self, src, dest=None, **kwargs): """Copy file to device via SCP. @@ -588,178 +595,147 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def install_os( # pylint: disable=too-many-positional-arguments,too-many-branches,too-many-statements - self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False - ): - """Install OS on device and reboot. + def _validate_member_status(self): + """Validate and log virtual-chassis member status. - Args: - image_name (str): Name of image. - reboot (bool): Whether to reboot the device after setting the boot options. Defaults to true. Ignored if nssu is true. - checksum (str): The checksum of the file. - hashing_algorithm (str): The hashing algorithm to use. Valid values are 'md5', 'sha1', and 'sha256'. Defaults to 'md5'. - issu (bool): Whether to perform an In-Service Software Upgrade (ISSU). Defaults to false. - nssu (bool): Whether to perform a Non-Stop Software Upgrade (NSSU). Defaults to false. When true, reboot is performed automatically. - """ - # Capture pre-install state for validation after reboot - self._uptime = None - pre_install_uptime = self.uptime - pre_install_version = self.os_version + Checks re_info for: + - Member count + - Each member's status (should be 'OK') + - Mastership state (master/backup) + - Last reboot reason - log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_install_version, pre_install_uptime) - - try: - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - issu=issu, - nssu=nssu, - timeout=3600, - ) - except RpcError as rpc_error: - # Check if error is due to pending reboot state from previous incomplete operation - if "reboot pending for software rollback" in str(rpc_error).lower(): - log.warning( - "Host %s: Device has pending reboot state; clearing with reboot.", - self.host, - ) - # Capture uptime before reboot for verification - self._uptime = None - pre_reboot_uptime = self.uptime + Returns: + dict: Member status info indexed by member ID. - try: - # Clear pending reboot state - self.native.rpc.request_reboot() - log.info("Host %s: Reboot issued to clear pending state.", self.host) + Raises: + OSInstallError: If any member status is not 'OK'. + """ + re_info = self.native.facts.get("re_info", {}).get("default", {}) + if not re_info: + log.warning("Host %s: No re_info available; cannot validate member status", self.host) + return {} + + member_status = {} + for member_id, member_info in re_info.items(): + if member_id == "default": + continue - # Wait for device to come back up - self._wait_for_device_reboot(pre_reboot_uptime) - log.info("Host %s: Device rebooted and reconnected.", self.host) + status = member_info.get("status", "UNKNOWN") + mastership = member_info.get("mastership_state", "UNKNOWN") + reboot_reason = member_info.get("last_reboot_reason", "UNKNOWN") + model = member_info.get("model", "UNKNOWN") - # Verify image file still exists - if not self.check_file_exists(image_name): - raise FileTransferError(message=f"Image file {image_name} missing after reboot") - log.info("Host %s: Image file verified after reboot.", self.host) + member_status[member_id] = { + "status": status, + "mastership": mastership, + "reboot_reason": reboot_reason, + "model": model, + } - # Retry install after clearing pending state - log.info("Host %s: Retrying install after clearing pending reboot state.", self.host) - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - issu=issu, - nssu=nssu, - timeout=3600, - ) - except Exception as retry_error: - log.error( - "Host %s: Failed to clear pending reboot state: %s", - self.host, - retry_error, - ) - raise - else: - # Re-raise if it's a different RPC error - raise - except ConnectClosedError: - # Connection loss during install is expected (device reboots) log.info( - "Host %s: Connection closed during install (expected for device reboot). " - "Waiting for device to come back online.", + "Host %s: Member %s - Status: %s, Mastership: %s, Model: %s", self.host, + member_id, + status, + mastership, + model, ) + log.debug("Host %s: Member %s reboot reason: %s", self.host, member_id, reboot_reason) - try: - # Wait for device to come back up - self._wait_for_device_reboot(pre_install_uptime) - log.info("Host %s: Device reconnected after reboot.", self.host) + if status != "OK": + log.error("Host %s: Member %s status is %s (expected OK)", self.host, member_id, status) + raise OSInstallError(hostname=self.hostname, desired_boot="N/A") - # Validate OS version changed - self._uptime = None # Force refresh - post_install_version = self.os_version + return member_status - if post_install_version == pre_install_version: - log.error( - "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", - self.host, - pre_install_version, - post_install_version, - ) - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + def _validate_post_install_state(self, expected_version): + """Validate post-install state via re_info. - log.info( - "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", - self.host, - pre_install_version, - post_install_version, - ) - install_ok = True + Checks re_info for: + - All members in 'OK' status + - All members running expected version + - Reboot reasons don't indicate failures + + Args: + expected_version (str): Expected OS version after install. - except Exception as reboot_error: + Raises: + OSInstallError: If any member is not in expected state. + """ + re_info = self.native.facts.get("re_info", {}).get("default", {}) + if not re_info: + log.warning("Host %s: No re_info available for post-install validation", self.host) + return + + for member_id, member_info in re_info.items(): + if member_id == "default": + continue + + status = member_info.get("status", "UNKNOWN") + reboot_reason = member_info.get("last_reboot_reason", "UNKNOWN") + model = member_info.get("model", "UNKNOWN") + + if status != "OK": log.error( - "Host %s: Failed during post-reboot verification: %s", + "Host %s: Member %s status is %s (expected OK) after install", self.host, - reboot_error, + member_id, + status, ) - raise + raise OSInstallError(hostname=self.hostname, desired_boot=expected_version) - # Sometimes install() returns a tuple of (ok, msg). Other times it returns a single bool - if isinstance(install_ok, tuple): - install_ok = install_ok[0] + log.info( + "Host %s: Member %s post-install state validated - Status: OK, Model: %s, Reboot reason: %s", + self.host, + member_id, + model, + reboot_reason, + ) - if not install_ok: - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + def _is_multiple_device_setup(self) -> bool: + """Check if device is in a multiple-device setup (virtual-chassis or chassis-cluster). - if nssu: - self.request_system_snapshot("slice alternate all-members") + Validates that: + - Device is vc_capable + - Multiple routing engines exist in re_info + - All members have status 'OK' - if not reboot: - log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) - return True + Returns: + bool: True if device is part of a multiple-device setup, False otherwise. - if not nssu: - self.reboot(wait_for_reload=True) + Raises: + OSInstallError: If any member status is not 'OK'. + """ + if not self.is_virtual_chassis: + return False + + re_info = self.native.facts.get("re_info", {}).get("default", {}) + members = {k: v for k, v in re_info.items() if k != "default"} - def install_os_multi( # pylint: disable=too-many-branches,too-many-statements - self, - image_name: str, - checksum: str, - hashing_algorithm: str = "md5", - reboot=True, - ) -> bool: - """Install OS on Juniper virtual-chassis devices. + if len(members) <= 1: + log.debug( + "Host %s: vc_capable=True but only %d members; treating as single device", self.host, len(members) + ) + return False - Uses sw.install(reboot=True) which applies to all members simultaneously, - followed by explicit reboot of all members and snapshot of alternate partitions. + log.info("Host %s: Detected multiple-device setup with %d members", self.host, len(members)) + self._validate_member_status() + return True - Flow: - 1. Install all members (sw.install with reboot) and wait for reconnection with version verification - 2. Explicit reboot of all members and wait for reconnection - 3. Snapshot alternate partitions on all members + def _validate_and_capture_preinstall_state(self, image_name, checksum, hashing_algorithm): + """Validate image and capture pre-install state. Args: - image_name (str): Path to OS image file on device (/var/tmp/image.tgz). + image_name (str): Path to OS image file on device. checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). Defaults to 'md5'. - reboot (bool): Whether to perform the upgrade sequence. Defaults to True. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). Returns: - bool: True if upgrade successful. + tuple: (pre_uptime, pre_version) Raises: FileTransferError: If image file not found or checksum mismatch. - OSInstallError: If upgrade fails. """ - # Pre-install checks - log.info("Host %s: Starting virtual-chassis OS upgrade", self.host) - if not self.check_file_exists(image_name): raise FileTransferError(message=f"Image {image_name} not found") @@ -768,16 +744,212 @@ def install_os_multi( # pylint: disable=too-many-branches,too-many-statements log.info("Host %s: Image verified - %s", self.host, image_name) - # Capture pre-install state self._uptime = None - pre_install_uptime = self.uptime - pre_install_version = self.os_version + pre_uptime = self.uptime + pre_version = self.os_version + + log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_version, pre_uptime) + + return pre_uptime, pre_version + + def _wait_and_verify_upgrade(self, pre_uptime, pre_version, image_name): + """Wait for device reboot and verify OS version changed. + + Args: + pre_uptime (int): Uptime before upgrade. + pre_version (str): OS version before upgrade. + image_name (str): Name of image being installed. + + Raises: + OSInstallError: If OS version did not change after reboot. + """ + self._wait_for_device_reboot(pre_uptime) + log.info("Host %s: Device reconnected after reboot.", self.host) + + self._uptime = None + post_version = self.os_version + + if post_version == pre_version: + log.error( + "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", + self.host, + pre_version, + post_version, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + log.info( + "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", + self.host, + pre_version, + post_version, + ) + + def _reboot_all_members_and_wait(self, pre_reboot_uptime): + """Issue explicit reboot of all members and wait for reconnection. + + Args: + pre_reboot_uptime (int): Uptime before reboot. + + Raises: + ConnectClosedError or Exception: If reboot command fails or device doesn't reconnect. + """ + log.info("Host %s: Issuing explicit reboot of all members", self.host) + try: + self.native.rpc.cli(command="request system reboot all-members", format="text") + log.info("Host %s: Reboot all-members command issued", self.host) + except ConnectClosedError: + log.debug("Host %s: Connection closed during reboot command (expected)", self.host) + except Exception as reboot_error: + log.error("Host %s: Failed to issue reboot command: %s", self.host, reboot_error) + raise + + try: + self._wait_for_device_reboot(pre_reboot_uptime) + log.info("Host %s: Device reconnected after all-members reboot", self.host) + except Exception as wait_error: + log.error("Host %s: Failed waiting for device after reboot: %s", self.host, wait_error) + raise + + def _disruptive_install_single(self, image_name, checksum, hashing_algorithm, pre_uptime, pre_version): + """Perform a disruptive OS installation on a single device. + + Args: + image_name (str): Path to OS image file on device. + checksum (str): Checksum of the image file. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). + pre_uptime (int): Uptime before upgrade. + pre_version (str): OS version before upgrade. + """ + log.info("Host %s: Installing software update with disruptive reboot (single device).", self.host) + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + reboot=True, + timeout=3600, + ) + + self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) + self._validate_post_install_state(self.os_version) + + if isinstance(install_ok, tuple): + install_ok = install_ok[0] + + if not install_ok: + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + def _disruptive_install_multiple(self, image_name, checksum, hashing_algorithm, pre_uptime, pre_version): + """Perform a disruptive OS installation on multiple-device virtual-chassis. + + Includes explicit reboot of all members and snapshot of alternate partitions. + + Args: + image_name (str): Path to OS image file on device. + checksum (str): Checksum of the image file. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). + pre_uptime (int): Uptime before upgrade. + pre_version (str): OS version before upgrade. + """ + log.info("Host %s: Installing software update with disruptive reboot (multiple-device).", self.host) + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + reboot=True, + timeout=3600, + ) + + self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) + self._validate_post_install_state(self.os_version) + + if isinstance(install_ok, tuple): + install_ok = install_ok[0] + + if not install_ok: + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + self._uptime = None + pre_reboot_uptime = self.uptime + self._reboot_all_members_and_wait(pre_reboot_uptime) + self.request_system_snapshot("slice alternate all-members") + self._validate_post_install_state(self.os_version) + + def _non_disruptive_install_single( + self, image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + ): + """Perform a non-disruptive OS installation on a single device. + + Args: + image_name (str): Path to OS image file on device. + checksum (str): Checksum of the image file. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). + issu (bool): Whether to perform ISSU. + nssu (bool): Whether to perform NSSU. + pre_uptime (int): Uptime before upgrade. + pre_version (str): OS version before upgrade. + """ + log.info("Host %s: Installing software update using ISSU or NSSU (single device).", self.host) + try: + install_ok = self.sw.install( + package=image_name, + checksum=checksum, + checksum_algorithm=hashing_algorithm, + progress=True, + validate=True, + no_copy=True, + reboot=False, + issu=issu, + nssu=nssu, + timeout=3600, + ) + except RpcError as rpc_error: + if "reboot pending for software rollback" in str(rpc_error).lower(): + log.warning( + "Host %s: Device has pending reboot state; A manual reboot will be required to continue.", + self.host, + ) + raise + except ConnectClosedError: + log.info( + "Host %s: Connection closed during install (expected for device reboot). " + "Waiting for device to come back online.", + self.host, + ) + + self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) + self._validate_post_install_state(self.os_version) + + if isinstance(install_ok, tuple): + install_ok = install_ok[0] + + if not install_ok: + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) + + def _non_disruptive_install_multiple( + self, image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + ): + """Perform a non-disruptive OS installation on multiple-device virtual-chassis. - log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_install_version, pre_install_uptime) + Includes explicit reboot of all members and snapshot of alternate partitions. - # 1: Install with automatic first reboot + Args: + image_name (str): Path to OS image file on device. + checksum (str): Checksum of the image file. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). + issu (bool): Whether to perform ISSU. + nssu (bool): Whether to perform NSSU. + pre_uptime (int): Uptime before upgrade. + pre_version (str): OS version before upgrade. + """ + log.info("Host %s: Installing software update using ISSU or NSSU (multiple-device).", self.host) try: - log.info("Host %s: Calling sw.install(reboot=True) for all members", self.host) install_ok = self.sw.install( package=image_name, checksum=checksum, @@ -785,7 +957,9 @@ def install_os_multi( # pylint: disable=too-many-branches,too-many-statements progress=True, validate=True, no_copy=True, - reboot=reboot, + reboot=False, + issu=issu, + nssu=nssu, timeout=3600, ) except RpcError as rpc_error: @@ -811,7 +985,9 @@ def install_os_multi( # pylint: disable=too-many-branches,too-many-statements progress=True, validate=True, no_copy=True, - reboot=reboot, + reboot=False, + issu=issu, + nssu=nssu, timeout=3600, ) except Exception as retry_error: @@ -826,33 +1002,8 @@ def install_os_multi( # pylint: disable=too-many-branches,too-many-statements self.host, ) - try: - self._wait_for_device_reboot(pre_install_uptime) - log.info("Host %s: Device reconnected after reboot.", self.host) - - self._uptime = None - post_install_version = self.os_version - - if post_install_version == pre_install_version: - log.error( - "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", - self.host, - pre_install_version, - post_install_version, - ) - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - log.info( - "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", - self.host, - pre_install_version, - post_install_version, - ) - install_ok = True - - except Exception as reboot_error: - log.error("Host %s: Failed during post-reboot verification: %s", self.host, reboot_error) - raise + self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) + self._validate_post_install_state(self.os_version) if isinstance(install_ok, tuple): install_ok = install_ok[0] @@ -860,42 +1011,63 @@ def install_os_multi( # pylint: disable=too-many-branches,too-many-statements if not install_ok: raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - if not reboot: - log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) - return True - - # 2: Issue explicit reboot of all members - log.info("Host %s: Issuing explicit reboot of all members", self.host) self._uptime = None pre_reboot_uptime = self.uptime + self._reboot_all_members_and_wait(pre_reboot_uptime) + self.request_system_snapshot("slice alternate all-members") + self._validate_post_install_state(self.os_version) - try: - self.native.rpc.cli(command="request system reboot all-members", format="text") - log.info("Host %s: Reboot all-members command issued", self.host) - except ConnectClosedError: - log.debug("Host %s: Connection closed during reboot command (expected)", self.host) - except Exception as reboot_error: - log.error("Host %s: Failed to issue reboot command: %s", self.host, reboot_error) - raise + def _disruptive_install(self, image_name, checksum, hashing_algorithm, reboot): + """Perform a disruptive OS installation.""" - # Wait for device to come back after second reboot - try: - self._wait_for_device_reboot(pre_reboot_uptime) - log.info("Host %s: Device reconnected after all-members reboot", self.host) - except Exception as wait_error: - log.error("Host %s: Failed waiting for device after reboot: %s", self.host, wait_error) - raise + def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False) -> bool: + """Install OS on device. - # 3: Snapshot alternate partitions on all members - log.info("Host %s: Taking snapshot of alternate partitions on all members", self.host) - try: - self.request_system_snapshot("slice alternate all-members") - log.info("Host %s: Snapshot completed successfully", self.host) - except Exception as snap_error: - log.error("Host %s: Snapshot failed: %s", self.host, snap_error) - raise + Supports both disruptive and non-disruptive upgrades on single and multiple-device setups. + Automatically detects virtual-chassis and applies all-members operations when needed. + + Args: + image_name (str): Path to OS image file on device. + checksum (str): Checksum of the image file. + reboot (bool): Whether to reboot the device. Defaults to True. Ignored if nssu is True. + hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). Defaults to 'md5'. + issu (bool): Whether to perform ISSU. Defaults to False. + nssu (bool): Whether to perform NSSU. Defaults to False. When True, reboot is automatic. + + Returns: + bool: True if upgrade successful. + + Raises: + FileTransferError: If image file not found or checksum mismatch. + OSInstallError: If upgrade fails. + """ + pre_uptime, pre_version = self._validate_and_capture_preinstall_state(image_name, checksum, hashing_algorithm) + + is_multiple_device = self._is_multiple_device_setup() + is_disruptive = not (issu or nssu or not reboot) + + if is_disruptive: + if is_multiple_device: + self._disruptive_install_multiple(image_name, checksum, hashing_algorithm, pre_uptime, pre_version) + else: + self._disruptive_install_single(image_name, checksum, hashing_algorithm, pre_uptime, pre_version) + else: + if is_multiple_device: + self._non_disruptive_install_multiple( + image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + ) + else: + self._non_disruptive_install_single( + image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + ) + + if not reboot: + log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) + return True + + if not nssu: + self.reboot(wait_for_reload=True) - log.info("Host %s: Virtual-chassis OS upgrade completed successfully", self.host) return True def open(self): diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 28271a64..354a7d6f 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -427,17 +427,27 @@ def test_file_copy_local_md5(self): result = self.device._file_copy_local_md5(fp.name) self.assertEqual(result, checksum) - def test_install_os(self): + @mock.patch.object(JunosDevice, "_validate_post_install_state") + @mock.patch.object(JunosDevice, "_validate_and_capture_preinstall_state") + @mock.patch.object(JunosDevice, "_is_multiple_device_setup") + def test_install_os(self, mock_is_multi, mock_validate_preinstall, mock_validate_postinstall): + mock_validate_preinstall.return_value = (1000, "15.1F4.15") + mock_is_multi.return_value = False + with mock.patch.object(self.device, "reboot") as mock_reboot: with self.subTest("sw.install returns a bool"): self.device.sw.install.return_value = True - self.device.install_os(image_name="image.bin", checksum="c0ffee") - mock_reboot.assert_called_once_with(wait_for_reload=True) + with mock.patch.object(JunosDevice, "_wait_and_verify_upgrade"): + self.device.install_os(image_name="image.bin", checksum="c0ffee") + mock_reboot.assert_called_once_with(wait_for_reload=True) + mock_validate_postinstall.assert_called() with self.subTest("sw.install returns a tuple and fails"): self.device.sw.install.return_value = (False, "install failure") - with self.assertRaises(OSInstallError): - self.device.install_os(image_name="image.bin", checksum="c0ffee") + mock_reboot.reset_mock() + with mock.patch.object(JunosDevice, "_wait_and_verify_upgrade"): + with self.assertRaises(OSInstallError): + self.device.install_os(image_name="image.bin", checksum="c0ffee") def test_check_file_exists(self): self.device.check_file_exists("foo.txt") @@ -563,6 +573,221 @@ def test_get_remote_checksum_rejects_unsupported_algorithm(self): assert "sha512" in str(ctx.exception) self.device.fs.checksum.assert_not_called() + def test_is_virtual_chassis_true(self): + """Test is_virtual_chassis returns True when vc_capable is set.""" + self.device.native.facts["vc_capable"] = True + self.assertTrue(self.device.is_virtual_chassis) + + def test_is_virtual_chassis_false(self): + """Test is_virtual_chassis returns False when vc_capable is False.""" + self.device.native.facts["vc_capable"] = False + self.assertFalse(self.device.is_virtual_chassis) + + def test_is_virtual_chassis_caches_value(self): + """Test is_virtual_chassis caches the value after first access.""" + self.device.native.facts["vc_capable"] = True + first_call = self.device.is_virtual_chassis + self.device.native.facts["vc_capable"] = False + second_call = self.device.is_virtual_chassis + self.assertEqual(first_call, second_call) + self.assertTrue(second_call) + + def test_validate_member_status_ok(self): + """Test _validate_member_status with all members OK.""" + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + "member1": { + "status": "OK", + "mastership_state": "backup", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + } + } + result = self.device._validate_member_status() + self.assertEqual(len(result), 2) + self.assertEqual(result["member0"]["status"], "OK") + self.assertEqual(result["member1"]["status"], "OK") + + def test_validate_member_status_raises_on_bad_status(self): + """Test _validate_member_status raises OSInstallError if member status is not OK.""" + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + "member1": { + "status": "DOWN", + "mastership_state": "unknown", + "last_reboot_reason": "unknown", + "model": "EX3300", + }, + } + } + with pytest.raises(OSInstallError): + self.device._validate_member_status() + + def test_is_multiple_device_setup_single_device(self): + """Test _is_multiple_device_setup returns False for single device.""" + self.device.native.facts["vc_capable"] = False + self.assertFalse(self.device._is_multiple_device_setup()) + + def test_is_multiple_device_setup_vc_capable_one_member(self): + """Test _is_multiple_device_setup returns False for vc_capable with only 1 member.""" + self.device._is_virtual_chassis = None + self.device.native.facts["vc_capable"] = True + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + } + } + } + self.assertFalse(self.device._is_multiple_device_setup()) + + def test_is_multiple_device_setup_multiple_members(self): + """Test _is_multiple_device_setup returns True for multiple members.""" + self.device._is_virtual_chassis = None + self.device.native.facts["vc_capable"] = True + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + "member1": { + "status": "OK", + "mastership_state": "backup", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + } + } + self.assertTrue(self.device._is_multiple_device_setup()) + + @mock.patch.object(JunosDevice, "check_file_exists") + @mock.patch.object(JunosDevice, "compare_file_checksum") + @mock.patch.object(JunosDevice, "uptime", new_callable=mock.PropertyMock) + @mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) + def test_validate_and_capture_preinstall_state(self, mock_version, mock_uptime, mock_checksum, mock_exists): + """Test _validate_and_capture_preinstall_state returns pre-install state.""" + mock_exists.return_value = True + mock_checksum.return_value = True + mock_uptime.return_value = 1000 + mock_version.return_value = "15.1F4.15" + + pre_uptime, pre_version = self.device._validate_and_capture_preinstall_state( + "/var/tmp/image.bin", "abc123", "md5" + ) + self.assertEqual(pre_uptime, 1000) + self.assertEqual(pre_version, "15.1F4.15") + + @mock.patch.object(JunosDevice, "check_file_exists") + def test_validate_and_capture_preinstall_state_missing_image(self, mock_exists): + """Test _validate_and_capture_preinstall_state raises if image missing.""" + mock_exists.return_value = False + with pytest.raises(FileTransferError): + self.device._validate_and_capture_preinstall_state("/var/tmp/image.bin", "abc123", "md5") + + @mock.patch.object(JunosDevice, "check_file_exists") + @mock.patch.object(JunosDevice, "compare_file_checksum") + def test_validate_and_capture_preinstall_state_checksum_mismatch(self, mock_checksum, mock_exists): + """Test _validate_and_capture_preinstall_state raises on checksum mismatch.""" + mock_exists.return_value = True + mock_checksum.return_value = False + with pytest.raises(FileTransferError): + self.device._validate_and_capture_preinstall_state("/var/tmp/image.bin", "abc123", "md5") + + @mock.patch.object(JunosDevice, "_wait_for_device_reboot") + def test_wait_and_verify_upgrade_success(self, mock_wait): + """Test _wait_and_verify_upgrade succeeds when version changes.""" + with mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) as mock_version: + mock_version.side_effect = ["15.1F5.15"] + self.device._wait_and_verify_upgrade(1000, "15.1F4.15", "/var/tmp/image.bin") + mock_wait.assert_called_once_with(1000) + + @mock.patch.object(JunosDevice, "_wait_for_device_reboot") + def test_wait_and_verify_upgrade_fails_on_unchanged_version(self, mock_wait): + """Test _wait_and_verify_upgrade raises if version unchanged.""" + with mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) as mock_version: + mock_version.return_value = "15.1F4.15" + with pytest.raises(OSInstallError): + self.device._wait_and_verify_upgrade(1000, "15.1F4.15", "/var/tmp/image.bin") + + def test_validate_post_install_state_single_device(self): + """Test _validate_post_install_state succeeds with single device OK status.""" + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + } + } + } + self.device._validate_post_install_state("15.1F5.15") + + def test_validate_post_install_state_multiple_devices(self): + """Test _validate_post_install_state succeeds with multiple devices all OK.""" + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + "member1": { + "status": "OK", + "mastership_state": "backup", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + } + } + self.device._validate_post_install_state("15.1F5.15") + + def test_validate_post_install_state_raises_on_bad_status(self): + """Test _validate_post_install_state raises OSInstallError if member status is not OK.""" + self.device.native.facts["re_info"] = { + "default": { + "member0": { + "status": "OK", + "mastership_state": "master", + "last_reboot_reason": "normal", + "model": "EX3300", + }, + "member1": { + "status": "DOWN", + "mastership_state": "unknown", + "last_reboot_reason": "crash", + "model": "EX3300", + }, + } + } + with pytest.raises(OSInstallError): + self.device._validate_post_install_state("15.1F5.15") + + def test_validate_post_install_state_no_re_info(self): + """Test _validate_post_install_state handles missing re_info gracefully.""" + self.device.native.facts["re_info"] = {} + self.device._validate_post_install_state("15.1F5.15") + class TestJnprFreeSpace(unittest.TestCase): """Tests for JunOS pre-transfer free-space verification (NAPPS-1085).""" From 43bb7c6eaeb0bcb3aa567cf931424caf47df7c89 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 8 Jul 2026 08:53:40 -0400 Subject: [PATCH 05/14] Update install logic for multiple devices --- changes/400.added | 1 + changes/mm.added | 1 - pyntc/devices/jnpr_device.py | 851 ++++++++------------ tests/unit/test_devices/test_jnpr_device.py | 402 ++++----- 4 files changed, 512 insertions(+), 743 deletions(-) create mode 100644 changes/400.added delete mode 100644 changes/mm.added diff --git a/changes/400.added b/changes/400.added new file mode 100644 index 00000000..0a7c013f --- /dev/null +++ b/changes/400.added @@ -0,0 +1 @@ +Added support for ISSU and NSSU non-disruptive OS upgrades on Juniper devices. diff --git a/changes/mm.added b/changes/mm.added deleted file mode 100644 index 89f03afc..00000000 --- a/changes/mm.added +++ /dev/null @@ -1 +0,0 @@ -Added issu and nssu upgrade choices for install on juniper devices. \ No newline at end of file diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index a1b8585d..51943c3c 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -9,10 +9,8 @@ from urllib.parse import urlparse from jnpr.junos import Device as JunosNativeDevice -from jnpr.junos.exception import (ConfigLoadError, ConnectClosedError, - RpcError, RpcTimeoutError) -from jnpr.junos.op.ethport import \ - EthPortTable # pylint: disable=import-error,no-name-in-module +from jnpr.junos.exception import ConfigLoadError, RpcTimeoutError +from jnpr.junos.op.ethport import EthPortTable # pylint: disable=import-error,no-name-in-module from jnpr.junos.utils.config import Config as JunosNativeConfig from jnpr.junos.utils.fs import FS as JunosNativeFS from jnpr.junos.utils.scp import SCP @@ -20,11 +18,15 @@ from pyntc import log from pyntc.devices.base_device import BaseDevice, fix_docs -from pyntc.devices.tables.jnpr.loopback import \ - LoopbackTable # pylint: disable=no-name-in-module -from pyntc.errors import (CommandError, CommandListError, - FileSystemNotFoundError, FileTransferError, - OSInstallError, RebootTimeoutError) +from pyntc.devices.tables.jnpr.loopback import LoopbackTable # pylint: disable=no-name-in-module +from pyntc.errors import ( + CommandError, + CommandListError, + FileSystemNotFoundError, + FileTransferError, + OSInstallError, + RebootTimeoutError, +) from pyntc.utils.models import FileCopyModel # Multipliers for Junos ``df``-style size suffixes. Junos formats available @@ -71,7 +73,7 @@ class JunosDevice(BaseDevice): """Juniper JunOS Device Implementation.""" vendor = "juniper" - DEFAULT_TIMEOUT = 180 + DEFAULT_TIMEOUT = 120 def __init__(self, host, username, password, *args, **kwargs): # noqa: D403 """PyNTC device implementation for Juniper JunOS. @@ -92,7 +94,6 @@ def __init__(self, host, username, password, *args, **kwargs): # noqa: D403 self.cu = JunosNativeConfig(self.native) # pylint: disable=invalid-name self.fs = JunosNativeFS(self.native) # pylint: disable=invalid-name self.sw = JunosNativeSW(self.native) # pylint: disable=invalid-name - self._is_virtual_chassis = None def _file_copy_local_file_exists(self, filepath): return os.path.isfile(filepath) @@ -227,7 +228,7 @@ def _uptime_to_string(self, uptime_full_string): days, hours, minutes, seconds = self._uptime_components(uptime_full_string) return f"{days:02d}:{hours:02d}:{minutes:02d}:{seconds:02d}" - def _wait_for_device_reboot(self, original_uptime, timeout=7200): + def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=False): """Block until the device reboots and accepts a fresh connection. Drops the existing NETCONF session and polls for the device to come back. @@ -235,6 +236,9 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200): device reports an uptime lower than ``original_uptime`` (i.e., it has booted since the reboot was issued). + For multi-device setups (virtual-chassis, chassis-cluster), checks that all + members have rebooted by verifying no members in re_info are in pending state. + The pre-reboot session must be discarded first: once the device restarts, PyEZ still reports it as connected even though the transport is dead, so it is closed here to force each probe to establish a fresh connection. @@ -242,6 +246,7 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200): Args: original_uptime (int): Device uptime in seconds captured before the reboot. timeout (int, optional): Max seconds to wait for the device to return. Defaults to 2 hours. + is_multiple (bool, optional): Whether device is in multi-device configuration. Defaults to False. """ start = time.time() @@ -252,52 +257,268 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200): except Exception as close_exc: # pylint: disable=broad-exception-caught log.debug("Host %s: Pre-reboot disconnect raised %s (ignored).", self.host, close_exc) + # Give the device time to boot before polling (avoid hammering a device that's still starting up) + log.info("Host %s: Waiting 180 seconds for device to boot before polling...", self.host) + time.sleep(180) + while time.time() - start < timeout: try: self.open() self._uptime = None current_uptime = self.uptime - if current_uptime is not None and current_uptime < original_uptime: - log.info( - "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", + + if is_multiple: + # For multi-device, check that no members are in pending state + self.native.facts_refresh() + re_info = self.native.facts.get("re_info", {}) + # Virtual-chassis structure: members are under re_info['default'] + members = re_info.get("default", {}) + pending_members = [ + name + for name, info in members.items() + if name != "default" and isinstance(info, dict) and info.get("status") == "pending" + ] + if pending_members: + log.debug( + "Host %s: Members still pending reboot: %s; still waiting.", + self.host, + pending_members, + ) + elif current_uptime is not None and current_uptime < original_uptime: + log.info( + "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", + self.host, + current_uptime, + original_uptime, + ) + return + else: + if current_uptime is not None and current_uptime < original_uptime: + log.info( + "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", + self.host, + current_uptime, + original_uptime, + ) + return + log.debug( + "Host %s: Reachable but uptime %ss >= pre-reboot %ss; still waiting.", self.host, current_uptime, original_uptime, ) + except Exception as exc: # pylint: disable=broad-exception-caught + log.debug("Host %s: Reboot probe failed (%s); will retry.", self.host, exc) + self.native.connected = False + time.sleep(30) + + raise RebootTimeoutError(hostname=self.hostname, wait_time=timeout) + + def _wait_for_nssu_completion(self, target_version, timeout=3600, interval=60): + """Wait for all members to complete NSSU and run target version. + + Polls device to verify all members are running the same (target) software version. + Used to ensure NSSU/ISSU has fully completed before proceeding. + + Args: + target_version (str): Target software version (e.g., "15.1R7-S2"). + timeout (int, optional): Max seconds to wait. Defaults to 1 hour. + interval (int, optional): Seconds between version checks. Defaults to 60. + + Raises: + OSInstallError: When all members don't match target version within timeout. + """ + start = time.time() + + while time.time() - start < timeout: + try: + members_versions = self._get_all_members_version() + if not members_versions: + log.debug("Host %s: Could not retrieve member versions; will retry.", self.host) + time.sleep(interval) + continue + + # Check if all members are running the target version + all_match = all(v == target_version for v in members_versions.values()) + mismatched = {m: v for m, v in members_versions.items() if v != target_version} + + if all_match: + log.info( + "Host %s: All members running target version %s.", + self.host, + target_version, + ) return + log.debug( - "Host %s: Reachable but uptime %ss >= pre-reboot %ss; still waiting.", + "Host %s: Members not all on target version %s. Still waiting on: %s", self.host, - current_uptime, - original_uptime, + target_version, + mismatched, ) + except Exception as exc: # pylint: disable=broad-exception-caught - log.debug("Host %s: Reboot probe failed (%s); will retry.", self.host, exc) - self.native.connected = False - time.sleep(10) + log.debug("Host %s: Version check failed (%s); will retry.", self.host, exc) - raise RebootTimeoutError(hostname=self.hostname, wait_time=timeout) + time.sleep(interval) - def _wait_for_system_snapshot(self, timeout=1800, interval=180): + log.error( + "Host %s: Not all members reached target version %s within %s seconds.", + self.host, + target_version, + timeout, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=target_version) + + def _get_all_members_version(self): + """Get software version running on all members. + + Parses `show version all-members` output to extract version for each member. + + Returns: + dict: Member IDs mapped to their running version. Example: + {'0': '15.1R7-S2', '1': '12.3R12-S10'} + """ + try: + version_output = self.show("show version all-members") + members_versions = {} + current_member = None + + for line in version_output.split("\n"): + # Detect member header (fpc0:, fpc1:, etc.) + if line.startswith("fpc") and line.endswith(":"): + current_member = line.split("fpc")[1].rstrip(":") + members_versions[current_member] = None + + # Extract version from JUNOS Base OS Software Suite line + if current_member and "JUNOS Base OS Software Suite" in line: + # Extract version from format: JUNOS Base OS Software Suite [15.1R7-S2] + match = re.search(r"\[([^\]]+)\]", line) + if match: + members_versions[current_member] = match.group(1) + + log.debug("Host %s: Members versions: %s", self.host, members_versions) + return members_versions + except Exception as exc: # pylint: disable=broad-exception-caught + log.error("Host %s: Failed to get all members version: %s", self.host, exc) + return {} + + def _verify_install_version(self, target_version, is_multiple): + """Verify that the target version is running after install/reboot. + + Args: + target_version (str): The expected version (e.g., "15.1R7-S2"). + is_multiple (bool): Whether this is a multi-device setup. + + Raises: + OSInstallError: If the target version is not running on any member/device. + """ + if is_multiple: + members_versions = self._get_all_members_version() + if not members_versions: + log.warning( + "Host %s: Could not verify member versions after install; proceeding without verification", + self.host, + ) + return + + mismatched = [member for member, version in members_versions.items() if version != target_version] + if mismatched: + log.error( + "Host %s: Version mismatch after install. Expected %s, got %s", + self.host, + target_version, + members_versions, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=target_version) + + log.info("Host %s: All members running target version %s", self.host, target_version) + else: + # For single device, check facts + self.native.facts_refresh() + current_version = self.native.facts.get("version", "") + if current_version != target_version: + log.error( + "Host %s: Version mismatch after install. Expected %s, got %s", + self.host, + target_version, + current_version, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=target_version) + + log.info("Host %s: Device running target version %s", self.host, target_version) + + def _request_system_reboot_all_members(self): + """Issue system reboot command for all members using RPC CLI. + + Used for disruptive multi-member OS upgrades where both members reboot together. + """ + try: + log.info("Host %s: Issuing 'request system reboot all-members' command", self.host) + response = self.native.rpc.cli(command="request system reboot all-members") + log.debug("Host %s: Reboot command response: %s", self.host, response) + except Exception as exc: # pylint: disable=broad-exception-caught + log.error("Host %s: Failed to issue reboot command: %s", self.host, exc) + raise + + def _validate_multiple_device(self): + """Check if device is in virtual-chassis or chassis-cluster configuration. + + Inspects re_info from PyEZ facts to determine if multiple members are present. + + Returns: + bool: True if multiple members are present, False otherwise. + """ + try: + self.native.facts_refresh() + re_info = self.native.facts.get("re_info", {}) + + # Virtual-chassis structure: {'default': {'0': {...}, '1': {...}, 'default': {...}}} + # Count members excluding the 'default' key itself + if "default" in re_info and isinstance(re_info["default"], dict): + members = {k: v for k, v in re_info["default"].items() if k != "default"} + is_multiple = len(members) > 1 + log.info("Host %s: Multiple device configuration detected: %s", self.host, is_multiple) + return is_multiple + + log.info("Host %s: Multiple device configuration detected: False", self.host) + return False + except Exception as exc: # pylint: disable=broad-exception-caught + log.warning("Host %s: Could not validate multiple devices: %s", self.host, exc) + return False + + def _wait_for_system_snapshot(self, timeout=900, interval=30): """Poll device to verify system snapshot completion. Periodically checks ``show system snapshot media internal`` to verify the snapshot was successfully taken. Used when the ``request system snapshot`` RPC times out. Args: - timeout (int, optional): Max seconds to wait for snapshot verification. Defaults to 1800 (30 minutes). - interval (int, optional): Seconds between verification polls. Defaults to 180 (3 minutes). + timeout (int, optional): Max seconds to wait for snapshot verification. Defaults to 900 (15 minutes). + interval (int, optional): Seconds between verification polls. Defaults to 30 seconds. Raises: - OSInstallError: When the snapshot verification indicates a failure. + TimeoutError: When the snapshot verification does not complete within the timeout. """ + log.info( + "Host %s: Polling to verify system snapshot completion (timeout: %s seconds, interval: %s seconds)", + self.host, + timeout, + interval, + ) start = time.time() + # Give the snapshot time to complete before polling + log.info("Host %s: Waiting 180 seconds for snapshot to complete before polling...", self.host) + time.sleep(180) + while time.time() - start < timeout: try: output = self.native.cli("show system snapshot media internal") - if "snapshot" in output.lower(): - log.info("Host %s: System snapshot verified.", self.host) + # Check for actual snapshot success: "Creation date:" indicates snapshots exist + # and timestamps indicate they were recently created + if "Creation date:" in output: + log.info("Host %s: System snapshot verified (snapshots with creation dates found).", self.host) return log.debug("Host %s: Snapshot verification in progress; will retry.", self.host) @@ -307,16 +528,7 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=180): time.sleep(interval) log.error("Host %s: System snapshot did not complete within %s seconds.", self.host, timeout) - raise OSInstallError(hostname=self.hostname, desired_boot="system snapshot") - - def backup_running_config(self, filename): - """Backup current running configuration. - - Args: - filename (str): Name used for backup file. - """ - with open(filename, "w", encoding="utf-8") as file_name: - file_name.write(self.running_config) + raise TimeoutError(f"System snapshot did not complete within {timeout} seconds on {self.hostname}") def request_system_snapshot(self, parameters=None): """Request a system snapshot and verify completion. @@ -346,9 +558,9 @@ def request_system_snapshot(self, parameters=None): rpc_timed_out = False try: - log.debug("Host %s: Issuing RPC: %s", self.host, command) + log.info("Host %s: Issuing snapshot RPC: %s", self.host, command) response = self.native.rpc.cli(command=command, format="text") - log.debug("Host %s: snapshot RPC completed.", self.host) + log.info("Host %s: Snapshot RPC response: %s", self.host, response) except RpcTimeoutError: log.debug("Host %s: snapshot RPC timed out; will poll device to verify.", self.host) @@ -368,6 +580,15 @@ def request_system_snapshot(self, parameters=None): log.debug("Host %s: Snapshot response unclear or timed out; polling device to verify.", self.host) self._wait_for_system_snapshot() + def backup_running_config(self, filename): + """Backup current running configuration. + + Args: + filename (str): Name used for backup file. + """ + with open(filename, "w", encoding="utf-8") as file_name: + file_name.write(self.running_config) + @property def boot_options(self): """Get os version on device. @@ -539,14 +760,6 @@ def serial_number(self): return self._serial_number - @property - def is_virtual_chassis(self) -> bool: - """Check if device is in virtual-chassis mode.""" - if self._is_virtual_chassis is None: - self._is_virtual_chassis = self.native.facts.get("vc_capable", False) - - return self._is_virtual_chassis - def file_copy(self, src, dest=None, **kwargs): """Copy file to device via SCP. @@ -595,479 +808,115 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def _validate_member_status(self): - """Validate and log virtual-chassis member status. - - Checks re_info for: - - Member count - - Each member's status (should be 'OK') - - Mastership state (master/backup) - - Last reboot reason - - Returns: - dict: Member status info indexed by member ID. - - Raises: - OSInstallError: If any member status is not 'OK'. - """ - re_info = self.native.facts.get("re_info", {}).get("default", {}) - if not re_info: - log.warning("Host %s: No re_info available; cannot validate member status", self.host) - return {} - - member_status = {} - for member_id, member_info in re_info.items(): - if member_id == "default": - continue - - status = member_info.get("status", "UNKNOWN") - mastership = member_info.get("mastership_state", "UNKNOWN") - reboot_reason = member_info.get("last_reboot_reason", "UNKNOWN") - model = member_info.get("model", "UNKNOWN") - - member_status[member_id] = { - "status": status, - "mastership": mastership, - "reboot_reason": reboot_reason, - "model": model, - } - - log.info( - "Host %s: Member %s - Status: %s, Mastership: %s, Model: %s", - self.host, - member_id, - status, - mastership, - model, - ) - log.debug("Host %s: Member %s reboot reason: %s", self.host, member_id, reboot_reason) - - if status != "OK": - log.error("Host %s: Member %s status is %s (expected OK)", self.host, member_id, status) - raise OSInstallError(hostname=self.hostname, desired_boot="N/A") - - return member_status + def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", nssu=False, issu=False): # pylint: disable=too-many-positional-arguments,too-many-branches + """Install OS on device and reboot. - def _validate_post_install_state(self, expected_version): - """Validate post-install state via re_info. - - Checks re_info for: - - All members in 'OK' status - - All members running expected version - - Reboot reasons don't indicate failures + For multi-device setups (virtual-chassis, chassis-cluster), supports NSSU/ISSU + upgrades and system snapshots after reboot. Args: - expected_version (str): Expected OS version after install. - - Raises: - OSInstallError: If any member is not in expected state. - """ - re_info = self.native.facts.get("re_info", {}).get("default", {}) - if not re_info: - log.warning("Host %s: No re_info available for post-install validation", self.host) - return - - for member_id, member_info in re_info.items(): - if member_id == "default": - continue - - status = member_info.get("status", "UNKNOWN") - reboot_reason = member_info.get("last_reboot_reason", "UNKNOWN") - model = member_info.get("model", "UNKNOWN") - - if status != "OK": - log.error( - "Host %s: Member %s status is %s (expected OK) after install", - self.host, - member_id, - status, - ) - raise OSInstallError(hostname=self.hostname, desired_boot=expected_version) - - log.info( - "Host %s: Member %s post-install state validated - Status: OK, Model: %s, Reboot reason: %s", - self.host, - member_id, - model, - reboot_reason, - ) - - def _is_multiple_device_setup(self) -> bool: - """Check if device is in a multiple-device setup (virtual-chassis or chassis-cluster). - - Validates that: - - Device is vc_capable - - Multiple routing engines exist in re_info - - All members have status 'OK' - - Returns: - bool: True if device is part of a multiple-device setup, False otherwise. - - Raises: - OSInstallError: If any member status is not 'OK'. - """ - if not self.is_virtual_chassis: - return False - - re_info = self.native.facts.get("re_info", {}).get("default", {}) - members = {k: v for k, v in re_info.items() if k != "default"} - - if len(members) <= 1: - log.debug( - "Host %s: vc_capable=True but only %d members; treating as single device", self.host, len(members) - ) - return False - - log.info("Host %s: Detected multiple-device setup with %d members", self.host, len(members)) - self._validate_member_status() - return True - - def _validate_and_capture_preinstall_state(self, image_name, checksum, hashing_algorithm): - """Validate image and capture pre-install state. - - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). - - Returns: - tuple: (pre_uptime, pre_version) - - Raises: - FileTransferError: If image file not found or checksum mismatch. - """ - if not self.check_file_exists(image_name): - raise FileTransferError(message=f"Image {image_name} not found") - - if not self.compare_file_checksum(checksum, image_name, hashing_algorithm): - raise FileTransferError(message=f"Checksum mismatch for {image_name}") - - log.info("Host %s: Image verified - %s", self.host, image_name) - - self._uptime = None - pre_uptime = self.uptime - pre_version = self.os_version - - log.info("Host %s: Pre-install state - OS: %s, uptime: %ss", self.host, pre_version, pre_uptime) - - return pre_uptime, pre_version - - def _wait_and_verify_upgrade(self, pre_uptime, pre_version, image_name): - """Wait for device reboot and verify OS version changed. - - Args: - pre_uptime (int): Uptime before upgrade. - pre_version (str): OS version before upgrade. - image_name (str): Name of image being installed. - - Raises: - OSInstallError: If OS version did not change after reboot. - """ - self._wait_for_device_reboot(pre_uptime) - log.info("Host %s: Device reconnected after reboot.", self.host) - - self._uptime = None - post_version = self.os_version - - if post_version == pre_version: - log.error( - "Host %s: Device rebooted but OS version unchanged. Expected change from %s, still running %s", - self.host, - pre_version, - post_version, - ) - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - log.info( - "Host %s: OS upgrade verified. Previous version: %s. Current version: %s", - self.host, - pre_version, - post_version, - ) - - def _reboot_all_members_and_wait(self, pre_reboot_uptime): - """Issue explicit reboot of all members and wait for reconnection. - - Args: - pre_reboot_uptime (int): Uptime before reboot. + image_name (str): Name of image. + checksum (str): The checksum of the file. + reboot (bool): Whether to reboot the device after setting the boot options. Defaults to True. + hashing_algorithm (str): The hashing algorithm to use. Valid values are 'md5', 'sha1', and 'sha256'. Defaults to 'md5'. + nssu (bool): Enable Nonstop Software Upgrade. Defaults to False. + issu (bool): Enable In-Service Software Upgrade. Defaults to False. Raises: - ConnectClosedError or Exception: If reboot command fails or device doesn't reconnect. - """ - log.info("Host %s: Issuing explicit reboot of all members", self.host) - try: - self.native.rpc.cli(command="request system reboot all-members", format="text") - log.info("Host %s: Reboot all-members command issued", self.host) - except ConnectClosedError: - log.debug("Host %s: Connection closed during reboot command (expected)", self.host) - except Exception as reboot_error: - log.error("Host %s: Failed to issue reboot command: %s", self.host, reboot_error) - raise - - try: - self._wait_for_device_reboot(pre_reboot_uptime) - log.info("Host %s: Device reconnected after all-members reboot", self.host) - except Exception as wait_error: - log.error("Host %s: Failed waiting for device after reboot: %s", self.host, wait_error) - raise - - def _disruptive_install_single(self, image_name, checksum, hashing_algorithm, pre_uptime, pre_version): - """Perform a disruptive OS installation on a single device. - - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). - pre_uptime (int): Uptime before upgrade. - pre_version (str): OS version before upgrade. - """ - log.info("Host %s: Installing software update with disruptive reboot (single device).", self.host) - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - reboot=True, - timeout=3600, - ) - - self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) - self._validate_post_install_state(self.os_version) - - if isinstance(install_ok, tuple): - install_ok = install_ok[0] - - if not install_ok: - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - def _disruptive_install_multiple(self, image_name, checksum, hashing_algorithm, pre_uptime, pre_version): - """Perform a disruptive OS installation on multiple-device virtual-chassis. - - Includes explicit reboot of all members and snapshot of alternate partitions. - - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). - pre_uptime (int): Uptime before upgrade. - pre_version (str): OS version before upgrade. + ValueError: When both nssu and issu are True (mutually exclusive). """ - log.info("Host %s: Installing software update with disruptive reboot (multiple-device).", self.host) - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - reboot=True, - timeout=3600, - ) - - self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) - self._validate_post_install_state(self.os_version) - + if nssu and issu: + raise ValueError("nssu and issu are mutually exclusive; only one can be True") + + is_multiple = self._validate_multiple_device() + + install_kwargs = { + "package": image_name, + "checksum": checksum, + "checksum_algorithm": hashing_algorithm, + "progress": True, + "validate": True, + "no_copy": True, + "timeout": 3600, + } + + # Add NSSU/ISSU parameters for multi-device setups + if is_multiple: + if nssu: + install_kwargs["nssu"] = True + log.info("Host %s: NSSU enabled for multi-device upgrade", self.host) + elif issu: + install_kwargs["issu"] = True + log.info("Host %s: ISSU enabled for multi-device upgrade", self.host) + + install_ok = self.sw.install(**install_kwargs) + + # Sometimes install() returns a tuple of (ok, msg). Other times it returns a single bool if isinstance(install_ok, tuple): install_ok = install_ok[0] - if not install_ok: - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - self._uptime = None - pre_reboot_uptime = self.uptime - self._reboot_all_members_and_wait(pre_reboot_uptime) - self.request_system_snapshot("slice alternate all-members") - self._validate_post_install_state(self.os_version) - - def _non_disruptive_install_single( - self, image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version - ): - """Perform a non-disruptive OS installation on a single device. - - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). - issu (bool): Whether to perform ISSU. - nssu (bool): Whether to perform NSSU. - pre_uptime (int): Uptime before upgrade. - pre_version (str): OS version before upgrade. - """ - log.info("Host %s: Installing software update using ISSU or NSSU (single device).", self.host) - try: - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - reboot=False, - issu=issu, - nssu=nssu, - timeout=3600, - ) - except RpcError as rpc_error: - if "reboot pending for software rollback" in str(rpc_error).lower(): - log.warning( - "Host %s: Device has pending reboot state; A manual reboot will be required to continue.", - self.host, - ) - raise - except ConnectClosedError: - log.info( - "Host %s: Connection closed during install (expected for device reboot). " - "Waiting for device to come back online.", - self.host, - ) - - self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) - self._validate_post_install_state(self.os_version) - - if isinstance(install_ok, tuple): - install_ok = install_ok[0] + log.info("Host %s: install_ok result: %s (type: %s)", self.host, install_ok, type(install_ok).__name__) + # If install_ok is False, it may mean a reboot is pending (not necessarily a failure). + # We'll proceed with the reboot and verify the install was successful afterward. if not install_ok: - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - def _non_disruptive_install_multiple( - self, image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version - ): - """Perform a non-disruptive OS installation on multiple-device virtual-chassis. - - Includes explicit reboot of all members and snapshot of alternate partitions. - - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). - issu (bool): Whether to perform ISSU. - nssu (bool): Whether to perform NSSU. - pre_uptime (int): Uptime before upgrade. - pre_version (str): OS version before upgrade. - """ - log.info("Host %s: Installing software update using ISSU or NSSU (multiple-device).", self.host) - try: - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - reboot=False, - issu=issu, - nssu=nssu, - timeout=3600, - ) - except RpcError as rpc_error: - if "reboot pending for software rollback" in str(rpc_error).lower(): - log.warning("Host %s: Device has pending reboot state; clearing with reboot.", self.host) - self._uptime = None - pre_reboot_uptime = self.uptime - try: - self.native.rpc.request_reboot() - log.info("Host %s: Reboot issued to clear pending state.", self.host) - self._wait_for_device_reboot(pre_reboot_uptime) - log.info("Host %s: Device rebooted and reconnected.", self.host) - - if not self.check_file_exists(image_name): - raise FileTransferError(message=f"Image file {image_name} missing after reboot") - log.info("Host %s: Image file verified after reboot.", self.host) - - log.info("Host %s: Retrying install after clearing pending state.", self.host) - install_ok = self.sw.install( - package=image_name, - checksum=checksum, - checksum_algorithm=hashing_algorithm, - progress=True, - validate=True, - no_copy=True, - reboot=False, - issu=issu, - nssu=nssu, - timeout=3600, - ) - except Exception as retry_error: - log.error("Host %s: Failed to clear pending state: %s", self.host, retry_error) - raise - else: - raise - except ConnectClosedError: - log.info( - "Host %s: Connection closed during install (expected for device reboot). " - "Waiting for device to come back online.", + log.warning( + "Host %s: install_ok returned False; a reboot may be pending. Proceeding with reboot and post-reboot verification.", self.host, ) - self._wait_and_verify_upgrade(pre_uptime, pre_version, image_name) - self._validate_post_install_state(self.os_version) - - if isinstance(install_ok, tuple): - install_ok = install_ok[0] - - if not install_ok: - raise OSInstallError(hostname=self.hostname, desired_boot=image_name) - - self._uptime = None - pre_reboot_uptime = self.uptime - self._reboot_all_members_and_wait(pre_reboot_uptime) - self.request_system_snapshot("slice alternate all-members") - self._validate_post_install_state(self.os_version) - - def _disruptive_install(self, image_name, checksum, hashing_algorithm, reboot): - """Perform a disruptive OS installation.""" - - def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu=False, nssu=False) -> bool: - """Install OS on device. - - Supports both disruptive and non-disruptive upgrades on single and multiple-device setups. - Automatically detects virtual-chassis and applies all-members operations when needed. + if not reboot: + log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) + return True - Args: - image_name (str): Path to OS image file on device. - checksum (str): Checksum of the image file. - reboot (bool): Whether to reboot the device. Defaults to True. Ignored if nssu is True. - hashing_algorithm (str): Hash algorithm ('md5', 'sha1', 'sha256'). Defaults to 'md5'. - issu (bool): Whether to perform ISSU. Defaults to False. - nssu (bool): Whether to perform NSSU. Defaults to False. When True, reboot is automatic. + log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) + original_uptime = self.uptime - Returns: - bool: True if upgrade successful. + # Issue reboot command based on device configuration + if is_multiple: + self._request_system_reboot_all_members() + else: + self.sw.reboot(in_min=0) - Raises: - FileTransferError: If image file not found or checksum mismatch. - OSInstallError: If upgrade fails. - """ - pre_uptime, pre_version = self._validate_and_capture_preinstall_state(image_name, checksum, hashing_algorithm) + self._wait_for_device_reboot(original_uptime, is_multiple=is_multiple) - is_multiple_device = self._is_multiple_device_setup() - is_disruptive = not (issu or nssu or not reboot) + # Extract target version from image name for verification + # Matches formats: 15.1R7-S2, 20.4R3, 21.4X38-D10, 18.4R2.7, etc. + match = re.search(r"(\d+\.\d+[A-Z]+[\d\.]+(?:[-][A-Z]+\d+)*)", image_name) + target_version = match.group(1) if match else None - if is_disruptive: - if is_multiple_device: - self._disruptive_install_multiple(image_name, checksum, hashing_algorithm, pre_uptime, pre_version) - else: - self._disruptive_install_single(image_name, checksum, hashing_algorithm, pre_uptime, pre_version) - else: - if is_multiple_device: - self._non_disruptive_install_multiple( - image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + # For NSSU/ISSU on multi-device, wait for all members to reach target version before snapshot + if (nssu or issu) and is_multiple: + if target_version: + log.info( + "Host %s: Waiting for all members to complete %s upgrade to version %s", + self.host, + "NSSU" if nssu else "ISSU", + target_version, ) + self._wait_for_nssu_completion(target_version) else: - self._non_disruptive_install_single( - image_name, checksum, hashing_algorithm, issu, nssu, pre_uptime, pre_version + log.warning( + "Host %s: Could not extract version from image name %s; skipping NSSU/ISSU completion wait", + self.host, + image_name, ) - if not reboot: - log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) - return True + # Perform system snapshot after reboot/upgrade + snapshot_params = "slice alternate all-members" if is_multiple else "slice alternate" + self.request_system_snapshot(parameters=snapshot_params) - if not nssu: - self.reboot(wait_for_reload=True) + # Verify the install was successful by checking the running version + if target_version: + self._verify_install_version(target_version, is_multiple) + else: + log.warning( + "Host %s: Could not extract version from image name %s; skipping post-reboot version verification", + self.host, + image_name, + ) + log.info("Host %s: OS image %s installed successfully.", self.host, image_name) return True def open(self): @@ -1246,16 +1095,6 @@ def compare_file_checksum(self, checksum, filename, hashing_algorithm="md5"): """ return checksum == self.get_remote_checksum(filename, hashing_algorithm) - @staticmethod - def _netloc(src: FileCopyModel) -> str: - """Return host:port or just host from a FileCopyModel.""" - return f"{src.hostname}:{src.port}" if src.port else src.hostname - - @staticmethod - def _source_path(src: FileCopyModel) -> str: - """Return the file path from URL, using file_name if path is empty.""" - return src.path if src.path and src.path != "/" else f"/{src.file_name}" - def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: str | None = None, **kwargs): """Copy a file to a remote device. diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 354a7d6f..48628e67 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -427,27 +427,172 @@ def test_file_copy_local_md5(self): result = self.device._file_copy_local_md5(fp.name) self.assertEqual(result, checksum) - @mock.patch.object(JunosDevice, "_validate_post_install_state") - @mock.patch.object(JunosDevice, "_validate_and_capture_preinstall_state") - @mock.patch.object(JunosDevice, "_is_multiple_device_setup") - def test_install_os(self, mock_is_multi, mock_validate_preinstall, mock_validate_postinstall): - mock_validate_preinstall.return_value = (1000, "15.1F4.15") - mock_is_multi.return_value = False - - with mock.patch.object(self.device, "reboot") as mock_reboot: - with self.subTest("sw.install returns a bool"): - self.device.sw.install.return_value = True - with mock.patch.object(JunosDevice, "_wait_and_verify_upgrade"): - self.device.install_os(image_name="image.bin", checksum="c0ffee") - mock_reboot.assert_called_once_with(wait_for_reload=True) - mock_validate_postinstall.assert_called() - - with self.subTest("sw.install returns a tuple and fails"): - self.device.sw.install.return_value = (False, "install failure") - mock_reboot.reset_mock() - with mock.patch.object(JunosDevice, "_wait_and_verify_upgrade"): - with self.assertRaises(OSInstallError): - self.device.install_os(image_name="image.bin", checksum="c0ffee") + def test_validate_multiple_device(self): + with self.subTest("single device"): + self.device.native.facts = {"re_info": {"RE0": {"status": "OK"}}} + result = self.device._validate_multiple_device() + self.assertFalse(result) + + with self.subTest("multi-device (virtual-chassis)"): + self.device.native.facts = { + "re_info": { + "default": { + "0": {"status": "OK"}, + "1": {"status": "OK"}, + "default": {"status": "OK"}, + } + } + } + result = self.device._validate_multiple_device() + self.assertTrue(result) + + def test_install_os_single_device(self): + with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: + with mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot: + with mock.patch.object(self.device, "request_system_snapshot"): + with mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime: + mock_uptime.return_value = 1000 + with self.subTest("install succeeds, reboot requested"): + with mock.patch.object(self.device, "_verify_install_version"): + mock_validate.return_value = False + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/image-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + self.assertTrue(result) + self.device.sw.install.assert_called_once() + self.device.sw.reboot.assert_called_once_with(in_min=0) + mock_wait_reboot.assert_called_once() + + with self.subTest("install fails"): + self.device.sw.install.reset_mock() + self.device.sw.install.return_value = False + # When install_ok is False, version check will fail and raise error + # Use an image name with a version so _verify_install_version is called + with mock.patch.object( + self.device, + "_verify_install_version", + side_effect=OSInstallError(hostname="test_host", desired_boot="15.1R7-S2"), + ): + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + ) + + with self.subTest("reboot=False"): + with mock.patch.object(self.device, "_verify_install_version"): + self.device.sw.install.reset_mock() + self.device.sw.reboot.reset_mock() + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=False, + ) + self.assertTrue(result) + self.device.sw.reboot.assert_not_called() + + def test_install_os_multi_device(self): + with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: + with mock.patch.object(self.device, "_request_system_reboot_all_members") as mock_reboot_all: + with mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot: + with mock.patch.object(self.device, "request_system_snapshot"): + with mock.patch.object(self.device, "_verify_install_version"): + with mock.patch.object( + type(self.device), "uptime", new_callable=mock.PropertyMock + ) as mock_uptime: + mock_uptime.return_value = 1000 + mock_validate.return_value = True + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/image-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + self.assertTrue(result) + # Should call multi-device reboot, not sw.reboot + mock_reboot_all.assert_called_once() + self.device.sw.reboot.assert_not_called() + mock_wait_reboot.assert_called_once() + + def test_install_os_with_nssu(self): + with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: + with mock.patch.object(self.device, "_request_system_reboot_all_members"): + with mock.patch.object(self.device, "_wait_for_device_reboot"): + with mock.patch.object(self.device, "_wait_for_nssu_completion") as mock_nssu_wait: + with mock.patch.object(self.device, "request_system_snapshot"): + with mock.patch.object(self.device, "_verify_install_version"): + with mock.patch.object( + type(self.device), "uptime", new_callable=mock.PropertyMock + ) as mock_uptime: + mock_uptime.return_value = 1000 + mock_validate.return_value = True + self.device.sw.install.return_value = True + self.device.install_os( + image_name="/var/tmp/jinstall-ex-3300-15.1R7-S2-domestic-signed.tgz", + checksum="c0ffee", + nssu=True, + ) + # Should have called nssu wait with extracted version + mock_nssu_wait.assert_called_once() + args = mock_nssu_wait.call_args[0] + self.assertEqual(args[0], "15.1R7-S2") + + def test_request_system_snapshot(self): + with mock.patch.object(self.device, "_wait_for_system_snapshot"): + # Configure mock rpc attribute + self.device.native.rpc = mock.MagicMock() + + with self.subTest("snapshot with parameters"): + self.device.native.rpc.cli.return_value = "filesystems were archived" + self.device.request_system_snapshot(parameters="slice alternate") + self.device.native.rpc.cli.assert_called_with( + command="request system snapshot slice alternate", format="text" + ) + + with self.subTest("snapshot RPC times out, falls back to polling"): + from jnpr.junos.exception import RpcTimeoutError + + self.device.native.rpc.cli.side_effect = RpcTimeoutError( + self.device.native, "request system snapshot slice alternate", 30 + ) + # Should not raise, should fall through to polling + self.device.request_system_snapshot(parameters="slice alternate") + + @mock.patch("pyntc.devices.jnpr_device.time.sleep") + def test_wait_for_system_snapshot(self, mock_sleep): + with self.subTest("snapshot completes successfully"): + self.device.native.cli.return_value = "Creation date: 2024-01-15 10:30:00\nSnapshot verified" + # Should not raise when "Creation date:" is found + self.device._wait_for_system_snapshot() + # Verify initial 180s sleep was called + self.assertEqual(mock_sleep.call_count, 1) + self.assertEqual(mock_sleep.call_args_list[0][0][0], 180) + + with self.subTest("snapshot times out"): + mock_sleep.reset_mock() + self.device.native.cli.return_value = "No snapshots found" + with self.assertRaises(TimeoutError) as ctx: + self.device._wait_for_system_snapshot(timeout=10) + self.assertIn("did not complete", str(ctx.exception)) + + def test_get_all_members_version(self): + with self.subTest("single device"): + output = "fpc0:\nJUNOS Base OS Software Suite [15.1R7-S2]\n" + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2"}) + + with self.subTest("multi-device"): + output = ( + "fpc0:\nJUNOS Base OS Software Suite [15.1R7-S2]\nfpc1:\nJUNOS Base OS Software Suite [15.1R7-S2]\n" + ) + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2", "1": "15.1R7-S2"}) def test_check_file_exists(self): self.device.check_file_exists("foo.txt") @@ -573,221 +718,6 @@ def test_get_remote_checksum_rejects_unsupported_algorithm(self): assert "sha512" in str(ctx.exception) self.device.fs.checksum.assert_not_called() - def test_is_virtual_chassis_true(self): - """Test is_virtual_chassis returns True when vc_capable is set.""" - self.device.native.facts["vc_capable"] = True - self.assertTrue(self.device.is_virtual_chassis) - - def test_is_virtual_chassis_false(self): - """Test is_virtual_chassis returns False when vc_capable is False.""" - self.device.native.facts["vc_capable"] = False - self.assertFalse(self.device.is_virtual_chassis) - - def test_is_virtual_chassis_caches_value(self): - """Test is_virtual_chassis caches the value after first access.""" - self.device.native.facts["vc_capable"] = True - first_call = self.device.is_virtual_chassis - self.device.native.facts["vc_capable"] = False - second_call = self.device.is_virtual_chassis - self.assertEqual(first_call, second_call) - self.assertTrue(second_call) - - def test_validate_member_status_ok(self): - """Test _validate_member_status with all members OK.""" - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - "member1": { - "status": "OK", - "mastership_state": "backup", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - } - } - result = self.device._validate_member_status() - self.assertEqual(len(result), 2) - self.assertEqual(result["member0"]["status"], "OK") - self.assertEqual(result["member1"]["status"], "OK") - - def test_validate_member_status_raises_on_bad_status(self): - """Test _validate_member_status raises OSInstallError if member status is not OK.""" - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - "member1": { - "status": "DOWN", - "mastership_state": "unknown", - "last_reboot_reason": "unknown", - "model": "EX3300", - }, - } - } - with pytest.raises(OSInstallError): - self.device._validate_member_status() - - def test_is_multiple_device_setup_single_device(self): - """Test _is_multiple_device_setup returns False for single device.""" - self.device.native.facts["vc_capable"] = False - self.assertFalse(self.device._is_multiple_device_setup()) - - def test_is_multiple_device_setup_vc_capable_one_member(self): - """Test _is_multiple_device_setup returns False for vc_capable with only 1 member.""" - self.device._is_virtual_chassis = None - self.device.native.facts["vc_capable"] = True - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - } - } - } - self.assertFalse(self.device._is_multiple_device_setup()) - - def test_is_multiple_device_setup_multiple_members(self): - """Test _is_multiple_device_setup returns True for multiple members.""" - self.device._is_virtual_chassis = None - self.device.native.facts["vc_capable"] = True - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - "member1": { - "status": "OK", - "mastership_state": "backup", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - } - } - self.assertTrue(self.device._is_multiple_device_setup()) - - @mock.patch.object(JunosDevice, "check_file_exists") - @mock.patch.object(JunosDevice, "compare_file_checksum") - @mock.patch.object(JunosDevice, "uptime", new_callable=mock.PropertyMock) - @mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) - def test_validate_and_capture_preinstall_state(self, mock_version, mock_uptime, mock_checksum, mock_exists): - """Test _validate_and_capture_preinstall_state returns pre-install state.""" - mock_exists.return_value = True - mock_checksum.return_value = True - mock_uptime.return_value = 1000 - mock_version.return_value = "15.1F4.15" - - pre_uptime, pre_version = self.device._validate_and_capture_preinstall_state( - "/var/tmp/image.bin", "abc123", "md5" - ) - self.assertEqual(pre_uptime, 1000) - self.assertEqual(pre_version, "15.1F4.15") - - @mock.patch.object(JunosDevice, "check_file_exists") - def test_validate_and_capture_preinstall_state_missing_image(self, mock_exists): - """Test _validate_and_capture_preinstall_state raises if image missing.""" - mock_exists.return_value = False - with pytest.raises(FileTransferError): - self.device._validate_and_capture_preinstall_state("/var/tmp/image.bin", "abc123", "md5") - - @mock.patch.object(JunosDevice, "check_file_exists") - @mock.patch.object(JunosDevice, "compare_file_checksum") - def test_validate_and_capture_preinstall_state_checksum_mismatch(self, mock_checksum, mock_exists): - """Test _validate_and_capture_preinstall_state raises on checksum mismatch.""" - mock_exists.return_value = True - mock_checksum.return_value = False - with pytest.raises(FileTransferError): - self.device._validate_and_capture_preinstall_state("/var/tmp/image.bin", "abc123", "md5") - - @mock.patch.object(JunosDevice, "_wait_for_device_reboot") - def test_wait_and_verify_upgrade_success(self, mock_wait): - """Test _wait_and_verify_upgrade succeeds when version changes.""" - with mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) as mock_version: - mock_version.side_effect = ["15.1F5.15"] - self.device._wait_and_verify_upgrade(1000, "15.1F4.15", "/var/tmp/image.bin") - mock_wait.assert_called_once_with(1000) - - @mock.patch.object(JunosDevice, "_wait_for_device_reboot") - def test_wait_and_verify_upgrade_fails_on_unchanged_version(self, mock_wait): - """Test _wait_and_verify_upgrade raises if version unchanged.""" - with mock.patch.object(JunosDevice, "os_version", new_callable=mock.PropertyMock) as mock_version: - mock_version.return_value = "15.1F4.15" - with pytest.raises(OSInstallError): - self.device._wait_and_verify_upgrade(1000, "15.1F4.15", "/var/tmp/image.bin") - - def test_validate_post_install_state_single_device(self): - """Test _validate_post_install_state succeeds with single device OK status.""" - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - } - } - } - self.device._validate_post_install_state("15.1F5.15") - - def test_validate_post_install_state_multiple_devices(self): - """Test _validate_post_install_state succeeds with multiple devices all OK.""" - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - "member1": { - "status": "OK", - "mastership_state": "backup", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - } - } - self.device._validate_post_install_state("15.1F5.15") - - def test_validate_post_install_state_raises_on_bad_status(self): - """Test _validate_post_install_state raises OSInstallError if member status is not OK.""" - self.device.native.facts["re_info"] = { - "default": { - "member0": { - "status": "OK", - "mastership_state": "master", - "last_reboot_reason": "normal", - "model": "EX3300", - }, - "member1": { - "status": "DOWN", - "mastership_state": "unknown", - "last_reboot_reason": "crash", - "model": "EX3300", - }, - } - } - with pytest.raises(OSInstallError): - self.device._validate_post_install_state("15.1F5.15") - - def test_validate_post_install_state_no_re_info(self): - """Test _validate_post_install_state handles missing re_info gracefully.""" - self.device.native.facts["re_info"] = {} - self.device._validate_post_install_state("15.1F5.15") - class TestJnprFreeSpace(unittest.TestCase): """Tests for JunOS pre-transfer free-space verification (NAPPS-1085).""" From ee87f790e8b0a3c1ab6b963e2c2b0ccb4e3b67b1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 8 Jul 2026 10:50:34 -0400 Subject: [PATCH 06/14] post ntc-review comments --- pyntc/devices/jnpr_device.py | 39 +++++++--- tests/unit/test_devices/test_jnpr_device.py | 85 +++++++++++++++++---- 2 files changed, 100 insertions(+), 24 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 51943c3c..5147f736 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -548,7 +548,7 @@ def request_system_snapshot(self, parameters=None): parameters (str, optional): Parameters to pass to the RPC call. Defaults to None. Raises: - OSInstallError: If snapshot verification fails or times out. + TimeoutError: When the snapshot verification does not complete within the timeout. """ command = "request system snapshot" if parameters is not None: @@ -849,29 +849,46 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", install_kwargs["issu"] = True log.info("Host %s: ISSU enabled for multi-device upgrade", self.host) - install_ok = self.sw.install(**install_kwargs) + install_result = self.sw.install(**install_kwargs) # Sometimes install() returns a tuple of (ok, msg). Other times it returns a single bool - if isinstance(install_ok, tuple): - install_ok = install_ok[0] + install_msg = None + if isinstance(install_result, tuple): + install_ok, install_msg = install_result[0], install_result[1] + else: + install_ok = install_result - log.info("Host %s: install_ok result: %s (type: %s)", self.host, install_ok, type(install_ok).__name__) + log.info("Host %s: install_ok result: %s (type: %s)", self.host, install_ok, type(install_result).__name__) + if install_msg: + log.debug("Host %s: install message: %s", self.host, install_msg) - # If install_ok is False, it may mean a reboot is pending (not necessarily a failure). - # We'll proceed with the reboot and verify the install was successful afterward. - if not install_ok: - log.warning( - "Host %s: install_ok returned False; a reboot may be pending. Proceeding with reboot and post-reboot verification.", + # Check if reboot is required (indicated by specific message in output) + reboot_required = install_msg and "A reboot is required" in str(install_msg) + + if not install_ok and not reboot_required: + log.error( + "Host %s: SW install failed for image %s. Device is in undefined state.", self.host, + image_name, ) + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) if not reboot: + if reboot_required: + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) return True log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) + self._uptime = None original_uptime = self.uptime + if original_uptime is None: + raise CommandError( + command="install_os", + message="Could not determine pre-reboot uptime; refusing to reboot.", + ) + # Issue reboot command based on device configuration if is_multiple: self._request_system_reboot_all_members() @@ -882,7 +899,7 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", # Extract target version from image name for verification # Matches formats: 15.1R7-S2, 20.4R3, 21.4X38-D10, 18.4R2.7, etc. - match = re.search(r"(\d+\.\d+[A-Z]+[\d\.]+(?:[-][A-Z]+\d+)*)", image_name) + match = re.search(r"(\d+\.\d+[A-Z]+\d+(?:\.\d+)?(?:[-][A-Z]+\d+)?)", image_name) target_version = match.group(1) if match else None # For NSSU/ISSU on multi-device, wait for all members to reach target version before snapshot diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 48628e67..9981ed92 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -466,21 +466,18 @@ def test_install_os_single_device(self): self.device.sw.reboot.assert_called_once_with(in_min=0) mock_wait_reboot.assert_called_once() - with self.subTest("install fails"): + with self.subTest("install fails immediately"): self.device.sw.install.reset_mock() + self.device.sw.reboot.reset_mock() self.device.sw.install.return_value = False - # When install_ok is False, version check will fail and raise error - # Use an image name with a version so _verify_install_version is called - with mock.patch.object( - self.device, - "_verify_install_version", - side_effect=OSInstallError(hostname="test_host", desired_boot="15.1R7-S2"), - ): - with self.assertRaises(OSInstallError): - self.device.install_os( - image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", - checksum="c0ffee", - ) + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + # Should not have called reboot after install failure + self.device.sw.reboot.assert_not_called() with self.subTest("reboot=False"): with mock.patch.object(self.device, "_verify_install_version"): @@ -718,6 +715,68 @@ def test_get_remote_checksum_rejects_unsupported_algorithm(self): assert "sha512" in str(ctx.exception) self.device.fs.checksum.assert_not_called() + def test_install_os_version_extraction(self): + """Test version regex extraction from various image name formats.""" + import re + + # Pattern from jnpr_device.py line 906 + pattern = r"(\d+\.\d+[A-Z]+\d+(?:\.\d+)?(?:[-][A-Z]+\d+)?)" + + test_cases = [ + ("/var/tmp/jinstall-15.1R7-S2-signed.tgz", "15.1R7-S2"), + ("/var/tmp/image-20.4R3-signed.tgz", "20.4R3"), + ("/var/tmp/junos-21.4X38-D10.tgz", "21.4X38-D10"), + ("/var/tmp/image-18.4R2.7-signed.tgz", "18.4R2.7"), + ("vmhost-21.4R3.15-20230801.11", "21.4R3.15"), + ] + + for image_name, expected_version in test_cases: + with self.subTest(image_name=image_name): + match = re.search(pattern, image_name) + extracted = match.group(1) if match else None + self.assertEqual(extracted, expected_version, f"Failed to extract version from {image_name}") + + def test_install_os_uptime_none_raises_error(self): + """Test that install_os raises error when uptime cannot be determined.""" + with mock.patch.object(self.device, "_validate_multiple_device", return_value=False): + with mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime: + mock_uptime.return_value = None + self.device.sw.install.return_value = True + with self.assertRaises(CommandError) as ctx: + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + self.assertIn("uptime", str(ctx.exception).lower()) + + def test_install_os_reboot_required_with_reboot_false_raises_error(self): + """Test that install_os raises error when reboot is required but reboot=False.""" + with mock.patch.object(self.device, "_validate_multiple_device", return_value=False): + with mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime: + mock_uptime.return_value = 1000 + reboot_msg = "WARNING: A reboot is required to install the software" + self.device.sw.install.return_value = (False, reboot_msg) + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=False, + ) + + def test_install_os_install_failure_reboot_false_raises_error(self): + """Test that install_os raises error immediately when install fails with reboot=False.""" + with mock.patch.object(self.device, "_validate_multiple_device", return_value=False): + with mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime: + mock_uptime.return_value = 1000 + self.device.sw.install.return_value = False + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=False, + ) + class TestJnprFreeSpace(unittest.TestCase): """Tests for JunOS pre-transfer free-space verification (NAPPS-1085).""" From a49fb86b49ed1f812341f4c88f9a1d0f56d921fa Mon Sep 17 00:00:00 2001 From: James Williams Date: Wed, 8 Jul 2026 10:35:42 -0500 Subject: [PATCH 07/14] update to juniper remote_file_copy for virtual chassis --- changes/400.fixed | 2 + pyntc/devices/jnpr_device.py | 65 ++++++++---- tests/unit/test_devices/test_jnpr_device.py | 103 +++++++++++++------- 3 files changed, 119 insertions(+), 51 deletions(-) create mode 100644 changes/400.fixed diff --git a/changes/400.fixed b/changes/400.fixed new file mode 100644 index 00000000..f63976d6 --- /dev/null +++ b/changes/400.fixed @@ -0,0 +1,2 @@ +Fixed `JunosDevice._get_free_space` raising `FileSystemNotFoundError` on virtual-chassis, multi-RE, and cluster devices by handling the nested per-member storage output and returning the smallest member's free space. +Fixed `JunosDevice.remote_file_copy` discarding the device's actual error message on a failed copy; the underlying RPC error is now logged and included in the raised `FileTransferError`. diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 5147f736..5bb1b583 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -9,7 +9,7 @@ from urllib.parse import urlparse from jnpr.junos import Device as JunosNativeDevice -from jnpr.junos.exception import ConfigLoadError, RpcTimeoutError +from jnpr.junos.exception import ConfigLoadError, RpcError, RpcTimeoutError from jnpr.junos.op.ethport import EthPortTable # pylint: disable=import-error,no-name-in-module from jnpr.junos.utils.config import Config as JunosNativeConfig from jnpr.junos.utils.fs import FS as JunosNativeFS @@ -124,6 +124,12 @@ def _get_free_space(self, file_system=None): back the correct filesystem's free space. ``/`` is always a fallback when nothing more specific matches. + On virtual-chassis, multi-RE, and cluster platforms, PyEZ returns a + nested ``{member: {filesystem: info}}`` dict instead of the flat + ``{filesystem: info}`` shape. The mount is resolved per member and the + **minimum** free space across members is returned — an install needs + room on every member. + Args: file_system (str, optional): Target path. When ``None`` (the default), the probe uses ``_JUNOS_DEFAULT_FILE_SYSTEM`` @@ -131,21 +137,43 @@ def _get_free_space(self, file_system=None): on Junos). Returns: - int: Free bytes available on the resolved filesystem. + int: Free bytes available on the resolved filesystem (the smallest + member's value on multi-member platforms). Raises: FileSystemNotFoundError: When no mount point encloses ``file_system`` - (i.e., not even ``/`` is present in ``storage_usage``). - CommandError: When the ``avail`` format string cannot be parsed. + on any member (i.e., not even ``/`` is present in + ``storage_usage`` for that member). + CommandError: When an ``avail`` format string cannot be parsed. """ if file_system is None: file_system = _JUNOS_DEFAULT_FILE_SYSTEM usage = self.fs.storage_usage() + # Flat entries always carry a "mount" key; on multi-member platforms + # the top-level values are per-member {filesystem: info} dicts instead. + is_nested = bool(usage) and all(isinstance(info, dict) and "mount" not in info for info in usage.values()) + member_groups = usage if is_nested else {"": usage} + + free_bytes = min( + self._free_bytes_for_mount(filesystems, file_system, member=member) + for member, filesystems in member_groups.items() + ) + log.debug( + "Host %s: %s bytes free (resolved from %s across %d member(s)).", + self.host, + free_bytes, + file_system, + len(member_groups), + ) + return free_bytes + + def _free_bytes_for_mount(self, filesystems, file_system, member=""): + """Resolve ``file_system`` within one member's storage output and return its free bytes.""" best_info = None best_mount = None best_len = -1 - for _dev, info in usage.items(): + for _dev, info in filesystems.items(): mount = info.get("mount") if not mount or not _mount_encloses_path(mount, file_system): continue @@ -156,9 +184,10 @@ def _get_free_space(self, file_system=None): if best_info is None: log.error( - "Host %s: no mount encloses %s in storage_usage output.", + "Host %s: no mount encloses %s in storage_usage output%s.", self.host, file_system, + f" for member {member}" if member else "", ) raise FileSystemNotFoundError(hostname=self.host, command="show system storage") @@ -166,10 +195,11 @@ def _get_free_space(self, file_system=None): match = _JUNOS_AVAIL_FORMAT_RE.match(str(avail)) if match is None: log.error( - "Host %s: could not parse avail %r for mount %s.", + "Host %s: could not parse avail %r for mount %s%s.", self.host, avail, best_mount, + f" on member {member}" if member else "", ) raise CommandError( command="show system storage", @@ -177,15 +207,7 @@ def _get_free_space(self, file_system=None): ) size = float(match.group(1)) multiplier = _JUNOS_SIZE_UNIT_MULTIPLIERS[match.group(2).upper()] - free_bytes = int(size * multiplier) - log.debug( - "Host %s: %s bytes free on %s (resolved from %s).", - self.host, - free_bytes, - best_mount, - file_system, - ) - return free_bytes + return int(size * multiplier) def _get_interfaces(self): eth_ifaces = EthPortTable(self.native) @@ -1146,8 +1168,15 @@ def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: st if not urlparse(source_url).path.strip("/"): source_url = f"{source_url.rstrip('/')}/{src.file_name}" - if not self.fs.cp(from_path=source_url, to_path=dest, dev_timeout=src.timeout): - raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}") + # Issue the file-copy RPC directly rather than through PyEZ ``fs.cp``, + # which wraps it in a bare ``except`` and returns False — discarding the + # device's actual error message. A successful reply may be the bool True + # (empty rpc-reply) or an XML element; only an exception means failure. + try: + self.native.rpc.file_copy(source=source_url, destination=dest, dev_timeout=src.timeout) + except RpcError as exc: + log.error("Host %s: file copy from %s failed: %s", self.host, src.clean_url, exc) + raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}: {exc}") from exc # Some devices take a while to sync the filesystem after a copy but netconf returns before the sync completes for _ in range(5): diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 9981ed92..27eb8691 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -4,7 +4,7 @@ import mock import pytest -from jnpr.junos.exception import ConfigLoadError +from jnpr.junos.exception import ConfigLoadError, RpcTimeoutError from pyntc.devices import JunosDevice from pyntc.errors import ( @@ -75,6 +75,9 @@ def setUp(self): self.device = JunosDevice("host", "user", "pass") self.device.native.facts = DEVICE_FACTS + # ``rpc`` is created in Device.__init__ so the autospec mock lacks it. + self.device.native.rpc = mock.MagicMock() + self.device.native.rpc.file_copy.return_value = True def tearDown(self): self.mock_sw.stop() @@ -625,7 +628,9 @@ def test_remote_file_copy(self, mock_sleep): file_size=330656851, ) - self.device.fs.cp.return_value = True + self.device.native.rpc = mock.MagicMock() + # PyEZ returns the bool True (not an XML element) for an empty success reply. + self.device.native.rpc.file_copy.return_value = True self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY with mock.patch.object(self.device, "verify_file") as mock_verify_file: @@ -636,7 +641,7 @@ def test_remote_file_copy(self, mock_sleep): with self.subTest("file already exists"): mock_verify_file.return_value = True result = self.device.remote_file_copy(src_file, dest=dest_file) - self.device.fs.cp.assert_not_called() + self.device.native.rpc.file_copy.assert_not_called() self.assertIsNone(result) with self.subTest("copy successful"): @@ -644,9 +649,9 @@ def test_remote_file_copy(self, mock_sleep): mock_verify_file.side_effect = [False, False, True] mock_verify_file.reset_mock() result = self.device.remote_file_copy(src_file, dest=dest_file) - self.device.fs.cp.assert_called_once_with( - from_path=src_file.download_url, - to_path=dest_file, + self.device.native.rpc.file_copy.assert_called_once_with( + source=src_file.download_url, + destination=dest_file, dev_timeout=src_file.timeout, ) verify_file_calls = [ @@ -657,15 +662,15 @@ def test_remote_file_copy(self, mock_sleep): self.assertIsNone(result) with self.subTest("copy succeeded but checksum failed"): - self.device.fs.cp.reset_mock() + self.device.native.rpc.file_copy.reset_mock() mock_verify_file.reset_mock() mock_verify_file.side_effect = None mock_verify_file.return_value = False with self.assertRaises(FileTransferError): result = self.device.remote_file_copy(src_file, dest=dest_file) - self.device.fs.cp.assert_called_once_with( - from_path=src_file.download_url, - to_path=dest_file, + self.device.native.rpc.file_copy.assert_called_once_with( + source=src_file.download_url, + destination=dest_file, dev_timeout=src_file.timeout, ) verify_file_calls = [ @@ -674,16 +679,17 @@ def test_remote_file_copy(self, mock_sleep): mock_verify_file.assert_has_calls(verify_file_calls * 6) - with self.subTest("copy failed"): - self.device.fs.cp.reset_mock() - self.device.fs.cp.return_value = False - with self.assertRaises(FileTransferError): + with self.subTest("copy failed surfaces the device's RPC error"): + self.device.native.rpc.file_copy.reset_mock() + mock_verify_file.side_effect = None + mock_verify_file.return_value = False + rpc_exc = RpcTimeoutError(self.device.native, "file-copy-rpc", 900) + self.device.native.rpc.file_copy.side_effect = rpc_exc + with self.assertRaises(FileTransferError) as ctx: result = self.device.remote_file_copy(src_file, dest=dest_file) - self.device.fs.cp.assert_called_once_with( - from_path=src_file.download_url, - to_path=dest_file, - dev_timeout=src_file.timeout, - ) + # The FileTransferError must carry the underlying RpcError, not discard it. + self.assertIn("file-copy-rpc", str(ctx.exception)) + self.assertIs(ctx.exception.__cause__, rpc_exc) def test_verify_file(self): checksum = "c0ffee" @@ -794,6 +800,9 @@ def setUp(self): self.device = JunosDevice("host", "user", "pass") self.device.native.facts = DEVICE_FACTS + # ``rpc`` is created in Device.__init__ so the autospec mock lacks it. + self.device.native.rpc = mock.MagicMock() + self.device.native.rpc.file_copy.return_value = True def tearDown(self): self.mock_sw.stop() @@ -856,6 +865,37 @@ def test_get_free_space_raises_on_unparseable_avail(self): with self.assertRaises(CommandError): self.device._get_free_space() + def test_get_free_space_virtual_chassis_returns_minimum_across_members(self): + """On VC/multi-RE devices storage_usage nests per member; return the smallest member's free space.""" + self.device.fs.storage_usage.return_value = { + "fpc0": {"/dev/da0s3d": {"mount": "/var/tmp", "avail": "225M"}}, + "fpc1": {"/dev/da0s3d": {"mount": "/var/tmp", "avail": "323M"}}, + } + self.assertEqual(self.device._get_free_space(), 225 * 1024**2) + + def test_get_free_space_cluster_longest_prefix_match_per_member(self): + """Mount resolution (longest prefix) applies within each cluster member independently.""" + self.device.fs.storage_usage.return_value = { + "node0": { + "/dev/ad0s1a": {"mount": "/", "avail": "500M"}, + "/dev/ad0s1f": {"mount": "/var", "avail": "2.0G"}, + }, + "node1": { + "/dev/ad0s1a": {"mount": "/", "avail": "3.0G"}, + }, + } + # node0 resolves /var/tmp to /var (2G); node1 falls back to / (3G). Minimum is 2G. + self.assertEqual(self.device._get_free_space(), 2 * 1024**3) + + def test_get_free_space_raises_when_a_member_has_no_matching_mount(self): + """A member with no mount enclosing the path raises rather than overstating free space.""" + self.device.fs.storage_usage.return_value = { + "fpc0": {"/dev/da0s3d": {"mount": "/var/tmp", "avail": "1.0G"}}, + "fpc1": {"cgroups": {"mount": "/sys/fs/cgroup", "avail": "0B"}}, + } + with self.assertRaises(FileSystemNotFoundError): + self.device._get_free_space() + @mock.patch("pyntc.devices.jnpr_device.os.path.getsize", return_value=10**12) @mock.patch("pyntc.devices.jnpr_device.SCP") def test_file_copy_raises_not_enough_free_space(self, mock_scp, _getsize): @@ -869,7 +909,7 @@ def test_file_copy_raises_not_enough_free_space(self, mock_scp, _getsize): mock_scp.assert_not_called() def test_remote_file_copy_raises_not_enough_free_space(self): - """remote_file_copy raises NotEnoughFreeSpaceError and never invokes fs.cp.""" + """remote_file_copy raises NotEnoughFreeSpaceError and never invokes the file-copy RPC.""" self.device.fs.storage_usage.return_value = { "/dev/ad0s1f": {"mount": "/var/tmp", "avail": "10M"}, } @@ -883,11 +923,10 @@ def test_remote_file_copy_raises_not_enough_free_space(self): with mock.patch.object(self.device, "verify_file", return_value=False): with self.assertRaises(NotEnoughFreeSpaceError): self.device.remote_file_copy(oversized, dest="/var/tmp/file.bin") - self.device.fs.cp.assert_not_called() + self.device.native.rpc.file_copy.assert_not_called() def test_remote_file_copy_skips_space_check_when_file_size_omitted(self): """When FileCopyModel has no file_size, _check_free_space is NOT called.""" - self.device.fs.cp.return_value = True model = FileCopyModel( download_url="ftp://example.com/file.bin", checksum="c0ffee", @@ -900,11 +939,10 @@ def test_remote_file_copy_skips_space_check_when_file_size_omitted(self): ): self.device.remote_file_copy(model, dest="/var/tmp/file.bin") mock_check.assert_not_called() - self.device.fs.cp.assert_called_once() + self.device.native.rpc.file_copy.assert_called_once() def test_remote_file_copy_appends_filename_when_url_has_no_path(self): - """A bare ``ftp://host`` URL gets ``/`` appended before ``fs.cp``.""" - self.device.fs.cp.return_value = True + """A bare ``ftp://host`` URL gets ``/`` appended before the file-copy RPC.""" self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY model = FileCopyModel( download_url="ftp://ntc:pw@10.1.100.220", # no path @@ -915,15 +953,14 @@ def test_remote_file_copy_appends_filename_when_url_has_no_path(self): ) with mock.patch.object(self.device, "verify_file", side_effect=[False, True]): self.device.remote_file_copy(model, dest="/var/tmp/image.bin") - self.device.fs.cp.assert_called_once_with( - from_path="ftp://ntc:pw@10.1.100.220/image.bin", - to_path="/var/tmp/image.bin", + self.device.native.rpc.file_copy.assert_called_once_with( + source="ftp://ntc:pw@10.1.100.220/image.bin", + destination="/var/tmp/image.bin", dev_timeout=mock.ANY, ) def test_remote_file_copy_keeps_url_intact_when_path_is_present(self): - """When the URL already contains a path, ``fs.cp`` receives it unchanged.""" - self.device.fs.cp.return_value = True + """When the URL already contains a path, the file-copy RPC receives it unchanged.""" self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY model = FileCopyModel( download_url="ftp://ntc:pw@10.1.100.220/subdir/image.bin", @@ -934,9 +971,9 @@ def test_remote_file_copy_keeps_url_intact_when_path_is_present(self): ) with mock.patch.object(self.device, "verify_file", side_effect=[False, True]): self.device.remote_file_copy(model, dest="/var/tmp/image.bin") - self.device.fs.cp.assert_called_once_with( - from_path="ftp://ntc:pw@10.1.100.220/subdir/image.bin", - to_path="/var/tmp/image.bin", + self.device.native.rpc.file_copy.assert_called_once_with( + source="ftp://ntc:pw@10.1.100.220/subdir/image.bin", + destination="/var/tmp/image.bin", dev_timeout=mock.ANY, ) From f3e13690c80c3498c7c22271e74d29702d7dd176 Mon Sep 17 00:00:00 2001 From: James Williams Date: Wed, 8 Jul 2026 15:40:17 -0500 Subject: [PATCH 08/14] update jnpr remote file copy based on testing --- changes/400.fixed | 1 + pyntc/devices/jnpr_device.py | 98 ++++++++++++-- tests/unit/test_devices/test_jnpr_device.py | 134 +++++++++++++++----- 3 files changed, 193 insertions(+), 40 deletions(-) diff --git a/changes/400.fixed b/changes/400.fixed index f63976d6..a8f523dc 100644 --- a/changes/400.fixed +++ b/changes/400.fixed @@ -1,2 +1,3 @@ Fixed `JunosDevice._get_free_space` raising `FileSystemNotFoundError` on virtual-chassis, multi-RE, and cluster devices by handling the nested per-member storage output and returning the smallest member's free space. Fixed `JunosDevice.remote_file_copy` discarding the device's actual error message on a failed copy; the underlying RPC error is now logged and included in the raised `FileTransferError`. +Fixed `JunosDevice.remote_file_copy` failing with "filesystem is full" on small-flash platforms (e.g., EX switches) by downloading via a shell `fetch` directly to the destination path instead of the `file copy` RPC, which stages the transfer under the user's home directory on `/var`; the RPC remains the fallback for non-fetch schemes and platforms without a `fetch` binary. diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 5bb1b583..bc387599 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -3,6 +3,7 @@ import hashlib import os import re +import shlex import time import warnings from tempfile import NamedTemporaryFile @@ -14,6 +15,7 @@ from jnpr.junos.utils.config import Config as JunosNativeConfig from jnpr.junos.utils.fs import FS as JunosNativeFS from jnpr.junos.utils.scp import SCP +from jnpr.junos.utils.start_shell import StartShell from jnpr.junos.utils.sw import SW as JunosNativeSW from pyntc import log @@ -47,6 +49,10 @@ # mount point, not a local temp directory). _JUNOS_DEFAULT_FILE_SYSTEM = "/var/tmp" # noqa: S108 +# URL schemes the FreeBSD ``fetch`` binary on Junos can download. Other +# schemes (e.g., scp) go through the ``file copy`` RPC instead. +_JUNOS_FETCH_SCHEMES = {"ftp", "http", "https"} + # Hashing algorithms that Junos implements for the ``file checksum`` RPC. # Junos does NOT implement sha512; callers passing it will be rejected at the # driver boundary rather than surfacing PyEZ's raw ValueError deeper in the @@ -1134,9 +1140,79 @@ def compare_file_checksum(self, checksum, filename, hashing_algorithm="md5"): """ return checksum == self.get_remote_checksum(filename, hashing_algorithm) + def _remote_file_copy_shell_fetch(self, src, source_url, dest): + r"""Download ``source_url`` straight to ``dest`` with the shell ``fetch`` binary. + + The ``file copy`` RPC stages remote fetches under the calling user's + home directory (``/var/home/`` on the ``/var`` partition) and + only moves the file to the destination afterwards, so it fails with + "filesystem is full" whenever the image is larger than the free space + on ``/var`` — even when the destination filesystem has plenty of room. + ``fetch -o`` writes directly to the destination path with no staging + copy. ``-q`` is required, not cosmetic: fetch's progress lines + (``89% of 119 MB``) can match PyEZ's shell-prompt pattern + ``(%|#|$)\s`` and would end the prompt wait mid-transfer. + + Returns: + bool: True when the file was downloaded; False when the platform + has no ``fetch`` binary (e.g., Junos Evolved) and the caller + should fall back to the ``file copy`` RPC. + + Raises: + FileTransferError: When ``fetch`` ran and failed, or the shell + session itself could not be established. + """ + fetch_cmd = f"fetch -q -o {shlex.quote(dest)} {shlex.quote(source_url)}" + if src.scheme == "ftp": + fetch_cmd = f"setenv FTP_PASSIVE_MODE {'yes' if src.ftp_passive else 'no'}; {fetch_cmd}" + + exit_ok, output = False, "" + try: + with StartShell(self.native) as shell: + exit_ok, output = shell.run(fetch_cmd, timeout=src.timeout) + if not exit_ok: + if "not found" in output.lower(): + log.warning( + "Host %s: no fetch binary on this platform; falling back to the file-copy RPC.", + self.host, + ) + return False + # Remove the partial file so it does not consume the space a retry needs. + shell.run(f"rm -f {shlex.quote(dest)}") + except Exception as exc: # pylint: disable=broad-exception-caught + log.error("Host %s: shell fetch from %s failed: %s", self.host, src.clean_url, exc) + raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}: {exc}") from exc + + if not exit_ok: + # The echoed fetch command carries the URL credential; mask it. + error = output.replace(src.token, "*****").strip() if src.token else output.strip() + log.error("Host %s: shell fetch from %s failed: %s", self.host, src.clean_url, error) + raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}: {error}") + return True + + def _remote_file_copy_rpc(self, src, source_url, dest): + """Copy ``source_url`` to ``dest`` with the ``file copy`` RPC. + + Issued directly rather than through PyEZ ``fs.cp``, which wraps the RPC + in a bare ``except`` and returns False — discarding the device's actual + error message. A successful reply may be the bool True (empty rpc-reply) + or an XML element; only an exception means failure. + """ + try: + self.native.rpc.file_copy(source=source_url, destination=dest, dev_timeout=src.timeout) + except RpcError as exc: + log.error("Host %s: file copy from %s failed: %s", self.host, src.clean_url, exc) + raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}: {exc}") from exc + def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: str | None = None, **kwargs): """Copy a file to a remote device. + For ``ftp``/``http``/``https`` URLs the transfer runs as a shell + ``fetch`` writing directly to ``dest``, avoiding the ``file copy`` + RPC's staging copy in the user's home directory on ``/var`` (which + fails on small-flash platforms). Other schemes, and platforms without + a ``fetch`` binary, use the ``file copy`` RPC. + Args: src (FileCopyModel): The source file model. dest (str): The destination file path on the remote device. @@ -1162,21 +1238,23 @@ def remote_file_copy(self, src: FileCopyModel = None, dest=None, file_system: st self._pre_transfer_space_check(src, file_system=file_system) - # Junos ``fs.cp`` requires the filename in the URL; append ``src.file_name`` + # The download URL requires the filename; append ``src.file_name`` # when the URL carries no path so callers can point at a bare host. source_url = src.download_url if not urlparse(source_url).path.strip("/"): source_url = f"{source_url.rstrip('/')}/{src.file_name}" - # Issue the file-copy RPC directly rather than through PyEZ ``fs.cp``, - # which wraps it in a bare ``except`` and returns False — discarding the - # device's actual error message. A successful reply may be the bool True - # (empty rpc-reply) or an XML element; only an exception means failure. - try: - self.native.rpc.file_copy(source=source_url, destination=dest, dev_timeout=src.timeout) - except RpcError as exc: - log.error("Host %s: file copy from %s failed: %s", self.host, src.clean_url, exc) - raise FileTransferError(message=f"Unable to copy file from remote url {src.clean_url}: {exc}") from exc + # The ``file copy`` RPC stages remote fetches in the calling user's home + # directory (on the small ``/var`` partition) before moving them to the + # destination, so it fails with "filesystem is full" on small-flash + # platforms even when the destination filesystem has room. Prefer a + # shell ``fetch`` which writes straight to ``dest``; fall back to the + # RPC for schemes ``fetch`` cannot handle or platforms without the binary. + copied = False + if src.scheme in _JUNOS_FETCH_SCHEMES: + copied = self._remote_file_copy_shell_fetch(src, source_url, dest) + if not copied: + self._remote_file_copy_rpc(src, source_url, dest) # Some devices take a while to sync the filesystem after a copy but netconf returns before the sync completes for _ in range(5): diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 27eb8691..2b9e01b9 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -613,8 +613,9 @@ def test_compare_file_checksum(self): mock_get_remote_checksum.assert_called_once_with("foo.txt", "sha1") self.assertFalse(result) + @mock.patch("pyntc.devices.jnpr_device.StartShell") @mock.patch("pyntc.devices.jnpr_device.time.sleep") - def test_remote_file_copy(self, mock_sleep): + def test_remote_file_copy(self, mock_sleep, mock_start_shell): ftp_url = "ftp://example.com/file.bin" md5_checksum = "c0ffee" filename = "file.bin" @@ -627,11 +628,12 @@ def test_remote_file_copy(self, mock_sleep): timeout=1200, file_size=330656851, ) + fetch_cmd = f"setenv FTP_PASSIVE_MODE yes; fetch -q -o {dest_file} {ftp_url}" self.device.native.rpc = mock.MagicMock() - # PyEZ returns the bool True (not an XML element) for an empty success reply. - self.device.native.rpc.file_copy.return_value = True self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY + mock_shell = mock_start_shell.return_value.__enter__.return_value + mock_shell.run.return_value = (True, "") with mock.patch.object(self.device, "verify_file") as mock_verify_file: with self.subTest("invalid src argument"): @@ -641,19 +643,17 @@ def test_remote_file_copy(self, mock_sleep): with self.subTest("file already exists"): mock_verify_file.return_value = True result = self.device.remote_file_copy(src_file, dest=dest_file) + mock_start_shell.assert_not_called() self.device.native.rpc.file_copy.assert_not_called() self.assertIsNone(result) - with self.subTest("copy successful"): + with self.subTest("copy successful via shell fetch"): # First False because file does not already exist, then emulate device returning the wrong checksum while the file syncs mock_verify_file.side_effect = [False, False, True] mock_verify_file.reset_mock() result = self.device.remote_file_copy(src_file, dest=dest_file) - self.device.native.rpc.file_copy.assert_called_once_with( - source=src_file.download_url, - destination=dest_file, - dev_timeout=src_file.timeout, - ) + mock_shell.run.assert_called_once_with(fetch_cmd, timeout=src_file.timeout) + self.device.native.rpc.file_copy.assert_not_called() verify_file_calls = [ mock.call(src_file.checksum, dest_file, hashing_algorithm=src_file.hashing_algorithm) ] @@ -662,31 +662,98 @@ def test_remote_file_copy(self, mock_sleep): self.assertIsNone(result) with self.subTest("copy succeeded but checksum failed"): - self.device.native.rpc.file_copy.reset_mock() + mock_shell.run.reset_mock() mock_verify_file.reset_mock() mock_verify_file.side_effect = None mock_verify_file.return_value = False with self.assertRaises(FileTransferError): result = self.device.remote_file_copy(src_file, dest=dest_file) + mock_shell.run.assert_called_once_with(fetch_cmd, timeout=src_file.timeout) + verify_file_calls = [ + mock.call(src_file.checksum, dest_file, hashing_algorithm=src_file.hashing_algorithm) + ] + + mock_verify_file.assert_has_calls(verify_file_calls * 6) + + with self.subTest("fetch failure surfaces the device's error and removes the partial file"): + mock_shell.run.reset_mock() + mock_verify_file.side_effect = None + mock_verify_file.return_value = False + mock_shell.run.side_effect = [ + (False, "fetch: /var/tmp/file.bin: No space left on device"), + (True, ""), # rm -f cleanup of the partial file + ] + with self.assertRaises(FileTransferError) as ctx: + result = self.device.remote_file_copy(src_file, dest=dest_file) + self.assertIn("No space left on device", str(ctx.exception)) + mock_shell.run.assert_called_with(f"rm -f {dest_file}") + + with self.subTest("fetch failure masks the URL credential"): + mock_shell.run.reset_mock() + src_with_creds = FileCopyModel( + download_url="ftp://ntc:s3cret@example.com/file.bin", + checksum=md5_checksum, + file_name=filename, + file_size=330656851, + ) + mock_shell.run.side_effect = [ + (False, "fetch -q -o /var/tmp/file.bin ftp://ntc:s3cret@example.com/file.bin\nfetch: failed"), + (True, ""), + ] + with self.assertRaises(FileTransferError) as ctx: + result = self.device.remote_file_copy(src_with_creds, dest=dest_file) + self.assertNotIn("s3cret", str(ctx.exception)) + self.assertIn("*****", str(ctx.exception)) + + with self.subTest("missing fetch binary falls back to the file-copy RPC"): + mock_shell.run.reset_mock() + mock_verify_file.side_effect = [False, True] + mock_shell.run.side_effect = [(False, "fetch: Command not found.")] + # PyEZ returns the bool True (not an XML element) for an empty success reply. + self.device.native.rpc.file_copy.return_value = True + result = self.device.remote_file_copy(src_file, dest=dest_file) self.device.native.rpc.file_copy.assert_called_once_with( source=src_file.download_url, destination=dest_file, dev_timeout=src_file.timeout, ) - verify_file_calls = [ - mock.call(src_file.checksum, dest_file, hashing_algorithm=src_file.hashing_algorithm) - ] + self.assertIsNone(result) - mock_verify_file.assert_has_calls(verify_file_calls * 6) + with self.subTest("non-fetch scheme uses the file-copy RPC"): + mock_start_shell.reset_mock() + mock_shell.run.reset_mock() + mock_shell.run.side_effect = None + self.device.native.rpc.file_copy.reset_mock() + mock_verify_file.side_effect = [False, True] + scp_src = FileCopyModel( + download_url="scp://example.com/file.bin", + checksum=md5_checksum, + file_name=filename, + file_size=330656851, + ) + result = self.device.remote_file_copy(scp_src, dest=dest_file) + mock_start_shell.assert_not_called() + self.device.native.rpc.file_copy.assert_called_once_with( + source=scp_src.download_url, + destination=dest_file, + dev_timeout=scp_src.timeout, + ) + self.assertIsNone(result) - with self.subTest("copy failed surfaces the device's RPC error"): + with self.subTest("RPC copy failure surfaces the device's RPC error"): self.device.native.rpc.file_copy.reset_mock() mock_verify_file.side_effect = None mock_verify_file.return_value = False rpc_exc = RpcTimeoutError(self.device.native, "file-copy-rpc", 900) self.device.native.rpc.file_copy.side_effect = rpc_exc + scp_src = FileCopyModel( + download_url="scp://example.com/file.bin", + checksum=md5_checksum, + file_name=filename, + file_size=330656851, + ) with self.assertRaises(FileTransferError) as ctx: - result = self.device.remote_file_copy(src_file, dest=dest_file) + result = self.device.remote_file_copy(scp_src, dest=dest_file) # The FileTransferError must carry the underlying RpcError, not discard it. self.assertIn("file-copy-rpc", str(ctx.exception)) self.assertIs(ctx.exception.__cause__, rpc_exc) @@ -925,8 +992,11 @@ def test_remote_file_copy_raises_not_enough_free_space(self): self.device.remote_file_copy(oversized, dest="/var/tmp/file.bin") self.device.native.rpc.file_copy.assert_not_called() - def test_remote_file_copy_skips_space_check_when_file_size_omitted(self): + @mock.patch("pyntc.devices.jnpr_device.StartShell") + def test_remote_file_copy_skips_space_check_when_file_size_omitted(self, mock_start_shell): """When FileCopyModel has no file_size, _check_free_space is NOT called.""" + mock_shell = mock_start_shell.return_value.__enter__.return_value + mock_shell.run.return_value = (True, "") model = FileCopyModel( download_url="ftp://example.com/file.bin", checksum="c0ffee", @@ -939,11 +1009,14 @@ def test_remote_file_copy_skips_space_check_when_file_size_omitted(self): ): self.device.remote_file_copy(model, dest="/var/tmp/file.bin") mock_check.assert_not_called() - self.device.native.rpc.file_copy.assert_called_once() + mock_shell.run.assert_called_once() - def test_remote_file_copy_appends_filename_when_url_has_no_path(self): - """A bare ``ftp://host`` URL gets ``/`` appended before the file-copy RPC.""" + @mock.patch("pyntc.devices.jnpr_device.StartShell") + def test_remote_file_copy_appends_filename_when_url_has_no_path(self, mock_start_shell): + """A bare ``ftp://host`` URL gets ``/`` appended before the transfer.""" self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY + mock_shell = mock_start_shell.return_value.__enter__.return_value + mock_shell.run.return_value = (True, "") model = FileCopyModel( download_url="ftp://ntc:pw@10.1.100.220", # no path checksum="c0ffee", @@ -953,15 +1026,17 @@ def test_remote_file_copy_appends_filename_when_url_has_no_path(self): ) with mock.patch.object(self.device, "verify_file", side_effect=[False, True]): self.device.remote_file_copy(model, dest="/var/tmp/image.bin") - self.device.native.rpc.file_copy.assert_called_once_with( - source="ftp://ntc:pw@10.1.100.220/image.bin", - destination="/var/tmp/image.bin", - dev_timeout=mock.ANY, + mock_shell.run.assert_called_once_with( + "setenv FTP_PASSIVE_MODE yes; fetch -q -o /var/tmp/image.bin ftp://ntc:pw@10.1.100.220/image.bin", + timeout=mock.ANY, ) - def test_remote_file_copy_keeps_url_intact_when_path_is_present(self): - """When the URL already contains a path, the file-copy RPC receives it unchanged.""" + @mock.patch("pyntc.devices.jnpr_device.StartShell") + def test_remote_file_copy_keeps_url_intact_when_path_is_present(self, mock_start_shell): + """When the URL already contains a path, the transfer receives it unchanged.""" self.device.fs.storage_usage.return_value = STORAGE_USAGE_PLENTY + mock_shell = mock_start_shell.return_value.__enter__.return_value + mock_shell.run.return_value = (True, "") model = FileCopyModel( download_url="ftp://ntc:pw@10.1.100.220/subdir/image.bin", checksum="c0ffee", @@ -971,10 +1046,9 @@ def test_remote_file_copy_keeps_url_intact_when_path_is_present(self): ) with mock.patch.object(self.device, "verify_file", side_effect=[False, True]): self.device.remote_file_copy(model, dest="/var/tmp/image.bin") - self.device.native.rpc.file_copy.assert_called_once_with( - source="ftp://ntc:pw@10.1.100.220/subdir/image.bin", - destination="/var/tmp/image.bin", - dev_timeout=mock.ANY, + mock_shell.run.assert_called_once_with( + "setenv FTP_PASSIVE_MODE yes; fetch -q -o /var/tmp/image.bin ftp://ntc:pw@10.1.100.220/subdir/image.bin", + timeout=mock.ANY, ) From 63f6afe8c6ddf9885620680838796db313420f1c Mon Sep 17 00:00:00 2001 From: James Williams Date: Wed, 8 Jul 2026 16:06:59 -0500 Subject: [PATCH 09/14] bump wait_for_snapshot timeout --- pyntc/devices/jnpr_device.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index bc387599..86dcc211 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -515,7 +515,7 @@ def _validate_multiple_device(self): log.warning("Host %s: Could not validate multiple devices: %s", self.host, exc) return False - def _wait_for_system_snapshot(self, timeout=900, interval=30): + def _wait_for_system_snapshot(self, timeout=1800, interval=30): """Poll device to verify system snapshot completion. Periodically checks ``show system snapshot media internal`` to verify the snapshot From ba74f2e869142ad4a782033e96aad32e56991a59 Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 9 Jul 2026 09:59:16 -0500 Subject: [PATCH 10/14] update parser for post upgrade parser --- changes/400.fixed | 1 + pyntc/devices/jnpr_device.py | 14 +++-- tests/unit/test_devices/test_jnpr_device.py | 58 +++++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/changes/400.fixed b/changes/400.fixed index a8f523dc..2fd31dff 100644 --- a/changes/400.fixed +++ b/changes/400.fixed @@ -1,3 +1,4 @@ Fixed `JunosDevice._get_free_space` raising `FileSystemNotFoundError` on virtual-chassis, multi-RE, and cluster devices by handling the nested per-member storage output and returning the smallest member's free space. Fixed `JunosDevice.remote_file_copy` discarding the device's actual error message on a failed copy; the underlying RPC error is now logged and included in the raised `FileTransferError`. Fixed `JunosDevice.remote_file_copy` failing with "filesystem is full" on small-flash platforms (e.g., EX switches) by downloading via a shell `fetch` directly to the destination path instead of the `file copy` RPC, which stages the transfer under the user's home directory on `/var`; the RPC remains the fallback for non-fetch schemes and platforms without a `fetch` binary. +Fixed `JunosDevice` post-install version verification failing on platforms whose `show version` output lacks a "JUNOS Base OS Software Suite" line (e.g., EX switches on 15.1); the per-member version is now read from the "Junos:" line with a fallback to the first JUNOS package line. diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 86dcc211..da11363e 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -417,10 +417,18 @@ def _get_all_members_version(self): if line.startswith("fpc") and line.endswith(":"): current_member = line.split("fpc")[1].rstrip(":") members_versions[current_member] = None + continue + + if current_member is None or members_versions[current_member] is not None: + continue - # Extract version from JUNOS Base OS Software Suite line - if current_member and "JUNOS Base OS Software Suite" in line: - # Extract version from format: JUNOS Base OS Software Suite [15.1R7-S2] + # Junos 13.2+ prints a dedicated "Junos: " line. Older + # releases only list packages, and the package names vary by + # platform (EX 15.1 has no "JUNOS Base OS Software Suite" line), + # so fall back to the first "JUNOS []" line. + if line.startswith("Junos:"): + members_versions[current_member] = line.split(":", 1)[1].strip() + elif line.startswith("JUNOS"): match = re.search(r"\[([^\]]+)\]", line) if match: members_versions[current_member] = match.group(1) diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 2b9e01b9..da1a98c2 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -594,6 +594,64 @@ def test_get_all_members_version(self): result = self.device._get_all_members_version() self.assertEqual(result, {"0": "15.1R7-S2", "1": "15.1R7-S2"}) + with self.subTest("EX VC on 15.1 has no Base OS line; version comes from the Junos: line"): + # Verbatim `show version all-members` output from an EX3300-48T VC + # running 15.1R7-S2 — the platform this parser silently failed on. + output = ( + "fpc0:\n" + "--------------------------------------------------------------------------\n" + "Hostname: colo-jnpr-ex3300-sw-1a\n" + "Model: ex3300-48t-bf\n" + "Junos: 15.1R7-S2\n" + "JUNOS EX Software Suite [15.1R7-S2]\n" + "JUNOS FIPS mode utilities [15.1R7-S2]\n" + "JUNOS Online Documentation [15.1R7-S2]\n" + "JUNOS EX 3300 Software Suite [15.1R7-S2]\n" + "JUNOS Web Management Platform Package [15.1R7-S2]\n" + "\n" + "fpc1:\n" + "--------------------------------------------------------------------------\n" + "Hostname: colo-jnpr-ex3300-sw-1a\n" + "Model: ex3300-48t\n" + "Junos: 15.1R7-S2\n" + "JUNOS EX Software Suite [15.1R7-S2]\n" + "JUNOS FIPS mode utilities [15.1R7-S2]\n" + "JUNOS Online Documentation [15.1R7-S2]\n" + "JUNOS EX 3300 Software Suite [15.1R7-S2]\n" + "JUNOS Web Management Platform Package [15.1R7-S2]\n" + ) + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2", "1": "15.1R7-S2"}) + + with self.subTest("pre-13.2 output without a Junos: line falls back to the first JUNOS package"): + output = ( + "fpc0:\n" + "--------------------------------------------------------------------------\n" + "Hostname: colo-jnpr-ex3300-sw-1a\n" + "Model: ex3300-48t-bf\n" + "JUNOS Base OS boot [12.3R12-S10]\n" + "JUNOS Base OS Software Suite [12.3R12-S10]\n" + "JUNOS Kernel Software Suite [12.3R12-S10]\n" + "\n" + "fpc1:\n" + "--------------------------------------------------------------------------\n" + "Hostname: colo-jnpr-ex3300-sw-1a\n" + "Model: ex3300-48t\n" + "JUNOS Base OS boot [12.3R12-S10]\n" + "JUNOS Base OS Software Suite [12.3R12-S10]\n" + "JUNOS Kernel Software Suite [12.3R12-S10]\n" + ) + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "12.3R12-S10", "1": "12.3R12-S10"}) + + with self.subTest("mid-upgrade mixed versions are reported per member"): + output = "fpc0:\nJunos: 15.1R7-S2\n\nfpc1:\nJunos: 12.3R12-S10\n" + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2", "1": "12.3R12-S10"}) + def test_check_file_exists(self): self.device.check_file_exists("foo.txt") self.device.fs.ls.assert_called_once_with("foo.txt") From aed1b2dc93b79803973c2c7230960459561ff76c Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 9 Jul 2026 14:49:12 -0500 Subject: [PATCH 11/14] updates per review --- changes/400.fixed | 9 +- pyntc/devices/jnpr_device.py | 308 ++++++++++++-------- tests/unit/test_devices/test_jnpr_device.py | 296 +++++++++++++------ 3 files changed, 393 insertions(+), 220 deletions(-) diff --git a/changes/400.fixed b/changes/400.fixed index 2fd31dff..dad6f294 100644 --- a/changes/400.fixed +++ b/changes/400.fixed @@ -1,4 +1,5 @@ -Fixed `JunosDevice._get_free_space` raising `FileSystemNotFoundError` on virtual-chassis, multi-RE, and cluster devices by handling the nested per-member storage output and returning the smallest member's free space. -Fixed `JunosDevice.remote_file_copy` discarding the device's actual error message on a failed copy; the underlying RPC error is now logged and included in the raised `FileTransferError`. -Fixed `JunosDevice.remote_file_copy` failing with "filesystem is full" on small-flash platforms (e.g., EX switches) by downloading via a shell `fetch` directly to the destination path instead of the `file copy` RPC, which stages the transfer under the user's home directory on `/var`; the RPC remains the fallback for non-fetch schemes and platforms without a `fetch` binary. -Fixed `JunosDevice` post-install version verification failing on platforms whose `show version` output lacks a "JUNOS Base OS Software Suite" line (e.g., EX switches on 15.1); the per-member version is now read from the "Junos:" line with a fallback to the first JUNOS package line. +Fixed `JunosDevice._get_free_space` raising `FileSystemNotFoundError` on virtual-chassis, multi-RE, and cluster devices; the smallest member's free space is now returned. +Fixed `JunosDevice.remote_file_copy` discarding the device's actual error message on a failed copy; the underlying error is now included in the raised `FileTransferError`. +Fixed `JunosDevice.remote_file_copy` failing with "filesystem is full" on small-flash platforms (e.g., EX switches); remote images are now downloaded directly to the destination path instead of being staged in the user's home directory on `/var`. +Fixed `JunosDevice` post-install version verification failing on platforms whose `show version` output has no "JUNOS Base OS Software Suite" line (e.g., EX switches on 15.1). +Fixed `JunosDevice.install_os` issuing a disruptive full-chassis reboot after an NSSU/ISSU upgrade, which already reboots each member in service during the install. diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index da11363e..c8f90a88 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -53,6 +53,14 @@ # schemes (e.g., scp) go through the ``file copy`` RPC instead. _JUNOS_FETCH_SCHEMES = {"ftp", "http", "https"} +# Head start given to reboots and snapshots before the first status poll — +# probing earlier burns round-trips against a device that cannot be ready yet. +_JUNOS_POLL_WARMUP_SECONDS = 180 + +# Extracts a Junos version from an install image filename. +# Matches formats: 15.1R7-S2, 20.4R3, 21.4X38-D10, 18.4R2.7, etc. +_JUNOS_VERSION_RE = re.compile(r"(\d+\.\d+[A-Z]+\d+(?:\.\d+)?(?:[-][A-Z]+\d+)?)") + # Hashing algorithms that Junos implements for the ``file checksum`` RPC. # Junos does NOT implement sha512; callers passing it will be rejected at the # driver boundary rather than surfacing PyEZ's raw ValueError deeper in the @@ -286,8 +294,12 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=Fal log.debug("Host %s: Pre-reboot disconnect raised %s (ignored).", self.host, close_exc) # Give the device time to boot before polling (avoid hammering a device that's still starting up) - log.info("Host %s: Waiting 180 seconds for device to boot before polling...", self.host) - time.sleep(180) + log.info( + "Host %s: Waiting %s seconds for device to boot before polling...", + self.host, + _JUNOS_POLL_WARMUP_SECONDS, + ) + time.sleep(_JUNOS_POLL_WARMUP_SECONDS) while time.time() - start < timeout: try: @@ -295,40 +307,21 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=Fal self._uptime = None current_uptime = self.uptime - if is_multiple: - # For multi-device, check that no members are in pending state - self.native.facts_refresh() - re_info = self.native.facts.get("re_info", {}) - # Virtual-chassis structure: members are under re_info['default'] - members = re_info.get("default", {}) - pending_members = [ - name - for name, info in members.items() - if name != "default" and isinstance(info, dict) and info.get("status") == "pending" - ] - if pending_members: - log.debug( - "Host %s: Members still pending reboot: %s; still waiting.", - self.host, - pending_members, - ) - elif current_uptime is not None and current_uptime < original_uptime: - log.info( - "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", - self.host, - current_uptime, - original_uptime, - ) - return + if is_multiple and (pending_members := self._pending_reboot_members()): + log.debug( + "Host %s: Members still pending reboot: %s; still waiting.", + self.host, + pending_members, + ) + elif current_uptime is not None and current_uptime < original_uptime: + log.info( + "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", + self.host, + current_uptime, + original_uptime, + ) + return else: - if current_uptime is not None and current_uptime < original_uptime: - log.info( - "Host %s: Device rebooted (uptime %ss < pre-reboot %ss).", - self.host, - current_uptime, - original_uptime, - ) - return log.debug( "Host %s: Reachable but uptime %ss >= pre-reboot %ss; still waiting.", self.host, @@ -342,51 +335,92 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=Fal raise RebootTimeoutError(hostname=self.hostname, wait_time=timeout) - def _wait_for_nssu_completion(self, target_version, timeout=3600, interval=60): - """Wait for all members to complete NSSU and run target version. + def _pending_reboot_members(self): + """Return VC member names whose re_info status is still "pending".""" + # Scoped refresh: a full facts_refresh() re-collects every fact (many RPCs) + # when only re_info is needed here. + try: + self.native.facts_refresh(keys="re_info") + except RuntimeError: + # PyEZ only supports scoped refreshes with fact_style="new" (its default); + # "old"/"both" raise RuntimeError, so fall back to a full refresh. + self.native.facts_refresh() + # Virtual-chassis structure: members are under re_info['default'] + members = self.native.facts.get("re_info", {}).get("default", {}) + return [ + name + for name, info in members.items() + if name != "default" and isinstance(info, dict) and info.get("status") == "pending" + ] + + def _vc_member_count(self): + """Count VC members from the currently cached re_info facts (no refresh).""" + members = self.native.facts.get("re_info", {}).get("default", {}) + return len([name for name in members if name != "default"]) + + def _wait_for_nssu_completion(self, target_version, timeout=3600, interval=60, expected_members=None): + """Wait for all members to complete NSSU/ISSU and run target version. Polls device to verify all members are running the same (target) software version. - Used to ensure NSSU/ISSU has fully completed before proceeding. + The in-service upgrade reboots each member itself (the old master goes down right + as the install RPC returns), so each poll reconnects if the session dropped. Args: target_version (str): Target software version (e.g., "15.1R7-S2"). timeout (int, optional): Max seconds to wait. Defaults to 1 hour. interval (int, optional): Seconds between version checks. Defaults to 60. + expected_members (int, optional): Number of members that must report a + version before the result is trusted. Guards against declaring + success while a member is mid-reboot and absent from + ``show version all-members``. When ``None``, any non-empty result + is compared as-is. Raises: OSInstallError: When all members don't match target version within timeout. """ start = time.time() + # Drop the pre-install NETCONF session first: the old master's reboot kills the + # transport, but PyEZ can still report it as connected — the same quirk + # _wait_for_device_reboot guards against. Closing forces a fresh connection. + try: + self.close() + except Exception as close_exc: # pylint: disable=broad-exception-caught + log.debug("Host %s: Pre-poll disconnect raised %s (ignored).", self.host, close_exc) + while time.time() - start < timeout: try: + self.open() members_versions = self._get_all_members_version() - if not members_versions: - log.debug("Host %s: Could not retrieve member versions; will retry.", self.host) - time.sleep(interval) - continue - - # Check if all members are running the target version - all_match = all(v == target_version for v in members_versions.values()) mismatched = {m: v for m, v in members_versions.items() if v != target_version} - if all_match: + if not members_versions: + log.debug("Host %s: Could not retrieve member versions; will retry.", self.host) + elif expected_members is not None and len(members_versions) < expected_members: + log.debug( + "Host %s: Only %s of %s members reporting a version; still waiting.", + self.host, + len(members_versions), + expected_members, + ) + elif not mismatched: log.info( "Host %s: All members running target version %s.", self.host, target_version, ) return - - log.debug( - "Host %s: Members not all on target version %s. Still waiting on: %s", - self.host, - target_version, - mismatched, - ) + else: + log.debug( + "Host %s: Members not all on target version %s. Still waiting on: %s", + self.host, + target_version, + mismatched, + ) except Exception as exc: # pylint: disable=broad-exception-caught log.debug("Host %s: Version check failed (%s); will retry.", self.host, exc) + self.native.connected = False time.sleep(interval) @@ -412,7 +446,10 @@ def _get_all_members_version(self): members_versions = {} current_member = None - for line in version_output.split("\n"): + # splitlines() + strip() normalize CRLF endings and stray indentation + # from the CLI transport, which would otherwise silently defeat the + # startswith/endswith checks below. + for line in (raw_line.strip() for raw_line in version_output.splitlines()): # Detect member header (fpc0:, fpc1:, etc.) if line.startswith("fpc") and line.endswith(":"): current_member = line.split("fpc")[1].rstrip(":") @@ -422,12 +459,17 @@ def _get_all_members_version(self): if current_member is None or members_versions[current_member] is not None: continue - # Junos 13.2+ prints a dedicated "Junos: " line. Older + # Junos 13.2+ prints a dedicated "Junos: " line, which + # always precedes the package list when both exist. Older # releases only list packages, and the package names vary by # platform (EX 15.1 has no "JUNOS Base OS Software Suite" line), # so fall back to the first "JUNOS []" line. if line.startswith("Junos:"): - members_versions[current_member] = line.split(":", 1)[1].strip() + # First token only: drops qualifiers like "15.1R7-S2 Limited" + # that would break exact-match comparison to target_version. + tokens = line.split(":", 1)[1].split() + if tokens: + members_versions[current_member] = tokens[0] elif line.startswith("JUNOS"): match = re.search(r"\[([^\]]+)\]", line) if match: @@ -545,8 +587,12 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=30): start = time.time() # Give the snapshot time to complete before polling - log.info("Host %s: Waiting 180 seconds for snapshot to complete before polling...", self.host) - time.sleep(180) + log.info( + "Host %s: Waiting %s seconds for snapshot to complete before polling...", + self.host, + _JUNOS_POLL_WARMUP_SECONDS, + ) + time.sleep(_JUNOS_POLL_WARMUP_SECONDS) while time.time() - start < timeout: try: @@ -591,7 +637,6 @@ def request_system_snapshot(self, parameters=None): command = f"{command} {parameters}" response = None - rpc_timed_out = False try: log.info("Host %s: Issuing snapshot RPC: %s", self.host, command) @@ -600,7 +645,6 @@ def request_system_snapshot(self, parameters=None): except RpcTimeoutError: log.debug("Host %s: snapshot RPC timed out; will poll device to verify.", self.host) - rpc_timed_out = True except Exception as rpc_error: log.error("Host %s: Failed to request system snapshot (%s).", self.host, rpc_error) @@ -611,9 +655,8 @@ def request_system_snapshot(self, parameters=None): log.info("Host %s: Snapshot completed successfully (fast path).", self.host) return - # If RPC timed out or response didn't indicate completion, poll to verify (slow path) - if rpc_timed_out or response is None: - log.debug("Host %s: Snapshot response unclear or timed out; polling device to verify.", self.host) + # Response unclear or RPC timed out; poll to verify (slow path) + log.debug("Host %s: Snapshot response unclear or timed out; polling device to verify.", self.host) self._wait_for_system_snapshot() def backup_running_config(self, filename): @@ -848,7 +891,10 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", """Install OS on device and reboot. For multi-device setups (virtual-chassis, chassis-cluster), supports NSSU/ISSU - upgrades and system snapshots after reboot. + upgrades and system snapshots after reboot. NSSU/ISSU performs its own rolling + reboot member-by-member during the install, so no separate reboot is issued on + that path; completion is verified by polling until every member reports the + target version. Args: image_name (str): Name of image. @@ -859,13 +905,25 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", issu (bool): Enable In-Service Software Upgrade. Defaults to False. Raises: - ValueError: When both nssu and issu are True (mutually exclusive). + ValueError: When both nssu and issu are True (mutually exclusive), or when + ``reboot=False`` is combined with an in-service (NSSU/ISSU) upgrade on a + multi-device setup — the in-service install reboots the members itself, + so there is no reboot left to defer. """ if nssu and issu: raise ValueError("nssu and issu are mutually exclusive; only one can be True") is_multiple = self._validate_multiple_device() + # In-service upgrades only apply to multi-device setups; on a standalone + # device the nssu/issu flags are ignored and a standard install runs. + in_service = (nssu or issu) and is_multiple + if in_service and not reboot: + raise ValueError( + "reboot=False cannot be combined with nssu/issu on a multi-device setup; " + "the in-service upgrade reboots the members as part of the install" + ) + install_kwargs = { "package": image_name, "checksum": checksum, @@ -876,30 +934,29 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", "timeout": 3600, } - # Add NSSU/ISSU parameters for multi-device setups - if is_multiple: - if nssu: - install_kwargs["nssu"] = True - log.info("Host %s: NSSU enabled for multi-device upgrade", self.host) - elif issu: - install_kwargs["issu"] = True - log.info("Host %s: ISSU enabled for multi-device upgrade", self.host) - - install_result = self.sw.install(**install_kwargs) + if in_service: + install_kwargs["nssu" if nssu else "issu"] = True + log.info( + "Host %s: %s enabled for multi-device upgrade", + self.host, + "NSSU" if nssu else "ISSU", + ) # Sometimes install() returns a tuple of (ok, msg). Other times it returns a single bool + install_ok = self.sw.install(**install_kwargs) install_msg = None - if isinstance(install_result, tuple): - install_ok, install_msg = install_result[0], install_result[1] - else: - install_ok = install_result + if isinstance(install_ok, tuple): + install_ok, install_msg = install_ok[0], install_ok[1] - log.info("Host %s: install_ok result: %s (type: %s)", self.host, install_ok, type(install_result).__name__) + log.info("Host %s: install_ok result: %s", self.host, install_ok) if install_msg: log.debug("Host %s: install message: %s", self.host, install_msg) - # Check if reboot is required (indicated by specific message in output) - reboot_required = install_msg and "A reboot is required" in str(install_msg) + # Check if reboot is required (indicated by specific message in output). + # NSSU/ISSU per-member output contains "A reboot is required" as informational + # text, but the rolling reboot already happens inside the install — don't + # treat it as an outstanding manual reboot on that path. + reboot_required = bool(install_msg) and "A reboot is required" in str(install_msg) and not in_service if not install_ok and not reboot_required: log.error( @@ -915,59 +972,64 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) return True - log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) - self._uptime = None - original_uptime = self.uptime - - if original_uptime is None: - raise CommandError( - command="install_os", - message="Could not determine pre-reboot uptime; refusing to reboot.", + if in_service: + # The in-service upgrade already rolled through the members and rebooted + # each one inside ``sw.install()`` — the old master is typically still + # rebooting when the install RPC returns. Issuing another reboot here + # would take the whole chassis down and race that in-progress reboot. + log.info( + "Host %s: %s performed its rolling reboot during install; skipping manual reboot", + self.host, + "NSSU" if nssu else "ISSU", ) - - # Issue reboot command based on device configuration - if is_multiple: - self._request_system_reboot_all_members() else: - self.sw.reboot(in_min=0) + log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) + if is_multiple: + self._uptime = None + original_uptime = self.uptime - self._wait_for_device_reboot(original_uptime, is_multiple=is_multiple) + if original_uptime is None: + raise CommandError( + command="install_os", + message="Could not determine pre-reboot uptime; refusing to reboot.", + ) + + self._request_system_reboot_all_members() + self._wait_for_device_reboot(original_uptime, is_multiple=True) + else: + self.reboot(wait_for_reload=True) # Extract target version from image name for verification - # Matches formats: 15.1R7-S2, 20.4R3, 21.4X38-D10, 18.4R2.7, etc. - match = re.search(r"(\d+\.\d+[A-Z]+\d+(?:\.\d+)?(?:[-][A-Z]+\d+)?)", image_name) + match = _JUNOS_VERSION_RE.search(image_name) target_version = match.group(1) if match else None + if target_version is None: + log.warning( + "Host %s: Could not extract version from image name %s; skipping post-install version checks", + self.host, + image_name, + ) - # For NSSU/ISSU on multi-device, wait for all members to reach target version before snapshot - if (nssu or issu) and is_multiple: - if target_version: - log.info( - "Host %s: Waiting for all members to complete %s upgrade to version %s", - self.host, - "NSSU" if nssu else "ISSU", - target_version, - ) - self._wait_for_nssu_completion(target_version) - else: - log.warning( - "Host %s: Could not extract version from image name %s; skipping NSSU/ISSU completion wait", - self.host, - image_name, - ) + # For in-service upgrades, wait for all members to reach the target version before + # snapshot. This wait already confirms every member runs target_version, so the + # post-snapshot verification below would just repeat the same RPC and check. + verified_by_completion_wait = bool(target_version) and in_service + if verified_by_completion_wait: + log.info( + "Host %s: Waiting for all members to complete %s upgrade to version %s", + self.host, + "NSSU" if nssu else "ISSU", + target_version, + ) + # Member count from the pre-install facts: a member absent from + # ``show version all-members`` while it reboots must not count as done. + self._wait_for_nssu_completion(target_version, expected_members=self._vc_member_count() or None) # Perform system snapshot after reboot/upgrade - snapshot_params = "slice alternate all-members" if is_multiple else "slice alternate" - self.request_system_snapshot(parameters=snapshot_params) + self.request_system_snapshot(parameters="slice alternate all-members" if is_multiple else "slice alternate") # Verify the install was successful by checking the running version - if target_version: + if target_version and not verified_by_completion_wait: self._verify_install_version(target_version, is_multiple) - else: - log.warning( - "Host %s: Could not extract version from image name %s; skipping post-reboot version verification", - self.host, - image_name, - ) log.info("Host %s: OS image %s installed successfully.", self.host, image_name) return True diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index da1a98c2..75d6c301 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -1,3 +1,4 @@ +import itertools import os import unittest from tempfile import NamedTemporaryFile @@ -6,7 +7,7 @@ import pytest from jnpr.junos.exception import ConfigLoadError, RpcTimeoutError -from pyntc.devices import JunosDevice +from pyntc.devices import JunosDevice, jnpr_device from pyntc.errors import ( CommandError, CommandListError, @@ -450,96 +451,192 @@ def test_validate_multiple_device(self): self.assertTrue(result) def test_install_os_single_device(self): - with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: - with mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot: - with mock.patch.object(self.device, "request_system_snapshot"): - with mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime: - mock_uptime.return_value = 1000 - with self.subTest("install succeeds, reboot requested"): - with mock.patch.object(self.device, "_verify_install_version"): - mock_validate.return_value = False - self.device.sw.install.return_value = True - result = self.device.install_os( - image_name="/var/tmp/image-15.1R7-S2-signed.tgz", - checksum="c0ffee", - reboot=True, - ) - self.assertTrue(result) - self.device.sw.install.assert_called_once() - self.device.sw.reboot.assert_called_once_with(in_min=0) - mock_wait_reboot.assert_called_once() - - with self.subTest("install fails immediately"): - self.device.sw.install.reset_mock() - self.device.sw.reboot.reset_mock() - self.device.sw.install.return_value = False - with self.assertRaises(OSInstallError): - self.device.install_os( - image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", - checksum="c0ffee", - reboot=True, - ) - # Should not have called reboot after install failure - self.device.sw.reboot.assert_not_called() - - with self.subTest("reboot=False"): - with mock.patch.object(self.device, "_verify_install_version"): - self.device.sw.install.reset_mock() - self.device.sw.reboot.reset_mock() - self.device.sw.install.return_value = True - result = self.device.install_os( - image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", - checksum="c0ffee", - reboot=False, - ) - self.assertTrue(result) - self.device.sw.reboot.assert_not_called() + with ( + mock.patch.object(self.device, "_validate_multiple_device") as mock_validate, + mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot, + mock.patch.object(self.device, "request_system_snapshot"), + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + with self.subTest("install succeeds, reboot requested"): + with mock.patch.object(self.device, "_verify_install_version"): + mock_validate.return_value = False + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/image-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + self.assertTrue(result) + self.device.sw.install.assert_called_once() + self.device.sw.reboot.assert_called_once_with(in_min=0) + mock_wait_reboot.assert_called_once() + + with self.subTest("install fails immediately"): + self.device.sw.install.reset_mock() + self.device.sw.reboot.reset_mock() + self.device.sw.install.return_value = False + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + # Should not have called reboot after install failure + self.device.sw.reboot.assert_not_called() + + with self.subTest("reboot=False"): + with mock.patch.object(self.device, "_verify_install_version"): + self.device.sw.install.reset_mock() + self.device.sw.reboot.reset_mock() + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=False, + ) + self.assertTrue(result) + self.device.sw.reboot.assert_not_called() def test_install_os_multi_device(self): - with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: - with mock.patch.object(self.device, "_request_system_reboot_all_members") as mock_reboot_all: - with mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot: - with mock.patch.object(self.device, "request_system_snapshot"): - with mock.patch.object(self.device, "_verify_install_version"): - with mock.patch.object( - type(self.device), "uptime", new_callable=mock.PropertyMock - ) as mock_uptime: - mock_uptime.return_value = 1000 - mock_validate.return_value = True - self.device.sw.install.return_value = True - result = self.device.install_os( - image_name="/var/tmp/image-15.1R7-S2-signed.tgz", - checksum="c0ffee", - reboot=True, - ) - self.assertTrue(result) - # Should call multi-device reboot, not sw.reboot - mock_reboot_all.assert_called_once() - self.device.sw.reboot.assert_not_called() - mock_wait_reboot.assert_called_once() + with ( + mock.patch.object(self.device, "_validate_multiple_device") as mock_validate, + mock.patch.object(self.device, "_request_system_reboot_all_members") as mock_reboot_all, + mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot, + mock.patch.object(self.device, "request_system_snapshot"), + mock.patch.object(self.device, "_verify_install_version"), + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + mock_validate.return_value = True + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/image-15.1R7-S2-signed.tgz", + checksum="c0ffee", + reboot=True, + ) + self.assertTrue(result) + # Should call multi-device reboot, not sw.reboot + mock_reboot_all.assert_called_once() + self.device.sw.reboot.assert_not_called() + mock_wait_reboot.assert_called_once() def test_install_os_with_nssu(self): - with mock.patch.object(self.device, "_validate_multiple_device") as mock_validate: - with mock.patch.object(self.device, "_request_system_reboot_all_members"): - with mock.patch.object(self.device, "_wait_for_device_reboot"): - with mock.patch.object(self.device, "_wait_for_nssu_completion") as mock_nssu_wait: - with mock.patch.object(self.device, "request_system_snapshot"): - with mock.patch.object(self.device, "_verify_install_version"): - with mock.patch.object( - type(self.device), "uptime", new_callable=mock.PropertyMock - ) as mock_uptime: - mock_uptime.return_value = 1000 - mock_validate.return_value = True - self.device.sw.install.return_value = True - self.device.install_os( - image_name="/var/tmp/jinstall-ex-3300-15.1R7-S2-domestic-signed.tgz", - checksum="c0ffee", - nssu=True, - ) - # Should have called nssu wait with extracted version - mock_nssu_wait.assert_called_once() - args = mock_nssu_wait.call_args[0] - self.assertEqual(args[0], "15.1R7-S2") + with ( + mock.patch.object(self.device, "_validate_multiple_device") as mock_validate, + mock.patch.object(self.device, "_request_system_reboot_all_members") as mock_reboot_all, + mock.patch.object(self.device, "_wait_for_device_reboot") as mock_wait_reboot, + mock.patch.object(self.device, "_wait_for_nssu_completion") as mock_nssu_wait, + mock.patch.object(self.device, "request_system_snapshot"), + mock.patch.object(self.device, "_verify_install_version") as mock_verify, + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + mock_validate.return_value = True + self.device.native.facts = { + **DEVICE_FACTS, + "re_info": {"default": {"0": {"status": "OK"}, "1": {"status": "OK"}, "default": {"status": "OK"}}}, + } + self.device.sw.install.return_value = True + self.device.install_os( + image_name="/var/tmp/jinstall-ex-3300-15.1R7-S2-domestic-signed.tgz", + checksum="c0ffee", + nssu=True, + ) + # NSSU performs its own rolling reboot inside sw.install(); a second, + # manual reboot would take the whole chassis down mid-switchover. + mock_reboot_all.assert_not_called() + mock_wait_reboot.assert_not_called() + self.device.sw.reboot.assert_not_called() + # Completion is verified by polling member versions instead, and a member + # absent from the output mid-reboot must not count as done. + mock_nssu_wait.assert_called_once_with("15.1R7-S2", expected_members=2) + # The completion wait already confirms every member's version; + # the separate post-snapshot verification must not repeat it. + mock_verify.assert_not_called() + + def test_install_os_nssu_with_reboot_false_raises_value_error(self): + with mock.patch.object(self.device, "_validate_multiple_device", return_value=True): + with self.assertRaises(ValueError): + self.device.install_os( + image_name="/var/tmp/jinstall-ex-3300-15.1R7-S2-domestic-signed.tgz", + checksum="c0ffee", + nssu=True, + reboot=False, + ) + # The contradiction must be rejected before touching the device. + self.device.sw.install.assert_not_called() + + def test_install_os_nssu_on_single_device_runs_standard_install(self): + with ( + mock.patch.object(self.device, "_validate_multiple_device", return_value=False), + mock.patch.object(self.device, "_wait_for_device_reboot"), + mock.patch.object(self.device, "request_system_snapshot"), + mock.patch.object(self.device, "_verify_install_version"), + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + self.device.sw.install.return_value = True + result = self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + nssu=True, + ) + self.assertTrue(result) + # nssu is ignored on a standalone device: standard install + manual reboot. + self.assertNotIn("nssu", self.device.sw.install.call_args.kwargs) + self.device.sw.reboot.assert_called_once_with(in_min=0) + + @mock.patch("pyntc.devices.jnpr_device.time.sleep") + def test_wait_for_nssu_completion(self, mock_sleep): + target = "15.1R7-S2" + with mock.patch.object(self.device, "_get_all_members_version") as mock_versions: + with self.subTest("succeeds when every expected member reports the target version"): + mock_versions.return_value = {"0": target, "1": target} + self.device._wait_for_nssu_completion(target, expected_members=2) + + with self.subTest("keeps waiting while a rebooting member is absent from the output"): + mock_versions.reset_mock() + mock_versions.side_effect = [ + {"1": target}, # old master rebooting; only the new master reports + {"0": target, "1": target}, + ] + self.device._wait_for_nssu_completion(target, expected_members=2) + self.assertEqual(mock_versions.call_count, 2) + + with self.subTest("reconnects after a dropped session and keeps polling"): + mock_versions.side_effect = [ + ConnectionError("session dropped with the old master"), + {"0": target, "1": target}, + ] + self.device.native.connected = True + self.device._wait_for_nssu_completion(target, expected_members=2) + # The failed poll must force a fresh connection on the next attempt. + self.assertFalse(self.device.native.connected is True) + + with self.subTest("raises OSInstallError when versions never converge"): + mock_versions.reset_mock() + mock_versions.side_effect = None + mock_versions.return_value = {"0": "12.3R12-S10", "1": target} + # Patching time.time patches the shared stdlib module (logging calls it + # too), so use a monotonic fake clock that tolerates extra calls. + fake_clock = itertools.count(start=0, step=10) + with mock.patch("pyntc.devices.jnpr_device.time.time", side_effect=lambda: next(fake_clock)): + with self.assertRaises(OSInstallError): + self.device._wait_for_nssu_completion(target, timeout=50, expected_members=2) + # At least one poll saw the mismatched member before giving up. + self.assertGreaterEqual(mock_versions.call_count, 1) + + def test_vc_member_count(self): + with self.subTest("two-member virtual chassis"): + self.device.native.facts = { + "re_info": {"default": {"0": {"status": "OK"}, "1": {"status": "OK"}, "default": {"status": "OK"}}} + } + self.assertEqual(self.device._vc_member_count(), 2) + + with self.subTest("no re_info fact"): + self.device.native.facts = {} + self.assertEqual(self.device._vc_member_count(), 0) def test_request_system_snapshot(self): with mock.patch.object(self.device, "_wait_for_system_snapshot"): @@ -652,6 +749,24 @@ def test_get_all_members_version(self): result = self.device._get_all_members_version() self.assertEqual(result, {"0": "15.1R7-S2", "1": "12.3R12-S10"}) + with self.subTest("qualifier suffix on the Junos: line is dropped"): + output = "fpc0:\nJunos: 15.1R7-S2 Limited\n" + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2"}) + + with self.subTest("CRLF line endings from the transport are normalized"): + output = "fpc0:\r\nJunos: 15.1R7-S2\r\n\r\nfpc1:\r\nJunos: 15.1R7-S2\r\n" + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2", "1": "15.1R7-S2"}) + + with self.subTest("leading whitespace on header and version lines is tolerated"): + output = " fpc0:\n Junos: 15.1R7-S2\n" + self.device.native.cli.return_value = output + result = self.device._get_all_members_version() + self.assertEqual(result, {"0": "15.1R7-S2"}) + def test_check_file_exists(self): self.device.check_file_exists("foo.txt") self.device.fs.ls.assert_called_once_with("foo.txt") @@ -848,11 +963,6 @@ def test_get_remote_checksum_rejects_unsupported_algorithm(self): def test_install_os_version_extraction(self): """Test version regex extraction from various image name formats.""" - import re - - # Pattern from jnpr_device.py line 906 - pattern = r"(\d+\.\d+[A-Z]+\d+(?:\.\d+)?(?:[-][A-Z]+\d+)?)" - test_cases = [ ("/var/tmp/jinstall-15.1R7-S2-signed.tgz", "15.1R7-S2"), ("/var/tmp/image-20.4R3-signed.tgz", "20.4R3"), @@ -863,7 +973,7 @@ def test_install_os_version_extraction(self): for image_name, expected_version in test_cases: with self.subTest(image_name=image_name): - match = re.search(pattern, image_name) + match = jnpr_device._JUNOS_VERSION_RE.search(image_name) extracted = match.group(1) if match else None self.assertEqual(extracted, expected_version, f"Failed to extract version from {image_name}") From c9d02e54c27a15d8053fbce7a34a5d629a3aab0a Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 9 Jul 2026 15:08:50 -0500 Subject: [PATCH 12/14] fixes per copilot comments --- pyntc/devices/jnpr_device.py | 69 ++++++++++++++++----- tests/unit/test_devices/test_jnpr_device.py | 40 +++++++++++- 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index c8f90a88..ae22cbc3 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -281,11 +281,10 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=Fal Args: original_uptime (int): Device uptime in seconds captured before the reboot. - timeout (int, optional): Max seconds to wait for the device to return. Defaults to 2 hours. + timeout (int, optional): Max seconds to poll for the device to return, + counted after the initial warm-up delay. Defaults to 2 hours. is_multiple (bool, optional): Whether device is in multi-device configuration. Defaults to False. """ - start = time.time() - # Drop the pre-reboot NETCONF session so subsequent probes can't read from # a stale connection PyEZ still reports as connected. try: @@ -301,6 +300,9 @@ def _wait_for_device_reboot(self, original_uptime, timeout=7200, is_multiple=Fal ) time.sleep(_JUNOS_POLL_WARMUP_SECONDS) + # Start the clock after the warm-up so ``timeout`` is the real polling window. + start = time.time() + while time.time() - start < timeout: try: self.open() @@ -354,7 +356,11 @@ def _pending_reboot_members(self): ] def _vc_member_count(self): - """Count VC members from the currently cached re_info facts (no refresh).""" + """Count VC members from the currently cached re_info facts (no refresh). + + Only the virtual-chassis ``re_info['default']`` structure is recognized; + other layouts (e.g., SRX chassis-cluster) yield 0. + """ members = self.native.facts.get("re_info", {}).get("default", {}) return len([name for name in members if name != "default"]) @@ -540,9 +546,11 @@ def _request_system_reboot_all_members(self): raise def _validate_multiple_device(self): - """Check if device is in virtual-chassis or chassis-cluster configuration. + """Check if device is in a multi-member configuration. Inspects re_info from PyEZ facts to determine if multiple members are present. + Currently only the virtual-chassis ``re_info['default']`` structure is + recognized; chassis-cluster (SRX) layouts are not yet detected. Returns: bool: True if multiple members are present, False otherwise. @@ -572,7 +580,8 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=30): was successfully taken. Used when the ``request system snapshot`` RPC times out. Args: - timeout (int, optional): Max seconds to wait for snapshot verification. Defaults to 900 (15 minutes). + timeout (int, optional): Max seconds to poll for snapshot verification, + counted after the initial warm-up delay. Defaults to 1800 (30 minutes). interval (int, optional): Seconds between verification polls. Defaults to 30 seconds. Raises: @@ -584,8 +593,6 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=30): timeout, interval, ) - start = time.time() - # Give the snapshot time to complete before polling log.info( "Host %s: Waiting %s seconds for snapshot to complete before polling...", @@ -594,6 +601,9 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=30): ) time.sleep(_JUNOS_POLL_WARMUP_SECONDS) + # Start the clock after the warm-up so ``timeout`` is the real polling window. + start = time.time() + while time.time() - start < timeout: try: output = self.native.cli("show system snapshot media internal") @@ -887,7 +897,7 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", nssu=False, issu=False): # pylint: disable=too-many-positional-arguments,too-many-branches + def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", nssu=False, issu=False): # pylint: disable=too-many-positional-arguments """Install OS on device and reboot. For multi-device setups (virtual-chassis, chassis-cluster), supports NSSU/ISSU @@ -999,10 +1009,41 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", else: self.reboot(wait_for_reload=True) - # Extract target version from image name for verification + self._post_install_checks(image_name, is_multiple, in_service, nssu, verification_required=not install_ok) + + log.info("Host %s: OS image %s installed successfully.", self.host, image_name) + return True + + def _post_install_checks(self, image_name, is_multiple, in_service, nssu, verification_required=False): # pylint: disable=too-many-positional-arguments + """Wait for in-service completion, snapshot, and verify the running version. + + Args: + image_name (str): Name of the installed image; the target version is + extracted from it. When no version can be extracted, the completion + wait and version verification are skipped with a warning. + is_multiple (bool): Whether device is in multi-device configuration. + in_service (bool): Whether the install ran as NSSU/ISSU. + nssu (bool): True for NSSU, False for ISSU; only used for log labels. + verification_required (bool): True when the install only proceeded on the + "A reboot is required" heuristic (PyEZ reported failure); post-reboot + version verification is then the only proof the install worked, so an + unverifiable image name raises instead of warning. + + Raises: + OSInstallError: When ``verification_required`` is True and no version can + be extracted from ``image_name`` to verify against. + """ match = _JUNOS_VERSION_RE.search(image_name) target_version = match.group(1) if match else None if target_version is None: + if verification_required: + log.error( + "Host %s: Install of %s reported failure and its result cannot be verified " + "(no parseable version in the image name); treating as failed.", + self.host, + image_name, + ) + raise OSInstallError(hostname=self.hostname, desired_boot=image_name) log.warning( "Host %s: Could not extract version from image name %s; skipping post-install version checks", self.host, @@ -1031,9 +1072,6 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", if target_version and not verified_by_completion_wait: self._verify_install_version(target_version, is_multiple) - log.info("Host %s: OS image %s installed successfully.", self.host, image_name) - return True - def open(self): """Open connection to device.""" if not self.connected: @@ -1241,7 +1279,10 @@ def _remote_file_copy_shell_fetch(self, src, source_url, dest): with StartShell(self.native) as shell: exit_ok, output = shell.run(fetch_cmd, timeout=src.timeout) if not exit_ok: - if "not found" in output.lower(): + # csh reports "fetch: Command not found."; sh reports "fetch: not + # found". A bare "not found" check would also match fetch's own + # HTTP 404 error text and misroute a real transfer failure. + if "command not found" in output.lower() or "fetch: not found" in output.lower(): log.warning( "Host %s: no fetch binary on this platform; falling back to the file-copy RPC.", self.host, diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 75d6c301..6566ad33 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -672,8 +672,12 @@ def test_wait_for_system_snapshot(self, mock_sleep): with self.subTest("snapshot times out"): mock_sleep.reset_mock() self.device.native.cli.return_value = "No snapshots found" - with self.assertRaises(TimeoutError) as ctx: - self.device._wait_for_system_snapshot(timeout=10) + # time.sleep is mocked, so a real clock would busy-spin for the full + # timeout; a fake monotonic clock keeps the test fast and deterministic. + fake_clock = itertools.count(start=0, step=3) + with mock.patch("pyntc.devices.jnpr_device.time.time", side_effect=lambda: next(fake_clock)): + with self.assertRaises(TimeoutError) as ctx: + self.device._wait_for_system_snapshot(timeout=10) self.assertIn("did not complete", str(ctx.exception)) def test_get_all_members_version(self): @@ -892,6 +896,21 @@ def test_remote_file_copy(self, mock_sleep, mock_start_shell): ) self.assertIsNone(result) + with self.subTest("HTTP 404 is a transfer failure, not a missing fetch binary"): + mock_shell.run.reset_mock() + self.device.native.rpc.file_copy.reset_mock() + mock_verify_file.side_effect = None + mock_verify_file.return_value = False + mock_shell.run.side_effect = [ + (False, "fetch: ftp://example.com/file.bin: Not Found"), + (True, ""), # rm -f cleanup of the partial file + ] + with self.assertRaises(FileTransferError) as ctx: + result = self.device.remote_file_copy(src_file, dest=dest_file) + self.assertIn("Not Found", str(ctx.exception)) + # A 404 must not be misrouted to the doomed RPC fallback. + self.device.native.rpc.file_copy.assert_not_called() + with self.subTest("non-fetch scheme uses the file-copy RPC"): mock_start_shell.reset_mock() mock_shell.run.reset_mock() @@ -977,6 +996,23 @@ def test_install_os_version_extraction(self): extracted = match.group(1) if match else None self.assertEqual(extracted, expected_version, f"Failed to extract version from {image_name}") + def test_install_os_ambiguous_install_without_version_raises(self): + """An install that only proceeded on the reboot-required heuristic must not report success unverified.""" + with ( + mock.patch.object(self.device, "_validate_multiple_device", return_value=False), + mock.patch.object(self.device, "_wait_for_device_reboot"), + mock.patch.object(self.device, "request_system_snapshot"), + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + self.device.sw.install.return_value = (False, "WARNING: A reboot is required to install the software") + with self.assertRaises(OSInstallError): + self.device.install_os( + image_name="/var/tmp/jinstall-noversion.tgz", # no parseable version + checksum="c0ffee", + reboot=True, + ) + def test_install_os_uptime_none_raises_error(self): """Test that install_os raises error when uptime cannot be determined.""" with mock.patch.object(self.device, "_validate_multiple_device", return_value=False): From f50b8a9d3be427647d150a1da190d5906c96a477 Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 9 Jul 2026 15:17:25 -0500 Subject: [PATCH 13/14] bump default snapshot timeout to 3600 and extract install_os reboot phase --- pyntc/devices/jnpr_device.py | 58 +++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index ae22cbc3..3e978bd0 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -573,7 +573,7 @@ def _validate_multiple_device(self): log.warning("Host %s: Could not validate multiple devices: %s", self.host, exc) return False - def _wait_for_system_snapshot(self, timeout=1800, interval=30): + def _wait_for_system_snapshot(self, timeout=3600, interval=30): """Poll device to verify system snapshot completion. Periodically checks ``show system snapshot media internal`` to verify the snapshot @@ -581,7 +581,9 @@ def _wait_for_system_snapshot(self, timeout=1800, interval=30): Args: timeout (int, optional): Max seconds to poll for snapshot verification, - counted after the initial warm-up delay. Defaults to 1800 (30 minutes). + counted after the initial warm-up delay. Defaults to 3600 (60 minutes); + field-observed all-members alternate-slice snapshots on a 2-member + EX3300 VC have taken 25-35+ minutes. interval (int, optional): Seconds between verification polls. Defaults to 30 seconds. Raises: @@ -982,6 +984,26 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", log.info("Host %s: OS image %s boot options set. Reboot the device to apply", self.host, image_name) return True + self._reboot_to_apply(image_name, is_multiple, in_service, nssu) + + self._post_install_checks(image_name, is_multiple, in_service, nssu, verification_required=not install_ok) + + log.info("Host %s: OS image %s installed successfully.", self.host, image_name) + return True + + def _reboot_to_apply(self, image_name, is_multiple, in_service, nssu): + """Reboot to apply the installed image, unless the in-service upgrade already did. + + Args: + image_name (str): Name of the installed image; used for log context only. + is_multiple (bool): Whether device is in multi-device configuration. + in_service (bool): Whether the install ran as NSSU/ISSU. + nssu (bool): True for NSSU, False for ISSU; only used for log labels. + + Raises: + CommandError: When the pre-reboot uptime cannot be determined on the + multi-device path (the reboot is refused rather than issued blind). + """ if in_service: # The in-service upgrade already rolled through the members and rebooted # each one inside ``sw.install()`` — the old master is typically still @@ -992,27 +1014,23 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", self.host, "NSSU" if nssu else "ISSU", ) - else: - log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) - if is_multiple: - self._uptime = None - original_uptime = self.uptime - - if original_uptime is None: - raise CommandError( - command="install_os", - message="Could not determine pre-reboot uptime; refusing to reboot.", - ) + return - self._request_system_reboot_all_members() - self._wait_for_device_reboot(original_uptime, is_multiple=True) - else: - self.reboot(wait_for_reload=True) + log.info("Host %s: Rebooting device to apply OS image %s", self.host, image_name) + if is_multiple: + self._uptime = None + original_uptime = self.uptime - self._post_install_checks(image_name, is_multiple, in_service, nssu, verification_required=not install_ok) + if original_uptime is None: + raise CommandError( + command="install_os", + message="Could not determine pre-reboot uptime; refusing to reboot.", + ) - log.info("Host %s: OS image %s installed successfully.", self.host, image_name) - return True + self._request_system_reboot_all_members() + self._wait_for_device_reboot(original_uptime, is_multiple=True) + else: + self.reboot(wait_for_reload=True) def _post_install_checks(self, image_name, is_multiple, in_service, nssu, verification_required=False): # pylint: disable=too-many-positional-arguments """Wait for in-service completion, snapshot, and verify the running version. From 39e65596a36385d1393f7ff5770c405e3fd8c2ce Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 9 Jul 2026 18:22:04 -0500 Subject: [PATCH 14/14] make taking a snapshot optional --- changes/400.added | 1 + pyntc/devices/jnpr_device.py | 40 +++++++++++++++------ tests/unit/test_devices/test_jnpr_device.py | 39 ++++++++++++++++++++ 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/changes/400.added b/changes/400.added index 0a7c013f..6f9ec78a 100644 --- a/changes/400.added +++ b/changes/400.added @@ -1 +1,2 @@ Added support for ISSU and NSSU non-disruptive OS upgrades on Juniper devices. +Added a `snapshot` option to `JunosDevice.install_os` that takes a post-upgrade `request system snapshot slice alternate` and waits for completion; disabled by default because Junos does not require a snapshot to complete an upgrade. diff --git a/pyntc/devices/jnpr_device.py b/pyntc/devices/jnpr_device.py index 3e978bd0..b4c347d9 100644 --- a/pyntc/devices/jnpr_device.py +++ b/pyntc/devices/jnpr_device.py @@ -899,14 +899,16 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs): return True return False - def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", nssu=False, issu=False): # pylint: disable=too-many-positional-arguments + def install_os( + self, image_name, checksum, reboot=True, hashing_algorithm="md5", nssu=False, issu=False, snapshot=False + ): # pylint: disable=too-many-positional-arguments """Install OS on device and reboot. For multi-device setups (virtual-chassis, chassis-cluster), supports NSSU/ISSU - upgrades and system snapshots after reboot. NSSU/ISSU performs its own rolling - reboot member-by-member during the install, so no separate reboot is issued on - that path; completion is verified by polling until every member reports the - target version. + upgrades and optional system snapshots after reboot. NSSU/ISSU performs its own + rolling reboot member-by-member during the install, so no separate reboot is + issued on that path; completion is verified by polling until every member + reports the target version. Args: image_name (str): Name of image. @@ -915,6 +917,12 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", hashing_algorithm (str): The hashing algorithm to use. Valid values are 'md5', 'sha1', and 'sha256'. Defaults to 'md5'. nssu (bool): Enable Nonstop Software Upgrade. Defaults to False. issu (bool): Enable In-Service Software Upgrade. Defaults to False. + snapshot (bool): Take a post-upgrade ``request system snapshot slice alternate`` + to sync the alternate root with the new version. Junos does not require a + snapshot to complete an upgrade, but on dual-root platforms an unsynced + alternate slice boots the OLD version if the device ever falls back to it. + Snapshots can take 25+ minutes per member on small-flash platforms. + Defaults to False. Raises: ValueError: When both nssu and issu are True (mutually exclusive), or when @@ -986,7 +994,14 @@ def install_os(self, image_name, checksum, reboot=True, hashing_algorithm="md5", self._reboot_to_apply(image_name, is_multiple, in_service, nssu) - self._post_install_checks(image_name, is_multiple, in_service, nssu, verification_required=not install_ok) + self._post_install_checks( + image_name, + is_multiple, + in_service, + nssu, + verification_required=not install_ok, + snapshot=snapshot, + ) log.info("Host %s: OS image %s installed successfully.", self.host, image_name) return True @@ -1032,8 +1047,10 @@ def _reboot_to_apply(self, image_name, is_multiple, in_service, nssu): else: self.reboot(wait_for_reload=True) - def _post_install_checks(self, image_name, is_multiple, in_service, nssu, verification_required=False): # pylint: disable=too-many-positional-arguments - """Wait for in-service completion, snapshot, and verify the running version. + def _post_install_checks( + self, image_name, is_multiple, in_service, nssu, verification_required=False, snapshot=False + ): # pylint: disable=too-many-positional-arguments + """Wait for in-service completion, optionally snapshot, and verify the running version. Args: image_name (str): Name of the installed image; the target version is @@ -1046,6 +1063,8 @@ def _post_install_checks(self, image_name, is_multiple, in_service, nssu, verifi "A reboot is required" heuristic (PyEZ reported failure); post-reboot version verification is then the only proof the install worked, so an unverifiable image name raises instead of warning. + snapshot (bool): Take a post-upgrade system snapshot and wait for it to + complete. Defaults to False. Raises: OSInstallError: When ``verification_required`` is True and no version can @@ -1083,8 +1102,9 @@ def _post_install_checks(self, image_name, is_multiple, in_service, nssu, verifi # ``show version all-members`` while it reboots must not count as done. self._wait_for_nssu_completion(target_version, expected_members=self._vc_member_count() or None) - # Perform system snapshot after reboot/upgrade - self.request_system_snapshot(parameters="slice alternate all-members" if is_multiple else "slice alternate") + # Optionally sync the alternate root with the new version after reboot/upgrade + if snapshot: + self.request_system_snapshot(parameters="slice alternate all-members" if is_multiple else "slice alternate") # Verify the install was successful by checking the running version if target_version and not verified_by_completion_wait: diff --git a/tests/unit/test_devices/test_jnpr_device.py b/tests/unit/test_devices/test_jnpr_device.py index 6566ad33..334c2e83 100644 --- a/tests/unit/test_devices/test_jnpr_device.py +++ b/tests/unit/test_devices/test_jnpr_device.py @@ -555,6 +555,45 @@ def test_install_os_with_nssu(self): # the separate post-snapshot verification must not repeat it. mock_verify.assert_not_called() + def test_install_os_snapshot_option(self): + with ( + mock.patch.object(self.device, "_validate_multiple_device") as mock_validate, + mock.patch.object(self.device, "_request_system_reboot_all_members"), + mock.patch.object(self.device, "_wait_for_device_reboot"), + mock.patch.object(self.device, "request_system_snapshot") as mock_snapshot, + mock.patch.object(self.device, "_verify_install_version"), + mock.patch.object(type(self.device), "uptime", new_callable=mock.PropertyMock) as mock_uptime, + ): + mock_uptime.return_value = 1000 + self.device.sw.install.return_value = True + + with self.subTest("snapshot is skipped by default"): + mock_validate.return_value = False + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + ) + mock_snapshot.assert_not_called() + + with self.subTest("snapshot=True on a single device"): + mock_validate.return_value = False + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + snapshot=True, + ) + mock_snapshot.assert_called_once_with(parameters="slice alternate") + + with self.subTest("snapshot=True on a virtual chassis"): + mock_snapshot.reset_mock() + mock_validate.return_value = True + self.device.install_os( + image_name="/var/tmp/jinstall-15.1R7-S2-signed.tgz", + checksum="c0ffee", + snapshot=True, + ) + mock_snapshot.assert_called_once_with(parameters="slice alternate all-members") + def test_install_os_nssu_with_reboot_false_raises_value_error(self): with mock.patch.object(self.device, "_validate_multiple_device", return_value=True): with self.assertRaises(ValueError):