dcnm_vrf fix issues #351, #356, #357

[allenrobel]

Summary

This PR includes:

1. A fix for issue dcnm_vrf: infinite loop in wait_for_vrf_del_ready() #351
2. A fix for issue dcnm_vrf: TOP_DOWN_VRF_VLAN resources are leaked #356
3. A fix for issue dcnm_vrf: 500 error at /rest/top-down/fabrics/f1/vrfs/attachments #357
4. Improve code readability
5. Harden against KeyErrors and misinterpreted booleans
6. Add logging
7. Standardize IT vars and task titles for dcnm_vrf integration tests
8. Add example dynamic inventory to playbooks/files/dynamic_inventory.py

Status

1. UT - seven unit tests are (incorrectly) failing with the fix for dcnm_vrf: 500 error at /rest/top-down/fabrics/f1/vrfs/attachments #357. Need to modify these unit tests.
2. IT - all tests pass (deleted, merged, overridden, replaced, query, scale).
3. NOTE: There are other tests in the dcnm_vrf IT directory (sanity.yaml, and a directory called "self-contained-tests". I'm not sure what these are. Do we need them?

Fix for issue #351

This involves a change to wait_for_vrf_del_ready() where we call fail_json() if both lanAttachedState == "DEPLOYED" and isLanAttached is True.

                            if (
                                atch["lanAttachState"] == "DEPLOYED"
                                and atch["isLanAttached"] is True
                            ):
                                vrf_name = atch.get("vrfName", "unknown")
                                fabric_name = atch.get("fabricName", "unknown")
                                switch_ip = atch.get("ipAddress", "unknown")
                                switch_name = atch.get("switchName", "unknown")
                                vlan_id = atch.get("vlanId", "unknown")
                                msg = f"{self.class_name}.{method_name}: "
                                msg += f"Network attachments associated with vrf {vrf_name} "
                                msg += "must be removed (e.g. using the dcnm_network module) "
                                msg += "prior to deleting the vrf. "
                                msg += f"Details: fabric_name: {fabric_name}, "
                                msg += f"vrf_name: {vrf_name}. "
                                msg += "Network attachments found on "
                                msg += f"switch_ip: {switch_ip}, "
                                msg += f"switch_name: {switch_name}, "
                                msg += f"vlan_id: {vlan_id}"
                                self.module.fail_json(msg=msg)

fix for issue #356

Added two methods:

• release_orphaned_resources()
• release_resources_by_id()

release_orphaned_resources() creates a list of resource IDs which are "orphaned". In this context, "orphaned" means that the resource's allocatedFlag is False and the resourcePool.vrfName field is null. It then checks that the resource's entityName matches the current VRF name, and that the ressourcePool.fabricName matches the current fabric name. If all of this is True, then it adds the resource's id to a list of IDs to be deleted and passes that list to release_resources_by_id() which releases the resource IDs in the list.

In push_to_remote(), a call to release_orphaned_resources() is then made for every VRF that was previously deleted in push_diff_delete().

fix for issue #357

Previously, fail_json() was called only if the switch did not contain any interfaces with extensionValues. If an interface in the playbook contains a vrf_lite config, and the interface in this vrf_lite config does not match an interface on the switch with extensionValues, but at least one interface on the switch DOES have extensionValues, the generated payload for the interface in the vrf_lite config will be incomplete, which generates the 500 error.

The fix is to call fail_json() if any interface in the playbook's vrf_lite config does not match an interface with extensionValues on the switch. This new check causes several unit-tests to fail, since the unit-tests meet all the conditions for fail_json() to be called with this new check in place. These unit-tests have been modiified so that the interface in the vrf_lite config will match one of the interfaces in the test case fixture that containsextensionValues.

Improve code readability

Refactored a few bits of code that were duplicated in multiple places into the following methods

• compare_properties()
• find_dict_in_list_by_key_value()
• to_bool()
• dict_values_differ()
• get_vrf_objects()
• get_vrf_lite_objects()
• get_items_to_detach()

Reversed the logic of several conditionals to reduce indentation.

Examples can be found in:

• get_have()
• push_to_remote()

Populate dicts before appending them to list

Previously, dicts were appended to a list (within another dict) and then populated. This resulted in code that was harder to read due to the multi-level accesses made when setting each key/value in the embedded dict. This was changed in a couple places so that we first populate the dict, and then append the dict to the list afterwards.

Examples in:

• get_have()
• update_attach_params()

Use in to simplify or conditionals

For example, changed:

if state == "merged" or state == "overridden" or state == "replaced":

To:

if state in ("merged", "overridden", "replaced"):

Rename vars whose names were hard to understand

In some cases, the name was hard to understand, and didn't add any value. In these cases, used a generic name like item instead:

a_l renamed to item

In other cases, the name was ambiguous:

dep_vrf seemed to me to mean "dependent vrf", but it actually means "deploy vrf", so renamed it to deploy_vrf. Several other cases along these lines.

Harden against KeyErrors and misinterpreted booleans

Removed all bool() casts

In multiple places bool(thing) was used to try to cast thing into a boolean. This can (potentially) result in misinterpretation of thing. For example:

bool("false") returns True.

Removed all cases of bool() casts since debugging these showed that, in all of them, the thing being casted was already a boolean. Hence, in these cases, there was no misinterpretation, but casting to bool should be considered bad practice in most circumstances.

Harden against KeyError

Several places were accessing dict keys directly without using dict.get(). Changed those that I noticed.

Add logging

Standard python logging was added (similarly to e.g. dcnm_fabric, etc).

• self.class_name was added to init() and used in all logged messages.
• inspect is used to add the method name to all logged messages and to messages passed to fail_json().
• Added logging of data structure contents in many places to help with future refactoring.

One issue that adding logging raised is that unit tests failed until the following was changed in main():

From:

    # Logging setup
    try:
        log = Log()
        log.commit()
    except (TypeError, ValueError) as error:
        module.fail_json(str(error))

To:

    # Logging setup
    try:
        log = Log()
        log.commit()
    except (TypeError, ValueError) as error:
        pass

The issue seems to be that the dcnm_vrf unit tests are run like this:

result = self.execute_module(changed=True, failed=False)

And somehow (which I don't understand) this is causing the following error in logging.config.DictConfigurator (part of the standard Python logging system), but ONLY when unit tests for log_v2.py are run in the same session as unit tests for dcnm_vrf.py, i.e.:

The following results in the error:

pytest -k "test_dcnm_vrf or test_log"

Whereas the following does not:

pytest -k test_log
pytest -k test_dcnm_vrf

                for name in sorted(handlers):
                    try:
                        handler = self.configure_handler(handlers[name])
                        handler.name = name
                        handlers[name] = handler
                    except Exception as e:
                        if ' not configured yet' in str(e.__cause__):
                            deferred.append(name)
                        else:
>                           raise ValueError('Unable to configure handler '
                                             '%r' % name) from e
E                                            ValueError: Unable to configure handler 'file'

Which is called from common/log_v2.py, in enable_logging() here:

        try:
            dictConfig(logging_config)
        except (RuntimeError, TypeError, ValueError) as error:
            msg = "logging.config.dictConfig: "
            msg += f"Unable to configure logging from {self.config}. "
            msg += f"Error detail: {error}"
>           raise ValueError(msg) from error
E           ValueError: logging.config.dictConfig: Unable to configure logging from /private/var/folders/xc/1kd3k6xx1vlgw_pfqrckmqsm0000gn/T/pytest-of-arobel/pytest-35/test_log_v2_002500/log_dir/logging_config.json. Error detail: Unable to configure handler 'file'

So, maybe something having to do with the use of the temporary path /private/var/..., or permissions on this path when both tests are run, or the path's length?

Will investigate further, but the "fix" (ugly as it is) above works for both the unit tests and for normal usage.

[mikewiebe]

Is the idea to use this dynamic_inventory file (eventually) for all of the integration tests? I suspect we will need to use a common naming convention across all of our tests.

[mikewiebe]

Never mind the question about how to run it... I see the instructions in the diffs below.

[mikewiebe]

Are you adding more information to our README.md? I don't see any updates to it

[allenrobel]

Oops. This is a holdover from when this was a separate repository. I'll modify to remove this reference.

EDIT: Added detailed usage to the Usage section of dynamic_inventory.py

[mikewiebe]

Thanks for adding this!

[mikewiebe]

Is this the full set of vars that are required by the vrf integration tests?

[allenrobel]

Good catch! I've updated it to include all the vars.