Add bgp speaker test by sihuihan88 · Pull Request #209 · sonic-net/sonic-mgmt

sihuihan88 · 2017-07-06T18:58:44Z

This is to test support for BGP speaker which requires bgp dynamic neighbor configuration in quagga.
We use three exabgp instances to simulate three BGP speakers in ptf, which will create bgp sessions and
announce routes to DUT. DUT has dynamic neighbor bgp configuration for bgp speakers, and should
have bgp speakers shown as dynamic neighbors after exbagp instances announce routes.
We use bgp facts to valid the behavior.

msftclas · 2017-07-06T18:58:48Z

@sihuihan88,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

pavel-shirshov

Some minor issues

pavel-shirshov · 2017-07-12T01:21:11Z

ansible/library/get_ip_in_range.py

+        m_args = self.module.params
+        exclude_ips = []
+        if m_args['exclude_ips'] is not None:
+            ip_list = m_args['exclude_ips'].split() 


You can define this parameter as a list. Something like this:
exclude_ips=dict(required=False, default=[], type='list'),

pavel-shirshov · 2017-07-12T01:25:23Z

ansible/roles/test/tasks/bgp_speaker.yml

+# Run BGP Speaker test
+#========================================
+
+- fail: msg="Information about tested missing"


check you have ptf host defined

pavel-shirshov · 2017-07-12T01:27:23Z

ansible/roles/test/files/helpers/announce_routes.py

+import os
+import time
+import sys
+from sets import Set


Just curious. Why? ;)

s = set() working without importing anything

lguohan · 2017-07-12T08:37:26Z

ansible/library/minigraph_facts.py

+			    bgp_peers_with_range.append({
+			        'name': name,
+			        'ip_range': ip_range
+			    })


convert tab into whitespaces

lguohan · 2017-07-12T08:48:22Z

ansible/roles/test/tasks/bgp_speaker.yml

+
+- name: set bgp speaker asn number
+  set_fact:
+     bgp_speaker_asn={{deployment_id_asn_map[deployment_id]}}


be consistent on the space, in line 19, you have space between braces and word. here you do not.

lguohan · 2017-07-12T08:54:46Z

ansible/roles/test/tasks/bgp_speaker.yml

+
+- name: Generate configuration for the first exabgp instance
+  template: src=roles/test/templates/exabgp/config.j2 dest=/tmp/exabgp/{{item.file_name}}
+  with_items:


why not combine them using

with_items:

item1

item2

item3?

?

lguohan · 2017-07-12T08:57:55Z

ansible/roles/test/tasks/bgp_speaker.yml

+  connection: local
+
+- name: Generate start file for exabgp instances
+  template: src=roles/test/templates/exabgp/start.j2 dest=/tmp/exabgp/{{item.file_name}}


template allows you to deploy the file to remove host, wonder why you choose not? it can also set the mode.

http://docs.ansible.com/ansible/template_module.html

lguohan · 2017-07-12T08:58:47Z

ansible/roles/test/tasks/bgp_speaker.yml

+  delegate_to: "{{ptf_host}}"
+
+- name: Start exabgp instances
+  shell: chmod +x /root/exabgp/start.sh  && sh /root/exabgp/start.sh


you can do the chmod in the template module.

lguohan · 2017-07-12T09:02:22Z

ansible/roles/test/tasks/bgp_speaker.yml

+  with_items: "['{{slb_ips[0].split('/')[0]}}', '{{slb_ips[1].split('/')[0]}}', '{{vlan_ips[0].split('/')[0]}}'] "
+
+
+- name: Kill exabgp instances


you can use killall to kill the process by name, you do not really need the get the pid first.

or use pkill

lguohan · 2017-07-12T09:04:50Z

ansible/roles/test/tasks/bgp_speaker.yml

+
+- name: Copy helper files to ptf container
+  copy: src=roles/test/files/helpers/announce_routes.py dest={{exabgp_dir}}
+  delegate_to: "{{ptf_host}}"


you can copy the the whole directory to the dest using copy module. do not need to specify file one-by-one.

lguohan

as comments

lguohan · 2017-07-14T06:00:55Z

👍

lguohan · 2017-07-14T17:13:57Z

@pavel-shirshov , can you check if there is anything need to change or approve?

* Add BGP speaker test * support the case when multiple peer range in single group

### Description of PR  Summary: Fixes # (issue) ### Type of change  - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request - [ ] 202012 - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 ### Approach #### What is the motivation for this PR? The eos default cred is all digits. It may be interpreted as an integer and raise below error during tests: ``` File "/var/src/sonic-mgmt/tests/common/devices/eos.py", line 85, in shutdown out = self.eos_config( File "/var/src/sonic-mgmt/tests/common/devices/base.py", line 131, in _run raise RunAnsibleModuleFail("run module {} failed".format(self.module_name), res) tests.common.errors.RunAnsibleModuleFail: run module eos_config failed, Ansible Results => failed = True module_stdout = module_stderr = Expected unicode or bytes, got 123456 msg = MODULE FAILURE See stdout/stderr for the exact error _ansible_no_log = None changed = False stdout = stderr = ``` #### How did you do it? Add quote to the all digits default cred to force string type. #### How did you verify/test it? #### Any platform specific information? #### Supported testbed topology if it's a new test case? ### Documentation

sonic-net#15654) #### Why I did it src/linkmgrd ``` * 3f89fce - (HEAD -> 202012, origin/202012) [202012] Use Vlan MAC as src MAC for link prober by default sonic-net#93 (sonic-net#209) (6 weeks ago) [Jing Zhang] ```

…lly (sonic-net#18574) #### Why I did it src/sonic-gnmi ``` * 4b12af9 - (HEAD -> 202305, origin/202305) Merge pull request sonic-net#209 from zbud-msft/revert_pfcwd_202305 (2 minutes ago) [StormLiangMS] * 802dbb7 - Revert "Replace PFC_WD_TABLE with PFC_WD (sonic-net#173)" (28 hours ago) [Zain Budhwani] * e1397c4 - Revert "Account for GLOBAL key in PFC_WD (sonic-net#178)" (28 hours ago) [Zain Budhwani] ``` #### How I did it #### How to verify it #### Description for the changelog

Add bgp speaker test

c5c8e3b

msftclas added the cla-not-required label Jul 6, 2017

pavel-shirshov suggested changes Jul 12, 2017

View reviewed changes

lguohan reviewed Jul 12, 2017

View reviewed changes

lguohan requested changes Jul 12, 2017

View reviewed changes

Sihui Han added 2 commits July 12, 2017 23:08

[bgpspeaker]: update as review comments

cdce1c0

support the case when multiple peer range in single group

9e6cf52

lguohan approved these changes Jul 14, 2017

View reviewed changes

pavel-shirshov approved these changes Jul 15, 2017

View reviewed changes

stcheng merged commit d231911 into sonic-net:master Jul 17, 2017

lguohan pushed a commit that referenced this pull request Aug 3, 2017

[test]: Add BGP speaker test (#209)

66e85fb

* Add BGP speaker test * support the case when multiple peer range in single group

sihuihan88 deleted the dev/sihan/mux branch August 3, 2017 22:51

		with_items: "['{{slb_ips[0].split('/')[0]}}', '{{slb_ips[1].split('/')[0]}}', '{{vlan_ips[0].split('/')[0]}}'] "


		- name: Kill exabgp instances

Conversation

sihuihan88 commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Jul 6, 2017

Uh oh!

pavel-shirshov 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!

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!

lguohan commented Jul 14, 2017

Uh oh!

lguohan commented Jul 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sihuihan88 commented Jul 6, 2017 •

edited

Loading