dcnm_fabric (Ready for review) #286

allenrobel · 2024-04-20T18:27:10Z

Opening a PR to run dcmn_fabric through our sanity tests and fix whatever issues these reveal.

This module is working for deleted, merged, and query states.

No additional major changes are expected at this point.

This is a work in progress. merged state works, but it still needs a lot of cleaning up and support for additional fabric parameters. TODO: general cleanup query state unit tests integration tests

No dynamic template capability in this commit.

A Results() class is introduced. The intent is to: 1. Standardize results processing and output 2. Rremove the distraction of results processing from the main logic of a given module. 3. Enable result format changes to be made in a single place, should we ever want to do that.

Add class-level doc strings with Usage examples to the following classes: - FabricCreateBulk - FabricDelete - FabricQuery - FabricUpdateBulk - Results

Previously, results contained an "OK" and "FAILED" key for each of results, response, diff. Results for existing modules do not have this. So, we've changed the results for dcnm_fabric to more closely match the results of existing modules. We've still retained the unique sequence_number for each of result, response, and diff entries so that they can be correlated. Also: Move template_parse_each_fabric.py from module_utils/fabric to module_utils/fabric/vxlan

Change the import for TemplateParseEasyFabric

RestSend and Results are now generic enough to use across all of dcnm_image_upgrade, dcnm_image_policy, and dcnm_fabric modules. Hence, we can use module_utils/common/rest_send.py for dcnm_fabric and delete rest_send_fabric.py/

- Add unit tests for: -- FabricQuery() -- FabricCreate() - Copy the following from dcnm_image_upgrade/plugins/module_utils/common -- rest_send.py -- results.py

- test_fabric_create_bulk.py: Update docstrings for a few tests - update.py: FabricUpdateCommon._send_payloads(), fix reference to Results() property that no longer exists - query.py: run thru black - fabric_summary.py: use a private copy of Results() - fabric_details.py: use a private copy of Results() - delete.py: run thru black - create.py: add type hinting to a few methods - common.py: fix bad f-string interpolation

FabricCreateCommon() and FabricUpdateCommon(): move _fixup_payloads_to_commit() to FabricCommon() and modify it to raise ValueError if ANYCAST_GW_MAC is not convertable. test_fabric_update_bulk.py: Add unit test to verify behavior when ANYCAST_GW_MAC is not convertable. test_fabric_update_bulk.py: Modify unit test 00031 to include ANYCAST_GW_MAC that IS convertable.

FabricCreate().commit(): Leverage FabricCreateCommon()._set_fabric_create_endpoint() FabricCreate().commit(): Set RestSend().check_mode and RestSend().timeout prior to sending the request.

Breakup assert statements into sections for visual consistency across test cases.

FabricUpdateBulk: Send payload only when it would change the controller fabric config. FabricUpdate: Remove this class for now since it's not used.

module_utils/fabric/ruleset.py: TODO: We still need to handle the case where "and" and "or" are present in a rule. This impacts handling of one rule in the Easy_Fabric template. Specifically for ANYCAST_RP_IP_RANGE: Rule: ( STATIC_UNDERLAY_IP_ALLOC == ' False' and UNDERLAY_IS_V6 == ' False' and REPLICATION_MODE == 'Multicast' ) or ( STATIC_UNDERLAY_IP_ALLOC == ' True' and UNDERLAY_IS_V6 == ' False' and REPLICATION_MODE == 'Multicast' and RP_MODE == 'bidir' )

Guard against deploying fabrics with either DEPLOYMENT_FREEZE == True or IS_READ_ONLY == True. Previously, we were collecting fabrics to be config-saved and config-deployed prior to pushing their config changes. And we were not checking DEPLOYMENT_FREEZE or IS_READ_ONLY values. Changes for the above entailed: 1. Don't collect fabrics to be config-deployed prior to pushing their new configs (since the new configs may enable either DEPLOYMENT_FREEZE or IS_READ_ONLY). 2. Instead, we now do the following: a. push the configs for all fabrics that have changes. b. config-save the fabrics c. config-deploy each fabric only after refreshing FabricDetailsByName() and verifying that both DEPLOYMENT_FREEZE and IS_READ_ONLY are False. 3. Update all test cases to reflect the above changes. 4. FabricConfigDeploy(): Add properties for fabric_details and fabric_summary so that all verifications regarding whether a fabric is deployable are done within this class.

We've standardized the following: - _build_properties() only populates an existing properties dict. - _init_properties() initializes a properties dict (and potentially populates it).

1. FabricUpdateBulk(): remove unneeded testcase. Update docstrings. 2. FabricReplacedBulk(): improve _translate_payload_for_comparison() testcase 3. FabricCommon(): rename _prepare_anycast_gw_mac_for_comparison() to translate_anycast_gw_mac() 4. FabricCommon(): add testcase for translate_anycast_gw_mac() 5. FabricReplacedBulk(): use renamed translate_anycast_gw_mac() from 3 above. 6. ParamInfo(): remove debug statement 7. FabricCommon(): remove unused ControllerResponseError import. 8. FabricCommon(): remove unused code. 9. FabricCommon(): relocate _fixup_anycast_gw_mac() closer to where it is used.

… files

Due to issues found while writing integration tests, am modifying behavior of: FabricConfigSave() FabricConfigDeploy() There were two problems with these: 1. They were called regardless of the value of the value of the playbook DEPLOY parameter. 2. They were not granular in their intepretation of "failed" when setting results. To solve 1 above, we now pass the payload to each class, rather than just the fabric_name. If DEPLOY is False (or missing), both classes set result to success and provide a message indicating that config-save, config-deploy were skipped due to the DEPLOY setting. To solve 2 above, we track (within each class) what specifically caused config-save or config-deploy to be skipped. If these were skipped due to errors, we set results accordingly and provide a message describing the failure in the response. All unit tests are updated to reflect the above changes.

Also: - dcnm_fabric_deleted_basic.yaml: Add a DESCRIPTION section - dcnm_fabric_deleted_basic.yaml: Fix dcnm_fabric_name_* numbering in comments.

1. FabricConfigSave().commit(): An earlier commit changed FabricConfigSave() to accept the payload via a payload property. In commit(), the guard conditional was not also changed to test that payload is set. Fixed. 2. 1. FabricConfigSave(): Fix usage in docstring. 3. FabricConfigDeploy(): Fix docstrings. 4. dcnm_fabric.py Merged().commit(): Fix docstring. 5. test_fabric_config_save.py: Update unit tests to reflect change in 1 above.

…into dcnm_fabric # Conflicts: # tests/sanity/ignore-2.13.txt # tests/sanity/ignore-2.14.txt

1. Improved error message explain why the user might have encountered the error. Changed from: "Parameter {value} not found in self.info." To: "Parameter {value} not found in fabric template. This likely means that the parameter {value} is not appropriate for the fabric type." 2. Add a unit test for the above change.

… it. Caught in integration tests. The playbook below would fail, as it should. BUT for the wrong reason. The reason given was: "ParamInfo.parameter: Parameter UNDERLAY_IS_V6 not found in fabric template. This means that either the parameter UNDERLAY_IS_V6, or a parameter that is dependent on UNDERLAY_IS_V6, is not appropriate for the fabric type." The actual reason is that SUBNET_TARGET_MASK is not a valid parameter for LAN_CLASSIC fabric. The reason that the message is wrong is because RuleSet() was not resetting self.ruleset when refresh() was called. For the scenario below, this resulted in the following: 1. RuleSet() was updated for VXLAN_EVPN fabric template and the VXLAN_EVPN params were validated. 2. RuleSet() was updated for LAN_CLASSIC fabric template (but not initialized/result, so it still contained the keys from VXLAN_EVPN fabric, which include SUBNET_TARGET_MASK). As a result of 2, SUBNET_TARGET_MASK parameter in the LAN_CLASSIC fabric playbook config was matched against the remnents of the VXLAN_EVPN template, which resulted in the above misleading error. The fix is to reset the ruleset in RuleSet().refresh() prior to updating it with the new template. We now get the correct error for the playbook below, which is: "FabricUpdateBulk._fabric_needs_update_for_merged_state: Invalid key: SUBNET_TARGET_MASK found in payload for fabric LAN_CLASSIC_Fabric" tasks: - cisco.dcnm.dcnm_fabric: state: merged config: - FABRIC_NAME: VXLAN_EVPN_Fabric FABRIC_TYPE: VXLAN_EVPN BGP_AS: 65535 DEPLOY: false - FABRIC_NAME: LAN_CLASSIC_Fabric FABRIC_TYPE: LAN_CLASSIC IS_READ_ONLY: false SUBNET_TARGET_MASK: 31 BOOTSTRAP_ENABLE: false DEPLOY: false Also, cleaned up pylint line-too-long in debug message.

Make the code a bit easier to read by initializing fabric_name and fabric_type vars at the top of the method, rather than pulling these out of the want dict within the method's code.

Since RestSend() displays the result, no need to duplicate that in FabricDetails().

dcnm_fabric_name_* -> fabric_name_* dcnm_fabric_type_* -> fabric_type_*

mikewiebe · 2024-04-25T14:29:46Z