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

Don't create the members@ array in config_db for PC when reading from minigraph #13660

Merged

Conversation

saiarcot895
Copy link
Contributor

Fixes #11873.

Signed-off-by: Saikrishna Arcot [email protected]

Why I did it

When loading from minigraph, for port channels, don't create the members@ array in config_db in the PORTCHANNEL table. This is no longer needed or used.

In addition, when adding a port channel member from the CLI, that member doesn't get added into the members@ array, resulting in a bit of inconsistency. This gets rid of that inconsistency.

How I did it

How to verify it

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

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

… minigraph

When loading from minigraph, for port channels, don't create the
members@ array in config_db in the PORTCHANNEL table. This is no longer
needed or used.

In addition, when adding a port channel member from the CLI, that member
doesn't get added into the members@ array, resulting in a bit of
inconsistency. This gets rid of that inconsistency.

Fixes sonic-net#11873.

Signed-off-by: Saikrishna Arcot <[email protected]>
@saiarcot895 saiarcot895 marked this pull request as ready for review February 6, 2023 22:20
@saiarcot895
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -532,9 +532,9 @@ def parse_dpg(dpg, hname):
intfs_inpc.append(pcmbr_list[i])
pc_members[(pcintfname, pcmbr_list[i])] = {}
if pcintf.find(str(QName(ns, "Fallback"))) != None:
pcs[pcintfname] = {'members': pcmbr_list, 'fallback': pcintf.find(str(QName(ns, "Fallback"))).text, 'min_links': str(int(math.ceil(len() * 0.75))), 'lacp_key': 'auto'}
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 10, 2023

Choose a reason for hiding this comment

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

members

Do you want to update src/sonic-yang-models/yang-models/sonic-portchannel.yang at the same time?

@ganglyu for vis. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed from the yang file.

@wen587
Copy link
Contributor

wen587 commented Feb 13, 2023

There may have many test file need to be updated in src/sonic-yang*

@saiarcot895
Copy link
Contributor Author

@ganglyu @wen587 could you re-review?

Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

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

LGTM

@qiluo-msft qiluo-msft merged commit 3e316cb into sonic-net:master Feb 23, 2023
yejianquan pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Feb 27, 2023
Signed-off-by: Saikrishna Arcot <[email protected]>

Description of PR
Summary:
Fixes sonic-net/sonic-buildimage#11873
sonic-net/sonic-buildimage#11873

Approach
What is the motivation for this PR?
The members entry in the PORTCHANNEL table has been removed. Switch to using PORTCHANNEL_MEMBERS instead.

Related to sonic-net/sonic-buildimage#13660
sonic-net/sonic-buildimage#13660
yejianquan pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Feb 27, 2023
Signed-off-by: Saikrishna Arcot <[email protected]>

Description of PR
Summary:
Fixes sonic-net/sonic-buildimage#11873
sonic-net/sonic-buildimage#11873

Approach
What is the motivation for this PR?
The members entry in the PORTCHANNEL table has been removed. Switch to using PORTCHANNEL_MEMBERS instead.

Related to sonic-net/sonic-buildimage#13660
sonic-net/sonic-buildimage#13660
@mssonicbld
Copy link
Collaborator

@saiarcot895 PR conflicts with 202205 branch

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Mar 19, 2023
… minigraph (sonic-net#13660)

Fixes sonic-net#11873.

#### Why I did it

When loading from minigraph, for port channels, don't create the members@ array in config_db in the PORTCHANNEL table. This is no longer needed or used.

In addition, when adding a port channel member from the CLI, that member doesn't get added into the members@ array, resulting in a bit of inconsistency. This gets rid of that inconsistency.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202211: #14343

mssonicbld pushed a commit that referenced this pull request Mar 20, 2023
… minigraph (#13660)

Fixes #11873.

#### Why I did it

When loading from minigraph, for port channels, don't create the members@ array in config_db in the PORTCHANNEL table. This is no longer needed or used.

In addition, when adding a port channel member from the CLI, that member doesn't get added into the members@ array, resulting in a bit of inconsistency. This gets rid of that inconsistency.
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Mar 21, 2023
KeyError: 'members' is introduced by this PR sonic-net/sonic-buildimage#13660.
In original config, we use PORTCHANNEL, now it is changed to PORTCHANNEL_MEMBER. So, the relevant test should be updated accordingly.

Test log:
  oports = []
  for ifname in ifnames:
  if po.has_key(ifname):
  oports.append([str(mg_facts['minigraph_ptf_indices'][x]) for x in po[ifname]['members']])
  E KeyError: 'members'

What is the motivation for this PR?
Fix test_inner_harhing issue

How did you do it?
PORTCHANNEL is changed to PORTCHANNEL_MEMBER

How did you verify/test it?
Run test_inner_hashing
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Mar 24, 2023
KeyError: 'members' is introduced by this PR sonic-net/sonic-buildimage#13660.
In original config, we use PORTCHANNEL, now it is changed to PORTCHANNEL_MEMBER. So, the relevant test should be updated accordingly.

Test log:
  oports = []
  for ifname in ifnames:
  if po.has_key(ifname):
  oports.append([str(mg_facts['minigraph_ptf_indices'][x]) for x in po[ifname]['members']])
  E KeyError: 'members'

What is the motivation for this PR?
Fix test_inner_harhing issue

How did you do it?
PORTCHANNEL is changed to PORTCHANNEL_MEMBER

How did you verify/test it?
Run test_inner_hashing
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Mar 28, 2023
Related work items: sonic-net#276, sonic-net#305, sonic-net#332, sonic-net#338, sonic-net#339, sonic-net#1188, sonic-net#1192, sonic-net#1197, sonic-net#1206, sonic-net#1685, sonic-net#1690, sonic-net#1696, sonic-net#1699, sonic-net#1709, sonic-net#1727, sonic-net#1737, sonic-net#1741, sonic-net#1742, sonic-net#2511, sonic-net#2512, sonic-net#2532, sonic-net#2559, sonic-net#2626, sonic-net#2638, sonic-net#2645, sonic-net#2649, sonic-net#2660, sonic-net#2669, sonic-net#2670, sonic-net#2678, sonic-net#10084, sonic-net#11442, sonic-net#11873, sonic-net#12047, sonic-net#12110, sonic-net#12207, sonic-net#12529, sonic-net#12678, sonic-net#13235, sonic-net#13287, sonic-net#13372, sonic-net#13395, sonic-net#13456, sonic-net#13497, sonic-net#13522, sonic-net#13545, sonic-net#13547, sonic-net#13552, sonic-net#13569, sonic-net#13572, sonic-net#13578, sonic-net#13591, sonic-net#13611, sonic-net#13647, sonic-net#13649, sonic-net#13660, sonic-net#13710, sonic-net#13716, sonic-net#13724, sonic-net#13726, sonic-net#13732, sonic-net#13735, sonic-net#13739, sonic-net#13757, sonic-net#13786, sonic-net#13792, sonic-net#13800, sonic-net#13801, sonic-net#13802, sonic-net#13805, sonic-net#13806, sonic-net#13812, sonic-net#13814, sonic-net#13822, sonic-net#13831, sonic-net#13834, sonic-net#13847, sonic-net#13870, sonic-net#13882, sonic-net#13884, sonic-net#13885, sonic-net#13894, sonic-net#13895, sonic-net#13926, sonic-net#13932, sonic-net#13935, sonic-net#13942, sonic-net#13951, sonic-net#13953, sonic-net#13964
@anamehra
Copy link
Contributor

Hi @saiarcot895, I am seeing internal back plane port channels in 'show interface portchannel'. It used to display only front-end port channels and '-d all' used to display all, as expected. Could this behavior change be due to the changes in this PR?
@abdosi , FYI.

@rlhui
Copy link
Contributor

rlhui commented Mar 29, 2023

@anamehra , please file an issue for this? Thanks.

@anamehra
Copy link
Contributor

@anamehra , please file an issue for this? Thanks.

Hi @rlhui , raised #14459

@rlhui
Copy link
Contributor

rlhui commented Mar 29, 2023

@saiarcot895 , @yxieca - is this PR indeed needed in 202205? please share more info, thanks

saiarcot895 added a commit to saiarcot895/sonic-buildimage that referenced this pull request Mar 31, 2023
In `show interface portchannel` and `show ip route`, backend port
channels and routes were being displayed. This is due to changes in sonic-net#13660.
Fix these issues by switching to reading from PORTCHANNEL_MEMBERS table
instead.

Fixes sonic-net#14459.

Signed-off-by: Saikrishna Arcot <[email protected]>
@saiarcot895
Copy link
Contributor Author

@rlhui This PR was to fix an inconsistency in how port channel members were specified in config_db.json. On regular T0 and T1 KVM devices, it appeared that there weren't any users of the members array, and that on the test script side, there appeared to be only one test case that used the members array. However, that only covered the KVM test cases, so I didn't notice the other test failures.

I requested it for 202205 and 202211 since it is a bug fix (there could be different port channel members returned depending on which table is used) and it didn't appear to cause any breakages, but because I didn't check the hardware test cases and I didn't check multi-asic, I didn't notice there would be breakages there.

rlhui pushed a commit that referenced this pull request Apr 15, 2023
* Fix backend port channels and routes being displayed
In `show interface portchannel` and `show ip route`, backend port
channels and routes were being displayed. This is due to changes in #13660.
Fix these issues by switching to reading from PORTCHANNEL_MEMBERS table
instead.
Fixes #14459.
* Replace table name with constant

Signed-off-by: Saikrishna Arcot <[email protected]>
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Apr 19, 2023
* Fix backend port channels and routes being displayed
In `show interface portchannel` and `show ip route`, backend port
channels and routes were being displayed. This is due to changes in sonic-net#13660.
Fix these issues by switching to reading from PORTCHANNEL_MEMBERS table
instead.
Fixes sonic-net#14459.
* Replace table name with constant

Signed-off-by: Saikrishna Arcot <[email protected]>
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.

[LAG] Inconsistency in LAG members configuration in config DB
9 participants