Conversation
eca3f58 to
1a126b7
Compare
monrog2
left a comment
There was a problem hiding this comment.
Changes requested
1. Restrict detection to PBR service graphs
File: aci-preupgrade-validation-script.py, around line 7340
CSCwt14573 and CSCwn95571 require locally configured PBR. The current implementation processes every applied service graph, so a non-PBR graph without translation entries is incorrectly reported as FAIL_O.
Please filter graph instances using vnsNodeInst.routingMode == "Redirect", consistent with the existing PBR detection in consumer_vzany_shared_services_check.
graph_query = (
'vnsGraphInst.json?'
'query-target-filter=eq(vnsGraphInst.configSt,"applied")'
'&rsp-subtree=full'
'&rsp-subtree-class='
'vnsNodeInst,vnsTermNodeInst,vnsConnectionInst,'
'vnsRsConnectionInstConns'
)
graph_insts = icurl('class', graph_query)
for graph_inst_mo in graph_insts:
graph_inst = graph_inst_mo.get('vnsGraphInst', {})
children = graph_inst.get('children', [])
is_pbr = any(
child.get('vnsNodeInst', {})
.get('attributes', {})
.get('routingMode') == 'Redirect'
for child in children
)
if not is_pbr:
continueAlso update docs/docs/validations.md to remove the statements that PBR is not required.
Please add tests proving:
routingMode="unspecified"returnsPASS.routingMode="Redirect"with a missing translation returnsFAIL_O.
2. Preserve every graph instance and stretched VRF
File: aci-preupgrade-validation-script.py, around lines 7383 and 7433
sg_by_contract and vzany_on_stretched are keyed only by contract DN. A contract can be used by multiple VRFs, and each scoped graph instance requires its own translation check. Later entries currently overwrite earlier ones.
I reproduced two stretched VRFs sharing one contract where the first translation was missing and the second existed. The check queried only the second instance and returned PASS.
Please retain all graph instances and identify relationships by both contract and VRF scope.
sg_by_contract = defaultdict(list)
# While processing graph instances:
sg_by_contract[contract_dn].append({
'gi_dn': gi_dn,
'graph_name': graph_name,
'scope_dn': scope_dn,
'first_node': first_node_name,
})
vzany_on_stretched = set()
# While processing vzAny relationships:
vzany_on_stretched.add((contract_dn, vrf_dn))
# Evaluate every graph instance matching the relationship's VRF:
for contract_dn, vrf_dn in vzany_on_stretched:
for sg_info in sg_by_contract.get(contract_dn, []):
if sg_info['scope_dn'] != vrf_dn:
continue
# Check the translation for this specific graph instance.Please add a regression test with two stretched VRFs using the same contract where only one graph instance has a translation. The result must identify the missing instance rather than return PASS.
3. Do not silently skip unparseable graph instances
File: aci-preupgrade-validation-script.py, around line 7440
When the first consumer node cannot be parsed, the check executes continue. If no other rows are generated, this produces a false PASS. A malformed contract DN is handled similarly.
Required parsing failures should produce ERROR and preserve the available graph evidence.
unformatted_headers = ['Graph Instance DN', 'Issue']
unformatted_data = []
# ...
if not first_node_name:
has_error = True
unformatted_data.append([
sg_info.get('gi_dn') or contract_dn,
'Unable to determine the first consumer node',
])
continue
contract_match = re.match(r'uni/tn-([^/]+)/brc-([^/]+)', contract_dn)
if not contract_match:
has_error = True
unformatted_data.append([
sg_info.get('gi_dn') or contract_dn,
'Unable to parse the contract DN',
])
continueReturn the accumulated evidence:
return Result(
result=result,
headers=headers,
data=data,
unformatted_headers=unformatted_headers,
unformatted_data=unformatted_data,
recommended_action=recommended_action,
doc_url=doc_url,
)Please add malformed/missing-node and malformed-contract tests asserting ERROR and the retained unformatted evidence.
4. Avoid retrieving every full service-graph subtree
File: aci-preupgrade-validation-script.py, around line 7340
The current request retrieves the complete subtree of every applied vnsGraphInst before establishing whether any relevant stretched VRF or vzAny relationship exists. This can create substantial APIC load on large fabrics.
Please collect stretched VRFs and relevant vzAny relationships first, then retrieve only the graph classes required for PBR and node discovery.
graph_query = (
'vnsGraphInst.json?'
'query-target-filter=eq(vnsGraphInst.configSt,"applied")'
'&rsp-subtree=full'
'&rsp-subtree-class='
'vnsNodeInst,vnsTermNodeInst,vnsConnectionInst,'
'vnsRsConnectionInstConns'
)The test query key must exactly match the optimized production query.
@lovkeshsharma702 Please review these requested changes, particularly the PBR trigger and multi-VRF handling, before the next revision.
vzany_stretched_vrf_aci-run-logs_Jun30.txt
vzany_stretched_vrf_pytest-logs_Jun30.txt
Fixes #404