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

[config] parse optional fields from hwsku #6155

Merged
merged 2 commits into from
Jan 25, 2021

Conversation

dmytroxshevchuk
Copy link
Contributor

@dmytroxshevchuk dmytroxshevchuk commented Dec 8, 2020

- Why I did it
For now hwsku.json and platform.json dont support optional fields. For example no way to add fec or autoneg field using platform.json and hwsku.json.

- How I did it
Added parsing of optional fields from hwsku.json.

- How to verify it
Add optional field to hwsku.json. After first boot will be generated new config_db.json or you can generate it using sonic-cfggen command. In this file must be optional field from hwsku.json or check using command redis-cli hgetall PORT_TABLE:Ethernet0
Example of hwsku.json, that must be parsed:

{
    "interfaces": {
        "Ethernet0": {
            "default_brkout_mode": "1x100G[40G]",
            "fec": "rs",
            "autoneg": "0"
        },
...
}

Example of generated config_db.json:

    "PORT": {
        "Ethernet0": {
            "alias": "Ethernet0",
            "lanes": "0,1,2,3",
            "speed": "100000",
            "index": "1",
            "admin_status": "up",
            "fec": "rs",
            "autoneg": "0",
            "mtu": "9100"
        },

So, we can see this entries in redis db:

admin@sonic:~$ redis-cli hgetall PORT_TABLE:Ethernet0

 1) "alias"
 2) "Ethernet0"
 3) "lanes"
 4) "0,1,2,3"
 5) "speed"
 6) "100000"
 7) "index"
 8) "1"
 9) "admin_status"
10) "up"
11) "fec"
12) "rs"
13) "autoneg"
14) "0"
15) "mtu"
16) "9100"
17) "description"
18) ""
19) "oper_status"
20) "up"

Also its way to fix show interface status, FEC field but also need add FEC field to hwsku.json.
Before:

admin@sonic:~$ show interfaces status
  Interface            Lanes    Speed    MTU    FEC        Alias    Vlan    Oper    Admin             Type    Asym PFC
-----------  ---------------  -------  -----  -----  -----------  ------  ------  -------  ---------------  ----------
  Ethernet0          0,1,2,3     100G   9100     N/A    Ethernet0  routed      up       up  QSFP28 or later         N/A

After:

admin@sonic:~$ show interfaces status
  Interface            Lanes    Speed    MTU    FEC        Alias    Vlan    Oper    Admin             Type    Asym PFC
-----------  ---------------  -------  -----  -----  -----------  ------  ------  -------  ---------------  ----------
  Ethernet0          0,1,2,3     100G   9100     rs    Ethernet0  routed      up       up  QSFP28 or later         N/A

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@dmytroxshevchuk
Copy link
Contributor Author

retest vsimage please

@dmytroxshevchuk
Copy link
Contributor Author

@praveen-li @lguohan please take a look

@dmytroxshevchuk
Copy link
Contributor Author

@praveen-li @lguohan @daall Are we going review/merge this PR?
Its blocker for us cause of we need additional fields in PORT table for save behavior as with the old port_config.ini.
Please let me know if exists another way for adding this fields: autoneg, fec. If its impossible than please review and merge this PR as soon as possible.

@akokhan
Copy link
Contributor

akokhan commented Jan 20, 2021

retest broadcom please

@@ -250,6 +250,14 @@ def parse_platform_json_file(hwsku_json_file, platform_json_file):
brkout_mode = hwsku_dict[INTF_KEY][intf][BRKOUT_MODE]

child_ports = get_child_ports(intf, brkout_mode, platform_json_file)

# take optional fields from hwsku.json
for key, item in hwsku_dict[INTF_KEY][intf].items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is probably better to take conservative way to introduce the new fields in the hwsku.json by explicitly define the new fields in a structure, and only allow the defined fields to be generated to PORT table. To make the detection earlier, we can also add something like https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-device-data/tests/platform_json_checker to do sanity check for hwsku.json at build time.

@dmytroxshevchuk
Copy link
Contributor Author

@zhenggen-xu done, for now entry will be added only if it exist in predefined list. Also added sanity check for hwsku.json.
Please check.

@akokhan
Copy link
Contributor

akokhan commented Jan 21, 2021

retest Azure.sonic-buildimage please

@dmytroxshevchuk
Copy link
Contributor Author

@lguohan please review

@akokhan
Copy link
Contributor

akokhan commented Jan 24, 2021

@lguohan , these changes help to make platform.json + hwsku.json functionality the same as port_config.ini. Please review and merge.

@lguohan lguohan merged commit dd0e110 into sonic-net:master Jan 25, 2021
@lguohan
Copy link
Collaborator

lguohan commented Jan 25, 2021

thanks for the enhancement.

lguohan pushed a commit that referenced this pull request Jan 25, 2021
…6155)

**- Why I did it**

For now `hwsku.json` and `platform.json` dont support optional fields. For example no way to add `fec` or `autoneg` field using `platform.json` and `hwsku.json`.

**- How I did it**
Added parsing of optional fields from hwsku.json.

**- How to verify it**
Add optional field to `hwsku.json`. After first boot will be generated new `config_db.json` or you can generate it using `sonic-cfggen` command. In this file must be optional field from `hwsku.json` or check using command `redis-cli hgetall PORT_TABLE:Ethernet0`
Example of `hwsku.json`, that must be parsed:
```
{
    "interfaces": {
        "Ethernet0": {
            "default_brkout_mode": "1x100G[40G]",
            "fec": "rs",
            "autoneg": "0"
        },
...
}
```
Example of generated `config_db.json`:
```
    "PORT": {
        "Ethernet0": {
            "alias": "Ethernet0",
            "lanes": "0,1,2,3",
            "speed": "100000",
            "index": "1",
            "admin_status": "up",
            "fec": "rs",
            "autoneg": "0",
            "mtu": "9100"
        },
```
So, we can see this entries in redis db:
```
admin@sonic:~$ redis-cli hgetall PORT_TABLE:Ethernet0

 1) "alias"
 2) "Ethernet0"
 3) "lanes"
 4) "0,1,2,3"
 5) "speed"
 6) "100000"
 7) "index"
 8) "1"
 9) "admin_status"
10) "up"
11) "fec"
12) "rs"
13) "autoneg"
14) "0"
15) "mtu"
16) "9100"
17) "description"
18) ""
19) "oper_status"
20) "up"
```

Also its way to fix `show interface status`, `FEC` field but also need add `FEC` field to `hwsku.json`.
Before:
```
admin@sonic:~$ show interfaces status
  Interface            Lanes    Speed    MTU    FEC        Alias    Vlan    Oper    Admin             Type    Asym PFC
-----------  ---------------  -------  -----  -----  -----------  ------  ------  -------  ---------------  ----------
  Ethernet0          0,1,2,3     100G   9100     N/A    Ethernet0  routed      up       up  QSFP28 or later         N/A
```
After:
```
admin@sonic:~$ show interfaces status
  Interface            Lanes    Speed    MTU    FEC        Alias    Vlan    Oper    Admin             Type    Asym PFC
-----------  ---------------  -------  -----  -----  -----------  ------  ------  -------  ---------------  ----------
  Ethernet0          0,1,2,3     100G   9100     rs    Ethernet0  routed      up       up  QSFP28 or later         N/A
```
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.

4 participants