Skip to content

[upgrade_path]Bug fix in test_upgrade_path #2230

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.

Sign up for GitHub

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

Merged
bingwang-ms merged 1 commit into from
Sep 18, 2020
Merged

[upgrade_path]Bug fix in test_upgrade_path #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions ansible/roles/test/files/ptftests/advanced-reboot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ def handle_post_reboot_test_reports(self):
self.assertTrue(is_good, errors)

def runTest(self):
self.pre_reboot_test_setup()
self.pre_reboot_test_setup()
try:
self.log("Check that device is alive and pinging")
self.fails['dut'].add("DUT is not ready for test")
Expand All @@ -989,6 +989,8 @@ def runTest(self):
self.handle_warm_reboot_health_check()
self.handle_post_reboot_health_check()

# Check sonic version after reboot
self.check_sonic_version_after_reboot()
Copy link
Contributor

@vaibhavhd vaibhavhd Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Minor concern in moving check_sonic_version_after_reboot towards the end is that even if upgrade fails now the health checks will be performed.
Earlier if the upgrade failure was seen thread.interrupt_main() was called and it would crash the main thread, skipping all the checks which are unnecessary if the upgrade itself has failed.

May be adding check_sonic_version_after_reboot right after self.wait_until_reboot() and before health_checks will be better?

Copy link
Collaborator Author

@bingwang-ms bingwang-ms Sep 18, 2020

Choose a reason for hiding this comment

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

It's a good suggestion. But I don't think checking sonic version right after wait_until_reboot is reliable. The wait_until_reboot only confirms the DUT is down, but we are not sure if it's up. So I place the check just at the very end of the test.

except Exception as e:
self.fails['dut'].add(e)
finally:
Expand Down Expand Up @@ -1019,10 +1021,8 @@ def check_sonic_version_after_reboot(self):
current_version = str(stdout[0]).replace('\n', '')
self.log("Current={} Target={}".format(current_version, target_version))
if current_version != target_version:
self.fails['dut'].add("Sonic upgrade failed. Target={} Current={}".format(\
raise Exception("Sonic upgrade failed. Target={} Current={}".format(\
target_version, current_version))
return False
return True

def extract_no_cpu_replies(self, arr):
"""
Expand All @@ -1047,9 +1047,6 @@ def reboot_dut(self):
if stderr != []:
self.log("stderr from %s: %s" % (self.reboot_type, str(stderr)))
self.log("return code from %s: %s" % (self.reboot_type, str(return_code)))
# Check sonic version after reboot
if not self.check_sonic_version_after_reboot():
thread.interrupt_main()

# Note: a timeout reboot in ssh session will return a 255 code
if return_code not in [0, 255]:
Expand Down Expand Up @@ -1323,6 +1320,7 @@ def check_forwarding_resume(self):
return self.asic_state.get_state_time(state), self.get_asic_vlan_reachability()

def ping_data_plane(self, light_probe=True):
self.dataplane.flush()
replies_from_servers = self.pingFromServers()
if replies_from_servers > 0 or not light_probe:
replies_from_upper = self.pingFromUpperTier()
Expand Down Expand Up @@ -1354,7 +1352,7 @@ def wait_dut_to_warm_up(self):
up_time = datetime.datetime.now()
up_secs = (datetime.datetime.now() - up_time).total_seconds()
if up_secs > dut_stabilize_secs:
break;
break
else:
# reset up_time
up_time = None
Expand Down
2 changes: 1 addition & 1 deletion tests/scripts/arp_responder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def __del__(self):
self.socket.close()

def bind(self):
self.socket = scapy2.conf.L2listen(iface=self.iface)
self.socket = scapy2.conf.L2listen(iface=self.iface, filter='arp')

def handler(self):
return self.socket
Expand Down
21 changes: 9 additions & 12 deletions tests/upgrade_path/test_upgrade_path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@

from tests.common.fixtures.ptfhost_utils import copy_ptftests_directory # lgtm[py/unused-import]
from tests.common.fixtures.ptfhost_utils import change_mac_addresses # lgtm[py/unused-import]
from tests.common.fixtures.ptfhost_utils import remove_ip_addresses # lgtm[py/unused-import]
from tests.common.fixtures.ptfhost_utils import copy_arp_responder_py # lgtm[py/unused-import]

pytestmark = [
pytest.mark.topology('any')
pytest.mark.topology('any'),
pytest.mark.sanity_check(skip_sanity=True),
pytest.mark.disable_loganalyzer
]

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -49,9 +53,6 @@ def cleanup(localhost, ptfhost, duthost, upgrade_path_lists):

def prepare_ptf(ptfhost, duthost):
logger.info("Preparing ptfhost")
ptfhost.script("./scripts/remove_ip.sh")
ptfhost.copy(src="../ansible/roles/test/files/helpers/arp_responder.py",
dest="/opt")

# Prapare vlan conf file
mg_facts = duthost.minigraph_facts(host=duthost.hostname)['ansible_facts']
Expand Down Expand Up @@ -80,7 +81,7 @@ def prepare_ptf(ptfhost, duthost):


@pytest.fixture(scope="module")
def ptf_params(duthost, nbrhosts):
def ptf_params(duthost, nbrhosts, creds):

mg_facts = duthost.minigraph_facts(host=duthost.hostname)['ansible_facts']
lo_v6_prefix = ""
Expand All @@ -96,14 +97,10 @@ def ptf_params(duthost, nbrhosts):
#TODO:Update to vm_hosts.append(value['host'].host.mgmt_ip)
vm_hosts.append(value['host'].host.options['inventory_manager'].get_host(value['host'].hostname).vars['ansible_host'])

hostVars = duthost.host.options['variable_manager']._hostvars[duthost.hostname]
inventory = hostVars['inventory_file'].split('/')[-1]
secrets = duthost.host.options['variable_manager']._hostvars[duthost.hostname]['secret_group_vars']

ptf_params = {
"verbose": False,
"dut_username": secrets[inventory]['sonicadmin_user'],
"dut_password": secrets[inventory]['sonicadmin_password'],
"dut_username": creds.get('sonicadmin_user'),
"dut_password": creds.get('sonicadmin_password'),
"dut_hostname": duthost.host.options['inventory_manager'].get_host(duthost.hostname).vars['ansible_host'],
"reboot_limit_in_seconds": 30,
"reboot_type": "warm-reboot",
Expand Down Expand Up @@ -161,6 +158,6 @@ def test_upgrade_path(localhost, duthost, ptfhost, upgrade_path_lists, ptf_param
platform_dir="ptftests",
params=test_params,
platform="remote",
qlen=1000,
qlen=10000,
Copy link
Contributor

@vaibhavhd vaibhavhd Sep 17, 2020

Choose a reason for hiding this comment

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

If qlen=10000 is found to be better setting, should we also change PTFRUNNER_QLEN to 10000 in advanced_reboot.py script too?

Copy link
Collaborator Author

@bingwang-ms bingwang-ms Sep 18, 2020

Choose a reason for hiding this comment

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

I'm not sure about that. We need to run test_advanced_reboot and check the ptf log. There will be a debug log in ptf log
Discarding oldest packet to make room

log_file=log_file)