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

[intfsorch] Create subport with the entry contains necessary attributes #1650

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

peter-yu
Copy link
Contributor

redis APPL_DB has two entries for subport
8) "INTF_TABLE:Ethernet4.20"
9) "INTF_TABLE:Ethernet4.20:192.169.1.1/24"

sometimes, if the entry with ip key are execute first,
the syslog is like
Feb 23 02:20:43.650184 as7726-32x-3 NOTICE swss#orchagent: :- addRouterIntfs: Create router interface Ethernet4.20 MTU 32761
Feb 23 02:20:43.651383 as7726-32x-3 ERR syncd#syncd:[none] brcm_sai_create_router_interface:284 Invalid MTU size

the mtu value is not init and is too large. sai will return SAI_STATUS_INVALID_PARAMETER

Thus, mtu should be initialized to 0

What I did
initialize mtu to 0
Why I did it
sometimes gPortsOrch->addSubPort(port, alias, adminUp, mtu) is called and
the mtu value is not init and is too large. sai will return SAI_STATUS_INVALID_PARAMETER

Thus, mtu should be initialized to 0

How I verified it
create/remove subport interface many times

Details if related

@ghost
Copy link

ghost commented Feb 28, 2021

I'd extend PR description to explicitly mentioned that in case MTU=0 the subport will inherit MTU value from the parent port.

ghost
ghost previously approved these changes Feb 28, 2021
@@ -588,7 +588,7 @@ void IntfsOrch::doTask(Consumer &consumer)
string vrf_name = "", vnet_name = "", nat_zone = "";
MacAddress mac;

uint32_t mtu;
uint32_t mtu = 0;
Copy link
Contributor

@wendani wendani Feb 28, 2021

Choose a reason for hiding this comment

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

Port object is not supposed to be created by "INTF_TABLE:Ethernet4.20:192.169.1.1/24".

In current fix, sub interface admin_status still faces a similar issue that adminUp get some random init value, most likely non-zero, to cause interface SAI ADMIN_STATE to be "true", ignorant of user config in CONFIG_DB.

Try

"INTF_TABLE:Ethernet4.20": {
    "admin_status": "false"
}

You will still get ADMIN_V4_STATE and ADMIN_V6_STATE to be "true" in ASIC_DB output.

1) "SAI_ROUTER_INTERFACE_ATTR_VIRTUAL_ROUTER_ID"
 2) "oid:0x3000000000022"
 3) "SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS"
 4) "02:42:AC:11:00:04"
 5) "SAI_ROUTER_INTERFACE_ATTR_TYPE"
 6) "SAI_ROUTER_INTERFACE_TYPE_SUB_PORT"
 7) "SAI_ROUTER_INTERFACE_ATTR_PORT_ID"
 8) "oid:0x1000000000012"
 9) "SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID"
10) "10"
11) "SAI_ROUTER_INTERFACE_ATTR_ADMIN_V4_STATE"
12) "true"
13) "SAI_ROUTER_INTERFACE_ATTR_ADMIN_V6_STATE"
14) "true"
15) "SAI_ROUTER_INTERFACE_ATTR_MTU"
16) "32761"
17) "SAI_ROUTER_INTERFACE_ATTR_NAT_ZONE_ID"
18) "0"

I believe on line 745, we need to amend the condition to create Port object with && !ip_prefix_in_key as root fix, though it does no harm to give mtu an explicit init value. To be consistent, we should do the same treatment to adminUp as mtu. I have reproduced this issue using vs docker, and have crafted a vs test case to validate the root fix.

AssertionError: Expected field/value pairs not found: expected={'SAI_ROUTER_INTERFACE_ATTR_TYPE': 'SAI_ROUTER_INTERFACE_TYPE_SUB_PORT', 'SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID': '10', 'SAI_ROUTER_INTERFACE_ATTR_ADMIN_V4_STATE': 'false', 'SAI_ROUTER_INTERFACE_ATTR_ADMIN_V6_STATE': 'false', 'SAI_ROUTER_INTERFACE_ATTR_MTU': '9100', 'SAI_ROUTER_INTERFACE_ATTR_VIRTUAL_ROUTER_ID': 'oid:0x3000000000022'}, received={'SAI_ROUTER_INTERFACE_ATTR_VIRTUAL_ROUTER_ID': 'oid:0x3000000000022', 'SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS': '02:42:AC:11:00:02', 'SAI_ROUTER_INTERFACE_ATTR_TYPE': 'SAI_ROUTER_INTERFACE_TYPE_SUB_PORT', 'SAI_ROUTER_INTERFACE_ATTR_PORT_ID': 'oid:0x1000000000012', 'SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID': '10', 'SAI_ROUTER_INTERFACE_ATTR_ADMIN_V4_STATE': 'true', 'SAI_ROUTER_INTERFACE_ATTR_ADMIN_V6_STATE': 'true', 'SAI_ROUTER_INTERFACE_ATTR_MTU': '32630', 'SAI_ROUTER_INTERFACE_ATTR_NAT_ZONE_ID': '0'}, key="oid:0x60000000005f6", table="ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

Copy link
Contributor

@wendani wendani left a comment

Choose a reason for hiding this comment

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

as comments

@peter-yu peter-yu marked this pull request as draft March 4, 2021 11:17
@@ -744,7 +744,7 @@ void IntfsOrch::doTask(Consumer &consumer)
{
if (isSubIntf)
{
if (!gPortsOrch->addSubPort(port, alias, adminUp, mtu))
if (!ip_prefix_in_key && !gPortsOrch->addSubPort(port, alias, adminUp, mtu))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be with line 745

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

redis APPL_DB has two entries for subport
8) "INTF_TABLE:Ethernet4.20"
9) "INTF_TABLE:Ethernet4.20:192.169.1.1/24"

Sometimes, if the entry with ip_prefix key is fetched first when adding subport, the mtu and adminUp may not have been initialized.
the syslog may like
Feb 23 02:20:43.650184 as7726-32x-3 NOTICE swss#orchagent: :- addRouterIntfs: Create router interface Ethernet4.20 MTU 32761
Feb 23 02:20:43.651383 as7726-32x-3 ERR syncd#syncd:[none] brcm_sai_create_router_interface:284 Invalid MTU size

The mtu is too large, and SAI returns SAI_STATUS_INVALID_PARAMETER

Create the subport by the entry w/o ip_prefix key. Also initialize mtu and adminUp to default value.
@peter-yu peter-yu changed the title [intfsorch] mtu should be initialized to 0 [intfsorch] Create subport with the entry contains necessary attributes Mar 5, 2021
@peter-yu peter-yu marked this pull request as ready for review March 5, 2021 07:41
@prsunny prsunny merged commit b6db9dd into sonic-net:master Mar 5, 2021
daall pushed a commit that referenced this pull request Apr 22, 2021
…es (#1650)

Subport mtu and admin_status initialized to values of 0 and false resepectively.
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
…es (sonic-net#1650)

Subport mtu and admin_status initialized to values of 0 and false resepectively.
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…model (sonic-net#1650)

- What I did
This PR brings in support for packages with YANG models and CLI auto generation capabilities for 3rd party packages.

- How I did it
Packages can set two new flags in manifest - "auto-generate-show" and "auto-generate-config" in addition to YANG module recorded in package image label "com.azure.sonic.yang-module".

- How to verify it
Build and run. Prepare some package with YANG model and test CLI is generated for it.

Signed-off-by: Stepan Blyshchak <[email protected]>
Co-authored-by: Vadym Hlushko <[email protected]>
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…kages with YANG model (sonic-net#1650)" (sonic-net#1972)" (sonic-net#1994)

This reverts commit fe00bbf.

- What I did
Revert previous revert, since the proposed fix has been merged - sonic-net/sonic-buildimage#9587

- How I did it
Revert the revert.

- How to verify it
Run build an on the switch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants