Egress ACL Outer DSCP Change Table #1743
Conversation
Signed-off-by: siqbal1986 <[email protected]>
siqbal1986
requested review from
prsunny,
kperumalbfn,
bingwang-ms and
prabhataravind
|
|
||
|
|
||
| ##### Pros | ||
|
|
There was a problem hiding this comment.
If some vendor supports unified V4 and V6 ACL tables, we could just have one ACL table.
There was a problem hiding this comment.
AFIK only broadcom supports both V4 and V6 tables. But Broadcom doesnt support vxlan so devices in production so having a separate table for one platofrm may not be very useful.
There was a problem hiding this comment.
There are other platforms that can combine v4&v6 rules together; e.g. Marvell Teralynx (Innovium) platform. So, orchagent should follow the existing logic for Mirror_combined or L3V4V6 table to decide whether it creates separate ingress MARK_META/V6 tables or a combined MARK_META_V4V6 table.
There was a problem hiding this comment.
In order to make this uniform for the end user in the future, the user accessible table type should just be UNDERLAY_SET_DSCP_V4V6 (i.e., a single table for the user). Orchagent should internally based on the platform make one of the below choices:
- A single egress ACL table to match L3 v4/v6 inner payload fields and set the egress DSCP
- A egress ACL table to match L3 v4 inner payload fields and set the egress DSCP, and another egress ACL table to match L3 v6 inner payload fields and set the egress DSCP
- A single ingress ACL table to match L3 v4/v6 inner payload fields into the user meta-data, and a single egress ACL table to set the egress DSCP based on the metadata
- An ingress ACL table to match L3 v4 payload fields and set the user-metadata, another ingress ACL table to match L3 v6 payload fields and set the user-metadata, and a single egress ACL table to set the egress DSCP based on the metadata
Exposing a single ACL table of type UNDERLAY_SET_DSCP_V4V6 is better to do this than exposing two ACL table types (UNDERLAY_SET_DSCP, and UNDERLAY_SET_DSCPV6) to prevent user from misconfiguring. E.g. https://github.com/sonic-net/SONiC/blob/master/doc/acl/Extend-L3V6ACLs.md#cons
There was a problem hiding this comment.
Above comments marked as addendum under the heading "Changes proposed by the Community to be introduced in the next iteration"
|
|
||
| #### SAI API | ||
|
|
||
| There are no new SAI APIs required for this feature. |
There was a problem hiding this comment.
There should be some new SAI attribute required for this feature, such as the capability check.
There was a problem hiding this comment.
The SAI attributes used in this design already exist and we dont need to define new SAI attributes.
| Due to this reason, this option may not be viable at all. | ||
|
|
||
|
|
||
| #### Option-C: Create a dummy table type which translates into MARK_META/V6 and EGR_SET_DSCP as in the approach mentioned above. Manage the metadata internally. |
There was a problem hiding this comment.
If we go with Option-C, does that mean we can't create this egress table standalone?
There was a problem hiding this comment.
Thats correct. The EGR_SET_DSCP table in this design has one match(META_DATA) and one action(SET_DSCP). I am not sure if there is any other use for it currently.
if in future we feel the need we cna expose that table as well and enhance it with more match attributes and actions.
|
|
||
| ##### Pros | ||
| - Ease of Implementation: The changes to the Orchagent would be minimal. | ||
| - The TCAM utilization would be minimal with the introduction of just one egress table. This table would be associated with all the ports excluding loopback and internal ports. |
There was a problem hiding this comment.
The idea that the TCAM can be shared between L3/L3V6 rules and the MARK_META/METAV6 rules is not always true. This is because either L3/L3V6 rules can shadow MARK_META/METAV6 rules or the other way.
There was a problem hiding this comment.
no, this is not the intent here. The Egress SET DSCP table only sets the DSCP based on the metadata value. the metadata value would be set by the ingress L4 or L6 tables. Here single egress table means that we dont need to have 2 separate egress set DSCP table for V4 and V6.
| The interface association of this table can be implemented in two ways. | ||
|
|
||
| 1) EGR_SET_DSCP association with all dataplane interfaces. This approach is resource intensive and would require changes in portsOrch to accomodate exposing all ports to the AclOrch. However this allows us to decouple the ingress stage table port association from EGR_SET_DSCP. | ||
| 2) EGR_SET_DSCP be associated with a superset of all interfaces which are associated with tables referencing it. e.g. if UNDERLAY_SET_DSCP is associated with interfaces [a,b,c] and UNDERLAY_SET_DSCPV6 is associated with [c,d,e] then EGR_SET_DSCP would be associated with [a,b,c,d,e]. This gives the user of the feature the choice to limit port association when needed. Based on common use case pattern, the tables would almost always be associated with all data-plane interfaces. |
There was a problem hiding this comment.
The suggested use-case was that these rules are going to be common for all the ports. In that case, it would be easier to implement this with bindpoint as SWITCH.
There was a problem hiding this comment.
agreed. added to the addendum and would be implemted in the next iteration.
| #### Option-C: Create a dummy table type which translates into MARK_META/V6 and EGR_SET_DSCP as in the approach mentioned above. Manage the metadata internally. | ||
|
|
||
| This option is similar to Option A except it tracks the metadata and EGR_SET_DSCP entry internally in the Orchagent. In this option, 5 different tables are defined, but two of them are user-facing and are not internally created. Instead, they are translated into the three internal tables by Orchagent. | ||
| The user would create ACL based on UNDERLAY_SET_DSCP/UNDERLAY_SET_DSCPV6 tables and add entries to them. The action parameters of these tables are SET_DSCP. The Orchagent would then allocate/assign metadata value based on the DSCP value and translate these entries into entries for MARK_META/V6 tables. It would also create the entry for EGR_SET_DSCP based on metadata and DSCP values. One instance of EGR_SET_DSCP would suffice for both V4 and V6 tables. |
There was a problem hiding this comment.
Orchagent should check the platform capability to decide whether it should create separate MARK_META/V6 ACL tables or a single combined MARK_META_V4V6 (Similar to L3V4V6) ACL table.
There was a problem hiding this comment.
agreed. added to the addendum and would be implemted in the next iteration.
|
|
||
|
|
||
| ##### Pros | ||
|
|
There was a problem hiding this comment.
There are other platforms that can combine v4&v6 rules together; e.g. Marvell Teralynx (Innovium) platform. So, orchagent should follow the existing logic for Mirror_combined or L3V4V6 table to decide whether it creates separate ingress MARK_META/V6 tables or a combined MARK_META_V4V6 table.
| ##### Pros | ||
|
|
||
| This approach has the benifit of providing a uniform interface to the user. The management and efficient utilization of resources is done by the Orchagent. | ||
|
|
There was a problem hiding this comment.
We should work with SAI community to identify platforms that can support matching inner packet fields and setting outer DSCP in the egress ACL tables. In the meantime, we could just do platform checks (like those done for Combined Mirror ACL tables), to identify platforms with this egress ACL table capability to match inner fields and set the outer DSCP.
On these platforms, Orchagent should use just the egress ACL table instead of wasting ingress ACL tables and the metadata bits.
There was a problem hiding this comment.
This is a slightly bigger ask but I can start the discussion. I have added this in the addendum
|
|
||
| ##### Cons | ||
|
|
||
| This approach adds complexity in the Orchagent and less resource effiecient as the user is forced to associate the UNDERLAY_SET_DSCP/V6 tables with interfaces which can have packets ingressing or egressing. |
There was a problem hiding this comment.
This is not true if we can use the SWITCH bind point.
There was a problem hiding this comment.
agreed. added the switch binding proposal to the addendum
|
@rck-innovium , @bingwang-ms , would you sign off on design? anything pending |
Hi @prsunny The feedback from @kperumalbfn is something important from a platform perspective. Please see #1743 (comment) That and several other discussions in the PR have not been resolved. |
|
@prsunny can you please help to approve this PR? I can merge after the approval. Thanks. |
|
@siqbal1986 can you please help to add the code PRs to this HLD by following #806 ? So that community can track the feature completion. |
|
/azp run |
|
No pipelines are associated with this pull request. |
…d be reflected in the next code iteration.
|
/azp run |
|
No pipelines are associated with this pull request. |
|
@rck-innovium , can you please re-review. The comments has been addressed. |
|
@prsunny can you please merge this |
This document describes the High level design of a new table type which can change the outer DSCP of a packet encapsulated by ther ASIC pipeline while preserving the orignal packets inner DSCP value.
Feedback /Enhacements.
3)Investigate the possibility that If the platform supports matching inner header fields and setting outer DSCP in the EGress ACL then we can sue a singel table.
Code PRs: