add test case bgp multipath relax by maggiemsft · Pull Request #284 · sonic-net/sonic-mgmt

maggiemsft · 2017-09-22T18:28:28Z

solve issue #283

lguohan · 2017-09-25T16:24:27Z

ansible/library/bgp_route.py

+def main():
+    module = AnsibleModule(
+            argument_spec=dict(
+               show_cmd=dict(required=False, default=''),


can you define parameter like
neighbor=?
direction=(adv/rcv)
prefix=?

direction is only available when neighbor is specified.

It it better for you to ask user to give parameter instead of letting use to give you the raw commands.
#Resolved

changed with the suggestion #Resolved

lguohan · 2017-09-25T16:25:40Z

ansible/roles/eos/templates/t1-64-lag-tor.j2

+{% endfor %}
+{% set index = 1 %}
+{% for subnet in host['vips']['ipv4']['range'] %}
+ip prefix-list test_vip_{{ index }} seq 10 permit {{ subnet }}


you also need to increase the seq here

lguohan · 2017-09-25T16:27:45Z

ansible/roles/eos/templates/t1-64-lag-tor.j2

+{% set index = index + 1 %}
+{% endfor %}
+{% set index = 1 %}
+{% for subnet in host['vips']['ipv4']['range'] %}


you can use only one subnet loop and add ip route, prefix-list and route-map in the loop.

I was thinking of it when I first built the template, since this template create the startup configure file directly, not configuration input sequence , and I looked at the actual EOS config, prefixes are in one section and ip route are in one section, I am not 100% sure if I mixed them would be OK, this way is more safe.

lguohan · 2017-09-25T16:28:33Z

ansible/roles/eos/templates/t1-64-lag-tor.j2

+{% endfor %}
+{% set index = 1 %}
+{% for subnet in host['vips']['ipv4']['range'] %}
+route-map PREPENDAS permit 2{{ index }}


what is 2{{ index }}, give a better name? vip_{{ index }}

This is router-map read/apply sequence number. not a name. so has to be int. we don't have complicated route-map here, it's just a random picked 21, 22, ...

lguohan · 2017-09-25T16:35:03Z

ansible/vars/topo_t1-64-lag.yml

        ipv6: fc00::4a/126
+      vips:
+        ipv4: 
+          range: 


range -> prefix

lguohan · 2017-09-25T16:41:12Z

ansible/roles/test/tasks/bgp_multipath_relax.yml

+     that:
+         - vips_prefix in bgp_rt_neiadv.keys()
+         - bgp_rt_neiadv[vips_prefix]['paths'] | length == 2
+         - vips_asn in bgp_rt_neiadv[vips_prefix]['paths']


can you check if vips_asn is the last one? vips_asn == bgp_rt_neiadv[vips_prefix]['paths'][-1]?

lguohan · 2017-09-25T16:51:21Z

ansible/roles/test/tasks/bgp_multipath_relax.yml

+     vips_prefixes: "{{ configuration[item]['vips']['ipv4']['range'] }}"
+     vips_asn: "{{ configuration[item]['vips']['ipv4']['asn'] }}"
+  with_items: "{{ dut_t0_nei }}"
+  when:  "{{ 'vips' in configuration[item].keys() }}" 


this seems little bit complicated, and it is not guarantee to work if not all t0 advertise same vip prefixes. The assumption here is that all t0 advertising same set of vip prefixes.

I would suggest to add vip section (duplicated) in configuration_properties

vips:
prefix:
- 20.0.1.0/26
t0:
- arista01t0
- arista01t1

it is duplicated data, but the ansible logic is easier.

yeah, this would only work when all vips advertise same set of prefixes. change the topology yaml file like suggested here, will need carefully synced with configuration with each VM and the topology for whole testbed.
I changed the playbook to pick a prefix to validate that prefix's path.

lguohan

as comments.

lguohan · 2017-09-29T16:43:41Z

ansible/library/bgp_route.py

+  bgp_route: show_cmd='100.0.0.1'
+
+- name: Get BGP route information
+  bgp_route: show_cmd='neighbor 10.0.0.1 advert'


change according to the new params

lguohan · 2017-09-29T16:49:09Z

ansible/roles/test/tasks/bgp_multipath_relax.yml

+
+- name: remove duplicate entries
+  set_fact:
+     vips_prefixes: "{{ vips_prefixes | unique }}"


why do we need another unique as we did the unique in line 44 already?

you are correct, this is duplicated, there were a syntax error for filter which didn't apply unique

lguohan · 2017-09-29T16:51:16Z

ansible/roles/test/tasks/bgp_multipath_relax.yml

+     vips_asn: "{{ configuration[item]['vips']['ipv4']['asn'] }}"
+  with_items:  "{{ dut_t0_nei }}"
+  when: vips_prefix in "{{ configuration[item]['vips']['ipv4']['prefixes'] }}"
+


need to verify the len(vips_t0) > 1 to multipath relax test, if the len() is <= 1, then we should not test and return failure.

fixed, before I wrote a presumption that prefix is multipathed, but this check is much better, thanks

lguohan · 2017-09-29T16:52:57Z

ansible/library/bgp_route.py

+def main():
+    module = AnsibleModule(
+            argument_spec=dict(
+                neighbor=dict(required=False, default=''),


can default be None? #Resolved

yes, this looks better #Resolved

lguohan · 2017-09-29T16:53:36Z

ansible/library/bgp_route.py

+        bgproute = BgpRoutes()
+        regex_ip = re.compile('[0-9a-fA-F.:]+')
+        regex_iprange = re.compile('[0-9a-fA-F.:]+\/\d+')
+        if neigh == '' and direction == '' and prefix == '':


if neigh == None and direction == None and prefix == None

…sary steps

lguohan · 2017-10-03T06:57:56Z

ansible/library/bgp_route.py

+        * show ip bgp 100.0.0.1  ### show ip bgp prefix info
+        * show ip bgp neighbor 10.0.0.1 adv   ### show ip bgp neighbor address advertised routes
+    - Retrieved facts will be inserted into the 'bgp_rt'  or 'bgp_rt_neiadv'
+'''


still lacking proper documentation of the module, such as requirements, options sections.

http://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html

In reply to: 142325420 [](ancestors = 142325420)

lguohan · 2017-10-03T06:58:43Z

ansible/library/bgp_route.py

+    module = AnsibleModule(
+            argument_spec=dict(
+                neighbor=dict(required=False, default=None),
+                direction=dict(required=False, default=None),


direction=dict(required=False, default=None [](start = 16, length = 43)

please use choices for direction option, check lldp_facts.py for the example. #Resolved

lguohan · 2017-10-03T08:34:01Z

ansible/library/bgp_route.py

+    def get_facts(self):
+        return self.facts
+
+    def parse_bgp_rt_adv(self, cmd, cmd_result):


parse_bgp_rt_adv(self, cmd, cmd_result): [](start = 8, length = 40)

we should do some unit test code to test your parse logic.

check here.

http://docs.ansible.com/ansible/latest/dev_guide/testing_units.html
#WontFix

lguohan · 2017-10-03T08:39:40Z

ansible/library/bgp_route.py

+                    continue
+            else:
+                if not re.match('^\*', line):
+                    continue


add comment here. only parse the valid route entries. #Resolved

lguohan · 2017-10-03T08:41:13Z

ansible/library/bgp_route.py

+        self.facts['bgp_rt_neiadv']['neighbor'] = neighbor
+        ### so far parsing by fixed length table
+        ### will improve this if find better way to parse it
+        header_position = [0, 3, 20, 40, 47, 54, 61]


why not use regx split('\s+') into several fields for a line. #Resolved

well, not every entry has all fields, so cannot split('\s+')
like following, loopback route has a matric of 0 which rest of the example lines do not have that field:
Network Next Hop Metric LocPrf Weight Path
*> 0.0.0.0 10.0.0.32 0 65200 ?
*> 10.1.0.32/32 10.0.0.32 0 32768 i
*> 100.1.0.1/32 10.0.0.32 0 65200 i #Resolved

I do not see the differences.

In reply to: 144104841 [](ancestors = 144104841)

lguohan · 2017-10-03T08:44:04Z

ansible/library/bgp_route.py

+        regex_prefix_paths = re.compile('^[0-9\s]+$')
+        regex_prefix_path_line1 = re.compile('.* from .*\([0-9a-fA-F.:]+\)')
+        regex_prefix_path_line2 = re.compile('\s+Origin')
+        regex_prefix_path_line3 = re.compile('\s+Last update:')


it is better to give each line a meaning instead of 1,2,3... #Resolved

for example, line1 is source, line 2 is the attributes, line 3 is the update_time.

In reply to: 142343743 [](ancestors = 142343743)

lguohan · 2017-10-03T08:46:51Z

ansible/library/bgp_route.py

+            self.facts['bgp_rt'][prefix]['found'] = False
+            return
+        result_lines = cmd_result.split('\n')
+        ## search_state = ['header', 'path_prop', 'prefix_prop1', 'prefix_prop2', 'paths', 'path_l1', 'path_l2', 'path_l3', 'err' ]


search_state = ['header [](start = 8, length = 26)

you seems like to like to compare strings, but it is better to define constant and then use the contant

for example,
HEADER_LINE=1
XXX_LINE=2
... #Resolved

lguohan

as comments.

lguohan · 2017-10-17T17:40:17Z

ansible/library/bgp_route.py

+        if regex_neighbor.match(cmd):
+            neighbor = regex_neighbor.match(cmd).group(1)
+        else:
+            raise Exception('cannot find neighbor in "show ip  bgp ' + cmd + '"')


[](start = 62, length = 1)

shoud be only one space here.

lguohan · 2017-10-17T17:40:31Z

ansible/library/bgp_route.py

+        else:
+            raise Exception('cannot find neighbor in "show ip  bgp ' + cmd + '"')
+        self.facts['bgp_route_neiadv']['neighbor'] = neighbor
+        ### so far parsing prifix, nexthop and paths only


prifix [](start = 27, length = 6)

prefix #Resolved

lguohan · 2017-10-17T17:40:47Z

ansible/library/bgp_route.py

+        else:
+            raise Exception('cannot find neighbor in "show ip  bgp ' + cmd + '"')
+        self.facts['bgp_route_neiadv']['neighbor'] = neighbor
+        ### so far parsing prifix, nexthop and paths only


paths [](start = 47, length = 5)

path -> as path #Resolved

lguohan · 2017-10-17T17:43:45Z

ansible/library/bgp_route.py

+        else:
+            raise Exception('cannot find neighbor in "show ip  bgp ' + cmd + '"')
+        self.facts['bgp_route_neiadv']['neighbor'] = neighbor
+        ### so far parsing prifix, nexthop and paths only


you also parse the origin, and also weight. #Resolved

lguohan · 2017-10-17T17:45:16Z

ansible/library/bgp_route.py

+                    line = result_lines.pop(0)
+                    nexthop = line.strip().split()[0]
+                paths = re_path.match(line).group(2)
+                origin = re_path.match(line).group(4)


we only need one match

m = re_aspath.match(line)

aspaths = m.group(2)
origin = m.group(4)

what if it does not aspath, is it still group 4? #Resolved

weight = m.group(1)

In reply to: 145203052 [](ancestors = 145203052)

lguohan · 2017-10-17T17:52:17Z

ansible/library/bgp_route.py

+                paths = re_path.match(line).group(2)
+                origin = re_path.match(line).group(4)
+                if paths:
+                    entry['paths'] = paths.split()


paths [](start = 27, length = 5)

paths -> aspath #Resolved

lguohan · 2017-10-17T17:59:02Z

ansible/library/bgp_route.py

+    def get_facts(self):
+        return self.facts
+
+    def parse_bgp_route_adv(self, cmd, cmd_result):


the argument should be neigh, you do not need to give the cmmand and use regex to re-parse it. #Resolved

lguohan · 2017-10-17T17:59:34Z

ansible/library/bgp_route.py

+        if neigh and 'adv' in direction.lower():
+            show_cmd = "neighbor " + neigh + " adv"
+            (rc, out, err) = collect_data(module, show_cmd)
+            bgproute.parse_bgp_route_adv(show_cmd, out)


proute.parse_bgp_route_adv(show_cmd, out) [](start = 14, length = 41)

parse_bgp_route_adv(neigh, out)

lguohan · 2017-10-17T18:01:04Z

ansible/library/bgp_route.py

+        if neigh == None and direction == None and prefix == None:
+            raise Exception("parsing 'show ip bgp' full prefix table not implemented yet")
+        if neigh and (not netaddr.valid_ipv4(neigh)) and (not netaddr.valid_ipv6(neigh)) or neigh and not direction:
+            raise Exception('No support parsing this command " show ip(v6) bgp neighbor %s %s" yet' % (neigh, direction))


this is parameter validation, should be in different block as the parsing the actual output.

lguohan · 2017-10-17T18:02:22Z

ansible/library/bgp_route.py

+    m_args = module.params
+    neigh = m_args['neighbor']
+    direction = m_args['direction']
+    prefix = m_args['prefix']


overall, we should first to parameter validation and raise any exception is parameters are incorrect.

Then, we run the command and get the results, raise errors when the return results are incorrect.

we should not mixed them in one block. #Resolved

lguohan · 2017-10-17T18:04:00Z

ansible/library/bgp_route.py

+                (rc, out, err) = collect_data(module, prefix)
+                bgproute.parse_bgp_route_prefix(prefix, out)
+            else:
+                raise Exception('No support of this command "show ip(v6) bgp %s " yet ' % prefix)


Exception('No support of this command "show ip(v6) bgp %s " yet ' % prefix) [](start = 22, length = 75)

this exception should be prefix not valid, not "no support of this command"

lguohan · 2017-10-17T18:04:47Z

ansible/library/bgp_route.py

+            else:
+                raise Exception('No support of this command "show ip(v6) bgp %s " yet ' % prefix)
+        else:
+            raise Exception('No support parsing this command " show ip(v6) bgp neighbor=%s direction=%s prefix=%s" yet' % (neigh, direction, prefix))


Exception [](start = 18, length = 9)

does exception work for ansible_module, will it return error, I thought we should always do module_exit_json.

bgp_route module improvement

lguohan

* msft_github/master: [test]: Add ptf_platform_dir: ptftests to enable more than 32 ports testbed (sonic-net#315) [bgp_speaker]: Support t0-64 topology (sonic-net#316) [Minigraph Templates]: Add new <DhcpRelays> node to Vlans as appropriate (sonic-net#298) [sensors]: Add comment to indicate the sensors data file path [ptf_runner]: Add ptf_platform_dir option to indicate --platform-dir option (sonic-net#304) Make fetch command arguments supported by newer ansible version (sonic-net#310) [arista7260cx3] define t0 116 ports topology [fast-reboot test]: Cosmetic fixes (sonic-net#307) [test]add more ARP and neighbor-mac tests (sonic-net#303) After boot_onie, admin up all bgp interfaces (sonic-net#306) Update README.test.md add test case bgp multipath relax (sonic-net#284) [minigraph]: Remove unnecessary quotes (sonic-net#301)

…emplate from config.bcm for TH5 platforms (sonic-net#284)  ### Description of PR  Summary: Fixes # (issue) This change enhances the `check_config` fixture in test_route_perf.py to support TH5 platforms in Broadcom (e.g., x86_64-arista_7060x6_64pe and x86_64-arista_7060x6_64pe_b) where the `l3_alpm_template` value is configured in the config.bcm file instead of being queried via `bcmcmd`. ### Type of change  - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [x] Test case improvement ### Back port request - [x] 202412 ### Approach #### What is the motivation for this PR? TH5 devices manage ALPM configuration through config.bcm, and bcmcmd does not reflect runtime ALPM mode (l3_alpm_template) for them. Test case route/test_route_perf.py::test_perf_add_remove_routes would failed on TH5 platforms because it didn't support get ALPM configuration from bcm shell. #### How did you do it? - Added `get_l3_alpm_template_from_config_bcm()` helper to locate and copy the config.bcm file to /tmp and parse and retrieve the value of `l3_alpm_template`. - Update the `check_config` fixture to use the new method for TH5 platforms. #### How did you verify/test it? Verified on TH5 device (x86_64-arista_7060x6_64pe): confirms the correct ALPM template value is retrieved and case test_perf_add_remove_routes can pass now. ``` route/test_route_perf.py::test_perf_add_remove_routes[4-str4-7060x6-64pe-10-None] PASSED [50%] route/test_route_perf.py::test_perf_add_remove_routes[6-str4-7060x6-64pe-10-None] PASSED [100%] ``` #### Any platform specific information? #### Supported testbed topology if it's a new test case? ### Documentation  Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

) ``` 81505c8 (az_upstream/202412) Merge branch '202411' of https://github.com/sonic-net/sonic-mgmt into 202412 30b7d73 (pub_upstream/202411) [202411 ] Updating the BGP, Reboot and LACP convergence cases to latest Snappi Api Model instead of Snappi_convergence module [ PR 18044 ] (sonic-net#18391) e146100 [action] [PR:18292] Fix Skip tests if no portchannels are found in prepare_test_port fixture (sonic-net#280) d8b9193 [action] [PR:17985] [tests_mark_conditions.yaml]: Skip GNMI/ZMQ test for isolated topology (sonic-net#281) ef951e6 [action] [PR:17943] Add default routes to t1-isolated-d128 (sonic-net#282) 2d3aec5 [action] [PR:18341] [route/test_route_perf] Support reading l3_alpm_template from config.bcm for TH5 platforms (sonic-net#284) 6390c7d [202412][PR: 18350] disable default pfcwd for all lossy DUT (sonic-net#271) b621644 [Cherry-pick] modifying Ignore message for 'Failed to get port by bridge port ID' (sonic-net#263) bc9a2d2 modifying Ignore message for 'Failed to get port by bridge port ID' ```

…ic-net#11790) linkmgrd: * e20e402 2022-08-18 | Update `handleMuxConfigNotification` logic (sonic-net#125) (HEAD -> 202205) [Jing Zhang] * 8a54c06 2022-08-17 | [active-active] post mux metrics events (sonic-net#123) [Jing Zhang] platform-daemon: * 1a833f6 2022-08-18 | [ycabled] enable telemetry for 'active-active'; fix gRPC portid ordering (sonic-net#284) (HEAD -> 202205) [vdahiya12] utilities: * 4bacc1c 2022-08-19 | Fix issue: exception in is_rj45_port in multi ASIC env (sonic-net#2313) (HEAD -> 202205) [Stephen Sun] swss: * 4085d3f 2022-07-25 | Revert hwinfo count change (sonic-net#2383) (HEAD -> 202205) [andywongarista] Signed-off-by: Ying Xie <ying.xie@microsoft.com> Signed-off-by: Ying Xie <ying.xie@microsoft.com>

add test case bgp multipath relax

23dd0f5

msftclas added the cla-not-required label Sep 22, 2017

maggiemsft added the Enhancement label Sep 23, 2017

lguohan reviewed Sep 25, 2017

View reviewed changes

lguohan requested changes Sep 25, 2017

View reviewed changes

msftgits removed the cla-not-required label Sep 26, 2017

sonic-net deleted a comment from msftclas Sep 27, 2017

changes based on feedback of review

27050ef

lguohan reviewed Sep 29, 2017

View reviewed changes

add one more validation for prefix is multipath check, remove unneces…

20dd384

…sary steps

lguohan reviewed Oct 3, 2017

View reviewed changes

lguohan requested changes Oct 3, 2017

View reviewed changes

maggiemsft added 2 commits October 11, 2017 18:50

improve module input and document section

d5c59a2

change adv route parsing

0e5dbc4

lguohan reviewed Oct 17, 2017

View reviewed changes

resolve sonic.yml conflict;

6274d0d

bgp_route module improvement

lguohan approved these changes Oct 18, 2017

View reviewed changes

Merge branch 'master' into masun/bgp_multipath

27d444a

maggiemsft merged commit b3761a1 into sonic-net:master Oct 18, 2017

maggiemsft deleted the masun/bgp_multipath branch December 5, 2017 23:39

Conversation

maggiemsft commented Sep 22, 2017

Uh oh!

lguohan Sep 25, 2017 • 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!

maggiemsft Sep 27, 2017 • edited by lguohan 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!

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!

lguohan left a comment

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!

lguohan Sep 29, 2017 • 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!

maggiemsft Sep 29, 2017 • edited by lguohan 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!

lguohan Oct 3, 2017 • 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!

lguohan Oct 3, 2017 • 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!

lguohan Oct 3, 2017 • 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!

lguohan Oct 3, 2017 • 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!

lguohan Sep 25, 2017 •

edited

Loading

maggiemsft Sep 27, 2017 •

edited by lguohan

Loading

lguohan Sep 29, 2017 •

edited

Loading

maggiemsft Sep 29, 2017 •

edited by lguohan

Loading

lguohan Oct 3, 2017 •

edited

Loading

lguohan Oct 3, 2017 •

edited

Loading

lguohan Oct 3, 2017 •

edited

Loading

lguohan Oct 3, 2017 •

edited

Loading

maggiemsft Oct 11, 2017 •

edited by lguohan

Loading

lguohan Oct 3, 2017 •

edited

Loading

lguohan Oct 3, 2017 •

edited

Loading

lguohan Oct 17, 2017 •

edited

Loading

lguohan Oct 17, 2017 •

edited

Loading

lguohan Oct 17, 2017 •

edited

Loading

lguohan Oct 17, 2017 •

edited

Loading