Buffers configuration update on port speed change #1250

andriymoroz-mlnx · 2017-12-19T09:21:34Z

REVIEW ONLY.
Please do not commit yet.
submodules pointers needs to be updated after related PRs merged

related PRs:
sonic-net/sonic-swss#417
sonic-net/sonic-swss-common#169 (merged)
sonic-net/sonic-utilities#174 (merged)

- What I did
Config files for PG Buffers update on port speed change

- How I did it
See: https://github.com/Azure/SONiC/wiki/Run-Time-Buffers-Configuration-Update-design

- How to verify it
Buffers configuration should exist in Config DB after boot
redis-cli -n 4 keys \* | grep BUFFER

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

lguohan · 2017-12-20T17:28:05Z

@yxieca to check?

yxieca · 2017-12-28T17:50:27Z

device/mellanox/x86_64-mlnx_msn2700-r0/ACS-MSN2700/buffers.json.j2

+ {%- endif -%}
+ {%- endif -%}
+ {%- endfor -%}
+ {%- if cable_len -%}


I think you need to test if the list is non-empty, but this test seems to be testing if the list has been defined. Which is defined in line 17 above.

right, thanks. I already implemented this in #1263 need to back-merge to this PR as well

yxieca · 2017-12-28T17:50:54Z

device/mellanox/x86_64-mlnx_msn2700-r0/ACS-MSN2700/buffers.json.j2

+ {%- endif -%}
+ {%- endfor -%}
+ {%- if cable_len -%}
+ {{ cable_len.0 }}


Under what circumstances would there be more than one length in the list?

there should be no more than one value in the list. cable_len.0 is the way to get first (and only) element of the list. All that weird usage of the cable_len list here is the workaround for jinja templates limitation for variables visibility in blocks. Only list or dictionary can be modified in block and maintain that value outside the block.

yxieca · 2017-12-28T17:53:16Z

device/mellanox/x86_64-mlnx_msn2700-r0/ACS-MSN2700/buffers.json.j2

+ {%- set neighbor_role = neighbor.type -%}
+ {%- set roles1 = switch_role + '_' + neighbor_role %}
+ {%- set roles2 = neighbor_role + '_' + switch_role -%}
+ {%- if roles1 in ports2cable -%}


Do we have enforcement elsewhere to make sure that the switch_role and neighbor_role's case is as expected?

If we cannot guarantee the case, then we might want to cast the string to upper or lower case, just in case?

Same question for line 27 below.

no, we don't have case enforcement. Agree, need to convert to common case before the comparison

yxieca · 2017-12-28T18:04:29Z

device/mellanox/x86_64-mlnx_msn2700-r0/ACS-MSN2700/pg_profile_lookup.ini

+ 25000 300m 71680 18432 53248 1
+ 40000 300m 94208 18432 75776 1
+ 50000 300m 94208 18432 75776 1
+ 100000 300m 184320 18432 165888 1


I didn't see matching code using this look up table. Who is using it where and how?

it is passed as an argument for buffermgrd: https://github.com/Azure/sonic-buildimage/pull/1250/files#diff-849a3366595afffe34a13f4a543dca56

yxieca

The change generally looks good to me, with some minor issues inlined. These comments probably would apply to all commits. I didn't copy them to the next 2.

For converting the old buffer.json file to the new template, if possible, can you come up with a converting tool to take the source file and generate a template?

This tool would be really useful for other platforms under work.

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

liatgrozovik · 2018-01-15T15:08:04Z

following a session on that PR, it is agreed that the tool is nice to have and is not blocking the PR to be merged. But a wiki should be provided inorder to indicate how to convert an old configuration file to the new format. This should be done on another PR.

@andriymoroz-mlnx let's fix the build issues and move on to merge.

Conflicts: dockers/docker-orchagent/msn27xx.32ports.buffers.json.j2 dockers/docker-orchagent/swssconfig.sh src/sonic-config-engine/tests/sample_output/msn27.32ports.json

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

lguohan · 2018-01-16T08:48:49Z

@liatgrozovik , pending on sonic-net/sonic-swss#417, other PRs are merged.

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

- fixed support of sonic-to-sonic install Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

This reverts commit 814e50f.

…1340) This reverts commit 814e50f.

ccb52454a11e6906bb074d888740d279e4a3c8e3 (HEAD -> 201911, origin/201911) [fast-reboot] Fix fast-reboot when NDP entries are present (#1295) d09667b86abb7d3cd31b92bedf6e4d4bdac4937f Multi-ASIC support for show ip(v6) route (201911 branch) (#1283) 28399bfcad2a40f1a85095bc679540531c4e673c [201911-Mellanox] SKU creator Tool (#1163) (#1250) Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

ce8b1af9d2200c570c102a38e04cbd9659470585 (HEAD -> 201911, origin/201911) const initializer_list is not a constant expression (#1250) acdb03324fc268f5330d2632a02d0856f35fb880 [201911] Add lgtm.yml (#1901) Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

andriymoroz-mlnx added 3 commits December 19, 2017 11:13

Move buffer configuration to ConfigDB

8980911

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

Converted Dell and Arista configs

bbeb420

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

Add buffer configs for ACS-MSN2740

0b4afca

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

andriymoroz-mlnx changed the title ~~Buffers port speed~~ Buffers configuration update on port speed change Dec 21, 2017

yxieca reviewed Dec 28, 2017

View reviewed changes

Updated buffers template

2452364

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

andriymoroz-mlnx added 2 commits January 15, 2018 18:09

Merge branch 'master' into buffers_port_speed

459626e

Conflicts: dockers/docker-orchagent/msn27xx.32ports.buffers.json.j2 dockers/docker-orchagent/swssconfig.sh src/sonic-config-engine/tests/sample_output/msn27.32ports.json

Fixed j2 unit test

b9a8da7

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

andriymoroz-mlnx added 3 commits January 18, 2018 15:12

Update buffers config for Force10-S6100

06dab88

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

Update VS docker to support speed and buffers test

ff8332f

Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

Update buffers config generation

be97cfd

- fixed support of sonic-to-sonic install Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>

andriymoroz-mlnx mentioned this pull request Jan 24, 2018

Add speed and buffer set test sonic-net/sonic-swss#432

Merged

yxieca merged commit 814e50f into sonic-net:master Jan 26, 2018

yxieca added a commit that referenced this pull request Jan 26, 2018

Revert "Buffers configuration update on port speed change (#1250)"

ca46aab

This reverts commit 814e50f.

yxieca mentioned this pull request Jan 26, 2018

Revert "Buffers configuration update on port speed change" #1340

Merged

lguohan pushed a commit that referenced this pull request Jan 26, 2018

Revert "Buffers configuration update on port speed change (#1250)" (#…

2b91c96

…1340) This reverts commit 814e50f.

dzhangalibaba mentioned this pull request Apr 20, 2020

[sonic-py-swsssdk/snmpagent] update submodule for sonic-py-swsssdk / snmpagent #4421

Merged

judyjoseph mentioned this pull request Apr 25, 2020

[Multi-ASIC] To pass the asic instance ID to orchagent, Advance the swss, swss-common submodules. #4465

Merged

theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022

const initializer_list is not a constant expression (sonic-net#1250)

58627af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffers configuration update on port speed change #1250

Buffers configuration update on port speed change #1250

andriymoroz-mlnx commented Dec 19, 2017 •

edited

Loading

lguohan commented Dec 20, 2017

yxieca Dec 28, 2017

andriymoroz-mlnx Jan 9, 2018

yxieca Dec 28, 2017

andriymoroz-mlnx Jan 9, 2018

yxieca Dec 28, 2017

andriymoroz-mlnx Jan 9, 2018

yxieca Dec 28, 2017

andriymoroz-mlnx Jan 9, 2018

yxieca left a comment

liatgrozovik commented Jan 15, 2018

lguohan commented Jan 16, 2018

Buffers configuration update on port speed change #1250

Buffers configuration update on port speed change #1250

Conversation

andriymoroz-mlnx commented Dec 19, 2017 • edited Loading

lguohan commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxieca left a comment

Choose a reason for hiding this comment

liatgrozovik commented Jan 15, 2018

lguohan commented Jan 16, 2018

andriymoroz-mlnx commented Dec 19, 2017 •

edited

Loading