From cd329e032b9546351e35cf6d7841b3bc23b2202f Mon Sep 17 00:00:00 2001 From: Allen Robel Date: Wed, 17 Jan 2024 14:18:45 -1000 Subject: [PATCH] Refactor, more... 1. ImageUpgradeTask: refactor _build_policy_attach_payload(), _self_policy_attach_payload(), and handle_deleted_state() into a single method _attach_or_detach_policy() 2. ImageUpgradeTask: Fix _failure() and remove docstring contents that described the issue. 3. Modify test_image_mgmt_upgrade_task_00001 to remove assert for instance.payloads as this is no longer needed 4. test_image_upgrade_validate.py: run through black and isort 5. ImagePolicyAction() : modify to store list of diffs and leverage this in ImageUpgradeTask() when generating results. 6. ImagePolicyAction().validate_request() - add check for image policy does not exist on the controller. 7. ImagePolicyAction() - Add typing for all dict assignments. --- .../image_mgmt/image_policy_action.py | 102 ++++++--- plugins/modules/dcnm_image_upgrade.py | 203 +++++------------- .../test_image_upgrade_image_upgrade_task.py | 1 - .../test_image_upgrade_image_validate.py | 4 +- 4 files changed, 130 insertions(+), 180 deletions(-) diff --git a/plugins/module_utils/image_mgmt/image_policy_action.py b/plugins/module_utils/image_mgmt/image_policy_action.py index deeb41fca..85e2d8772 100644 --- a/plugins/module_utils/image_mgmt/image_policy_action.py +++ b/plugins/module_utils/image_mgmt/image_policy_action.py @@ -18,9 +18,11 @@ __metaclass__ = type __author__ = "Allen Robel" +import copy import inspect import json import logging +from typing import Any, Dict from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.api_endpoints import \ ApiEndpoints @@ -69,7 +71,6 @@ def __init__(self, module): self.log = logging.getLogger(f"dcnm.{self.class_name}") self.log.debug("ENTERED ImagePolicyAction()") - method_name = inspect.stack()[0][3] # pylint: disable=unused-variable self.endpoints = ApiEndpoints() self._init_properties() self.image_policies = ImagePolicies(self.module) @@ -96,6 +97,10 @@ def build_payload(self): caller _attach_policy() """ method_name = inspect.stack()[0][3] + + msg = "ENTERED" + self.log.debug(msg) + self.payloads = [] self.switch_issu_details.refresh() @@ -125,6 +130,9 @@ def validate_request(self): """ method_name = inspect.stack()[0][3] + msg = "ENTERED" + self.log.debug(msg) + if self.action is None: msg = f"{self.class_name}.{method_name}: " msg += "instance.action must be set before " @@ -149,10 +157,18 @@ def validate_request(self): self.image_policies.refresh() self.switch_issu_details.refresh() - # Fail if the image policy does not support the switch platform self.image_policies.policy_name = self.policy_name + # Fail if the image policy does not exist. + # Image policy creation is handled by a different module. + if self.image_policies.name is None: + msg = f"{self.class_name}.{method_name}: " + msg += f"policy {self.policy_name} does not exist on " + msg += "the controller" + self.module.fail_json(msg) + for serial_number in self.serial_numbers: self.switch_issu_details.filter = serial_number + # Fail if the image policy does not support the switch platform if self.switch_issu_details.platform not in self.image_policies.platform: msg = f"{self.class_name}.{method_name}: " msg += f"policy {self.policy_name} does not support platform " @@ -170,6 +186,9 @@ def commit(self): """ method_name = inspect.stack()[0][3] + msg = "ENTERED" + self.log.debug(msg) + self.validate_request() if self.action == "attach": self._attach_policy() @@ -186,42 +205,46 @@ def _attach_policy(self): """ Attach policy_name to the switch(es) associated with serial_numbers - NOTES: - 1. This method creates a list of responses and results which - are accessible via properties response and result, - respectively. + This method creates a list of diffs, one result, and one response. + These are accessable via: + self.diff = List[Dict[str, Any]] + self.result = result from the controller + self.response = response from the controller """ method_name = inspect.stack()[0][3] + msg = "ENTERED" + self.log.debug(msg) + self.build_payload() self.path = self.endpoints.policy_attach.get("path") self.verb = self.endpoints.policy_attach.get("verb") - responses = [] - results = [] - - for payload in self.payloads: - response = dcnm_send( - self.module, self.verb, self.path, data=json.dumps(payload) - ) - result = self._handle_response(response, self.verb) + payload: Dict[str, Any] = {} + payload["mappingList"] = self.payloads + response = dcnm_send( + self.module, self.verb, self.path, data=json.dumps(payload) + ) + result = self._handle_response(response, self.verb) + if not result["success"]: msg = f"{self.class_name}.{method_name}: " - msg += f"response: {json.dumps(response, indent=4)}" - self.log.debug(msg) - - if not result["success"]: - msg = f"{self.class_name}.{method_name}: " - msg += f"Bad result when attaching policy {self.policy_name} " - msg += f"to switch {payload['ipAddr']}." - self.module.fail_json(msg, **self.failed_result) + msg += f"Bad result when attaching policy {self.policy_name} " + msg += f"to switch {payload['ipAddr']}." + self.module.fail_json(msg, **self.failed_result) - responses.append(response) - results.append(result) + self.properties["result"] = result + self.properties["response"] = response - self.properties["response"] = responses - self.properties["result"] = results + for payload in self.payloads: + diff = {} + diff["action"] = self.action + diff["ip_address"] = payload["ipAddr"] + diff["logical_name"] = payload["hostName"] + diff["policy_name"] = payload["policyName"] + diff["serial_number"] = payload["serialNumber"] + self.diff = copy.deepcopy(diff) def _detach_policy(self): """ @@ -232,6 +255,9 @@ def _detach_policy(self): """ method_name = inspect.stack()[0][3] + msg = "ENTERED" + self.log.debug(msg) + self.path = self.endpoints.policy_detach.get("path") self.verb = self.endpoints.policy_detach.get("verb") @@ -253,8 +279,16 @@ def _detach_policy(self): msg += f"from the following device(s): {','.join(sorted(self.serial_numbers))}." self.module.fail_json(msg, **self.failed_result) + for serial_number in self.serial_numbers: + self.switch_issu_details.filter = serial_number + diff = {} + diff["action"] = self.action + diff["ip_address"] = self.switch_issu_details.ip_address + diff["logical_name"] = self.switch_issu_details.device_name + diff["policy_name"] = self.policy_name + diff["serial_number"] = serial_number + self.diff = copy.deepcopy(diff) self.changed = True - self.diff = self.response def _query_policy(self): """ @@ -278,6 +312,20 @@ def _query_policy(self): self.module.fail_json(msg, **self.failed_result) self.properties["query_result"] = self.response.get("DATA") + self.diff = self.response + + @property + def diff_null(self): + """ + Convenience property to return a null diff when no action is taken. + """ + diff: Dict[str, Any] = {} + diff["action"] = None + diff["ip_address"] = None + diff["logical_name"] = None + diff["policy"] = None + diff["serial_number"] = None + return diff @property def query_result(self): diff --git a/plugins/modules/dcnm_image_upgrade.py b/plugins/modules/dcnm_image_upgrade.py index 33f0cfae0..b0130e95a 100644 --- a/plugins/modules/dcnm_image_upgrade.py +++ b/plugins/modules/dcnm_image_upgrade.py @@ -414,8 +414,6 @@ ParamsMergeDefaults from ansible_collections.cisco.dcnm.plugins.module_utils.common.params_validate import \ ParamsValidate -from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.image_upgrade_task_result import \ - ImageUpgradeTaskResult from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.api_endpoints import \ ApiEndpoints from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.image_policies import \ @@ -428,6 +426,8 @@ ImageUpgrade from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.image_upgrade_common import \ ImageUpgradeCommon +from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.image_upgrade_task_result import \ + ImageUpgradeTaskResult from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.image_validate import \ ImageValidate from ansible_collections.cisco.dcnm.plugins.module_utils.image_mgmt.install_options import \ @@ -469,9 +469,6 @@ def __init__(self, module): self.path = None self.verb = None - # populated in self._build_policy_attach_payload() - self.payloads = [] - self.config = module.params.get("config", {}) if not isinstance(self.config, dict): @@ -963,104 +960,65 @@ def _validate_switch_configs(self) -> None: validator.parameters = switch validator.commit() - def _build_policy_attach_payload(self) -> None: + def _attach_or_detach_image_policy(self, action=None) -> None: """ - Build the payload for the policy attach request - Verify that the image policy exists on the controller - Verify that the image policy supports the switch platform + Attach or detach image policies to/from switches + action valid values: attach, detach - Callers: + Caller: - self.handle_merged_state + - self.handle_deleted_state + + NOTES: + - Sanity checking for action is done in ImagePolicyAction """ - method_name = inspect.stack()[0][3] + msg = f"ENTERED: action: {action}" + self.log.debug(msg) - self.payloads = [] - self.attach_devices = [] + serial_numbers_to_update: Dict[str, Any] = {} self.switch_details.refresh() self.image_policies.refresh() - for switch in self.need: - if switch.get("policy_changed") is False: - continue + for switch in self.need: self.switch_details.ip_address = switch.get("ip_address") self.image_policies.policy_name = switch.get("policy") + # ImagePolicyAction wants a policy name and a list of serial_number + # Build dictionary, serial_numbers_to_udate, keyed on policy name + # whose value is the list of serial numbers to attach/detach. + if self.image_policies.name not in serial_numbers_to_update: + serial_numbers_to_update[self.image_policies.policy_name] = [] - # Fail if the image policy does not exist. - # Image policy creation is handled by a different module. - if self.image_policies.name is None: - msg = f"{self.class_name}.{method_name}: " - msg += f"policy {switch.get('policy')} does not exist on " - msg += "the controller" - self.module.fail_json(msg) - - # Fail if the image policy does not support the switch platform - if self.switch_details.platform not in self.image_policies.platform: - msg = f"{self.class_name}.{method_name}: " - msg += f"policy {switch.get('policy')} does not support " - msg += f"platform {self.switch_details.platform}. " - msg += f"Policy {switch.get('policy')} " - msg += "supports the following platform(s): " - msg += f"{self.image_policies.platform}" - self.module.fail_json(msg) + serial_numbers_to_update[self.image_policies.policy_name].append( + self.switch_details.serial_number + ) - payload: Dict[str, Any] = {} - payload["policyName"] = self.image_policies.name - # switch_details.host_name is always None in 12.1.2e - # so we're using logical_name instead - payload["hostName"] = self.switch_details.logical_name - payload["ipAddr"] = self.switch_details.ip_address - payload["platform"] = self.switch_details.platform - payload["serialNumber"] = self.switch_details.serial_number - - attach_device: Dict[str, Any] = {} - attach_device["action"] = "attach" - attach_device["logical_name"] = self.switch_details.logical_name - attach_device["ip_address"] = self.switch_details.ip_address - attach_device["serial_number"] = self.switch_details.serial_number - attach_device["policy"] = self.image_policies.name - - for key, value in payload.items(): - if value is None: - msg = f"{self.class_name}.{method_name}: " - msg += f"Unable to determine {key} for switch " - msg += f"{switch.get('ip_address')}. " - msg += "Please verify that the switch is managed by " - msg += "the controller." - self.module.fail_json(msg) - - self.payloads.append(copy.deepcopy(payload)) - self.attach_devices.append(copy.deepcopy(attach_device)) - - def _send_policy_attach_payload(self) -> None: - """ - Send the policy attach payload to the controller - Handle the response - Set the result and diff + instance = ImagePolicyAction(self.module) + if len(serial_numbers_to_update) == 0: + msg = f"No policies to {action}" + self.log.debug(msg) - Callers: - - self.handle_merged_state - """ - if len(self.payloads) == 0: - # TODO: set a proper result and diff here! + if action == "attach": + self.result.diff_attach_policy = instance.diff_null + elif action == "detach": + self.result.diff_detach_policy = instance.diff_null return - self.path = self.endpoints.policy_attach.get("path") - self.verb = self.endpoints.policy_attach.get("verb") + for key, value in serial_numbers_to_update.items(): + instance.policy_name = key + instance.action = action + instance.serial_numbers = value + instance.commit() + self.result.response_attach_policy = copy.deepcopy(instance.response) - payload: Dict[str, Any] = {} - payload["mappingList"] = self.payloads - response = dcnm_send( - self.module, self.verb, self.path, data=json.dumps(payload) - ) - result = self._handle_response(response, self.verb) - self.result.response_attach_policy = copy.deepcopy(response) - for attach_device in self.attach_devices: - msg = f"attach_device: {json.dumps(attach_device, indent=4, sort_keys=True)}" + for diff in instance.diff: + msg = ( + f"{instance.action} diff: {json.dumps(diff, indent=4, sort_keys=True)}" + ) self.log.debug(msg) - self.result.diff_attach_policy = copy.deepcopy(attach_device) - - if not result["success"]: - self._failure(self.result.response_attach_policy) + if action == "attach": + self.result.diff_attach_policy = copy.deepcopy(diff) + elif action == "detach": + self.result.diff_detach_policy = copy.deepcopy(diff) def _stage_images(self, serial_numbers) -> None: """ @@ -1096,6 +1054,7 @@ def _validate_images(self, serial_numbers) -> None: instance.commit() for diff in instance.diff: self.result.diff_validate = copy.deepcopy(diff) + instance.response.pop("DATA", None) self.result.response_validate = instance.response def _verify_install_options(self, devices) -> None: @@ -1243,8 +1202,10 @@ def handle_merged_state(self) -> None: Caller: main() """ - self._build_policy_attach_payload() - self._send_policy_attach_payload() + msg = "ENTERED" + self.log.debug(msg) + + self._attach_or_detach_image_policy(action="attach") stage_devices: List[str] = [] validate_devices: List[str] = [] @@ -1285,53 +1246,10 @@ def handle_deleted_state(self) -> None: Caller: main() """ - detach_policy_devices: Dict[str, Any] = {} - detach_policy_serial_numbers: Dict[str, Any] = {} - self.switch_details.refresh() - self.image_policies.refresh() - - for switch in self.need: - self.switch_details.ip_address = switch.get("ip_address") - self.image_policies.policy_name = switch.get("policy") - device = {} - device["serial_number"] = self.switch_details.serial_number - device["logical_name"] = self.switch_details.logical_name - device["ip_address"] = self.switch_details.ip_address - device["policy"] = self.image_policies.policy_name - - if self.image_policies.name not in detach_policy_devices: - detach_policy_devices[self.image_policies.policy_name] = [] - if self.image_policies.name not in detach_policy_serial_numbers: - detach_policy_serial_numbers[self.image_policies.policy_name] = [] - - detach_policy_serial_numbers[self.image_policies.policy_name].append( - self.switch_details.serial_number - ) - detach_policy_devices[self.image_policies.policy_name].append(device) - - if len(detach_policy_devices) == 0: - # TODO: Need to build a proper response here. - return - - instance = ImagePolicyAction(self.module) - for key, value in detach_policy_serial_numbers.items(): - detach_diff = {} - instance.policy_name = key - instance.action = "detach" - instance.serial_numbers = value - instance.commit() - self.result.response_detach_policy = copy.deepcopy(instance.response) - for device in detach_policy_devices[key]: - detach_diff["action"] = "detach" - detach_diff["ip_address"] = device.get("ip_address") - detach_diff["logical_name"] = device.get("logical_name") - detach_diff["policy"] = key - detach_diff["serial_number"] = device.get("serial_number") - - msg = f"item: {json.dumps(detach_diff, indent=4, sort_keys=True)}" - self.log.debug(msg) + msg = "ENTERED" + self.log.debug(msg) - self.result.diff_detach_policy = copy.deepcopy(detach_diff) + self._attach_or_detach_image_policy("detach") def handle_query_state(self) -> None: """ @@ -1353,25 +1271,10 @@ def handle_query_state(self) -> None: def _failure(self, resp) -> None: """ Caller: self.attach_policies() - - This came from dcnm_inventory.py, but doesn't seem to be correct - for the case where resp["DATA"] does not exist? - - If resp["DATA"] does not exist, the contents of the - if block don't seem to actually do anything: - - data will be None - - Hence, data.get("stackTrace") will also be None - - Hence, data.update() and res.update() are never executed - - So, the only two lines that will actually ever be executed are - the happy path: - - res = copy.deepcopy(resp) - self.module.fail_json(msg=res) """ res = copy.deepcopy(resp) - if not resp.get("DATA"): + if resp.get("DATA"): data = copy.deepcopy(resp.get("DATA")) if data.get("stackTrace"): data.update( diff --git a/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_upgrade_task.py b/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_upgrade_task.py index 88e988ff1..c9eb6096d 100644 --- a/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_upgrade_task.py +++ b/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_upgrade_task.py @@ -84,7 +84,6 @@ def test_image_mgmt_upgrade_task_00001(image_upgrade_task_bare) -> None: assert instance.switch_configs == [] assert instance.path is None assert instance.verb is None - assert instance.payloads == [] assert instance.config == {"switches": [{"ip_address": "172.22.150.105"}]} assert instance.check_mode is False assert instance.validated == {} diff --git a/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_validate.py b/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_validate.py index 2e8a20224..54f80d2e2 100644 --- a/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_validate.py +++ b/tests/unit/modules/dcnm/dcnm_image_upgrade/test_image_upgrade_image_validate.py @@ -494,8 +494,8 @@ def test_image_mgmt_validate_00022(image_validate) -> None: instance = image_validate instance.serial_numbers = [] instance.commit() - assert instance.response == {'response': 'No serial numbers to validate.'} - assert instance.result == {'success': True} + assert instance.response == {"response": "No serial numbers to validate."} + assert instance.result == {"success": True} def test_image_mgmt_validate_00023(monkeypatch, image_validate) -> None: