Adding Minigraph Parsing Test Case to test_fgnhg.py by kktheballer · Pull Request #2500 · sonic-net/sonic-mgmt

kktheballer · 2020-11-11T05:23:02Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
[ x] Test case(new/improvement)

Approach

What is the motivation for this PR?

To be able to pull FG_ECMP information from DUT's minigraph should that be the user's preference
for test

How did you do it?

Added minigraph parsing for FG_NHG data in minigraph_facts and added test case to test_fgnhg.py

How did you verify/test it?

Verified its functionality with an appropriate DUT in STARlab

Any platform specific information?

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

lgtm-com · 2020-11-11T05:40:19Z

This pull request introduces 1 alert when merging 88119b5b0ec21f6d43187b0787795be579bb5f22 into 401a796 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

anish-n · 2020-12-11T03:12:47Z

tests/ecmp/test_fgnhg.py

Why do we need to copy to duthost, here? Isn't FG_NHG already programmed by the minigraph?

Removed this function altogether

anish-n · 2020-12-11T03:13:08Z

tests/ecmp/test_fgnhg.py

nit: remove print and only have log

Function is removed

anish-n · 2020-12-11T03:15:29Z

tests/ecmp/test_fgnhg.py

Does duthost.minigraph_facts call into the switch to get the minigraph facts? If that is the case, I would recommend doing a single call of duthost.minigraph_facts instead of multiple in setup_neighbors and generate_fgnhg. This should help reduce test time and improve performance of test.

This function is removed

anish-n · 2020-12-11T03:17:19Z

tests/ecmp/test_fgnhg.py

I see multiple spacing changes throughout the file, is this because of some IDE? I would prefer that we keep the original formats unless there are spacing issues in the original file.

Spaces have been reverted

anish-n · 2020-12-11T03:17:52Z

tests/ecmp/test_fgnhg.py

Spacing comment

anish-n · 2020-12-11T03:18:04Z

tests/ecmp/test_fgnhg.py

Spacing comment

anish-n · 2020-12-11T03:20:09Z

tests/ecmp/test_fgnhg.py

Recommend using mg_facts defined here, across all the minigraph config generation code as an optimization.

anish-n · 2020-12-11T03:21:29Z

tests/ecmp/test_fgnhg.py

Cleanup extra space here

lgtm-com · 2020-12-18T01:29:59Z

This pull request introduces 2 alerts when merging 30077a236f8f3a9d56ac6a6f3fa5173e597e1b83 into 0c8fcd4 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-12-24T07:32:36Z

This pull request introduces 3 alerts when merging 2f3200b4775d7d25d4828a88325d136c9062d530 into ec6cd52 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Except block handles 'BaseException'
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-12-24T21:42:14Z

This pull request introduces 3 alerts when merging 8524854fcaa3bcfcb603a85a3ee7d2407245ca06 into ec6cd52 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Except block handles 'BaseException'
1 for Module is imported with 'import' and 'import from'

anish-n · 2020-12-28T20:38:14Z

tests/ecmp/test_fgnhg.py

What is the output of the replace etp command here, I have some doubts about how this is functioning. bank_x_port lists the ptf interface id, etpxx does not correspond to the ptf interface id and they in fact differ by 1. Eg etp1 corresponds to interface 0 of ptf.
Why do we not get Ethernetxx as interface names but rather etpxx?
Can we refer to the configure_interfaces to use similar logic to obtain the interface id of ptf?

cfg_facts['port_index_map'] offers a Ethernet to etp map but not an etp to ptf port map.
Is there some other way of getting this mapping? We may need to add some checks for port range
that is entered in the parsed minigraph itself

cfg_facts['port_index_map'] offers a map from Ethernet to index, starting from 0. This corresponds to the ptf interfaces and does not offer a "Ethernet to etp map". REF: https://github.com/Azure/sonic-mgmt/blob/ed0d49321a25d21e9e331c523863b28c7e2c0789/ansible/library/config_facts.py#L75

On whether there is an etp to Ethernet to ptf index map, I am not sure on that. I would recommend checking some of the other tests if they have something like this, I will try to check it as well. One suggestion I have is that the "link" field in FG_NHG_MEMBER in cfg_facts may already contain what you need here since it will have an Ethernetxx mapped to the IP.

anish-n · 2021-01-22T02:32:06Z

tests/ecmp/test_fgnhg.py

Can we move the parsing and config obtaining part to a different function, ie: line 347 to 394 in a separate function and return the relevant values from that function?

Sure will do that

anish-n · 2021-01-22T02:32:15Z

tests/ecmp/test_fgnhg.py

Remove commented code?

anish-n · 2021-01-22T02:34:52Z

tests/ecmp/test_fgnhg.py

Just a suggestion: see a lot of diffs with spacing changes, just wondering why that is, if we can limit that then that would be great, but not a big thing even if we commit as-is.

Let me look into. This always happens , I'm not sure exactly why. I will try to minimize the space delta as much as possible

Gives user the option of obtaining FG_NHG config from device rather than just injecting synthetic values. Addresses first round of code review comments 1. Added more error handling 2. Took out some unecessary functions 3. Fix spacing 4. Variable name changes

ALso: 1. minigraph_facts.py parsing updated 2. ACS-MSN3800 SKU added to port_utils.py

anish-n · 2021-02-02T22:21:17Z

tests/ecmp/test_fgnhg.py

+
+        # IPv4 test
+        duthost.command('config interface ip add Vlan' + str(DEFAULT_VLAN_ID) + ' ' + str(default_vlan_ipv4_mg))
+        generate_fgnhg_config(duthost, ipv4_to_port_mg, bank_0_port_ipv4_mg, bank_1_port_ipv4_mg, prefix_ipv4_mg)


Why do we need generate_fgnhg_config if we are using minigraph? Same comment for IPv6 below

#### Why I did it Update for following swss commits: 96180bf - 2023-01-13 : [202012] Bfd default multiplier change (sonic-net#2615) [siqbal1986] 07506ac - 2023-01-11 : Add missing parameter to on_switch_shutdown_request method. (sonic-net#2567) [Hua Liu] 3253cc8 - 2022-11-30 : Use github code scanning instead of LGTM (sonic-net#2546) [Liu Shilong] f4df524 - 2023-01-11 : [orchagent]:add local_discriminator to state_db (sonic-net#2587) [Baorong Liu] f3cd02d - 2022-12-05 : [202012][muxorch] Adding case for maintaining current state (sonic-net#2500) [Nikola Dancejic]

…#12974) utilities: * 4b51e41 2022-12-06 | [config] Add check in config interface ip command to block if the interface is portchannel member (sonic-net#2539) (HEAD -> 202205) [Sudharsan Dhamal Gopalarathnam] * e53b32e 2022-12-06 | [generate_dump] [Mellanox] Fix the duplicate dfw dump collection problem by adding symlinks (sonic-net#2536) [Vivek] * 0391221 2022-12-02 | [GCU] Add RemoveCreateOnlyDependency Validator/Generator (sonic-net#2500) [jingwenxie] * e3658e9 2022-04-13 | [scripts/fast-reboot] Shutdown remaining containers through systemd (sonic-net#2133) [Stepan Blyshchak] swss: * 1a4a5d9 2022-12-02 | [ACL] Support ACTION_COUNTER action in custom ACL table type (sonic-net#2550) [bingwang-ms] * 33b0a9e 2022-12-05 | [muxorch] Adding case for maintaining current state (sonic-net#2280) [Nikola Dancejic] sairedis: * b29bb45 2022-12-02 | enable cisco8000 SAI bulk API feature (sonic-net#1153) (HEAD -> 202205) [Keith Lu] Signed-off-by: Ying Xie <ying.xie@microsoft.com> Signed-off-by: Ying Xie <ying.xie@microsoft.com>

sonic-net#13202 advance sonic-utilities submodule for 202211 branch 34428157 - (HEAD, origin/202211) Revert "Optimize the execution time of the 'show techsupport' script to 5-10%, (Qos config change sonic-net#2504)" (6 days ago) [stormliang] c3bd01f6 - Revert "[generate_dump] Optimize the execution time of 'show techsupport' CLI by parallel function execution ([201811][Devices] Add new device CIG CS6436-56P sonic-net#2512)" (6 days ago) [stormliang] 5a326d8b - [Mellanox] Change severity to NOTICE in Mellanox buffer migrator when unable to fetch DEVICE_METADATA due to empty CONFIG_DB during initialization ([warm boot] cherry-pick PR sonic-net#2538 and advance related sub-modules in 201811 branch sonic-net#2569) (2 weeks ago) [Stephen Sun] 50b36ef3 - Fix issue: unconfigured PGs are displayed in watermarkstat ([docker-lldp]: fix several issues in lldpd docker sonic-net#2556) (2 weeks ago) [Stephen Sun] a9fd2a79 - [Command Ref] Add doc for syslog rate limit ([sub module] move sairedis and swss to 201811 branch sonic-net#2508) (2 weeks ago) [Junchao-Mellanox] 80546ff3 - [generate_dump] Optimize the execution time of 'show techsupport' CLI by parallel function execution ([201811][Devices] Add new device CIG CS6436-56P sonic-net#2512) (2 weeks ago) [Vadym Hlushko] 6649ca8a - [timer.unit.j2] use wanted-by in timer unit ([201803] [services] Restart SwSS service upon unexpected critical process exit sonic-net#2546) (2 weeks ago) [Stepan Blyshchak] dd23d0ef - Fixes [Sub-If|VRF] Unbind sub-interface from VRF is failed sonic-net#12170: Delete subinterface and recreate the subinterface in ([VLAN] "show mac" doesn't work when interface added to vlan as tagged member sonic-net#2513) (2 weeks ago) [Preetham] 236749d3 - [db_migrator] Fix migration of Loopback data: handle all Loopback interfaces (DellEMC S6000 xcvrd support sonic-net#2560) (2 weeks ago) [Vaibhav Hemant Dixit] 5762d814 - Optimize the execution time of the 'show techsupport' script to 5-10%, (Qos config change sonic-net#2504) (2 weeks ago) [Vadym Hlushko] d3c3e368 - [muxcable][show] update show mux tunnel-route to separate ASIC and kernel into two columns (build errors on branch 201811 for centec platform sonic-net#2553) (2 weeks ago) [Jing Zhang] c98648a1 - [show]Fix show route return code on error (Dell SMF driver hwmon number reorder fix for Dell S6100/Z9100 sonic-net#2542) (2 weeks ago) [Sudharsan Dhamal Gopalarathnam] 01374673 - [route_check]: Ignore ASIC only SOC IPs (Added new SN3700/SN3700C Mellanox platforms sonic-net#2548) (2 weeks ago) [Lawrence Lee] d2967805 - YANG Validation for ConfigDB Updates: WARM_RESTART, SFLOW_SESSION, SFLOW, VXLAN_TUNNEL, VXLAN_EVPN_NVO, VXLAN_TUNNEL_MAP, MGMT_VRF_CONFIG, CABLE_LENGTH, VRF tables ([submodule 201811] advance sairedis and swss submodule for 201811 branch sonic-net#2526) (2 weeks ago) [isabelmsft] 88b01ffd - [db_migrator] Remove import of swsssdk as it is not supported in master ([build]: apply proxy setting to curl. sonic-net#2544) (2 weeks ago) [Vaibhav Hemant Dixit] 4ae970c6 - Support syslog rate limit configuration for containers and host (Move FRR from 4.0 to 6.0.2 and make new frr version and pkg compile sonic-net#2454) (2 weeks ago) [Junchao-Mellanox] 608ed147 - [generate_dump] [Mellanox] Fix the duplicate dfw dump collection problem by adding symlinks ('show vlan config' is not displaying the VLAN members, after the clear config and reload with default l2 configuration. sonic-net#2536) (2 weeks ago) [Vivek] bdc2599f - [config] Add check in config interface ip command to block if the interface is portchannel member ([sub module] advance sonic-swss sub module sonic-net#2539) (2 weeks ago) [Sudharsan Dhamal Gopalarathnam] cff4fed5 - [system-health] Improve code structure of system health CLIs ([sub-module] advance sonic-swss sub-module sonic-net#2453) (2 weeks ago) [Junchao-Mellanox] 488e5714 - Transceiver eeprom dom CLI modification to show output from TRANSCEIVER_DOM_THRESHOLD table (Fix for KeyError: 'DEVICE_NEIGHBOR' when executing 'show interfaces neighbor expected' command sonic-net#2535) (2 weeks ago) [mihirpat1] 07ca5def - sonic-utilities: Update config reload() to verify formatting of an input file ([ntp]: Do not disable reader for error ENOBUFS sonic-net#2529) (2 weeks ago) [Caitlin Choate] f0f083a2 - [GCU] Add RemoveCreateOnlyDependency Validator/Generator (Enabling Fast-reboot command in s6100 loaded with T0 topo getting Failed unmounting /host error sonic-net#2500) (2 weeks ago) [jingwenxie] eca0253c - [QoS] Introduce delay to the qos reload flow (Config reload/load_minigraph not clearing State DB sonic-net#2503) (2 weeks ago) [DavidZagury] 35158ee0 - Use github code scanning instead of LGTM ([sub module] sub module sonic-swss-common tracking 201811 branch sonic-net#2530) (2 weeks ago) [Liu Shilong] 682b5cee - Change show kube command default value of insecure key to True ([submodule] update sonic-snmpagent sonic-net#2517) (2 weeks ago) [lixiaoyuner] ce19e631 - Add db_migrator_constants.py script to setup.py (Add device data for Arista 7060PX/DX4-32 sonic-net#2534) (2 weeks ago) [Vaibhav Hemant Dixit] 0d0c2693 - [drop counters] Fix CLI script for unconfigured PGs ([config] Do not fail for minigraphs which do not have neighbors listed in <Devices> section sonic-net#2518) (2 weeks ago) [Lior Avramov] 2c69d0fd - Update vrf add, del commands for duplicate/non-existing VRFs (solve package build dependency issue sonic-net#2467) (2 weeks ago) [Muhammad Danish] efc09280 - Port 202012 DB migration changes to newer branches ([vs]: Force10-S6000 buffer settings for virtual switch sonic-net#2515) (2 weeks ago) [Vaibhav Hemant Dixit] 70a15aaa - [VXLAN]Fixing traceback in show remotemac when mac moves during command execution ([build] When generating image version, handle case where current commit has no reachable tags sonic-net#2506) (2 weeks ago) [Sudharsan Dhamal Gopalarathnam]

anish-n reviewed Dec 11, 2020

View reviewed changes

kktheballer force-pushed the master branch from 30077a2 to 2f3200b Compare December 24, 2020 07:01

kktheballer force-pushed the master branch from 2f3200b to 8524854 Compare December 24, 2020 21:17

kktheballer force-pushed the master branch from 8524854 to 8a22739 Compare December 27, 2020 21:50

anish-n reviewed Dec 28, 2020

View reviewed changes

kktheballer force-pushed the master branch from 8a22739 to a78f8b2 Compare January 12, 2021 18:12

anish-n reviewed Jan 22, 2021

View reviewed changes

kktheballer added 5 commits January 27, 2021 23:10

Few small revisions and some variable name changes

e51559b

Address LGTM warnings

d8c3c7b

Fix etp port to ptf port mapping

f44eb22

ALso: 1. minigraph_facts.py parsing updated 2. ACS-MSN3800 SKU added to port_utils.py

mg_facts ECMP field extraction done with separate function

d5cdaae

kktheballer force-pushed the master branch from a78f8b2 to d5cdaae Compare January 28, 2021 01:07

anish-n reviewed Feb 2, 2021

View reviewed changes

anish-n mentioned this pull request Mar 25, 2021

[test_fgnhg] Test enhancement for link operational change and matchmode changes to fine grained ECMP #2670

Merged

3 tasks

kktheballer requested a review from a team as a code owner March 2, 2022 17:37

Conversation

kktheballer commented Nov 11, 2020

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Uh oh!

lgtm-com bot commented Nov 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Dec 18, 2020

Uh oh!

lgtm-com bot commented Dec 24, 2020

Uh oh!

lgtm-com bot commented Dec 24, 2020

Uh oh!

anish-n Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anish-n Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anish-n Dec 28, 2020 •

edited

Loading

anish-n Feb 2, 2021 •

edited

Loading