issue NewValidation: CSCwn81692#339
Conversation
Fixes #297 fixed
monrog2
left a comment
monrog2
left a comment
There was a problem hiding this comment.
The new validation has multiple false-PASS paths and can discard outage evidence. Please address the inline findings and add the requested regression coverage before merging.
| node_id = dn.group("node") | ||
| for mo in presListener: | ||
| if ( | ||
| node_id in mo['presListener']['attributes']['lstDn'] and |
There was a problem hiding this comment.
Blocking: This substring check treats leaf 101 as present when the only MO belongs to 1101, producing a false PASS and potentially missing the documented port outage. Parse the terminal node ID or compare lstDn with the exact expected leaf path. Please add a 101/1101 collision test that expects FAIL_O and verifies the reported node.
| if not found: | ||
| result = FAIL_O | ||
| data.append([pod_id, node_id, state]) | ||
| if not fabric_nodes: |
There was a problem hiding this comment.
Blocking: fabric_nodes normally includes controllers and spines, so a non-empty inventory with no active leaves skips every entry but still returns PASS. Track how many active leaves were actually evaluated and return MANUAL when that count is zero. Please cover controller-only and disabled-leaf inventories.
| state = fabric_node['fabricNode']['attributes']['fabricSt'] | ||
| if state != 'active': | ||
| continue | ||
| dn = re.search(node_regex, fabric_node['fabricNode']['attributes']['dn']) |
There was a problem hiding this comment.
High: The regex result is dereferenced without validation. One malformed active-leaf DN raises an exception, changes the whole check to ERROR, and discards any missing-MO evidence already collected. Preserve valid findings and report malformed objects through the existing unformatted-data mechanism; add a mixed valid/malformed regression test.
| doc_url = 'https://datacenter.github.io/ACI-Pre-Upgrade-Validation-Script/validations#missing-preslistener-mo' | ||
| # fabricNode.fabricSt shows `disabled` for both Decommissioned and Maintenance (GIR). | ||
| # fabricRsDecommissionNode.debug==yes is required to show `disabled (Maintenance)`. | ||
| presListener = icurl('class','presListener.json') |
There was a problem hiding this comment.
High: This full-class query runs before the target-version guards. Therefore a missing or unaffected target can become ERROR if collection fails even though the result should be MANUAL or N/A, and every run unnecessarily retrieves all presListener objects. Move the guards before collection and use a server-side filter for the relevant class-4307/node entries. Please test that icurl is not called for missing or unaffected targets.
| if not tversion: | ||
| return Result(result=MANUAL, msg=TVER_MISSING) | ||
|
|
||
| if tversion.newer_than("6.1(3f)"): |
There was a problem hiding this comment.
High: The exact release boundary is not documented or tested: this condition keeps 6.1(3f) in the affected range, while the tests jump from 6.1(3a) to 6.1(4a). Reconcile the threshold with CSCwn81692, document the affected/fixed releases, and add tests immediately below, exactly at, and immediately above the fixed boundary.
2 participants
Fixes #297 fixed