-
Notifications
You must be signed in to change notification settings - Fork 47
Added WRED with affected Leaf/LC/FM model check #379
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
base: v4.1.0-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6293,6 +6293,77 @@ def multipod_modular_spine_bootscript_check(tversion, fabric_nodes, username, pa | |
| return Result(result=result, headers=headers, data=data, recommended_action=recommended_action, doc_url=doc_url) | ||
|
|
||
|
|
||
| @check_wrapper(check_title='WRED with Affected FM Models') | ||
| def wred_affected_model_check(tversion, fabric_nodes, **kwargs): | ||
| result = PASS | ||
| headers = ["Node ID", "Node Name", "Source", "Model"] | ||
| data = [] | ||
| recommended_action = 'Disable WRED on the affected nodes or move to a release newer than 6.1(5e) in the 6.1(x) train or newer than 6.2(1g) in the 6.2(x) train.' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls replace "move" with "upgrade" |
||
| doc_url = 'https://datacenter.github.io/ACI-Pre-Upgrade-Validation-Script/validations/#wred-with-affected-fm-models' | ||
|
|
||
| if not tversion: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to add tversion missing check, if tversion is not given script will prompt for tversion to provide the input.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is consistent with the existing pattern used across the codebase. It handles the debug mode case where a user may run a single check without providing a target version, and the check needs to handle that gracefully instead of throwing an exception. Keeping it for consistency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though the pattern is consistent across the script, that would be old code. As I cited earlier, when the tversion is not provided as an input, script will prompt to provide the input, there won't be any exception. This change needs to be incorporated across the script. Pls address it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This version check can be removed. tversion is not a optional |
||
| return Result(result=MANUAL, msg=TVER_MISSING) | ||
|
|
||
| version_affected = ( | ||
| (tversion.major1 == '6' and tversion.major2 == '1' and (tversion.older_than('6.1(5e)') or tversion.same_as('6.1(5e)'))) | ||
| or (tversion.major1 == '6' and tversion.major2 == '2' and (tversion.older_than('6.2(1g)') or tversion.same_as('6.2(1g)'))) | ||
| ) | ||
| if not version_affected: | ||
| return Result(result=NA, msg=VER_NOT_AFFECTED) | ||
|
|
||
| affected_models = { | ||
| 'N9K-C9504-FM-E', | ||
| 'N9K-C9508-FM-E', | ||
| 'N9K-C9516-FM-E', | ||
| } | ||
|
|
||
| node_name_map = {} | ||
| for node in fabric_nodes: | ||
| node_id = node['fabricNode']['attributes']['id'] | ||
| node_name_map[node_id] = node['fabricNode']['attributes']['name'] | ||
|
|
||
| impacted = set() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls use generic variable names as per the structure of the script.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. Replaced generic variable names to match the script's conventions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls update variable name "impacted" with "affected_nodes". |
||
|
|
||
| # FM model gate | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you use copilot to align this code as per structure and styling of whole script. |
||
| for obj in icurl('class', 'eqptFC.json'): | ||
| model = obj['eqptFC']['attributes']['model'] | ||
| if model not in affected_models: | ||
| continue | ||
| dn = obj['eqptFC']['attributes']['dn'] | ||
| dn_match = re.search(node_regex, dn) | ||
| if not dn_match: | ||
| continue | ||
| node_id = dn_match.group('node') | ||
| impacted.add((node_id, node_name_map.get(node_id, ''), 'FM', model)) | ||
|
|
||
| if not impacted: | ||
| return Result(result=PASS, msg='No affected hardware models found. Skipping.') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls check the result. I think it should be NA, as this issue is specific to the model after version check. Result will be PASS only if there is affected model and wred is disabled. |
||
|
|
||
| qosCong = icurl('class', 'qosCong.json') | ||
| wred_enabled = False | ||
| for cong in qosCong: | ||
| algo = cong.get('qosCong', {}).get('attributes', {}).get('algo', '') | ||
| if algo == 'wred': | ||
| wred_enabled = True | ||
| break | ||
|
|
||
| if not wred_enabled: | ||
| return Result(result=PASS, msg='WRED not enabled. Skipping.') | ||
|
|
||
| def sort_key(row): | ||
| node_id = row[0] | ||
| try: | ||
| node_key = int(node_id) | ||
| except (TypeError, ValueError): | ||
| node_key = node_id | ||
| return (node_key, row[2], row[3]) | ||
|
|
||
| data = [list(row) for row in sorted(impacted, key=sort_key)] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need this sort operation and use data list alone instead use impacted and data both. |
||
| result = FAIL_O | ||
|
|
||
| return Result(result=result, headers=headers, data=data, recommended_action=recommended_action, doc_url=doc_url) | ||
|
|
||
|
|
||
| # ---- Script Execution ---- | ||
|
|
||
|
|
||
|
|
@@ -6474,6 +6545,7 @@ class CheckManager: | |
| # Bugs | ||
| observer_db_size_check, | ||
| multipod_modular_spine_bootscript_check, | ||
| wred_affected_model_check, | ||
| ] | ||
| cli_checks = [ | ||
| # General | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,7 @@ Items | Defect | This Script | |
| [Rogue EP Exception List missing on switches][d30] | CSCwp64296 | :white_check_mark: | :no_entry_sign: | ||
| [N9K-C9408 with more than 5 N9K-X9400-16W LEMs][d31] | CSCws82819 | :white_check_mark: | :no_entry_sign: | ||
| [Multi-Pod Modular Spine Bootscript File][d32] | CSCwr66848 | :white_check_mark: | :no_entry_sign: | ||
| [WRED with Affected FM Models][d33] | CSCwt50713 | :white_check_mark: | :no_entry_sign: | ||
|
|
||
| [d1]: #ep-announce-compatibility | ||
| [d2]: #eventmgr-db-size-defect-susceptibility | ||
|
|
@@ -231,6 +232,7 @@ Items | Defect | This Script | |
| [d30]: #rogue-ep-exception-list-missing-on-switches | ||
| [d31]: #n9k-c9408-with-more-than-5-n9k-x9400-16w-lems | ||
| [d32]: #multi-pod-modular-spine-bootscript-file | ||
| [d33]: #wred-with-affected-fm-models | ||
|
|
||
| ## General Check Details | ||
|
|
||
|
|
@@ -2753,6 +2755,17 @@ This issue happens only when the target version is specifically 6.1(4h). | |
| To avoid this issue, change the target version to another version. Or verify that the `bootscript` file exists in the bootflash of each modular spine switch prior to upgrading to 6.1(4h). If the file is missing, you have to do clean reboot on the impacted spine to ensure that `/bootflash/bootscript` gets created again. In case you already upgraded your spine and you are experiencing the traffic impact due to this issue, clean reboot of the spine will restore the traffic. | ||
|
|
||
|
|
||
| ### WRED with Affected FM Models | ||
|
|
||
| Due to [CSCwt50713][67], when WRED (Weighted Random Early Detection) is enabled and specific Fabric Module (FM) hardware models are present in the fabric, the spine switch may crash after moving to an affected ACI release in the 6.1(x) or 6.2(x) range. | ||
|
|
||
| Affected versions: ACI 6.1(x) up to and including 6.1(5e), and ACI 6.2(x) up to and including 6.2(1g). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct the statement . Impacted aci version 6.1(5e) and below, and 6.2(1g). |
||
|
|
||
| Affected hardware models: N9K-C9504-FM-E, N9K-C9508-FM-E, N9K-C9516-FM-E. | ||
|
|
||
| To avoid this issue, disable WRED on the affected nodes or move to a release newer than 6.1(5e) in the 6.1(x) train or newer than 6.2(1g) in the 6.2(x) train. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls replace "move" with "upgrade". |
||
|
|
||
|
|
||
| [0]: https://github.com/datacenter/ACI-Pre-Upgrade-Validation-Script | ||
| [1]: https://www.cisco.com/c/dam/en/us/td/docs/Website/datacenter/apicmatrix/index.html | ||
| [2]: https://www.cisco.com/c/en/us/support/switches/nexus-9000-series-switches/products-release-notes-list.html | ||
|
|
@@ -2820,3 +2833,4 @@ To avoid this issue, change the target version to another version. Or verify tha | |
| [64]: https://bst.cloudapps.cisco.com/bugsearch/bug/CSCwp64296 | ||
| [65]: https://bst.cloudapps.cisco.com/bugsearch/bug/CSCws82819 | ||
| [66]: https://bst.cloudapps.cisco.com/bugsearch/bug/CSCwr66848 | ||
| [67]: https://bst.cloudapps.cisco.com/bugsearch/bug/CSCwt50713 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| [ | ||
| { | ||
| "eqptFC": { | ||
| "attributes": { | ||
| "dn": "topology/pod-1/node-1001/sys/ch/fcslot-1/fc", | ||
| "model": "N9K-C9508-FM-E" | ||
| } | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| [] |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls remove this file as we're focusing only on FC models. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| [ | ||
| { | ||
| "eqptLC": { | ||
| "attributes": { | ||
| "dn": "topology/pod-1/node-1001/sys/ch/lcslot-1/lc", | ||
| "model": "N9K-C92304QC" | ||
| } | ||
| } | ||
| } | ||
| ] |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls remove this file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| [] |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls remove this file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| [ | ||
| { | ||
| "fabricNode": { | ||
| "attributes": { | ||
| "dn": "topology/pod-1/node-101", | ||
| "id": "101", | ||
| "name": "leaf101", | ||
| "role": "leaf", | ||
| "model": "N9K-C9236C" | ||
| } | ||
| } | ||
| } | ||
| ] |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need spine model check json file here, if we are focusing only on FM model check. Do we have any model compatibility like FM models N9K-C9504,9508,9516 will be supported only on spines with model N9K-C9504/08/16? Pls check and confirm. If yes, then we should add the model check from fabricNode moquery at the beginning itself, instead of checking the FM models. If the model is matching with "'N9K-C9504'|'N9K-C9508'|'N9K-C9516'" then you should go for next check for FM. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| [ | ||
| { | ||
| "fabricNode": { | ||
| "attributes": { | ||
| "dn": "topology/pod-1/node-1001", | ||
| "id": "1001", | ||
| "name": "spine1001", | ||
| "role": "spine", | ||
| "model": "N9K-C9504" | ||
| } | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| [ | ||
| { | ||
| "qosCong": { | ||
| "attributes": { | ||
| "algo": "tail-drop" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "qosCong": { | ||
| "attributes": { | ||
| "algo": "wred" | ||
| } | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| [ | ||
| { | ||
| "qosCong": { | ||
| "attributes": { | ||
| "algo": "tail-drop" | ||
| } | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| [ | ||
| { | ||
| "qosCong": { | ||
| "attributes": { | ||
| "algo": "wred" | ||
| } | ||
| } | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| import os | ||
| import pytest | ||
| import importlib | ||
| from helpers.utils import read_data | ||
|
|
||
| script = importlib.import_module("aci-preupgrade-validation-script") | ||
|
|
||
| dir = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
||
| test_function = "wred_affected_model_check" | ||
|
|
||
| # icurl queries | ||
| qosCong_api = "qosCong.json" | ||
| eqptFC_api = "eqptFC.json" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "tversion, fabric_nodes, icurl_outputs, expected_result, expected_data", | ||
| [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls add the comments for each test cases to understand what test case is doing, then will review.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. Added comments to all the test cases. |
||
| # Case 1: No target version provided (-t flag missing). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove this case, as it's not required. Script will prompt to provide the tversion when input is not provided. |
||
| # Check cannot determine version gate. Expected: MANUAL CHECK REQUIRED. | ||
| ( | ||
| None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls add the json files and read the json files for each test case and provide the test result accordingly instead of hard-coding here. Pls follow the existing structure.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. Replaced all hardcoded data with JSON fixture files |
||
| read_data(dir, "fabricNode_leaf_affected.json"), | ||
| {}, | ||
| script.MANUAL, | ||
| [], | ||
| ), | ||
| # Case 2: Target version 6.2(2a) is the first fixed release and not in the affected range. | ||
| # Version gate fails. Expected: NA without any API calls. | ||
| ( | ||
| "6.2(2a)", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about the fixed version exactly it will be 6.2(2a). Need to check with developer on this and update accordingly. |
||
| read_data(dir, "fabricNode_leaf_affected.json"), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the json file accordingly. |
||
| {}, | ||
| script.NA, | ||
| [], | ||
| ), | ||
| # Case 3: All 3 gates triggered via an affected FM on a spine node. | ||
| # Version 6.2(1f) is in affected range, WRED is enabled, FM model N9K-C9508-FM-E is affected. | ||
| # Expected: FAIL_O with node 1001 reported under Source=FM. | ||
| ( | ||
| "6.2(1f)", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls update the version with 6.2(1g). 6.2(1f) is unavailable. |
||
| read_data(dir, "fabricNode_spine.json"), | ||
| { | ||
| qosCong_api: read_data(dir, "qosCong_wred.json"), | ||
| eqptFC_api: read_data(dir, "eqptFC_affected.json"), | ||
| }, | ||
| script.FAIL_O, | ||
| [["1001", "spine1001", "FM", "N9K-C9508-FM-E"]], | ||
| ), | ||
| # Case 4: Version is affected but no affected FM hardware found. | ||
| # Hardware gate fails before WRED is checked. Expected: PASS. | ||
| ( | ||
| "6.1(5e)", | ||
| read_data(dir, "fabricNode_spine.json"), | ||
| { | ||
| eqptFC_api: read_data(dir, "eqptFC_empty.json"), | ||
| }, | ||
| script.PASS, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls update the test result as NA. Even though version is affected but the model is unaffected, since this issue is specific to the model. |
||
| [], | ||
| ), | ||
| # Case 5: Version is affected and FM is affected, but WRED is not enabled (tail-drop). | ||
| # WRED gate fails. Expected: PASS - confirms all 3 gates must be true simultaneously. | ||
| ( | ||
| "6.1(5e)", | ||
| read_data(dir, "fabricNode_spine.json"), | ||
| { | ||
| qosCong_api: read_data(dir, "qosCong_tail_drop.json"), | ||
| eqptFC_api: read_data(dir, "eqptFC_affected.json"), | ||
| }, | ||
| script.PASS, | ||
| [], | ||
| ), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls add the test cases for mixed scenarios, if there are multiple objects with one affected model and others unaffected with wred enabled and disabled combinations. |
||
| ], | ||
| ) | ||
| def test_logic(run_check, mock_icurl, tversion, fabric_nodes, expected_result, expected_data): | ||
| result = run_check( | ||
| tversion=script.AciVersion(tversion) if tversion else None, | ||
| fabric_nodes=fabric_nodes, | ||
| ) | ||
| assert result.result == expected_result | ||
| assert result.data == expected_data | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Disable WRED in fabric or upgrade to release > 6.1(5e), 6.2(1g)"