Skip to content
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-155 Allow forwarding of packet-outs like regular packets #262

Merged
merged 13 commits into from
May 27, 2021

Conversation

ccascone
Copy link
Member

@ccascone ccascone commented May 24, 2021

By default, all packet-outs are sent straight to the egress port passed as a controller packet-out metadata, bypassing the forwarding tables in the ingress pipe. With this change, the control plane can set a new packet-out metadata named do_forwarding to instruct the parser to forward packet-outs as regular packets.

This is required to support Xn-based handover for the DT trial, where PFCP agent will send "incomplete" packet-outs for the base stations carrying "end-marker" packets. It will be up to the switch to "complete" packet-outs by selecting the right egress port, VLAN tag, setting MAC addresses, etc.

When handling OutboundPacket in ONOS, the pipeconf (interpreter) uses the OUTPUT instruction with logical port TABLE to enable forwarding. This is consistent with the OpenFlow behavior, from the spec:

Required: TABLE: Represents the start of the OpenFlow pipeline (see 5.1). This port is only valid in an output action in the action list of a packet-out message (see 7.3.7), and submits the packet to the first flow table so that the packet can be processed through the regular OpenFlow pipeline.

TODO:

  • Add pipeconf Java unit test for when do_forwarding=1
  • Initialize filtering tables for cpu port in pipeliner

@ccascone ccascone changed the title Allow forwarding of packet-outs like regular packets SDFAB-155 Allow forwarding of packet-outs like regular packets May 24, 2021
@ccascone ccascone requested a review from pierventre May 24, 2021 23:01
@charlesmcchan
Copy link
Collaborator

charlesmcchan commented May 25, 2021

This could also help the following scenario if I understand correctly.

Imaging in a 2x2, host on leaf1 trying to ping switch interface IP of leaf2.
The request will get routed via host -> leaf1 -> spine1/2 -> leaf2 and get packet-in at leaf2.
Master of leaf2 will respond using packet-out. However, ONOS needs to first calculate the MPLS label of leaf1 and push that as part of the packet-out.

With this feature, ONOS could simply swap source and destination IP, set this flag, let the original routing logic on leaf2 push the MPLS label, and forward the packet back to leaf1.

@ccascone
Copy link
Member Author

@charlesmcchan your understanding is correct

@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #262 (f535888) into main (51b0f82) will decrease coverage by 0.01%.
The diff coverage is 77.91%.

❗ Current head f535888 differs from pull request most recent head 018ea78. Consider uploading reports for the commit 018ea78 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main     #262      +/-   ##
============================================
- Coverage     74.13%   74.12%   -0.02%     
- Complexity      273      360      +87     
============================================
  Files            18       31      +13     
  Lines          1875     2415     +540     
  Branches        153      211      +58     
============================================
+ Hits           1390     1790     +400     
- Misses          401      515     +114     
- Partials         84      110      +26     
Impacted Files Coverage Δ
...viour/pipeliner/ForwardingObjectiveTranslator.java 75.40% <0.00%> (ø)
...atumproject/fabric/tna/stats/HighlightManager.java 63.08% <63.08%> (ø)
...atumproject/fabric/tna/stats/StatisticManager.java 63.45% <63.45%> (ø)
...roject/fabric/tna/behaviour/FabricInterpreter.java 76.97% <66.66%> (+1.42%) ⬆️
...org/stratumproject/fabric/tna/stats/cli/Utils.java 80.00% <80.00%> (ø)
.../stratumproject/fabric/tna/stats/HighlightKey.java 92.59% <92.59%> (ø)
...abric/tna/behaviour/pipeliner/FabricPipeliner.java 41.77% <94.56%> (-4.15%) ⬇️
.../stratumproject/fabric/tna/stats/StatisticKey.java 95.23% <95.23%> (ø)
...atumproject/fabric/tna/stats/StatisticDataKey.java 96.55% <96.55%> (ø)
...umproject/fabric/tna/stats/StatisticDataValue.java 97.50% <97.50%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b0f82...018ea78. Read the comment docs.

ptf/tests/ptf/fabric.ptf/test.py Show resolved Hide resolved
ptf/tests/ptf/fabric_test.py Show resolved Hide resolved
ptf/tests/ptf/fabric_test.py Show resolved Hide resolved
pad0_md.value = stringify(0, 1)
# ether type
ether_type_md = packet_out.metadata.add()
ether_type_md.metadata_id = 4
ether_type_md.metadata_id = 5
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhilashendurthi will this require changes to the TV runner for fabric-tna? The packet-out metadata ID for ether_type has changed from 4 to 5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will require me to change the metadata ID for padding from 3 to 4 and metadata ID for ether_type from 4 to 5. Also for, do_forwarding metadata with ID 3, should the value be 0 in TV runner?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in TV runner do_forwarding should be 0

@ccascone
Copy link
Member Author

@pierventre @Yi-Tseng I think I have addressed all your comments, PTAL.

Copy link
Collaborator

@pierventre pierventre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@pierventre
Copy link
Collaborator

is the TODO list updated ?

@ccascone
Copy link
Member Author

ccascone commented May 26, 2021

@pierventre

is the TODO list updated ?

I think so. Why do you ask?

@ccascone ccascone merged commit 37b494a into main May 27, 2021
@ccascone ccascone deleted the pkt-out-fwd branch May 27, 2021 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants