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

[multi-dut] Support for gen-mg to work with multi-dut where VM's connect to only a single host and supervisor card #2700

Merged
merged 4 commits into from
Feb 8, 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)

Approach

What is the motivation for this PR?

For SONiC Chassis support gen-mg has the following limitations:

  • for multi-dut it assumes each VM is connected to all the DUT's (similar to the dualtor topology)
  • assumes that all interfaces connected to a PortChannel are sequential.

It also needs the following enhancements:

  • In a SONiC Chassis we have a 'supervisor' card which has no ports and thus no BGP/loopback/IpInterfaces/ACL's etc. The minigraph should not include info about these.
  • We need to introduce 'T3' router type as 'CoreRouter'

How did you do it?

For VMs to connect to a single DUT:

  • In vm_topo_config, the fields like interface_indexes, intfs, ip_intf etc would be either empty or null. Add checks for these where they are being used in gen-mg (config_sonic_basedon_testbed.yml and the minigraph templates).

For not all PortChannel ports being sequential:

  • In config_sonic_based_on_testbed.yml, changed interface_to_vms structure to be compatable to allow using ansible's 'with_subelements' loop to get the intf_names

For SONiC Chassis support:

  • For 'supervisor' card, there are no BGP peers, interfaces etc. required.
    • A 'supervisor' card is identified by having 'type' field in the inventory with value 'supervisor'.
    • Add this check to skip generation of portions not valid for supervisor card
  • Added 'CoreRouter' as the DUT type for T3 VM's that connect to 'T2' DUT.

How did you verify/test it?

  • Ran gen-mg against all the testbeds defined in vtestbed.csv with latest github master code, and then with our code changes. Verified that there are no differences in the generated minigraph files.

  • Ran against topo_t2.yml proposed in PR Introduce basic T2 topology #2638 where we have 3 linecards with 24 VM's each (8 on 2 port LAG and 16 on single port) and 1 supervisor card.

    • Validated generated minigraph files for the linecards are the same as those generated by running gen-mg against individual linecard testbed (with only 1 DUT).
    • Validate the generated minigraph file for supervisor card is acceptable to sonic-cfggen and the generated config_db.json is valid for the supervisor card.

Any platform specific information?

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

Documentation

@sanmalho-git
Copy link
Contributor Author

retest vs image

@sanmalho-git
Copy link
Contributor Author

@wangxin @yxieca - can you please review

@yxieca yxieca requested review from theasianpianist, a team and wangxin January 19, 2021 01:58
@@ -134,7 +134,8 @@

- name: find all interface indexes mapping connecting to VM
set_fact:
interface_to_vms: "{{ interface_to_vms|default({}) | combine({ item.key: item.value['interface_indexes'][dut_index|int] }) }}"
interface_to_vms: "{{ interface_to_vms|default([]) + [ {'name': item.key, 'ports': item.value['interface_indexes'][dut_index|int] }] }}"
when: item.value|length
Copy link
Collaborator

Choose a reason for hiding this comment

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

The item value is vm config dict gathered by the topo_facts module. The length filter will return number of keys in the vm config dict. According to current implementation, I don't see a chance that the length could be 0. Is the original purpose to check if item.value['interface_indexes'] is empty?

{%- for index in range(vms_number) %}
{% if 'port-channel' in vm_topo_config['vm'][vms[index]]['ip_intf'][dut_index|int]|lower %}
PortChannel{{ ((index+1) |string).zfill(4) }}{% if not loop.last %};{% endif %}
{% set a_intf = 'PortChannel' + ((index+1) |string).zfill(4) %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

PortChannel is based on physical interfaces in vm_topo_config['vm'][vms[index]]['intfs'][dut_index|int].
If there is no physical interface between VM and DUT, there would be no PortChannel and DUT. Shall we make this part conditional too based on vm_topo_config['vm'][vms[index]]['intfs'][dut_index|int]|length like the change just below?

ansible/templates/minigraph_dpg.j2 Show resolved Hide resolved
Copy link
Contributor

@saravanansv saravanansv left a comment

Choose a reason for hiding this comment

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

similar to ansible/templates/minigraph_*.j2 should there be a ansible/templates/minigraph_supe.j2 that holds the template for all the supervisor related config ?

@sanmalho-git
Copy link
Contributor Author

At the time of putting this PR, the configuration required for supervisor card was not finalized. For Nokia supervisor card, we have a bare minimal config. If we require a template, then we can remove the 'supervisor' related changes from this PR and a subsequent PR can handle the generation of minigraph for supervisor card.

@saravanansv
Copy link
Contributor

At the time of putting this PR, the configuration required for supervisor card was not finalized. For Nokia supervisor card, we have a bare minimal config. If we require a template, then we can remove the 'supervisor' related changes from this PR and a subsequent PR can handle the generation of minigraph for supervisor card.

I'm okay with merging this change. Just wondering what other changes are needed for supervior's config

@wangxin
Copy link
Collaborator

wangxin commented Feb 5, 2021

@sanmalho-git Can you resolve the merge conflict?

…a single host; support for supervisor card.

gen-mg supported multi-dut, but in a dualtor topology, each VM is assumed to
be connected to all the DUT's. To support the VM to be connected to a single DUT:
 - In vm_topo_config, the fields like interface_indexes, intfs, ip_intf etc would be
   either empty or null. Add checks for these where they are being used in gen-mg

gen-mg also assumes that all interfaces connected to a PortChannel are sequential. To fix this:
 - In config_sonic_based_on_testbed.yml, changed interface_to_vms structure to be compatable to
   allow using with_subelements to get the intf_names

For 'supervisor' card, there are no BGP peers, interfaces etc. required.
 - A 'supervisor' card is identified by having 'type' field in the inventory with value 'supervisor'.
 - Add this check to skip generation of portions not valid for supervisor card
@wangxin
Copy link
Collaborator

wangxin commented Feb 7, 2021

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wangxin wangxin merged commit 2b00e37 into sonic-net:master Feb 8, 2021
@sanmalho-git sanmalho-git deleted the genmg branch February 8, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants