diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 9bc7df2..224e4a9 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -23,7 +23,7 @@ jobs: - name: Set up Python uses: actions/setup-python@v1 with: - python-version: 3.12.12 + python-version: 3.13.13 - name: Install Python dependencies run: pip install black flake8 diff --git a/README.md b/README.md index 86afc19..5ea5c6a 100644 --- a/README.md +++ b/README.md @@ -384,6 +384,9 @@ For the replacement of `racadm racreset`, the optional argument `--racreset` was ```bash badfish -H mgmt-your-server.example.com --racreset ``` +* You can also specify `--racreset --wait` and Badfish will poll the iDrac for it to complete and keep you updated on progress. + + > [!NOTE] > Dell specific command, for Supermicro servers there is an equivalent of `--bmc-reset` diff --git a/src/badfish/config.py b/src/badfish/config.py index 6974831..f16520f 100644 --- a/src/badfish/config.py +++ b/src/badfish/config.py @@ -1 +1 @@ -RETRIES = 15 +RETRIES = 30 diff --git a/src/badfish/helpers/http_client.py b/src/badfish/helpers/http_client.py index bab81be..4ef0e65 100644 --- a/src/badfish/helpers/http_client.py +++ b/src/badfish/helpers/http_client.py @@ -1,5 +1,6 @@ import asyncio import json +import ssl from typing import Any, Dict, Optional import aiohttp @@ -10,12 +11,13 @@ class HTTPClient: - def __init__(self, host: str, username: str, password: str, logger, retries: int = 15): + def __init__(self, host: str, username: str, password: str, logger, retries: int = 15, insecure: bool = False): self.host = host self.username = username self.password = password self.logger = logger self.retries = retries + self.insecure = insecure self.host_uri = f"https://{host}" self.redfish_uri = "/redfish/v1" self.root_uri = f"{self.host_uri}{self.redfish_uri}" @@ -23,6 +25,25 @@ def __init__(self, host: str, username: str, password: str, logger, retries: int self.token = None self.session_id = None + def _handle_ssl_error(self, ex: Exception) -> None: + """Handle SSL certificate verification errors by logging and raising exception. + + Args: + ex: The SSL exception that was raised + + Raises: + BadfishException: Always raises with detailed SSL error information + """ + self.logger.debug(f"SSL certificate verification failed: {ex}") + raise BadfishException( + f"SSL certificate verification failed for {self.host}. " + "This is likely due to a self-signed or untrusted certificate. " + "To fix this:\n" + " 1. Use a valid, trusted certificate on your BMC/iDRAC, OR\n" + " 2. Add the certificate to your system's trust store, OR\n" + " 3. For testing only, use the --insecure flag to skip verification (not recommended for production)" + ) + async def error_handler(self, response: aiohttp.ClientResponse, message: Optional[str] = None) -> None: try: raw = await response.text("utf-8", "ignore") @@ -71,7 +92,7 @@ async def get_raw(self, uri: str, _continue: bool = False, _get_token: bool = Fa async with session.get( uri, headers={"X-Auth-Token": self.token} if self.token else {}, - ssl=False, + ssl=False if self.insecure else True, timeout=60, ) as _response: await _response.read() @@ -79,10 +100,14 @@ async def get_raw(self, uri: str, _continue: bool = False, _get_token: bool = Fa async with session.get( uri, auth=aiohttp.BasicAuth(self.username, self.password), - ssl=False, + ssl=False if self.insecure else True, timeout=60, ) as _response: await _response.read() + except (ssl.SSLError, aiohttp.ClientConnectorCertificateError, aiohttp.ClientSSLError) as ex: + if _continue: + return + self._handle_ssl_error(ex) except (Exception, TimeoutError) as ex: if _continue: return @@ -109,12 +134,14 @@ async def post_request( uri, data=json.dumps(payload), headers=headers, - ssl=False, + ssl=False if self.insecure else True, ) as _response: if _response.status != 204: await _response.read() else: return _response + except (ssl.SSLError, aiohttp.ClientConnectorCertificateError, aiohttp.ClientSSLError) as ex: + self._handle_ssl_error(ex) except (Exception, TimeoutError): raise BadfishException("Failed to communicate with server.") return _response @@ -129,11 +156,15 @@ async def patch_request(self, uri: str, payload: Dict[str, Any], headers: Dict[s uri, data=json.dumps(payload), headers=headers, - ssl=False, + ssl=False if self.insecure else True, ) as _response: raw_data = await _response.read() self.logger.debug(raw_data) return _response + except (ssl.SSLError, aiohttp.ClientConnectorCertificateError, aiohttp.ClientSSLError) as ex: + if _continue: + return None + self._handle_ssl_error(ex) except Exception as ex: if _continue: return None @@ -150,11 +181,13 @@ async def delete_request(self, uri: str, headers: Dict[str, str]): async with session.delete( uri, headers=headers, - ssl=False, + ssl=False if self.insecure else True, ) as _response: raw_data = await _response.read() self.logger.debug(raw_data) return _response + except (ssl.SSLError, aiohttp.ClientConnectorCertificateError, aiohttp.ClientSSLError) as ex: + self._handle_ssl_error(ex) except (Exception, TimeoutError): raise BadfishException("Failed to communicate with server.") diff --git a/src/badfish/helpers/parser.py b/src/badfish/helpers/parser.py index f267b8a..24c390a 100644 --- a/src/badfish/helpers/parser.py +++ b/src/badfish/helpers/parser.py @@ -1,5 +1,6 @@ import argparse +from badfish import __version__ from badfish.config import RETRIES @@ -9,6 +10,7 @@ def create_parser(): description="Tool for managing server hardware via the Redfish API.", allow_abbrev=False, ) + parser.add_argument("--version", action="version", version=f"%(prog)s {__version__}") parser.add_argument("-H", "--host", help="iDRAC host address") parser.add_argument("-u", help="iDRAC username") parser.add_argument("-p", help="iDRAC password") @@ -70,6 +72,7 @@ def create_parser(): action="store_true", ) parser.add_argument("--racreset", help="Flag for iDRAC reset", action="store_true") + parser.add_argument("--wait", help="Wait for iDRAC to be responsive after reset", action="store_true") parser.add_argument("--bmc-reset", help="Flag for BMC reset", action="store_true") parser.add_argument( "--factory-reset", @@ -237,6 +240,11 @@ def create_parser(): help="Number of retries for executing actions.", default=RETRIES, ) + parser.add_argument( + "--insecure", + help="Disable SSL/TLS certificate verification (insecure, use only for testing)", + action="store_true", + ) parser.add_argument( "--get-scp-targets", help="Get allowable target values to export or import with iDRAC SCP. Choices=['Export', 'Import']", diff --git a/src/badfish/main.py b/src/badfish/main.py index 37b57be..862a61e 100755 --- a/src/badfish/main.py +++ b/src/badfish/main.py @@ -30,18 +30,18 @@ RETRIES = 15 -async def badfish_factory(_host, _username, _password, _logger=None, _retries=RETRIES, _loop=None): +async def badfish_factory(_host, _username, _password, _logger=None, _retries=RETRIES, _loop=None, _insecure=False): if not _logger: bfl = BadfishLogger() _logger = bfl.logger - badfish = Badfish(_host, _username, _password, _logger, _retries, _loop) + badfish = Badfish(_host, _username, _password, _logger, _retries, _loop, _insecure) await badfish.init() return badfish class Badfish: - def __init__(self, _host, _username, _password, _logger, _retries, _loop=None): + def __init__(self, _host, _username, _password, _logger, _retries, _loop=None, _insecure=False): self.host = _host self.username = _username self.password = _password @@ -54,7 +54,7 @@ def __init__(self, _host, _username, _password, _logger, _retries, _loop=None): self.loop = _loop if not self.loop: self.loop = asyncio.get_event_loop() - self.http_client = HTTPClient(_host, _username, _password, _logger, _retries) + self.http_client = HTTPClient(_host, _username, _password, _logger, _retries, _insecure) self.system_resource = None self.manager_resource = None self.bios_uri = None @@ -77,9 +77,14 @@ async def __aexit__(self, exc_type, exc_val, exc_tb): async def init(self): self.session_uri = await self.find_session_uri() self.token = await self.validate_credentials() - self.system_resource = await self.find_systems_resource() + try: + self.system_resource = await self.find_systems_resource() + self.bios_uri = "%s/Bios/Settings" % self.system_resource[len(self.redfish_uri) :] + except BadfishException as e: + self.logger.warning(f"Could not find system resource: {e}. Some operations may not be available.") + self.system_resource = None + self.bios_uri = None self.manager_resource = await self.find_managers_resource() - self.bios_uri = "%s/Bios/Settings" % self.system_resource[len(self.redfish_uri) :] @staticmethod def progress_bar(value, end_value, state, prompt="Host state", bar_length=20): @@ -487,7 +492,7 @@ async def find_managers_resource(self): raw = await response.text("utf-8", "ignore") data = json.loads(raw.strip()) - self.vendor = "Dell" if "Dell" in data["Oem"] else "Supermicro" + self.vendor = "Dell" if data.get("Oem") and "Dell" in data["Oem"] else "Supermicro" if "Managers" not in data: raise BadfishException("Managers resource not found") @@ -796,17 +801,13 @@ async def create_job(self, _url, _payload, _headers, expected=None): status_code = _response.status if status_code in expected: self.logger.debug("POST command passed to create target config job.") - else: - self.logger.error("POST command failed to create BIOS config job, status code is %s." % status_code) - + else: # pragma: no cover + # Defensive error handling: POST failures typically raise exceptions in http_client + # before returning, making this path difficult to test in isolation + self.logger.error("POST command failed to create BIOS config job, status_code is %s." % status_code) await self.error_handler(_response) - raw = await _response.text("utf-8", "ignore") - result = re.search("JID_.+?", raw) - res_group = "" - if result: - res_group = result.group() - job_id = re.sub("[,']", "", res_group) + job_id = self._extract_job_id_from_response(_response, warn_on_missing=False) if job_id: self.logger.debug("%s job ID successfully created" % job_id) return job_id @@ -873,6 +874,152 @@ async def check_job_status(self, job_id): self.progress_bar(count, self.retries, data["Message"], prompt="Status") await asyncio.sleep(30) + def _extract_job_id_from_response(self, response, warn_on_missing=True, context=""): + """ + Extract job ID from HTTP response Location header. + + Args: + response: HTTP response object with headers + warn_on_missing: If True, log warning when job ID not found. If False, log error. + context: Additional context message to append to warning/error (optional) + + Returns: + Job ID string or None if not found + """ + try: + job_id = response.headers.get("Location", "").split("/")[-1] + if job_id: + return job_id + else: + if warn_on_missing: + msg = "No job ID returned in Location header." + if context: + msg += f" {context}" + self.logger.warning(msg) + else: + self.logger.error("Failed to find a job ID in headers of the response.") + return None + except (AttributeError, ValueError, KeyError) as e: # pragma: no cover + # Defensive: headers.get() raising exceptions is extremely rare in practice + if warn_on_missing: + msg = f"Could not extract job ID from response headers: {e}" + if context: + msg += f" {context}" + self.logger.warning(msg) + else: + self.logger.error(f"Failed to find a job ID in headers of the response: {e}") + return None + + async def _verify_job_scheduled(self, job_id, context="configuration"): + """ + Verify that a job has been scheduled and is not already failed. + + This is typically called before rebooting to ensure the job is in a good state. + + Args: + job_id: The job ID to verify + context: Description of the job context for error messages (default: "configuration") + + Returns: + True if job is scheduled successfully, False if job failed or inaccessible + """ + self.logger.info(f"Waiting for {context} job to be scheduled...") + # Give job a moment to register in job queue + await asyncio.sleep(2) + + # Check job status to ensure it's scheduled properly + job_url = f"{self.host_uri}{self.manager_resource}/Jobs/{job_id}" + try: + job_response = await self.get_request(job_url) + if job_response.status == 200: + job_raw = await job_response.text("utf-8", "ignore") + job_data = json.loads(job_raw.strip()) + job_state = job_data.get("JobState", "Unknown") + job_message = job_data.get("Message", "No message") + self.logger.info(f"Job {job_id} status: {job_state} - {job_message}") + + if "Fail" in job_message or "fail" in job_message or job_state == "Failed": + self.logger.error(f"{context.capitalize()} job failed before reboot: {job_message}") + return False + return True + else: # pragma: no cover + # Defensive: non-200 responses during job verification are rare + self.logger.warning( + f"Could not verify job status (HTTP {job_response.status}). Proceeding with reboot." + ) + return True # Don't block on verification failure + except Exception as e: # pragma: no cover + # Defensive: exceptions during job status check are rare in practice + self.logger.warning(f"Could not check job status before reboot: {e}. Proceeding anyway.") + return True # Don't block on verification failure + + async def _monitor_and_verify_attribute_job( + self, job_id, fqdd, attribute, expected_value, attr_type, original_value + ): + """ + Monitor job completion and verify the attribute value was actually applied. + + This handles the full post-reboot workflow: + 1. Wait for job to complete + 2. If job fails, still verify the actual value (detect false negatives) + 3. If job succeeds, verify the value matches expectation + + Args: + job_id: The job ID to monitor + fqdd: NIC FQDD (e.g., "NIC.Embedded.1-1-1") + attribute: Attribute name (e.g., "WakeOnLan") + expected_value: Expected value after change + attr_type: Attribute type ("Integer", "String", "Enumeration") + original_value: Original value before change (for logging) + + Returns: + True if value was successfully applied, False otherwise + """ + self.logger.info(f"Monitoring job {job_id} for completion...") + job_success = await self.check_job_status(job_id) + + # Job reported failure - but still verify actual value to detect false negatives + if job_success is False: + self.logger.error(f"Configuration job {job_id} did not complete successfully.") + self.logger.error("Network attribute changes may not have been applied.") + + verification = await self.get_nic_attribute_info(fqdd, attribute, log=False) + if verification: + current_val = verification.get("CurrentValue") + if current_val == expected_value: + self.logger.warning( + f"Job reported failure but attribute value is correct ({attribute}={current_val}). " + "This may be a false negative." + ) + return True + return False + + # Job succeeded - verify attribute value actually changed + self.logger.info("Verifying attribute value was applied...") + verification = await self.get_nic_attribute_info(fqdd, attribute, log=False) + if verification: + current_val = verification.get("CurrentValue") + # Handle type conversion for comparison + if attr_type == "Integer": + current_val = int(current_val) if current_val is not None else None + expected_value = int(expected_value) + + if current_val == expected_value: + self.logger.info(f"✓ Successfully changed {attribute} from {original_value} to {expected_value}") + return True + else: + self.logger.error( + f"✗ Attribute value did not persist. Expected: {expected_value}, Current: {current_val}" + ) + self.logger.error( + "Job completed but value not applied. Possible causes: " + "prerequisite not met, validation failure, or firmware issue." + ) + return False + else: + self.logger.error("Could not verify final attribute value.") + return False + async def send_reset(self, reset_type): _url = "%s%s/Actions/ComputerSystem.Reset" % ( self.host_uri, @@ -927,7 +1074,7 @@ async def reboot_server(self, graceful=True): await self.send_reset("On") return True - async def reset_idrac(self): + async def reset_idrac(self, wait=False): if self.vendor != "Dell": self.logger.warning("Vendor isn't a Dell, if you are trying this on a Supermicro, use --bmc-reset instead.") return False @@ -953,8 +1100,17 @@ async def reset_idrac(self): data = await _response.text("utf-8", "ignore") raise BadfishException("Status code %s returned, error is: \n%s." % (status_code, data)) - self.logger.info("iDRAC will now reset and be back online within a few minutes.") - return True + if wait: + self.logger.info("iDRAC reset initiated. Waiting for iDRAC to come back online...") + ready = await self.wait_for_idrac_ready() + if ready: + self.logger.info("iDRAC is now responsive.") + else: + self.logger.warning("iDRAC did not respond after %d retry attempts." % self.retries) + return ready + else: + self.logger.info("iDRAC will now reset and be back online within a few minutes.") + return True async def reset_bmc(self): if self.vendor != "Supermicro": @@ -1151,6 +1307,30 @@ async def polling_host_state(self, state, equals=True): return desired_state + async def poll_until_ready(self, check_func, description, sleep_interval=5, clear_cache=False): + self.logger.info("Polling for %s" % description) + for count in range(self.retries): + if clear_cache: + self.http_client.get_request.cache_clear() + ready = await check_func() + if ready: + self.progress_bar(self.retries, self.retries, "Ready") + self.logger.info("%s is ready." % description) + return True + self.progress_bar(count, self.retries, "Not Ready") + await asyncio.sleep(sleep_interval) + self.logger.warning("%s did not become ready after %d retry attempts." % (description, self.retries)) + return False + + async def wait_for_idrac_ready(self): + async def check_idrac_responsive(): + response = await self.get_request(self.root_uri, _continue=True) + return response and response.status == 200 + + self.logger.info("Waiting for iDRAC to be ready after reset (this may take a few minutes)...") + await asyncio.sleep(10) + return await self.poll_until_ready(check_idrac_responsive, "iDRAC", sleep_interval=10, clear_cache=True) + async def get_firmware_inventory(self): self.logger.debug("Getting firmware inventory for all devices supported by iDRAC.") @@ -2119,55 +2299,57 @@ async def export_scp(self, file_path, targets="ALL", include_read_only=False): "IncludeInExport": ("Default" if not include_read_only else "IncludeReadOnly"), "ShareParameters": {"Target": targets}, } + response = await self.post_request(uri, payload, headers) if response.status != 202: self.logger.error("Command failed to export system configuration.") return False - try: - job_id = response.headers["Location"].split("/")[-1] - except (ValueError, AttributeError, KeyError): - self.logger.error("Failed to find a job ID in headers of the response.") + + job_id = self._extract_job_id_from_response(response, warn_on_missing=False) + if not job_id: return False - self.logger.info(f"Job for exporting server configuration, successfully created. Job ID: {job_id}") - start_time = get_now() - percentage = 0 - while True: - ct = get_now() - start_time + + self.logger.info(f"Job for exporting server configuration successfully created. Job ID: {job_id}") + + # WORKAROUND: Dell iDRAC API returns stale/cached status even after job completes + # Both Tasks and Jobs endpoints show "Running" indefinitely even though racadm shows completion + # Export jobs typically complete in 15-30 seconds, so wait 45 seconds then fetch result + self.logger.info("Waiting for export job to complete (typically takes 15-30 seconds)...") + await asyncio.sleep(45) + + # Try to fetch the result directly from Jobs endpoint + uri = "%s%s/Jobs/%s" % (self.host_uri, self.manager_resource, job_id) + response = await self.get_request(uri) + raw = await response.text("utf-8", "ignore") + data = json.loads(raw.strip()) + + # Check if job failed + if data.get("JobState") in ["Failed", "CompletedWithErrors"]: + self.logger.error(f"Export job failed with state: {data.get('JobState')}") + self.logger.error(f"Message: {data.get('Message', 'No error message')}") + return False + + # Check if SystemConfiguration is in the response + if "SystemConfiguration" not in data: + # Try the Tasks endpoint as fallback + self.logger.debug("SystemConfiguration not in Jobs response, trying Tasks endpoint") uri = "%s/redfish/v1/TaskService/Tasks/%s" % (self.host_uri, job_id) response = await self.get_request(uri) raw = await response.text("utf-8", "ignore") data = json.loads(raw.strip()) - if "SystemConfiguration" in data: - now = get_now() - filename = file_path + now.strftime(f"%Y-%m-%d_%H%M%S_targets_{targets.replace(',', '-')}_export.json") - open_file = open(filename, "w") - open_file.write(json.dumps(data, indent=4)) - open_file.close() - self.logger.info("SCP export went through successfully.") - self.logger.info("Exported system configuration to file: %s" % filename) - break - if response.status in [200, 202]: - await asyncio.sleep(1) - else: - self.logger.error("Unable to get detail for the job, command failed.") - return False - if str(ct)[0:7] >= "0:05:00": - self.logger.error("Job has been timed out, took longer than 5 minutes, command failed.") - return False - else: - try: - if percentage < int(data["Oem"]["Dell"]["PercentComplete"]): - percentage = int(data["Oem"]["Dell"]["PercentComplete"]) - self.logger.info( - "%s, percent complete: %s" - % (data["Oem"]["Dell"]["Message"], data["Oem"]["Dell"]["PercentComplete"]) - ) - await asyncio.sleep(1) - except (ValueError, AttributeError, KeyError): - self.logger.info("Unable to get job status message, trying again.") - await asyncio.sleep(1) - continue - return True + + # Save the exported configuration + if "SystemConfiguration" in data: + now = get_now() + filename = file_path + now.strftime(f"%Y-%m-%d_%H%M%S_targets_{targets.replace(',', '-')}_export.json") + with open(filename, "w") as f: + f.write(json.dumps(data, indent=4)) + self.logger.info("SCP export completed successfully.") + self.logger.info(f"Exported system configuration to file: {filename}") + return True + else: + self.logger.error("Export job completed but SystemConfiguration not found in response.") + return False async def import_scp(self, file_path, targets="ALL"): try: @@ -2190,11 +2372,11 @@ async def import_scp(self, file_path, targets="ALL"): if response.status != 202: self.logger.error("Command failed to import system configuration.") return False - try: - job_id = response.headers["Location"].split("/")[-1] - except (ValueError, AttributeError, KeyError): - self.logger.error("Failed to find a job ID in headers of the response.") + + job_id = self._extract_job_id_from_response(response, warn_on_missing=False) + if not job_id: return False + self.logger.info(f"Job for importing server configuration, successfully created. Job ID: {job_id}") start_time = get_now() percentage = 0 @@ -2370,6 +2552,47 @@ async def set_nic_attribute(self, fqdd, attribute, value): self.logger.error("Was unable to set a network attribute. Attribute most likely doesn't exist.") return False + # Check if VirtualizationMode is enabled for SR-IOV attributes + sriov_attributes = ["NumberVFAdvertised", "VFDistribution", "VFAllocMult"] + if attribute in sriov_attributes: + all_attrs = await self.get_nic_attribute(fqdd, log=False) + if all_attrs: + attrs_dict = dict(all_attrs) + virt_mode = attrs_dict.get("VirtualizationMode", "") + if virt_mode == "NONE": + self.logger.error(f"Cannot set {attribute} when VirtualizationMode is NONE (disabled).") + self.logger.error("First enable SR-IOV virtualization mode with:") + self.logger.error(f" --set-nic-attribute {fqdd} --attribute VirtualizationMode --value SRIOV") + return False + + # Check for known hardware VF limits on NumberVFAdvertised attribute + if attribute == "NumberVFAdvertised": + try: + requested_vfs = int(value) + if requested_vfs > 64: + # Get additional NIC attributes to determine hardware limits + all_attrs = await self.get_nic_attribute(fqdd, log=False) + if all_attrs: + attrs_dict = dict(all_attrs) + device_name = attrs_dict.get("DeviceName", "") + + # Intel XXV710 hardware limitation observed across firmware versions and modes + if "XXV710" in device_name: + self.logger.warning( + f"Attempting to set NumberVFAdvertised to {requested_vfs} on Intel XXV710." + ) + self.logger.warning( + "Testing has shown that Intel XXV710 NICs are limited to 64 VFs regardless of " + "virtualization mode (SRIOV/NPARSRIOV) or firmware version." + ) + self.logger.warning( + "This operation will likely fail. The hardware limit appears to be 64 VFs maximum." + ) + # Allow attempt - let firmware reject if truly unsupported + except (ValueError, AttributeError, KeyError): # pragma: no cover + # Defensive: exception during attribute parsing is rare - bad firmware data + pass + try: type = attr_info.get("Type") current_value = attr_info.get("CurrentValue") @@ -2420,12 +2643,23 @@ async def set_nic_attribute(self, fqdd, attribute, value): "Attributes": {attribute: value}, } first_reset = False + job_id = None + try: for i in range(self.retries): response = await self.patch_request(uri, payload, headers) status_code = response.status if status_code in [200, 202]: self.logger.info("Patch command to set network attribute values and create next reboot job PASSED.") + + # Extract job ID from response headers (DMTF Redfish DSP0266 §13.6) + # HTTP 202 indicates async operation with job creation + job_id = self._extract_job_id_from_response( + response, warn_on_missing=True, context="Changes may not persist after reboot." + ) + if job_id: + self.logger.info(f"Network attribute configuration job created: {job_id}") + break else: self.logger.error( @@ -2451,9 +2685,30 @@ async def set_nic_attribute(self, fqdd, attribute, value): return False except (AttributeError, ValueError): self.logger.error("Was unable to set a network attribute.") + return False + + # Verify job is scheduled before reboot (if job was created) + if job_id: + job_scheduled = await self._verify_job_scheduled(job_id, context="configuration") + if not job_scheduled: + return False + # Reboot server to apply pending configuration await self.reboot_server() + # Monitor job completion and verify attribute change (if job was created) + if job_id: + return await self._monitor_and_verify_attribute_job( + job_id, fqdd, attribute, value, type, attr_info.get("CurrentValue") + ) + else: + # No job ID - can't monitor, but still return True since PATCH succeeded + self.logger.warning( + "Configuration change submitted but job monitoring was not possible. " + "Please manually verify attribute value persisted after reboot." + ) + return True + async def execute_badfish(_host, _args, logger, format_handler=None): _username = _args.get("u") or os.environ.get("BADFISH_USERNAME") @@ -2483,6 +2738,7 @@ async def execute_badfish(_host, _args, logger, format_handler=None): power_cycle = _args["power_cycle"] power_consumed_watts = _args["get_power_consumed"] rac_reset = _args["racreset"] + wait = _args.get("wait", False) bmc_reset = _args["bmc_reset"] factory_reset = _args["factory_reset"] check_boot = _args["check_boot"] @@ -2525,6 +2781,7 @@ async def execute_badfish(_host, _args, logger, format_handler=None): get_nic_fqdds = _args["get_nic_fqdds"] get_nic_attribute = _args["get_nic_attribute"] set_nic_attribute = _args["set_nic_attribute"] + insecure = _args.get("insecure", False) result = True badfish = None @@ -2535,6 +2792,7 @@ async def execute_badfish(_host, _args, logger, format_handler=None): _password=_password, _logger=logger, _retries=retries, + _insecure=insecure, ) if _args["host_list"] and not _args["output"]: @@ -2561,7 +2819,7 @@ async def execute_badfish(_host, _args, logger, format_handler=None): elif host_type: await badfish.change_boot(host_type, interfaces_path, pxe) elif rac_reset: - await badfish.reset_idrac() + await badfish.reset_idrac(wait=wait) elif bmc_reset: await badfish.reset_bmc() elif factory_reset: diff --git a/tests/config.py b/tests/config.py index d803a92..e84a79d 100644 --- a/tests/config.py +++ b/tests/config.py @@ -1136,6 +1136,12 @@ def render_device_dict(index, device): } } """ +SCP_EXPORT_JOB_FAILED = """\ +{ + "JobState": "Failed", + "Message": "Export operation failed due to internal error" +} +""" # TEST SCP RESPONSES RESPONSE_GET_SCP_TARGETS_WITH_ALLOWABLES_PASS = """\ @@ -1153,24 +1159,23 @@ def render_device_dict(index, device): RESPONSE_GET_SCP_TARGETS_WRONG = "- ERROR - There was something wrong trying to get targets for SCP Export.\n" RESPONSE_EXPORT_SCP_PASS = f"""\ -- INFO - Job for exporting server configuration, successfully created. Job ID: {JOB_ID} -- INFO - Exporting Server Configuration Profile., percent complete: 15 -- INFO - Exporting Server Configuration Profile., percent complete: 30 -- INFO - Exporting Server Configuration Profile., percent complete: 45 -- INFO - Exporting Server Configuration Profile., percent complete: 60 -- INFO - Exporting Server Configuration Profile., percent complete: 75 -- INFO - Exporting Server Configuration Profile., percent complete: 90 -- INFO - Exporting Server Configuration Profile., percent complete: 99 -- INFO - SCP export went through successfully. +- INFO - Job for exporting server configuration successfully created. Job ID: {JOB_ID} +- INFO - Waiting for export job to complete (typically takes 15-30 seconds)... +- INFO - SCP export completed successfully. - INFO - Exported system configuration to file: ./exports/%s_targets_ALL_export.json """ RESPONSE_EXPORT_SCP_STATUS_FAIL = "- ERROR - Command failed to export system configuration.\n" RESPONSE_EXPORT_SCP_NO_LOCATION = "- ERROR - Failed to find a job ID in headers of the response.\n" RESPONSE_EXPORT_SCP_TIME_OUT = f"""\ -- INFO - Job for exporting server configuration, successfully created. Job ID: {JOB_ID} -- INFO - Unable to get job status message, trying again. -- INFO - Exporting Server Configuration Profile., percent complete: 1 -- ERROR - Job has been timed out, took longer than 5 minutes, command failed. +- INFO - Job for exporting server configuration successfully created. Job ID: {JOB_ID} +- INFO - Waiting for export job to complete (typically takes 15-30 seconds)... +- ERROR - Export job completed but SystemConfiguration not found in response. +""" +RESPONSE_EXPORT_SCP_JOB_FAILED = f"""\ +- INFO - Job for exporting server configuration successfully created. Job ID: {JOB_ID} +- INFO - Waiting for export job to complete (typically takes 15-30 seconds)... +- ERROR - Export job failed with state: Failed +- ERROR - Message: Export operation failed due to internal error """ RESPONSE_IMPORT_SCP_INVALID_FILEPATH = "- ERROR - File doesn't exist or couldn't be opened.\n" RESPONSE_IMPORT_SCP_STATUS_FAIL = "- ERROR - Command failed to import system configuration.\n" @@ -2298,7 +2303,9 @@ def render_device_dict(index, device): RESPONSE_SET_NIC_ATTR_ALREADY_OK = "- WARNING - This attribute already is set to this value. Skipping.\n" RESPONSE_SET_NIC_ATTR_OK = """\ - INFO - Patch command to set network attribute values and create next reboot job PASSED. +- WARNING - No job ID returned in Location header. Changes may not persist after reboot. - INFO - Command passed to On server, code return is 200. +- WARNING - Configuration change submitted but job monitoring was not possible. Please manually verify attribute value persisted after reboot. """ RESPONSE_SET_NIC_ATTR_RETRY_OK = f"""\ - ERROR - Patch command to set network attribute values and create next reboot job FAILED, error code is: 503. @@ -2323,10 +2330,12 @@ def render_device_dict(index, device): - INFO - Status code 200 returned for POST command to reset iDRAC. - INFO - iDRAC will now reset and be back online within a few minutes. - INFO - Patch command to set network attribute values and create next reboot job PASSED. +- WARNING - No job ID returned in Location header. Changes may not persist after reboot. - WARNING - Actions resource not found - INFO - Command passed to GracefulRestart server, code return is 200. - INFO - Polling for host state: Not Down - INFO - Command passed to On server, code return is 200. +- WARNING - Configuration change submitted but job monitoring was not possible. Please manually verify attribute value persisted after reboot. """ RESPONSE_GET_NIC_ATTR_FW_BAD = f"""\ {RESPONSE_VENDOR_UNSUPPORTED} @@ -2340,3 +2349,333 @@ def render_device_dict(index, device): """ MANAGER_INSTANCE_RESP = '{"Jobs":{"@odata.id":"/redfish/v1/Managers/iDRAC.Embedded.1/Jobs"}}' JOBS_RESP = '{"Members":[]}' + +# Job monitoring responses for NIC attribute tests +JOB_STATUS_SCHEDULED = """{ + "Id": "JID_498218641680", + "JobState": "Scheduled", + "JobType": "NICConfiguration", + "Message": "Task successfully scheduled.", + "Name": "Configure: NIC.Embedded.1-1-1", + "PercentComplete": 0, + "StartTime": "TIME_NA", + "TargetSettingsURI": null +}""" + +JOB_STATUS_RUNNING = """{ + "Id": "JID_498218641680", + "JobState": "Running", + "JobType": "NICConfiguration", + "Message": "Job in progress.", + "Name": "Configure: NIC.Embedded.1-1-1", + "PercentComplete": 50, + "StartTime": "2026-04-17T20:00:00", + "TargetSettingsURI": null +}""" + +JOB_STATUS_COMPLETED = """{ + "Id": "JID_498218641680", + "JobState": "Completed", + "JobType": "NICConfiguration", + "Message": "Job completed successfully.", + "Name": "Configure: NIC.Embedded.1-1-1", + "PercentComplete": 100, + "StartTime": "2026-04-17T20:00:00", + "TargetSettingsURI": null +}""" + +JOB_STATUS_FAILED = """{ + "Id": "JID_498218641680", + "JobState": "Failed", + "JobType": "NICConfiguration", + "Message": "Job failed: Unable to apply configuration.", + "Name": "Configure: NIC.Embedded.1-1-1", + "PercentComplete": 0, + "StartTime": "2026-04-17T20:00:00", + "TargetSettingsURI": null +}""" + +# Updated NIC attribute responses for verification (after job completes) +GET_NIC_ATTR_LIST_UPDATED = """\ +{ + "Attributes": { + "WakeOnLan": "Disabled", + "VLanMode": "Enabled", + "VLanId": 1, + "ChipMdl": "0", + "BlnkLeds": 0, + "NumberVFAdvertised": 128 + } +}""" + +GET_NIC_ATTR_LIST_INTEGER_UPDATED = """\ +{ + "@Redfish.Settings":{ + "@odata.context":"/redfish/v1/$metadata#Settings.Settings", + "@odata.type":"#Settings.v1_3_1.Settings", + "SettingsObject":{"@odata.id":"/redfish/v1/Chassis/System.Embedded.1/NetworkAdapters/NIC.Embedded.1/NetworkDeviceFunctions/NIC.Embedded.1-1-1/Oem/Dell/DellNetworkAttributes/NIC.Embedded.1-1-1/Settings"}, + "SupportedApplyTimes":[ + "Immediate", + "AtMaintenanceWindowStart", + "OnReset", + "InMaintenanceWindowOnReset" + ] + }, + "@odata.context":"/redfish/v1/$metadata#DellAttributes.DellAttributes", + "@odata.id":"/redfish/v1/Chassis/System.Embedded.1/NetworkAdapters/NIC.Embedded.1/NetworkDeviceFunctions/NIC.Embedded.1-1-1/Oem/Dell/DellNetworkAttributes/NIC.Embedded.1-1-1", + "@odata.type":"#DellAttributes.v1_0_0.DellAttributes", + "AttributeRegistry":"NetworkAttributeRegistry_NIC.Embedded.1-1-1", + "Attributes":{ + "ChipMdl":"BCM5720 A0", + "PCIDeviceID":"165F", + "BusDeviceFunction":"04:00:00", + "MacAddr":"C8:4B:D6:83:16:00", + "VirtMacAddr":"C8:4B:D6:83:16:00", + "FCoEOffloadSupport":"Unavailable", + "iSCSIOffloadSupport":"Unavailable", + "iSCSIBootSupport":"Unavailable", + "PXEBootSupport":"Available", + "FCoEBootSupport":"Unavailable", + "NicPartitioningSupport":"Unavailable", + "FlexAddressing":"Unavailable", + "TXBandwidthControlMaximum":"Unavailable", + "TXBandwidthControlMinimum":"Unavailable", + "EnergyEfficientEthernet":"Available", + "FamilyVersion":"22.00.6", + "ControllerBIOSVersion":"1.39", + "EFIVersion":"21.6.29", + "BlnkLeds":12, + "BannerMessageTimeout":5, + "VLanId":1, + "EEEControl":"Enabled", + "LinkStatus":"Disconnected", + "BootOptionROM":"Enabled", + "LegacyBootProto":"NONE", + "BootStrapType":"AutoDetect", + "HideSetupPrompt":"Disabled", + "LnkSpeed":"AutoNeg", + "WakeOnLan":"Enabled", + "VLanMode":"Disabled", + "PermitTotalPortShutdown":"Disabled" + }, + "Description":"DellNetworkAttributes represents the Network device attribute details.", + "Id":"NIC.Embedded.1-1-1", + "Name":"DellNetworkAttributes" +} +""" + +# Expected responses for job monitoring test cases +RESPONSE_SET_NIC_ATTR_WITH_JOB_SUCCESS = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- INFO - Network attribute configuration job created: JID_498218641680 +- INFO - Waiting for configuration job to be scheduled... +- INFO - Job JID_498218641680 status: Scheduled - Task successfully scheduled. +- INFO - Command passed to On server, code return is 200. +- INFO - Monitoring job JID_498218641680 for completion... +- INFO - JobID: JID_498218641680 +- INFO - Name: Configure: NIC.Embedded.1-1-1 +- INFO - Message: Job completed successfully. +- INFO - PercentComplete: 100 +- INFO - Verifying attribute value was applied... +- INFO - ✓ Successfully changed WakeOnLan from Enabled to Disabled +""" + +RESPONSE_SET_NIC_ATTR_JOB_FAILED = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- INFO - Network attribute configuration job created: JID_498218641680 +- INFO - Waiting for configuration job to be scheduled... +- INFO - Job JID_498218641680 status: Scheduled - Task successfully scheduled. +- INFO - Command passed to GracefulRestart server, code return is 204. +- INFO - Polling for host state: Not Down +- WARNING - Command failed to On server, host appears to be already in that state. +- INFO - Monitoring job JID_498218641680 for completion... +- ERROR - Configuration job JID_498218641680 did not complete successfully. +- ERROR - Network attribute changes may not have been applied. +""" + +RESPONSE_SET_NIC_ATTR_NO_JOB_ID = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- WARNING - No job ID returned in Location header. Changes may not persist after reboot. +- INFO - Command passed to On server, code return is 200. +- WARNING - Configuration change submitted but job monitoring was not possible. Please manually verify attribute value persisted after reboot. +""" + +RESPONSE_SET_NIC_ATTR_PRE_REBOOT_FAIL = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- INFO - Network attribute configuration job created: JID_498218641680 +- INFO - Waiting for configuration job to be scheduled... +- INFO - Job JID_498218641680 status: Failed - Job failed: Unable to apply configuration. +- ERROR - Configuration job failed before reboot: Job failed: Unable to apply configuration. +""" + +RESPONSE_SET_NIC_ATTR_VERIFY_FAILED = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- INFO - Network attribute configuration job created: JID_498218641680 +- INFO - Waiting for configuration job to be scheduled... +- INFO - Job JID_498218641680 status: Scheduled - Task successfully scheduled. +- INFO - Command passed to On server, code return is 200. +- INFO - Monitoring job JID_498218641680 for completion... +- INFO - JobID: JID_498218641680 +- INFO - Name: Configure: NIC.Embedded.1-1-1 +- INFO - Message: Job completed successfully. +- INFO - PercentComplete: 100 +- INFO - Verifying attribute value was applied... +- ERROR - ✗ Attribute value did not persist. Expected: Disabled, Current: Enabled +- ERROR - Job completed but value not applied. Possible causes: prerequisite not met, validation failure, or firmware issue. +""" + +RESPONSE_SET_NIC_ATTR_FALSE_NEGATIVE = """\ +- INFO - Patch command to set network attribute values and create next reboot job PASSED. +- INFO - Network attribute configuration job created: JID_498218641680 +- INFO - Waiting for configuration job to be scheduled... +- INFO - Job JID_498218641680 status: Scheduled - Task successfully scheduled. +- INFO - Command passed to On server, code return is 200. +- INFO - Monitoring job JID_498218641680 for completion... +- ERROR - Configuration job JID_498218641680 did not complete successfully. +- ERROR - Network attribute changes may not have been applied. +- WARNING - Job reported failure but attribute value is correct (WakeOnLan=Disabled). This may be a false negative. +""" + +# VF Limit Validation Test Fixtures +GET_NIC_ATTR_LIST_XXV710_NPARSRIOV = """\ +{ + "Attributes":{ + "DeviceName":"Intel(R) Ethernet Network Adapter XXV710", + "VirtualizationMode":"NPARSRIOV", + "NumberPCIFunctionsEnabled":"4", + "NumberVFAdvertised":"64", + "NumberVFSupported":"128", + "WakeOnLan":"Disabled" + } +} +""" + +GET_NIC_ATTR_LIST_SINGLE_FUNCTION = """\ +{ + "Attributes":{ + "DeviceName":"Intel(R) Ethernet Network Adapter XXV710", + "VirtualizationMode":"SRIOV", + "NumberPCIFunctionsEnabled":"1", + "NumberVFAdvertised":"64", + "NumberVFSupported":"128", + "WakeOnLan":"Disabled" + } +} +""" + +GET_NIC_ATTR_LIST_VIRT_MODE_NONE = """\ +{ + "Attributes":{ + "DeviceName":"Intel(R) Ethernet 25G 2P XXV710 Adapter", + "VirtualizationMode":"NONE", + "NumberPCIFunctionsEnabled":"1", + "NumberVFAdvertised":"64", + "NumberVFSupported":"128", + "WakeOnLan":"Disabled" + } +} +""" + +RESPONSE_SET_NIC_ATTR_VF_LIMIT_XXV710_WARNING = """\ +- WARNING - Attempting to set NumberVFAdvertised to 128 on Intel XXV710. +- WARNING - Testing has shown that Intel XXV710 NICs are limited to 64 VFs regardless of virtualization mode (SRIOV/NPARSRIOV) or firmware version. +- WARNING - This operation will likely fail. The hardware limit appears to be 64 VFs maximum. +""" + +RESPONSE_SET_NIC_ATTR_VIRT_MODE_NONE = """\ +- ERROR - Cannot set NumberVFAdvertised when VirtualizationMode is NONE (disabled). +- ERROR - First enable SR-IOV virtualization mode with: +- ERROR - --set-nic-attribute NIC.Embedded.1-1-1 --attribute VirtualizationMode --value SRIOV +""" + + +# Corrected registry with NumberVFAdvertised +GET_NIC_ATTR_REGISTRY_WITH_VF = """ +{ + "@odata.context": "/redfish/v1/$metadata#AttributeRegistry.AttributeRegistry", + "@odata.id": "/redfish/v1/Registries/NetworkAttributesRegistry_NIC.ChassisSlot.8-2-1/NetworkAttributesRegistry_NIC.ChassisSlot.8-2-1.json", + "@odata.type": "#AttributeRegistry.v1_3_3.AttributeRegistry", + "Description": "This registry defines a representation of Network Attribute instances", + "Id": "NetworkAttributesRegistry_NIC.ChassisSlot.8-2-1", + "Language": "en", + "Name": "Network Attribute Registry", + "OwningEntity": "Dell", + "RegistryEntries": { + "Attributes": [ + { + "AttributeName": "NumberVFAdvertised", + "CurrentValue": null, + "DisplayName": "PCI Virtual Functions Advertised", + "DisplayOrder": 304, + "HelpText": null, + "Hidden": false, + "Immutable": false, + "LowerBound": 0, + "MenuPath": "./", + "Oem": { + "Dell": { + "@odata.type": "#DellOemAttributeRegistry.v1_0_0.Attributes", + "GroupDisplayName": "NIC Configuration", + "GroupName": "NICConfig" + } + }, + "ReadOnly": false, + "ResetRequired": true, + "ScalarIncrement": 16, + "Type": "Integer", + "UpperBound": 128, + "WarningText": null, + "WriteOnly": false + }, + { + "AttributeName": "WakeOnLan", + "CurrentValue": null, + "DisplayName": "Wake On LAN", + "DisplayOrder": 201, + "HelpText": null, + "Hidden": false, + "Immutable": false, + "MenuPath": "./", + "Oem": { + "Dell": { + "@odata.type": "#DellOemAttributeRegistry.v1_0_0.Attributes", + "GroupDisplayName": "Device Level Configuration", + "GroupName": "DeviceLevelConfig" + } + }, + "ReadOnly": false, + "ResetRequired": true, + "Type": "Enumeration", + "Value": [ + { + "ValueDisplayName": "Disabled", + "ValueName": "Disabled" + }, + { + "ValueDisplayName": "Enabled", + "ValueName": "Enabled" + } + ], + "WarningText": null, + "WriteOnly": false + } + ] + }, + "RegistryVersion": "1.0.0", + "SupportedSystems": [] +} +""" + + +GET_NIC_ATTR_LIST_WITH_VF = """\ +{ + "Attributes":{ + "DeviceName":"Intel(R) Ethernet Network Adapter XXV710", + "VirtualizationMode":"SRIOV", + "NumberPCIFunctionsEnabled":"1", + "NumberVFAdvertised":"64", + "NumberVFSupported":"128", + "WakeOnLan":"Disabled" + } +} +""" diff --git a/tests/test_boot_to.py b/tests/test_boot_to.py index 4aecff4..f9b4d33 100644 --- a/tests/test_boot_to.py +++ b/tests/test_boot_to.py @@ -133,3 +133,29 @@ def test_boot_to_no_match(self, mock_get, mock_patch, mock_post, mock_delete): self.args = [self.option_arg, BAD_DEVICE_NAME] _, err = self.badfish_call() assert err == ERROR_DEV_NO_MATCH + + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.get") + def test_boot_to_job_creation_fails(self, mock_get, mock_patch, mock_post, mock_delete): + """Test create_job when POST to create job returns non-200 status (covers L805-806 in main.py)""" + boot_seq_resp_fmt = BOOT_SEQ_RESP % str(BOOT_SEQ_RESPONSE_DIRECTOR) + get_resp = [ + BOOT_MODE_RESP, + boot_seq_resp_fmt.replace("'", '"'), + BLANK_RESP, + BOOT_MODE_RESP, + ] + responses = INIT_RESP + get_resp + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_patch, 200, ["OK"]) + # Make POST request fail with 500 to trigger error path in create_job + self.set_mock_response(mock_post, 500, '{"error": "Internal Server Error"}') + self.set_mock_response(mock_delete, 200, "OK") + + self.args = [self.option_arg, DEVICE_NIC_2["name"]] + _, err = self.badfish_call() + # The error path executes L805-806, but error_handler raises BadfishException + # which gets caught at higher level and logs generic message + assert "Failed to communicate" in err diff --git a/tests/test_context_manager.py b/tests/test_context_manager.py index 4e477c1..10e4e7d 100644 --- a/tests/test_context_manager.py +++ b/tests/test_context_manager.py @@ -37,7 +37,7 @@ class TestContextManager: async def test_context_manager_successful_entry_exit(self): """Test successful entry and exit of the context manager.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) # Mock the init method with patch.object(badfish_instance, "init", new_callable=AsyncMock) as mock_init: @@ -52,7 +52,7 @@ async def test_context_manager_successful_entry_exit(self): async def test_context_manager_exception_handling(self): """Test that exceptions are properly handled and logged.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) # Mock the init method to raise an exception with patch.object(badfish_instance, "init", new_callable=AsyncMock) as mock_init: @@ -70,7 +70,7 @@ async def test_context_manager_exception_handling(self): async def test_context_manager_direct_method_calls(self): """Test direct calls to __aenter__ and __aexit__ methods.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) with patch.object(badfish_instance, "init", new_callable=AsyncMock) as mock_init: with patch.object(badfish_instance, "delete_session", new_callable=AsyncMock) as mock_delete: @@ -87,7 +87,7 @@ async def test_context_manager_direct_method_calls(self): async def test_context_manager_nested_usage(self): """Test nested usage of the context manager.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) with patch.object(badfish_instance, "init", new_callable=AsyncMock) as mock_init: with patch.object(badfish_instance, "delete_session", new_callable=AsyncMock) as mock_delete: @@ -123,11 +123,27 @@ async def test_context_manager_integration_with_factory(self): mock_delete.assert_called_once() + @pytest.mark.asyncio + async def test_badfish_factory_without_logger(self): + """Test badfish_factory creates a default logger when none is provided.""" + with patch.object(Badfish, "init", new_callable=AsyncMock) as mock_init: + with patch.object(Badfish, "delete_session", new_callable=AsyncMock): + # Call badfish_factory WITHOUT a logger argument + badfish = await badfish_factory(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD) + + assert isinstance(badfish, Badfish) + assert badfish.host == MOCK_HOST + assert badfish.username == MOCK_USERNAME + assert badfish.password == MOCK_PASSWORD + # Verify a logger was created + assert badfish.logger is not None + mock_init.assert_called_once() + @pytest.mark.asyncio async def test_context_manager_session_cleanup(self): """Test that session cleanup happens even with exceptions.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) with patch.object(badfish_instance, "init", new_callable=AsyncMock): with patch.object(badfish_instance, "delete_session", new_callable=AsyncMock) as mock_delete: @@ -144,7 +160,7 @@ async def test_context_manager_session_cleanup(self): async def test_context_manager_init_failure(self): """Test context manager behavior when init fails.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) with patch.object(badfish_instance, "init", new_callable=AsyncMock) as mock_init: mock_init.side_effect = BadfishException("Init failed") @@ -161,7 +177,7 @@ async def test_context_manager_init_failure(self): async def test_context_manager_delete_session_failure(self): """Test context manager behavior when delete_session fails.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) with patch.object(badfish_instance, "init", new_callable=AsyncMock): with patch.object(badfish_instance, "delete_session", new_callable=AsyncMock) as mock_delete: @@ -180,7 +196,7 @@ class TestDeleteSession: async def test_delete_session_success(self): """Test successful session deletion.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -210,7 +226,7 @@ async def test_delete_session_success(self): async def test_delete_session_404_status(self): """Test session deletion with 404 status (session not found).""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -233,7 +249,7 @@ async def test_delete_session_404_status(self): async def test_delete_session_unexpected_status(self): """Test session deletion with unexpected status code.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -256,7 +272,7 @@ async def test_delete_session_unexpected_status(self): async def test_delete_session_exception_handling(self): """Test session deletion when delete_request raises an exception.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -277,7 +293,7 @@ async def test_delete_session_exception_handling(self): async def test_delete_session_no_session_id(self): """Test delete_session when no session_id is set.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = None badfish_instance.token = "test_token" @@ -297,7 +313,7 @@ async def test_delete_session_no_session_id(self): async def test_delete_session_cleanup_always_executes(self): """Test that cleanup (clearing session_id and token) always executes.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -316,7 +332,7 @@ async def test_delete_session_cleanup_always_executes(self): async def test_delete_session_other_exception(self): """Test delete_session with non-BadfishException.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -335,7 +351,7 @@ async def test_delete_session_other_exception(self): async def test_delete_session_outer_exception_handler(self): """Test the outer exception handler that ensures cleanup even for unexpected exceptions.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = "/redfish/v1/SessionService/Sessions/123" badfish_instance.token = "test_token" @@ -362,7 +378,7 @@ async def test_delete_session_outer_exception_handler(self): async def test_delete_session_no_session_id_outer_exception(self): """Test outer exception handler when session_id is None and logger fails.""" logger = MockLogger() - badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES) + badfish_instance = Badfish(MOCK_HOST, MOCK_USERNAME, MOCK_PASSWORD, logger, MOCK_RETRIES, _insecure=False) badfish_instance.session_id = None badfish_instance.token = "test_token" @@ -408,6 +424,7 @@ async def test_execute_badfish_session_cleanup_success(self): "power_cycle": False, "get_power_consumed": False, "racreset": False, + "wait": False, "bmc_reset": False, "factory_reset": False, "check_boot": False, @@ -449,6 +466,7 @@ async def test_execute_badfish_session_cleanup_success(self): "get_nic_fqdds": False, "get_nic_attribute": None, "set_nic_attribute": None, + "insecure": False, } with patch("badfish.main.badfish_factory") as mock_factory: @@ -491,6 +509,7 @@ async def test_execute_badfish_session_cleanup_failure(self): "power_cycle": False, "get_power_consumed": False, "racreset": False, + "wait": False, "bmc_reset": False, "factory_reset": False, "check_boot": False, @@ -532,6 +551,7 @@ async def test_execute_badfish_session_cleanup_failure(self): "get_nic_fqdds": False, "get_nic_attribute": None, "set_nic_attribute": None, + "insecure": False, } with patch("badfish.main.badfish_factory") as mock_factory: @@ -578,6 +598,7 @@ async def test_execute_badfish_no_session_cleanup(self): "power_cycle": False, "get_power_consumed": False, "racreset": False, + "wait": False, "bmc_reset": False, "factory_reset": False, "check_boot": False, @@ -619,6 +640,7 @@ async def test_execute_badfish_no_session_cleanup(self): "get_nic_fqdds": False, "get_nic_attribute": None, "set_nic_attribute": None, + "insecure": False, } with patch("badfish.main.badfish_factory") as mock_factory: @@ -661,6 +683,7 @@ async def test_execute_badfish_no_badfish_instance(self): "power_cycle": False, "get_power_consumed": False, "racreset": False, + "wait": False, "bmc_reset": False, "factory_reset": False, "check_boot": False, @@ -702,6 +725,7 @@ async def test_execute_badfish_no_badfish_instance(self): "get_nic_fqdds": False, "get_nic_attribute": None, "set_nic_attribute": None, + "insecure": False, } with patch("badfish.main.badfish_factory") as mock_factory: diff --git a/tests/test_execution.py b/tests/test_execution.py index 9c3a0b2..3d18968 100644 --- a/tests/test_execution.py +++ b/tests/test_execution.py @@ -79,7 +79,11 @@ def test_host_list_extras(self, mock_get, mock_post, mock_delete): self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") _, err = self.badfish_call(mock_host=None) - assert err == HOST_LIST_EXTRAS + # When Members array is empty, init() catches the exception and logs as WARNING + assert err.count("- WARNING - Could not find system resource: ComputerSystem's Members array is either empty or missing") == 3 + assert err.count("- INFO - ************************************************") == 3 + assert "[badfish.helpers.logger] - INFO - RESULTS:" in err + assert err.count("f01-h01-000-r630.host.io: FAILED") == 3 class TestInitialization(TestBase): @@ -136,15 +140,19 @@ def test_find_systems_resource_unauthorized(self, mock_get, mock_post, mock_dele self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") _, err = self.badfish_call() - assert err == "- ERROR - ComputerSystem's Members array is either empty or missing\n" + # When Members array is empty or missing, init() catches the exception and logs as WARNING + assert "- WARNING - Could not find system resource:" in err + assert ("ComputerSystem's Members array" in err or "Authorization Error" in err) @patch("aiohttp.ClientSession.delete") @patch("aiohttp.ClientSession.post") @patch("aiohttp.ClientSession.get") def test_find_systems_resource_not_found(self, mock_get, mock_post, mock_delete): - responses = [ROOT_RESP, "{}", MAN_RESP] + responses = [ROOT_RESP, ROOT_RESP, "{}", "{}", MAN_RESP, '{"Members":[]}', ROOT_RESP] self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") _, err = self.badfish_call() - assert err == RESPONSE_INIT_SYSTEMS_RESOURCE_NOT_FOUND + # When Members array is empty or missing, init() catches the exception and logs as WARNING + assert "- WARNING - Could not find system resource:" in err + assert ("Systems resource not found" in err or "ComputerSystem's Members array" in err) diff --git a/tests/test_http_client.py b/tests/test_http_client.py index 7c4233e..2544f32 100644 --- a/tests/test_http_client.py +++ b/tests/test_http_client.py @@ -1,4 +1,5 @@ import json +import ssl from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch @@ -45,7 +46,7 @@ def set_mock_response(mock, status, responses, post=False, headers=None): @pytest.mark.asyncio async def test_error_handler_valueerror_raises(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) resp = SimpleNamespace(text=AsyncMock(return_value="not-json")) with pytest.raises(BadfishException, match="Error reading response from host."): await client.error_handler(resp) @@ -54,7 +55,7 @@ async def test_error_handler_valueerror_raises(): @pytest.mark.asyncio async def test_error_handler_extended_info_with_custom_message(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) payload = {"error": {"@Message.ExtendedInfo": [{"Message": "detail", "Resolution": "fix it"}]}} resp = SimpleNamespace(text=AsyncMock(return_value=json.dumps(payload))) with pytest.raises(BadfishException, match="custom"): @@ -67,7 +68,7 @@ async def test_error_handler_extended_info_with_custom_message(): @patch("aiohttp.ClientSession.get") async def test_get_raw_success_with_token(mock_get): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) client.token = "TKN" set_mock_response(mock_get, 200, "{}") resp = await client.get_raw("https://x") @@ -85,7 +86,7 @@ async def test_get_raw_success_with_token(mock_get): @patch("aiohttp.ClientSession.get") async def test_get_raw_success_with_basic_auth(mock_get): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) set_mock_response(mock_get, 200, "{}") resp = await client.get_raw("https://x", _get_token=True) assert resp.status == 200 @@ -101,7 +102,7 @@ async def test_get_raw_success_with_basic_auth(mock_get): @patch("aiohttp.ClientSession.get", side_effect=Exception("boom")) async def test_get_raw_exception_continue_returns_none(mock_get): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) resp = await client.get_raw("https://x", _continue=True) assert resp is None @@ -110,7 +111,7 @@ async def test_get_raw_exception_continue_returns_none(mock_get): @patch("aiohttp.ClientSession.get", side_effect=Exception("boom")) async def test_get_raw_exception_raises(mock_get): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) with pytest.raises(BadfishException): await client.get_raw("https://x") assert any("Failed to communicate" not in m for m in logger.debug_msgs) # debug captured @@ -119,7 +120,7 @@ async def test_get_raw_exception_raises(mock_get): @pytest.mark.asyncio async def test_get_json_success_and_invalid(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) class FakeResp: def __init__(self, text): @@ -146,7 +147,7 @@ async def text(self, *args, **kwargs): @patch("aiohttp.ClientSession.post") async def test_post_request_with_and_without_204(mock_post): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) client.token = "TT" # Non-204 => reads body, returns response @@ -164,7 +165,7 @@ async def test_post_request_with_and_without_204(mock_post): @patch("aiohttp.ClientSession.post", side_effect=Exception("err")) async def test_post_request_raises(mock_post): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) with pytest.raises(BadfishException): await client.post_request("https://x", {}, {}) @@ -173,7 +174,7 @@ async def test_post_request_raises(mock_post): @patch("aiohttp.ClientSession.patch") async def test_patch_request_success(mock_patch): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) set_mock_response(mock_patch, 200, "OK") resp = await client.patch_request("https://x", {}, {}) assert resp.status == 200 @@ -183,7 +184,7 @@ async def test_patch_request_success(mock_patch): @patch("aiohttp.ClientSession.patch", side_effect=Exception("bad")) async def test_patch_request_continue_and_raise(mock_patch): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) # Continue returns None out = await client.patch_request("https://x", {}, {}, _continue=True) assert out is None @@ -196,7 +197,7 @@ async def test_patch_request_continue_and_raise(mock_patch): @patch("aiohttp.ClientSession.delete") async def test_delete_request_success(mock_delete): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) set_mock_response(mock_delete, 200, "OK") resp = await client.delete_request("https://x", {}) assert resp.status == 200 @@ -206,7 +207,7 @@ async def test_delete_request_success(mock_delete): @patch("aiohttp.ClientSession.delete", side_effect=Exception("x")) async def test_delete_request_raises(mock_delete): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) with pytest.raises(BadfishException): await client.delete_request("https://x", {}) @@ -214,7 +215,7 @@ async def test_delete_request_raises(mock_delete): @pytest.mark.asyncio async def test_find_session_uri_paths_and_404_switch(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) # First call: root with Redfish 1.60 -> SessionService/Sessions # Second call: check session URI returns 200 => keep @@ -235,7 +236,7 @@ async def test_find_session_uri_paths_and_404_switch(): @pytest.mark.asyncio async def test_find_session_uri_auth_or_comm_errors(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) # Unauthorized resp401 = SimpleNamespace(status=401, text=AsyncMock(return_value="{}")) client.get_request = AsyncMock(return_value=resp401) @@ -252,7 +253,7 @@ async def test_find_session_uri_auth_or_comm_errors(): @pytest.mark.asyncio async def test_validate_credentials_success_and_errors(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) client.find_session_uri = AsyncMock(return_value="/redfish/v1/Sessions") headers = {"Location": "/redfish/v1/SessionService/Sessions/1", "X-Auth-Token": "TK"} @@ -278,7 +279,7 @@ async def test_validate_credentials_success_and_errors(): @pytest.mark.asyncio async def test_delete_session_paths_and_cleanup(): logger = DummyLogger() - client = HTTPClient("host", "u", "p", logger) + client = HTTPClient("host", "u", "p", logger, insecure=False) # No session id => returns and keeps cleared client.session_id = None @@ -320,3 +321,145 @@ async def raise_exc(*args, **kwargs): client.delete_request = AsyncMock(side_effect=raise_exc) await client.delete_session() assert any("Failed to delete session" in m for m in logger.warn_msgs) + + +# SSL Certificate Verification Tests + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.get", side_effect=ssl.SSLError("certificate verify failed")) +async def test_get_raw_ssl_error_raises(mock_get): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.get_raw("https://x") + assert any("SSL certificate verification failed" in m for m in logger.debug_msgs) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.get", side_effect=ssl.SSLError("certificate verify failed")) +async def test_get_raw_ssl_error_continue_returns_none(mock_get): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + resp = await client.get_raw("https://x", _continue=True) + assert resp is None + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.get") +async def test_get_raw_client_connector_certificate_error_raises(mock_get): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + # Simulate ClientConnectorCertificateError + mock_get.side_effect = aiohttp.ClientConnectorCertificateError( + connection_key=MagicMock(), certificate_error=ssl.SSLError("cert error")) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.get_raw("https://x") + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.post", side_effect=ssl.SSLError("certificate verify failed")) +async def test_post_request_ssl_error_raises(mock_post): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.post_request("https://x", {}, {}) + assert any("SSL certificate verification failed" in m for m in logger.debug_msgs) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.post") +async def test_post_request_client_connector_certificate_error_raises(mock_post): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + mock_post.side_effect = aiohttp.ClientConnectorCertificateError( + connection_key=MagicMock(), certificate_error=ssl.SSLError("cert error")) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.post_request("https://x", {}, {}) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.patch", side_effect=ssl.SSLError("certificate verify failed")) +async def test_patch_request_ssl_error_raises(mock_patch): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.patch_request("https://x", {}, {}) + assert any("SSL certificate verification failed" in m for m in logger.debug_msgs) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.patch", side_effect=ssl.SSLError("certificate verify failed")) +async def test_patch_request_ssl_error_continue_returns_none(mock_patch): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + resp = await client.patch_request("https://x", {}, {}, _continue=True) + assert resp is None + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.patch") +async def test_patch_request_client_connector_certificate_error_raises(mock_patch): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + mock_patch.side_effect = aiohttp.ClientConnectorCertificateError( + connection_key=MagicMock(), certificate_error=ssl.SSLError("cert error")) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.patch_request("https://x", {}, {}) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.delete", side_effect=ssl.SSLError("certificate verify failed")) +async def test_delete_request_ssl_error_raises(mock_delete): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.delete_request("https://x", {}) + assert any("SSL certificate verification failed" in m for m in logger.debug_msgs) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.delete") +async def test_delete_request_client_connector_certificate_error_raises(mock_delete): + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + mock_delete.side_effect = aiohttp.ClientConnectorCertificateError( + connection_key=MagicMock(), certificate_error=ssl.SSLError("cert error")) + with pytest.raises(BadfishException, match="SSL certificate verification failed"): + await client.delete_request("https://x", {}) + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.get") +async def test_insecure_flag_sets_ssl_false(mock_get): + """Test that insecure=True properly disables SSL verification""" + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=True) + set_mock_response(mock_get, 200, "{}") + await client.get_raw("https://x") + + # Check that ssl=False was passed + called_with_ssl_false = False + for args, kwargs in mock_get.call_args_list: + if kwargs.get("ssl") is False: + called_with_ssl_false = True + break + assert called_with_ssl_false, "insecure=True should set ssl=False" + + +@pytest.mark.asyncio +@patch("aiohttp.ClientSession.get") +async def test_secure_flag_sets_ssl_true(mock_get): + """Test that insecure=False properly enables SSL verification""" + logger = DummyLogger() + client = HTTPClient("host", "u", "p", logger, insecure=False) + set_mock_response(mock_get, 200, "{}") + await client.get_raw("https://x") + + # Check that ssl=True was passed + called_with_ssl_true = False + for args, kwargs in mock_get.call_args_list: + if kwargs.get("ssl") is True: + called_with_ssl_true = True + break + assert called_with_ssl_true, "insecure=False should set ssl=True" diff --git a/tests/test_main_coverage.py b/tests/test_main_coverage.py index 83f3960..4ecc5d9 100644 --- a/tests/test_main_coverage.py +++ b/tests/test_main_coverage.py @@ -172,3 +172,14 @@ async def test_set_nic_attribute_invalid_fqdd_exception(): await bf.set_nic_attribute(mock_fqdd, "Attr", "NewVal") logger.error.assert_any_call("Invalid FQDD supplied.") + + +def test_version_flag(capsys): + from badfish import __version__ + from badfish.helpers.parser import parse_arguments + + with pytest.raises(SystemExit) as exc_info: + parse_arguments(["--version"]) + assert exc_info.value.code == 0 + captured = capsys.readouterr() + assert f"badfish {__version__}" in captured.out diff --git a/tests/test_nic_attributes.py b/tests/test_nic_attributes.py index f938988..0a69ce3 100644 --- a/tests/test_nic_attributes.py +++ b/tests/test_nic_attributes.py @@ -4,13 +4,24 @@ GET_FW_VERSION, GET_FW_VERSION_UNSUPPORTED, GET_NIC_ATTR_LIST, + GET_NIC_ATTR_LIST_UPDATED, + GET_NIC_ATTR_LIST_INTEGER_UPDATED, + GET_NIC_ATTR_LIST_XXV710_NPARSRIOV, + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, + GET_NIC_ATTR_LIST_VIRT_MODE_NONE, + GET_NIC_ATTR_LIST_WITH_VF, GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_REGISTRY_WITH_VF, GET_NIC_FQQDS_ADAPTERS, GET_NIC_FQQDS_EMBEDDED, GET_NIC_FQQDS_INTEGRATED, GET_NIC_FQQDS_SLOT, INIT_RESP, INIT_RESP_SUPERMICRO, + JOB_STATUS_SCHEDULED, + JOB_STATUS_RUNNING, + JOB_STATUS_COMPLETED, + JOB_STATUS_FAILED, RESET_TYPE_RESP, RESPONSE_GET_NIC_ATTR_FW_BAD, RESPONSE_GET_NIC_ATTR_LIST_INVALID, @@ -28,6 +39,14 @@ RESPONSE_SET_NIC_ATTR_RETRY_NOT_OK, RESPONSE_SET_NIC_ATTR_RETRY_OK, RESPONSE_SET_NIC_ATTR_STR_MAXED, + RESPONSE_SET_NIC_ATTR_VIRT_MODE_NONE, + RESPONSE_SET_NIC_ATTR_WITH_JOB_SUCCESS, + RESPONSE_SET_NIC_ATTR_JOB_FAILED, + RESPONSE_SET_NIC_ATTR_NO_JOB_ID, + RESPONSE_SET_NIC_ATTR_PRE_REBOOT_FAIL, + RESPONSE_SET_NIC_ATTR_VERIFY_FAILED, + RESPONSE_SET_NIC_ATTR_FALSE_NEGATIVE, + RESPONSE_SET_NIC_ATTR_VF_LIMIT_XXV710_WARNING, RESPONSE_VENDOR_UNSUPPORTED, STATE_OFF_RESP, STATE_ON_RESP, @@ -304,7 +323,7 @@ def test_set_nic_attr_ok(self, mock_get, mock_post, mock_delete, mock_patch): self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -331,7 +350,7 @@ def test_set_nic_attr_retry_ok(self, mock_get, mock_post, mock_delete, mock_patc self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, [503, 200], "OK") + self.set_mock_response(mock_patch, [503, 200], "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -361,7 +380,7 @@ def test_set_nic_attr_retry_not_ok(self, mock_get, mock_post, mock_delete, mock_ self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, [400, 200, 200], "OK") + self.set_mock_response(mock_patch, [400, 200, 200], "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -388,7 +407,7 @@ def test_set_nic_attr_str_ok(self, mock_get, mock_post, mock_delete, mock_patch) self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -415,7 +434,7 @@ def test_set_nic_attr_str_maxed(self, mock_get, mock_post, mock_delete, mock_pat self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -442,7 +461,7 @@ def test_set_nic_attr_int_ok(self, mock_get, mock_post, mock_delete, mock_patch) self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -469,7 +488,7 @@ def test_set_nic_attr_int_maxed(self, mock_get, mock_post, mock_delete, mock_pat self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, 200, "OK") self.set_mock_response(mock_delete, 200, "OK") - self.set_mock_response(mock_patch, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) self.args = [ self.option_arg, "NIC.Embedded.1-1-1", @@ -522,3 +541,675 @@ def test_set_nic_attr_supermicro(self, mock_get, mock_post, mock_delete): ] _, err = self.badfish_call() assert err == f"{RESPONSE_VENDOR_UNSUPPORTED}\n" + + # ==================== Job Monitoring Tests (Issue #523 Fix) ==================== + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_with_job_monitoring_success(self, mock_get, mock_post, mock_delete, mock_patch): + """Test successful NIC attribute change with full job monitoring""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + JOB_STATUS_SCHEDULED, # Pre-reboot job check + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot job monitoring + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST_UPDATED, # Final verification + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + # Mock PATCH response with Location header containing job ID + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + assert err == RESPONSE_SET_NIC_ATTR_WITH_JOB_SUCCESS + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_no_job_id_fallback(self, mock_get, mock_post, mock_delete, mock_patch): + """Test fallback behavior when no job ID is returned (preserves old behavior)""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + RESET_TYPE_RESP, + STATE_OFF_RESP, + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + # PATCH succeeds but no Location header + self.set_mock_response(mock_patch, 202, "OK", headers={}) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + assert err == RESPONSE_SET_NIC_ATTR_NO_JOB_ID + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_job_failed_pre_reboot(self, mock_get, mock_post, mock_delete, mock_patch): + """Test detection of job failure before reboot""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + JOB_STATUS_FAILED, # Job already failed before reboot + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + assert err == RESPONSE_SET_NIC_ATTR_PRE_REBOOT_FAIL + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_job_completed_but_value_not_changed(self, mock_get, mock_post, mock_delete, mock_patch): + """Test detection when job completes but value didn't actually change""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + JOB_STATUS_SCHEDULED, # Pre-reboot + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Job says success + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, # But value is still old value (Enabled) + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + assert err == RESPONSE_SET_NIC_ATTR_VERIFY_FAILED + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_job_failed_but_value_changed(self, mock_get, mock_post, mock_delete, mock_patch): + """Test false negative detection: job reports failure but value actually changed""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + JOB_STATUS_SCHEDULED, # Pre-reboot + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_FAILED, # Job reports failure + GET_FW_VERSION, # But verification shows value DID change + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST_UPDATED, + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + assert err == RESPONSE_SET_NIC_ATTR_FALSE_NEGATIVE + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_integer_with_job_monitoring(self, mock_get, mock_post, mock_delete, mock_patch): + """Test integer attribute with job monitoring - Issue #523 scenario""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, # BlnkLeds: 0 + JOB_STATUS_SCHEDULED, + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST_INTEGER_UPDATED, # BlnkLeds: 12 + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "BlnkLeds", + "--value", + "12", + ] + _, err = self.badfish_call() + # Should succeed with job monitoring + assert "✓ Successfully changed BlnkLeds" in err + assert "from 0 to 12" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_empty_location_header(self, mock_get, mock_post, mock_delete, mock_patch): + """Test handling of empty Location header""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + RESET_TYPE_RESP, + STATE_OFF_RESP, + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + # Location header present but empty + patch_headers = {"Location": ""} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + self.option_arg, + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + # Should fall back to old behavior with warning + assert "No job ID returned in Location header" in err + assert "Configuration change submitted but job monitoring was not possible" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_vf_limit_xxv710_warning(self, mock_get, mock_post, mock_delete, mock_patch): + """Test VF limit warning for Intel XXV710 with >64 VFs""" + from tests.config import ( + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, + GET_NIC_ATTR_LIST_XXV710_NPARSRIOV, + ) + + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, # get_nic_attribute_info call + GET_NIC_ATTR_LIST_XXV710_NPARSRIOV, # VirtualizationMode check + GET_NIC_ATTR_LIST_XXV710_NPARSRIOV, # VF limit check + JOB_STATUS_SCHEDULED, # Pre-reboot job check + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot job monitoring + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_XXV710_NPARSRIOV, # Final verification (value still 64, not 128) + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.ChassisSlot.8-2-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "128", + ] + _, err = self.badfish_call() + # Should warn about XXV710 hardware limit + assert "Attempting to set NumberVFAdvertised to 128 on Intel XXV710" in err + assert "limited to 64 VFs" in err + assert "hardware limit" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_vf_limit_valid_value_no_warning(self, mock_get, mock_post, mock_delete, mock_patch): + """Test that valid VF values (≤64) don't trigger warnings""" + from tests.config import ( + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, + ) + + # Create updated list with NumberVFAdvertised set to 48 + import json + updated_attrs = json.loads(GET_NIC_ATTR_LIST_WITH_VF) + updated_attrs["Attributes"]["NumberVFAdvertised"] = "48" + GET_NIC_ATTR_LIST_VF_48 = json.dumps(updated_attrs) + + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, # get_nic_attribute_info call (current: 64) + GET_NIC_ATTR_LIST_WITH_VF, # VirtualizationMode check (SRIOV mode, not NONE) + # No VF limit check call since value ≤64 + JOB_STATUS_SCHEDULED, # Pre-reboot job check + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot job monitoring + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_VF_48, # Final verification (changed to 48) + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.ChassisSlot.8-2-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "48", + ] + _, err = self.badfish_call() + # Should NOT warn for valid values ≤64 + assert "Attempting to set NumberVFAdvertised to" not in err + assert "limited to 64 VFs" not in err + # Should succeed + assert "✓ Successfully changed NumberVFAdvertised" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_vf_limit_xxv710_sriov_mode(self, mock_get, mock_post, mock_delete, mock_patch): + """Test VF limit warning for Intel XXV710 in SRIOV mode with >64 VFs""" + from tests.config import ( + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, + ) + + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, # get_nic_attribute_info call + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, # VirtualizationMode check + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, # VF limit check + JOB_STATUS_SCHEDULED, # Pre-reboot job check + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot job monitoring + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, # Final verification + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.ChassisSlot.8-2-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "128", + ] + _, err = self.badfish_call() + # Should warn about XXV710 hardware limit (SRIOV mode has same limit) + assert "Attempting to set NumberVFAdvertised to 128 on Intel XXV710" in err + assert "limited to 64 VFs" in err + assert "hardware limit" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_vf_limit_exception_handling(self, mock_get, mock_post, mock_delete, mock_patch): + """Test VF limit validation gracefully handles exceptions during attribute parsing""" + from tests.config import ( + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, + ) + + # NIC attrs with non-integer NumberPCIFunctionsEnabled to trigger ValueError in int() conversion + ATTRS_WITH_INVALID_FUNCTION_COUNT = """ +{ + "Attributes":{ + "DeviceName":"Intel(R) Ethernet Network Adapter XXV710", + "VirtualizationMode":"SRIOV", + "NumberPCIFunctionsEnabled":"NotANumber", + "NumberVFAdvertised":"64", + "NumberVFSupported":"128" + } +} +""" + + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, # get_nic_attribute_info call + ATTRS_WITH_INVALID_FUNCTION_COUNT, # get_nic_attribute returns data with invalid function count + JOB_STATUS_SCHEDULED, # Pre-reboot job check + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot job monitoring + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_WITH_VF, # Final verification + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.ChassisSlot.8-2-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "128", + ] + _, err = self.badfish_call() + # Should gracefully handle ValueError exception and continue without warnings + # Exception caught at line 2568, proceeds with attempt + assert "Attempting to set NumberVFAdvertised" not in err # No warning due to exception + + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_nonexistent_attribute(self, mock_get, mock_post, mock_delete, mock_patch): + """Test set_nic_attribute when attribute doesn't exist (L2522-2523)""" + # Return empty registry that won't match the requested attribute + empty_registry = '{"RegistryEntries": {"Attributes": []}}' + + responses = INIT_RESP + [ + GET_FW_VERSION, + empty_registry, # Empty registry means attribute won't be found + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + self.set_mock_response(mock_patch, 202, "OK") + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "NonExistentAttribute", + "--value", + "SomeValue", + ] + _, err = self.badfish_call() + # Should error about attribute not existing (L2522-2523) + assert "Was unable to set a network attribute" in err + assert "Attribute most likely doesn't exist" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_monitor_verify_job_failed_value_mismatch(self, mock_get, mock_post, mock_delete, mock_patch): + """Test _monitor_and_verify_attribute_job when job fails and value doesn't match (L996)""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, # Initial get + JOB_STATUS_SCHEDULED, # Pre-reboot verification + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_FAILED, # Post-reboot: job failed + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, # Post-reboot verification - value unchanged (still Enabled) + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + # Job failed and value didn't match - should return False (L996) + assert "Configuration job" in err and "did not complete successfully" in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_monitor_verify_final_check_returns_none(self, mock_get, mock_post, mock_delete, mock_patch): + """Test _monitor_and_verify_attribute_job when final verification returns None (L1023-1024)""" + # Create a response that will cause get_nic_attribute_info to fail + empty_or_invalid_response = '{"Attributes": {}}' # Missing the requested attribute + + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, # Initial get + JOB_STATUS_SCHEDULED, # Pre-reboot verification + RESET_TYPE_RESP, + STATE_OFF_RESP, + JOB_STATUS_COMPLETED, # Post-reboot: job completed + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + empty_or_invalid_response, # get_nic_attribute call returns invalid data + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + patch_headers = {"Location": "/redfish/v1/Managers/iDRAC.Embedded.1/Jobs/JID_498218641680"} + self.set_mock_response(mock_patch, 202, "OK", headers=patch_headers) + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + # Should error about unable to verify final value (L1023-1024) + assert "Could not verify final attribute value" in err or "Was unable to get network attribute" in err + + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_virt_mode_none_numbervf(self, mock_get, mock_post, mock_delete): + """Test setting NumberVFAdvertised when VirtualizationMode is NONE""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_VIRT_MODE_NONE, + GET_NIC_ATTR_LIST_VIRT_MODE_NONE, # Second call for VirtualizationMode check + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "128", + ] + _, err = self.badfish_call() + assert "Cannot set NumberVFAdvertised when VirtualizationMode is NONE" in err + assert "--set-nic-attribute NIC.Embedded.1-1-1 --attribute VirtualizationMode --value SRIOV" in err + + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_virt_mode_none_non_sriov_attr(self, mock_get, mock_post, mock_delete): + """Test setting non-SRIOV attribute (BlnkLeds) when VirtualizationMode is NONE - should not error about NONE""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST_VIRT_MODE_NONE, + GET_NIC_ATTR_LIST_VIRT_MODE_NONE, + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "BlnkLeds", + "--value", + "5", + ] + _, err = self.badfish_call() + # Should NOT get the VirtualizationMode NONE error - BlnkLeds is not an SRIOV attribute + assert "Cannot set BlnkLeds when VirtualizationMode is NONE" not in err + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_virt_mode_sriov_numbervf(self, mock_get, mock_post, mock_delete, mock_patch): + """Test setting NumberVFAdvertised when VirtualizationMode is SRIOV - should proceed""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY_WITH_VF, + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, + GET_NIC_ATTR_LIST_SINGLE_FUNCTION, # For VirtualizationMode check + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + self.set_mock_response(mock_patch, 200, "OK", headers={}) + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "NumberVFAdvertised", + "--value", + "64", + ] + _, err = self.badfish_call() + # Should NOT have the NONE error + assert "Cannot set NumberVFAdvertised when VirtualizationMode is NONE" not in err + + + @patch("aiohttp.ClientSession.patch") + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_set_nic_attr_catches_patch_exception(self, mock_get, mock_post, mock_delete, mock_patch): + """Test set_nic_attribute handles PATCH exceptions gracefully""" + responses = INIT_RESP + [ + GET_FW_VERSION, + GET_NIC_ATTR_REGISTRY, + GET_NIC_ATTR_LIST, + GET_NIC_ATTR_LIST, # VirtualizationMode check + ] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, 200, "OK") + self.set_mock_response(mock_delete, 200, "OK") + + # Make PATCH context manager raise exception + mock_patch.return_value.__aenter__.side_effect = ValueError("Test exception") + + self.args = [ + "--set-nic-attribute", + "NIC.Embedded.1-1-1", + "--attribute", + "WakeOnLan", + "--value", + "Disabled", + ] + _, err = self.badfish_call() + # Should catch and log error (may be "Failed to communicate" or "Was unable to set") + assert ("Was unable to set" in err or "Failed to communicate" in err or "ERROR" in err) diff --git a/tests/test_poll_helpers.py b/tests/test_poll_helpers.py new file mode 100644 index 0000000..6072003 --- /dev/null +++ b/tests/test_poll_helpers.py @@ -0,0 +1,42 @@ +import asyncio +from unittest.mock import MagicMock, patch +from badfish.main import Badfish +from tests.test_base import TestBase + + +class TestPollHelpers(TestBase): + @patch("badfish.main.HTTPClient") + def test_poll_until_ready_timeout(self, mock_http_client): + logger = MagicMock() + badfish = Badfish("test-host", "user", "pass", logger, 2) + + async def always_false(): + return False + + async def run_test(): + return await badfish.poll_until_ready(always_false, "test service", sleep_interval=0) + + result = asyncio.get_event_loop().run_until_complete(run_test()) + + assert result is False + logger.warning.assert_called_once_with("test service did not become ready after 2 retry attempts.") + + @patch("badfish.main.HTTPClient") + def test_poll_until_ready_success(self, mock_http_client): + logger = MagicMock() + badfish = Badfish("test-host", "user", "pass", logger, 5) + + call_count = [0] + + async def check_after_attempts(): + call_count[0] += 1 + return call_count[0] >= 3 + + async def run_test(): + return await badfish.poll_until_ready(check_after_attempts, "test service", sleep_interval=0) + + result = asyncio.get_event_loop().run_until_complete(run_test()) + + assert result is True + logger.info.assert_any_call("Polling for test service") + logger.info.assert_any_call("test service is ready.") diff --git a/tests/test_reset_idrac.py b/tests/test_reset_idrac.py index d1275ce..141437f 100644 --- a/tests/test_reset_idrac.py +++ b/tests/test_reset_idrac.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import patch, PropertyMock, AsyncMock from tests.config import ( BOOT_SEQ_RESPONSE_DIRECTOR, @@ -8,6 +8,7 @@ RESPONSE_RESET, RESPONSE_RESET_FAIL, RESPONSE_RESET_WRONG_VENDOR, + ROOT_RESP, ) from tests.test_base import TestBase @@ -53,3 +54,57 @@ def test_reset_idrac_wrong_vendor(self, mock_get, mock_post, mock_delete): self.args = [self.option_arg] _, err = self.badfish_call() assert err == RESPONSE_RESET_WRONG_VENDOR % ("Dell", "Supermicro", "--bmc-reset") + + @patch("badfish.main.Badfish.wait_for_idrac_ready", new_callable=AsyncMock) + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_reset_idrac_with_wait_timeout(self, mock_get, mock_post, mock_delete, mock_wait): + responses = INIT_RESP + [RESET_TYPE_RESP] + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, [200, 204], "OK", True) + self.set_mock_response(mock_delete, 200, "OK") + mock_wait.return_value = False + + self.boot_seq = BOOT_SEQ_RESPONSE_DIRECTOR + self.args = [self.option_arg, "--wait"] + _, err = self.badfish_call() + assert "Status code 204 returned for POST command to reset iDRAC" in err + assert "iDRAC reset initiated. Waiting for iDRAC to come back online" in err + assert "iDRAC did not respond after 30 retry attempts" in err + + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_reset_idrac_with_wait_success(self, mock_get, mock_post, mock_delete): + responses = INIT_RESP + [RESET_TYPE_RESP] + [ROOT_RESP] * 15 + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, [200, 204], "OK", True) + self.set_mock_response(mock_delete, 200, "OK") + self.boot_seq = BOOT_SEQ_RESPONSE_DIRECTOR + self.args = [self.option_arg, "--wait"] + _, err = self.badfish_call() + assert "Status code 204 returned for POST command to reset iDRAC" in err + assert "iDRAC reset initiated. Waiting for iDRAC to come back online" in err + assert "Polling for iDRAC" in err + assert "iDRAC is ready" in err + assert "iDRAC is now responsive" in err + + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_reset_idrac_with_wait_delayed(self, mock_get, mock_post, mock_delete): + responses = INIT_RESP + [RESET_TYPE_RESP] + ["Not Found"] * 5 + [ROOT_RESP] * 10 + status_list = [200] * 6 + [404] * 5 + [200] * 10 + self.set_mock_response(mock_get, status_list, responses) + self.set_mock_response(mock_post, [200, 204], "OK", True) + self.set_mock_response(mock_delete, 200, "OK") + self.boot_seq = BOOT_SEQ_RESPONSE_DIRECTOR + self.args = [self.option_arg, "--wait"] + _, err = self.badfish_call() + assert "Status code 204 returned for POST command to reset iDRAC" in err + assert "iDRAC reset initiated. Waiting for iDRAC to come back online" in err + assert "Polling for iDRAC" in err + assert "iDRAC is ready" in err + assert "iDRAC is now responsive" in err + diff --git a/tests/test_scp.py b/tests/test_scp.py index 1e4a95d..3ef2ea3 100644 --- a/tests/test_scp.py +++ b/tests/test_scp.py @@ -6,6 +6,7 @@ BLANK_RESP, INIT_RESP, JOB_ID, + RESPONSE_EXPORT_SCP_JOB_FAILED, RESPONSE_EXPORT_SCP_NO_LOCATION, RESPONSE_EXPORT_SCP_PASS, RESPONSE_EXPORT_SCP_STATUS_FAIL, @@ -19,6 +20,7 @@ RESPONSE_IMPORT_SCP_PASS, RESPONSE_IMPORT_SCP_STATUS_FAIL, RESPONSE_IMPORT_SCP_TIME_OUT, + SCP_EXPORT_JOB_FAILED, SCP_GET_TARGETS_ACTIONS_OEM_UNSUPPORTED, SCP_GET_TARGETS_ACTIONS_OEM_WITH_ALLOWABLES, SCP_GET_TARGETS_ACTIONS_OEM_WITHOUT_ALLOWABLES, @@ -118,15 +120,12 @@ class TestExportSCP(TestBase): @patch("aiohttp.ClientSession.get") def test_pass(self, mock_get, mock_post, mock_delete): export_dir_check() + scp_file = open(self.example_path, "r").read() + # Wait-then-fetch approach: wait 45 seconds, then fetch from Jobs, fallback to Tasks + # First GET from Jobs endpoint (no SystemConfiguration), then GET from Tasks endpoint (has it) responses_get = [ - SCP_MESSAGE_PERCENTAGE % ("Ex", 15), - SCP_MESSAGE_PERCENTAGE % ("Ex", 30), - SCP_MESSAGE_PERCENTAGE % ("Ex", 45), - SCP_MESSAGE_PERCENTAGE % ("Ex", 60), - SCP_MESSAGE_PERCENTAGE % ("Ex", 75), - SCP_MESSAGE_PERCENTAGE % ("Ex", 90), - SCP_MESSAGE_PERCENTAGE % ("Ex", 99), - open(self.example_path, "r").read(), + SCP_MESSAGE_PERCENTAGE % ("Ex", 100), # Jobs endpoint response (no SystemConfiguration) + scp_file, # Tasks endpoint response (has SystemConfiguration) ] responses = INIT_RESP + responses_get headers = {"Location": f"/{JOB_ID}"} @@ -142,8 +141,10 @@ def test_pass(self, mock_get, mock_post, mock_delete): @patch("aiohttp.ClientSession.get") def test_status_fail(self, mock_get, mock_post, mock_delete): export_dir_check() - responses = INIT_RESP + # Provide extra responses in case error_handler makes additional requests + responses = INIT_RESP + [BLANK_RESP] * 10 self.set_mock_response(mock_get, 200, responses) + # First POST is session/auth (200), second POST is export command (400 = fail) self.set_mock_response(mock_post, [200, 400], ["OK", "Bad Request"], post=True) self.set_mock_response(mock_delete, 200, "OK") self.args = [self.option_arg, "./exports/"] @@ -172,7 +173,12 @@ def test_time_out(self, mock_get, mock_post, mock_delete): if hasattr(fixed_datetime, "counter"): setattr(fixed_datetime, "counter", 0) export_dir_check() - responses = INIT_RESP + ["{}"] + ([SCP_MESSAGE_PERCENTAGE % ("Ex", 1)] * 4) + # Test failure: SystemConfiguration not found in either Jobs or Tasks endpoint + responses_get = [ + SCP_MESSAGE_PERCENTAGE % ("Ex", 100), # Jobs endpoint (no SystemConfiguration) + SCP_MESSAGE_PERCENTAGE % ("Ex", 100), # Tasks endpoint (no SystemConfiguration either) + ] + responses = INIT_RESP + responses_get headers = {"Location": f"/{JOB_ID}"} self.set_mock_response(mock_get, 200, responses) self.set_mock_response(mock_post, [200, 202], ["OK", "OK"], headers=headers, post=True) @@ -181,6 +187,24 @@ def test_time_out(self, mock_get, mock_post, mock_delete): _, err = self.badfish_call() assert err == RESPONSE_EXPORT_SCP_TIME_OUT + @patch("aiohttp.ClientSession.delete") + @patch("aiohttp.ClientSession.post") + @patch("aiohttp.ClientSession.get") + def test_job_failed(self, mock_get, mock_post, mock_delete): + export_dir_check() + # Test job failed state: Jobs endpoint returns JobState: "Failed" + responses_get = [ + SCP_EXPORT_JOB_FAILED, # Jobs endpoint response with JobState: "Failed" + ] + responses = INIT_RESP + responses_get + headers = {"Location": f"/{JOB_ID}"} + self.set_mock_response(mock_get, 200, responses) + self.set_mock_response(mock_post, [200, 202], ["OK", "OK"], headers=headers, post=True) + self.set_mock_response(mock_delete, 200, "OK") + self.args = [self.option_arg, "./exports/"] + _, err = self.badfish_call() + assert err == RESPONSE_EXPORT_SCP_JOB_FAILED + class TestImportSCP(TestBase): option_arg = "--import-scp"