Added APIC OOB connectivity checks for CSCwu91693 - #418
Added APIC OOB connectivity checks for CSCwu91693#418Priyanka-Patil14 wants to merge 10 commits into
Conversation
asraf-khan
left a comment
asraf-khan
left a comment
There was a problem hiding this comment.
Make it as single function check for 2 different validation with help of helper inside same function.
Done. Merged both validations into a single check function |
|
|
||
| ### APIC OOB Connectivity | ||
|
|
||
| Due to [CSCwu91693][77], when an APIC cluster upgrade is triggered, the orchestrating APIC fans out an HTTPS POST to every peer APIC over the OOB management network. If OOB connectivity to any peer APIC is broken at upgrade time, only the reachable APICs receive the trigger and start upgrading. The unreachable APICs are silently skipped, leaving the cluster partially upgraded — a state that cannot be recovered remotely. |
There was a problem hiding this comment.
Apic bootx use https POST for upgrade starting from 602. Due to [CSCwu91693][77], If OOB connectivity to any peer APIC is broken during upgrade, only the reachable APICs receive the trigger and start upgrading. The unreachable APICs are silently skipped, leaving the cluster partially upgraded.
|
|
||
| This check performs two verifications: | ||
|
|
||
| 1. **Default port (443)**: Used by APIC bootx starting from 6.0(2). Applicable when target version is 6.0(2) or above. |
There was a problem hiding this comment.
- Default port (443): APIC used bootx starting from 6.0(2). default https port used is 443.
remote all AI marked special symbol.
| This check performs two verifications: | ||
|
|
||
| 1. **Default port (443)**: Used by APIC bootx starting from 6.0(2). Applicable when target version is 6.0(2) or above. | ||
| 2. **Custom HTTPS port (from `commHttps`)**: Used by the upgrade fanout starting from 6.2(1). Applicable when both current and target versions are 6.2(1) or above. If the configured port is the same as the default (443), this step is skipped as it is already covered above. |
There was a problem hiding this comment.
- Custom HTTPS port (from
commHttps): starting from 6.2(1) Customer ports are supported. Applicable when both current and target versions are 6.2(1) or above.
| if subprocess.call( | ||
| 'curl --max-time 5 -k -s -o /dev/null https://{}:{} 2>/dev/null'.format(ip, port), | ||
| shell=True | ||
| ) in [7, 28]: |
There was a problem hiding this comment.
anything as !0 should be tracked as failure.
| has_error = True | ||
|
|
||
| # Custom HTTPS port check: upgrade fanout uses commHttps port from 6.2(1) | ||
| if not (cversion.older_than("6.2(1a)") or tversion.older_than("6.2(1a)")): |
There was a problem hiding this comment.
if (cversion.newer_than("6.2(1a)"). target version can be ignored.
There was a problem hiding this comment.
Updated to if not cversion.older_than "6.2(1a)".
| if tversion.older_than("6.0(2a)"): | ||
| return Result(result=NA, msg=VER_NOT_AFFECTED) | ||
|
|
||
| topSystems = icurl('class', 'topSystem.json?query-target-filter=eq(topSystem.role,"controller")') |
There was a problem hiding this comment.
topSystems as variable is confusing. please change this to something Apic_id_ip or so .
| has_error = False | ||
|
|
||
| # Default port check: APIC bootx uses port 443 from 6.0(2) | ||
| default_data, default_error = get_apic_oob_connectivity(topSystems, 443) |
There was a problem hiding this comment.
same topsystem as suggested earlier. Keep it more of variable not mo name.
| return Result(result=result, headers=headers, data=data, recommended_action=recommended_action, doc_url=doc_url) | ||
|
|
||
|
|
||
| @check_wrapper(check_title="APIC OOB Connectivity") |
There was a problem hiding this comment.
APIC OOB Connectivity --> APIC OOB Connectivity check
|
|
||
| This check verifies OOB reachability between APICs on the port(s) actually used for the upgrade trigger. The default port 443 is validated on all versions from 6.0(2) onward, since it is always used unless a custom HTTPS port is configured. From 6.2(1) onward, the upgrade trigger also honors a custom HTTPS port if one is configured via the `commHttps` policy; this custom port is validated only when the current version is 6.2(1) or later, and only when the configured port differs from 443, which is already covered by the default check. | ||
|
|
||
| For each applicable port, the script queries `topSystem` filtered to `role=controller` to collect the OOB management IP of every APIC in the cluster, then attempts an HTTPS connection to each peer on that port with a 5-second timeout. If any APIC is found unreachable on a port used for the upgrade trigger, the check fails. |
There was a problem hiding this comment.
For each applicable port, the script get OOB management IP of every APIC in the cluster, then attempts an HTTPS connection to each peer on that port with a 5-second timeout. If any APIC is found unreachable on a port used for the upgrade trigger, the check fails.
| [WRED with Affected FM Models][d35] | CSCwt50713 | :white_check_mark: | :no_entry_sign: | ||
| [N9K-C93180YC-FX3 Switch Memory Less Than 32GB][d36] | CSCwm42741 | :white_check_mark: | :no_entry_sign: | ||
| [Stale dbgacEpgSummaryTask Objects][d37] | CSCwt69100 | :white_check_mark: | :no_entry_sign: | ||
| [APIC OOB Connectivity][d38] | CSCwu91693 | :white_check_mark: | :white_check_mark: 6.2(2) |
There was a problem hiding this comment.
[APIC OOB Connectivity][d38] | CSCwu91693 | ✅ | ✅ 6.2(2)
---> correct.
[APIC OOB Connectivity][d38] | CSCwu91693 | ✅ | 🚫
| elif attrs.get('oobMgmtAddr6', '::') not in ('', '::', '0:0:0:0:0:0:0:0'): | ||
| ip = attrs.get('oobMgmtAddr6') | ||
| else: |
There was a problem hiding this comment.
curl --max-time 5 -k -s https://[2001:db8:abc:1::12]:443 -- make sure you use proper format for ipv6. please check why pytest is noyt handling this.
monrog2
left a comment
monrog2
left a comment
There was a problem hiding this comment.
Good implementation overall. A few suggestions to strengthen test coverage and version boundary validation. See inline comments.
monrog2
left a comment
monrog2
left a comment
There was a problem hiding this comment.
Good implementation overall with proper Result semantics and version gating. A few suggestions to strengthen test coverage and version boundary validation.
| dir = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
||
| test_function = "apic_oob_connectivity_check" | ||
|
|
There was a problem hiding this comment.
[Medium] Missing version boundary test immediately below 6.0(2a)
The test suite covers tversion="5.2(7f)" (well below) and tversion="6.0(2a)" (at boundary), but does not test immediately below the boundary (e.g., 6.0(1h)).
Per project review guidelines, version gate checks should test "immediately below, at, within, and immediately above each boundary" to catch off-by-one errors in version comparison logic.
Suggested addition:
# tversion = 6.0(1h) (immediately below 6.0(2a)) -> NA
(
{topSystem: [], commHttps: []},
"6.0(1h)",
"6.0(1h)",
[],
script.NA,
),| script.PASS, | ||
| ), | ||
| # tversion >= 6.0(2a), all APICs reachable on port 443 -> PASS | ||
| ( |
There was a problem hiding this comment.
[Medium] Missing cversion boundary test immediately below 6.2(1g)
The check has a version gate if not cversion.older_than("6.2(1g)") that enables custom port checking. Tests cover cversion="6.2(1g)" but don't test cversion="6.2(1f)" (immediately below) to confirm custom port logic is correctly skipped.
Suggested addition:
# cversion = 6.2(1f) (immediately below 6.2(1g)), custom port configured -> PASS
# (custom port check should be SKIPPED, only default 443 check runs)
(
{
topSystem: read_data(dir, "topSystem_3apics_oob.json"),
commHttps: read_data(dir, "commHttps_custom_port.json"),
},
"6.2(1f)",
"6.2(2a)",
[0, 0, 0], # Only 3 curl calls for port 443, not 6
script.PASS,
),| def mock_subprocess_call(cmd, shell=False): | ||
| # Verify that IPv6 addresses in the curl URL are wrapped in square brackets | ||
| import re | ||
| match = re.search(r'https://([^/]+):', cmd) |
There was a problem hiding this comment.
[Medium] Tests only assert result status, not payload content
All tests only verify result.result == expected_result. None verify result.data, result.headers, result.msg, or result.recommended_action.
Per project review guidelines: "Require status and payload assertions, not only execution without exception."
A check could return FAIL_UF with incorrect or empty data, misleading users about which APICs are unreachable.
Suggested addition for FAIL_UF cases:
assert result.result == expected_result
if expected_result == script.FAIL_UF:
assert result.headers == ["Node ID", "OOB IP", "Port", "Status"]
assert len(result.data) > 0, "FAIL_UF should include affected APIC data"
# Verify at least one row shows 'Unreachable'
assert any(row[3] == "Unreachable" for row in result.data)| shell=True | ||
| ) != 0: | ||
| data.append([node_id, ip, port, "Unreachable"]) | ||
| except Exception as e: |
There was a problem hiding this comment.
[Low] Consider avoiding shell=True in subprocess call
Using shell=True with string formatting is generally discouraged. While not a security risk here (IPs come from trusted APIC API), using a list is cleaner and more portable:
if subprocess.call(
['curl', '--max-time', '5', '-k', '-s', '-o', '/dev/null',
'https://{}:{}'.format(ip_formatted, port)],
stderr=subprocess.DEVNULL
) != 0:This is a minor suggestion and not blocking.
lovkeshsharma702
left a comment
lovkeshsharma702
left a comment
There was a problem hiding this comment.
all comments have been addressed.
monrog2
left a comment
monrog2
left a comment
There was a problem hiding this comment.
AI-ACIWISE classifies CSCwu91693 as BEHAVIOUR-CHANGE YES with high confidence across all three models. This PR therefore introduces a new catastrophic upgrade-blocking prerequisite: conditions that prevent complete evaluation must fail closed rather than yield PASS or N/A.
Read-only live validation on APIC 6.0(8e) confirmed that filtered topSystem and fabricNode return matching controller inventories, topSystem exposes OOB IPv4/IPv6 fields, and commHttps exposes the HTTPS port. It also showed that topSystem may expose fe80:: link-local IPv6 while the configured OOB relation has v6Addr=::, so link-local values are not sufficient evidence of usable IPv6 OOB connectivity. The live fabric has only comm-default and cannot establish how an active non-default communication policy is selected.
FAIL_UF is the appropriate result for both default- and custom-port failures because either condition must block the upgrade. Please align the external check specification with that behavior. Keep the conservative any-nonzero-curl-code failure rule; restricting it to only exit codes 7 and 28 could accept other unusable HTTPS paths.
The focused tests and CheckManager/AciResult tests pass, as do Python 2.7/3.8 CI, but the false-pass and policy-selection paths below remain blocking.
| if not tversion: | ||
| return Result(result=MANUAL, msg=TVER_MISSING) | ||
|
|
||
| if tversion.older_than("6.0(2a)"): |
There was a problem hiding this comment.
[Blocking] Reconcile this applicability gate with the defect contract.
The check definition says tversion >= 6.0(2), while the CSCwu91693 RNE condition says the running APIC is 6.0.2 or later. Those differ when an upgrade crosses 6.0(2). Please obtain defect-owner confirmation, then align the implementation and documentation to one explicit current/target matrix. Add cases for current below 6.0(2) with target at/above it, and current at/above 6.0(2).
| elif attrs.get('oobMgmtAddr6', '::') not in ('', '::', '0:0:0:0:0:0:0:0'): | ||
| ip = attrs.get('oobMgmtAddr6') | ||
| else: | ||
| continue |
There was a problem hiding this comment.
[Blocking] Do not skip a controller that has no usable OOB address.
Skipping it allows the new upgrade-blocking validation to return PASS without checking that peer. Return FAIL_UF with node-level evidence such as OOB address not configured. Also reject link-local-only IPv6 unless the required scope/interface is available: on the live 6.0(8e APIC, topSystem.oobMgmtAddr6 contained fe80:: addresses while mgmtRsOoBStNode.v6Addr was ::. Please cover no-address, link-local-only, IPv4-only, and configured global-IPv6 cases.
|
|
||
| apic_id_ip = icurl('class', 'topSystem.json?query-target-filter=eq(topSystem.role,"controller")') | ||
| if not apic_id_ip: | ||
| return Result(result=NA, msg="No APIC controller nodes found.") |
There was a problem hiding this comment.
[Blocking] Empty or incomplete required inventory must fail closed.
N/A becomes a passed, hidden validation, but a running APIC cannot legitimately establish this check with zero controllers. Return ERROR for an empty response. Also compare the returned controller IDs with the controller inventory already available to the script so a partial topSystem response cannot produce PASS. Add empty and partial inventory tests.
| has_error = True | ||
|
|
||
| # Custom HTTPS port check: upgrade fanout uses commHttps port from 6.2(1) | ||
| if not cversion.older_than("6.2(1g)"): |
There was a problem hiding this comment.
[Blocking] Enforce both custom-port version conditions.
The defined upgrade-fanout logic applies the custom HTTPS port only when both current and target releases are at or above the applicable 6.2 boundary. This branch checks only cversion, so a 6.2 current release targeting below the boundary still probes the custom port. Please add the target-version condition and a complete current/target boundary matrix using the defect-owner-confirmed CCO version.
| # Custom HTTPS port check: upgrade fanout uses commHttps port from 6.2(1) | ||
| if not cversion.older_than("6.2(1g)"): | ||
| port = 443 | ||
| commHttps = icurl('class', 'commHttps.json') |
There was a problem hiding this comment.
[Blocking] Resolve the effective commHttps policy deterministically and fail closed.
A class query may return default and non-default communication policies, so selecting [0] relies on undefined response order. Hard-coding comm-default is not correct either because a non-default policy may be active. Determine the policy actually used by upgrade fanout through the authoritative APIC configuration/relationship, then read that policy's commHttps child. Return ERROR when the effective policy or port is missing, malformed, or ambiguous. Add fixtures where default and non-default policies coexist and the non-default policy is effective. The live APIC had only comm-default, so this selection behavior still requires validation on an appropriate fabric.
| has_error = False | ||
|
|
||
| for apic in apic_id_ip: | ||
| attrs = apic['topSystem']['attributes'] |
There was a problem hiding this comment.
[High] Parse controller objects independently so malformed inventory does not erase valid evidence.
Direct indexing here lets one malformed object escape to the wrapper, which replaces the whole result with a generic ERROR and discards any unreachable controllers already collected. Validate each object's shape, preserve valid failure rows, and report malformed entries through unformatted_data or an error indication. Add a mixed valid-plus-malformed fixture.
| try: | ||
| ip_formatted = '[{}]'.format(ip) if ':' in ip else ip | ||
| with open(os.devnull, 'wb') as devnull: | ||
| if subprocess.call( |
There was a problem hiding this comment.
[Blocking] Verify that this probe topology satisfies APIC-to-APIC reachability.
This runs every curl from only the APIC executing the script. That proves one source can reach each OOB address, not that every upgrade-fanout source can reach every peer. Please document and validate the product guarantee that the executing APIC is the sole relevant fanout source, or perform the supported remote checks needed to cover every required source-to-peer path. Without that guarantee, the check can pass while another APIC-to-APIC path is broken.
| assert host.startswith('[') and host.endswith(']'), ( | ||
| "IPv6 address in curl URL must be wrapped in square brackets, got: {}".format(url) | ||
| ) | ||
| code = curl_exit_codes[idx[0]] if idx[0] < len(curl_exit_codes) else 0 |
There was a problem hiding this comment.
[Medium] Make unexpected curl calls fail the test.
Once the supplied exit-code list is exhausted, this mock returns success forever. The below-boundary custom-port test would therefore still pass if production made extra 8443 calls. Fail when idx exceeds the sequence and assert the final call count and exact URLs/ports for each version-gate case.
| 'https://{}:{}'.format(ip_formatted, port)], | ||
| stderr=devnull | ||
| ) != 0: | ||
| data.append([node_id, ip, port, "Unreachable"]) |
There was a problem hiding this comment.
[Low] Keep result-table payload values string-normalized.
Append str(port) rather than an integer and update the payload assertions. This keeps the new rows consistent across sorting, terminal formatting, and JSON consumers.
|
Also, what open issue is this closing? please file one if non existing and link them |
4 participants
Summary:
This PR adds a new validation check: APIC OOB Connectivity.
The check detects broken OOB management connectivity between APIC controllers, which can cause a partial or failed cluster upgrade.
What Changed:
Check Behavior:
APIC OOB Connectivity:
Test Results:
Pytest Logs: Full Run:
CSCwu91693_PytestFullRun_logs.txt
Pytest Logs: apic_oob_connectivity_check:
CSCwu91693_PytestLogs.txt
Script Full Run:
CSCwu91693_FullRun_logs.txt
Note: stale_dbgacEpgSummaryTask_check shows an ERROR (name 'timedelta' is not defined) in the full run, this is a pre-existing issue in v4.2.0-dev, not introduced by this PR. It will be resolved by Muthu's separate PR.
Script Run Logs (PASS / NA / FAIL):
CSCwu91693_PASS:NA:FAIL_logs.txt