-
Notifications
You must be signed in to change notification settings - Fork 15
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
[SDFAB-1097] Do not generate INT report for pruned packets #502
Conversation
@@ -297,7 +297,7 @@ control IntEgress (inout v1model_header_t hdr_v1model, | |||
fabric_md.int_report_md.mirror_type = FabricMirrorType_t.INT_REPORT; | |||
fabric_md.int_report_md.report_type = fabric_md.bridged.int_bmd.report_type; | |||
fabric_md.int_report_md.ig_port = fabric_md.bridged.base.ig_port; | |||
fabric_md.int_report_md.eg_port = (PortId_t)fabric_v1model.recirc_preserved_egress_port; | |||
fabric_md.int_report_md.eg_port = (PortId_t)standard_md.egress_port; |
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.
based on the logic from fabric-tna, we use the egress port number from intrinsic metadata, which is the standard metadata of v1model
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.
@EmanueleGallone 👆does this change make sense?
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.
Yes it does, because init_int_metadata
is performed BEFORE cloning/recirculating the packet. the standard_md.egress_port
information is lost AFTER cloning/recirculating the packet.
This is also confirmed by the CI, otherwise I would've expected some INT tests to fail when asserting the original packet's egress port. BTW, Thanks for catching this.
Codecov Report
@@ Coverage Diff @@
## main #502 +/- ##
=========================================
Coverage 69.85% 69.85%
+ Complexity 730 729 -1
=========================================
Files 63 63
Lines 4771 4771
Branches 528 528
=========================================
Hits 3333 3333
Misses 1161 1161
Partials 277 277 Continue to review full report at Codecov.
|
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.
LGTM. Thanks for adding a new test.
@@ -2149,8 +2148,12 @@ def runIPv4MulticastTest(self, pkt, in_port, out_ports, in_vlan, out_vlan): | |||
# Send packets and verify | |||
self.send_packet(in_port, pkt) | |||
for out_port in out_ports: | |||
if out_port == in_port: | |||
# Packet will be dropped by egress pipeline. | |||
continue |
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.
Does that mean before we were expecting to receive a packet on in_port
? If yes, since the pipeline performs inport pruning, how is it possible that the multicast test was passing before this change?
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.
For IPv4 multicast test, we didn't include ingress port to the egress port list, which means the clone group won't clone the packet to the ingress port
https://github.com/stratum/fabric-tna/blob/main/ptf/tests/unary/test.py#L118-L119
When sending multicast/broadcast traffic, we always drop packets if the egress port is equal to the ingress port.
This caused the INT pipeline to generate unnecessary INT drop reports.
To avoid this, we can set the report type to NO_REPORT for this case.