Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swim bug fixed #437

Conversation

syed-khadeerahmed
Copy link

@syed-khadeerahmed syed-khadeerahmed commented Oct 17, 2024

Description

Swim module incorrectly upgrades many devices even if one IP is provided

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • All the sanity checks have been completed and the sanity test cases have been executed

Ansible Best Practices

  • Tasks are idempotent (can be run multiple times without changing state)
  • Variables and secrets are handled securely (e.g., using ansible-vault or environment variables)
  • Playbooks are modular and reusable
  • Handlers are used for actions that need to run on change

Documentation

  • All options and parameters are documented clearly.
  • Examples are provided and tested.
  • Notes and limitations are clearly stated.

Screenshots (if applicable)

Notes to Reviewers

params=params,
)
self.log("Received API response from 'get_device_list': {0}".format(str(response)), "DEBUG")
self.log(params)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.log("Fetching device ID with parameters: {0}".format(params), "INFO")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the log

else:
self.msg = "Device with params: '{0}' not found in Cisco Catalyst Center so can't fetch the device id".format(str(params))
self.log(self.msg, "WARNING")
if (len(device_list) == 1):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  if device_list is None:
        self.log("Device list is empty; no devices found for given parameters.", "WARNING")
        raise ValueError("No devices found")

    if len(device_list) == 1:
        device_id = device_list[0].get("id")
        self.log("Successfully retrieved device ID: {0}".format(device_id), "INFO")
        return device_id
    
    self.log("Multiple devices found for parameters: {0}".format(params), "ERROR")
    raise ValueError("Multiple devices found")
    
    
except ValueError as ve:
    self.msg = "Error: {0}. Unable to fetch unique device ID with parameters: {1}".format(str(ve), params)
    self.status = "failed"
    self.log(self.msg, "ERROR")
    self.result['response'] = self.msg
    return None

except Exception as e:
    self.msg = "An unexpected error occurred while retrieving device ID: {0}".format(str(e))
    self.status = "failed"
    self.log(self.msg, "ERROR")
    self.result['response'] = self.msg
    return None

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

plugins/modules/swim_workflow_manager.py Show resolved Hide resolved
@@ -1703,13 +1715,14 @@ def get_diff_distribution(self):
self.result['changed'] = True
self.status = "success"
self.single_device_distribution = True
self.result['msg'] = "Image with Id {0} Distributed Successfully".format(image_id)
self.result['response'] = self.msg
self.result['msg'] = "Image with Id {0} Distributed Successfully for the device: {1} ".format(image_id, device_ip)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.result['msg'] = "Image '{0}' with ID {1} has been successfully distributed to device: {2}".format(image_name, image_id, device_ip)
OR
self.result['msg'] = "Successfully distributed image '{0}' (ID: {1}) to device: {2}".format(image_name, image_id, device_ip)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

break

if task_details.get("isError"):
self.status = "failed"
self.msg = "Image with Id {0} Distribution Failed".format(image_id)
self.msg = "Image with Id {0} Distribution Failed for the device : {1}".format(image_id, device_ip)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@@ -1730,6 +1743,7 @@ def get_diff_distribution(self):
distribution_task_dict = {}

for device_uuid in device_uuid_list:
self.log("Stating image distribution for multiple devices")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what image?

@@ -1673,8 +1683,10 @@ def get_diff_distribution(self):
self.complete_successful_distribution = False
self.partial_successful_distribution = False
self.single_device_distribution = False
device_ip = self.want.get("distribution_details").get("device_ip_address")

if self.have.get("distribution_device_id"):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print image version or image name ...

distribution_device_id = self.have.get("distribution_device_id")
if distribution_device_id:
    self.log("Starting image distribution for device IP {0} with ID {1}, targeting software version {2}.".format(
        device_ip, distribution_device_id, image_name
    ), "INFO")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

@@ -1799,8 +1813,10 @@ def get_diff_activation(self):
self.complete_successful_activation = False
self.partial_successful_activation = False
self.single_device_activation = False
device_ip = self.want.get("activation_details").get("device_ip_address")

if self.have.get("activation_device_id"):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as like getting distribution_device_id...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

@@ -1861,6 +1877,7 @@ def get_diff_activation(self):
activation_task_dict = {}

for device_uuid in device_uuid_list:
self.log("Stating image activation for multiple devices")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what image ?? Check the above log messages and make more meaningful

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

device_id = self.get_device_id(device_params)

if device_id is not None:
have["distribution_device_id"] = device_id
if (distribution_details.get("device_ip_address") or
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why do we need to get device_ip/mac... again? We already got those values in 1156?
Can we replace the whole logic from 1156?
Also why do we use camel case in 1156? Can we replace?

device_params = {
    "hostname": distribution_details.get("device_hostname"),
    "serial_number": distribution_details.get("device_serial_number"),
    "management_ip_address": distribution_details.get("device_ip_address"),
    "mac_address": distribution_details.get("device_mac_address")
}

if any(device_params.values()):
    device_id = self.get_device_id(device_params)

    if device_id is None:
        # Device not found, format error message
        params_list = []
        for key, value in device_params.items():
            if value:
                formatted_param = "{0}: {1}".format(key, value)
                params_list.append(formatted_param)

        # Join the parameters for the final error message
        params_message = ", ".join(params_list)
        self.status = "failed"
        self.msg = "The device with the following parameter(s): {0} could not be found in the Cisco Catalyst Center.".format(params_message)
        self.log(self.msg, "ERROR")
        self.result['response'] = self.msg
        self.check_return_status()
    else:
        # Device found, log success and store the device ID
        self.log("Device with ID {0} found and added to distribution details.".format(device_id), "DEBUG")
        have["distribution_device_id"] = device_id
else:
    self.log("No valid device identifier found in the distribution details.", "WARNING")
    What we do here ....???? 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code

@@ -207,8 +207,8 @@
type: bool
image_distribution_details:
description: Details for SWIM image distribution. Device on which the image needs to distributed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image_distribution_details:
  description: |
    Parameters for specifying the target device(s) for SWIM image distribution. The device can be identified using one of the following options:
    - device_serial_number
    - device_ip_address
    - device_hostname
    - device_mac_address
    - site_name (if specified, the image will be distributed to all devices within the site)
    
        At least one of these parameters must be provided. If 'site_name' is provided, additional filters—such as 'device_role', 'device_family_name', and 'device_series_name'—can be used to further narrow down the devices within the site.

@@ -256,8 +256,8 @@
type: str
image_activation_details:
description: Details for SWIM image activation. Device on which the image needs to activated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as like distribution

@madhansansel madhansansel merged commit 87a54c1 into madhansansel:main Oct 18, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants