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

gen-mg broken for multi-asic kvm - issue #4343 #4420

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

sanmalho-git
Copy link
Contributor

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911

Approach

What is the motivation for this PR?

Fix for issue #4343 when running gen-mg for multi-asic kvm.

How did you do it?

PR #3746 introduced generating minigraphs for multi-asic linecards in a VoQ chassis.

In generating minigraph for mulit-asic linecards in a VoQ chassis, switchids are required to be defined per asic. But, switchid for each asic doesn't exist for multi-asic KVM (pizza box) switchid. To fix this:

  • Use empty list as the default for switchids in the call to port_alias.

    • Originally the default was a list [0], but with multi-asic KVM,
  • In port_alias ansible library, check the length of switchids before using it.

  • Also, in minigraph_png, made the following fixes:

    • For supporting not all ports connected to fanout, in minigraph_png.j2, we added DeviceInterfaceLink for only ports that are defined in the device_conn (from connection graph). But, for masic KVM, device_conn is not defined, and thus we were not adding any port. The fix is that if the inventory_hostname is not present in the device_conn, then add DeviceInterfaceLink for all front_panel_asic_ifnames
    • 'Bandwidth' for each DeviceInterfaceLink was hard-coded to 40G, we added code to get it from port_speed which also gets populated from the connection graph. However, port_speed is empty for KVM, and thus generation was failing. Fix is to check if the port is present in port_speed, and if so, use the speed defined in port_speed. Else, use default 40G.
  • Reverted PR Revert "Support for gen-mg for multi-asic linecards in VoQ chassis" #4351

How did you verify/test it?

Ran the steps defined in #4343, and made sure that the generated minigraph with the 'master' code (which reverted PR #3746) and the above code changes are the same.

Also, ran for multi-asic and single-asic linecards of a VoQ Chassis - as long a switchids are defined in the ansible inventory for each linecard, it works.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

- Use empty list as the default for switchids in the call to port_alias.
- In port_alias ansible library, check the length of switchids before using it.
- In minigraph_png,
  - For supporting not all ports connected to fanout, in minigraph_png.j2, we added DeviceInterfaceLink for only ports that are defined in the device_conn (from connection graph). But, for masic KVM,
device_conn is not defined, and thus we were not adding any port. The fix is that if the inventory_hostname is not present in the device_conn, then add DeviceInterfaceLink for all front_panel_asic_ifnames
  - 'Bandwidth' for each DeviceInterfaceLink was hard-coded to 40G, we added code to get it from port_speed which also gets populated from the connection graph. However, port_speed is empty for KVM, and thus generation
was failing. Fix is to check if the port is present in port_speed, and if so, use the speed defined in port_speed. Else, use default 40G.
Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

@sanmalho-git

I have fixed the multi-asic issue also took your previous commit (multi-asic graph) in the PR: #4419

I would prefer to get my PR merge to avoid merge conflict. Are you ok with that ?

@sanmalho-git
Copy link
Contributor Author

sanmalho-git commented Oct 11, 2021

@abdosi I am ok with taking changes from this PR. You have included all the changes in my PR #4420 to fix issue #4343. I will close my PR

@abdosi abdosi reopened this Oct 12, 2021
@abdosi abdosi closed this Oct 12, 2021
@abdosi abdosi reopened this Oct 12, 2021
@abdosi
Copy link
Contributor

abdosi commented Oct 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@abdosi abdosi merged commit 9eda7d0 into sonic-net:master Oct 13, 2021
abdosi pushed a commit to abdosi/sonic-mgmt that referenced this pull request Oct 13, 2021
* Revert "Revert "[chassis] Support for gen-mg for multi-asic linecards in VoQ chassis (sonic-net#3746)" (sonic-net#4351)"

This reverts commit aa9c059.

* Fix for issue sonic-net#4343 when running gen-mg for multi-asic kvm

- Use empty list as the default for switchids in the call to port_alias.
- In port_alias ansible library, check the length of switchids before using it.
- In minigraph_png,
  - For supporting not all ports connected to fanout, in minigraph_png.j2, we added DeviceInterfaceLink for only ports that are defined in the device_conn (from connection graph). But, for masic KVM,
device_conn is not defined, and thus we were not adding any port. The fix is that if the inventory_hostname is not present in the device_conn, then add DeviceInterfaceLink for all front_panel_asic_ifnames
  - 'Bandwidth' for each DeviceInterfaceLink was hard-coded to 40G, we added code to get it from port_speed which also gets populated from the connection graph. However, port_speed is empty for KVM, and thus generation
was failing. Fix is to check if the port is present in port_speed, and if so, use the speed defined in port_speed. Else, use default 40G.
@sanmalho-git sanmalho-git deleted the gen-mg2 branch November 15, 2021 14:47
SuvarnaMeenakshi added a commit that referenced this pull request May 2, 2022
…eLink (#5538)

What is the motivation for this PR?
DeviceInterfaceLink for front panel interface to ASIC interface is not getting generated for multi-asic platform.
#4420 introduced a change in minigraph_png.j2 template which caused this change.

How did you do it?
Fix png template so that DeviceInterfaceLink with front panel interface to asic interface is generated for all platforms.

How did you verify/test it?
Verified on multi-asic platform, DeviceInterfaceLink with front panel interface to asic interface mapping is generated correctly.
In multi-asic linecard minigraph will nclude all DeviceInterfaceLinks.
In multi-asic linecard, DEVICE_NEIGHBOR table in default config_db gets populated.
wangxin pushed a commit that referenced this pull request May 5, 2022
…eLink (#5538)

What is the motivation for this PR?
DeviceInterfaceLink for front panel interface to ASIC interface is not getting generated for multi-asic platform.
#4420 introduced a change in minigraph_png.j2 template which caused this change.

How did you do it?
Fix png template so that DeviceInterfaceLink with front panel interface to asic interface is generated for all platforms.

How did you verify/test it?
Verified on multi-asic platform, DeviceInterfaceLink with front panel interface to asic interface mapping is generated correctly.
In multi-asic linecard minigraph will nclude all DeviceInterfaceLinks.
In multi-asic linecard, DEVICE_NEIGHBOR table in default config_db gets populated.
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.

2 participants