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

disk_check: Collect system data on failure #4546

Closed
wants to merge 26 commits into from

Conversation

renukamanavalan
Copy link
Contributor

@renukamanavalan renukamanavalan commented Oct 24, 2021

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911

Approach

  1. Direct syslog & auth.log to tmpfs before making disk RO
  2. Upon test failure, collect logs & additional data to help with debug.

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?

Documentation

1) Use skip_release from common
2) Wait until instead of blind sleep
…opped

2) If reboot command may fail, retry
3) Wait for ssh port to be started and give a time for stabilizing
2) In case of failure, collect more data
@renukamanavalan renukamanavalan requested review from yxieca and a team October 24, 2021 20:42
@renukamanavalan renukamanavalan self-assigned this Oct 24, 2021
@lgtm-com
Copy link

lgtm-com bot commented Oct 24, 2021

This pull request fixes 1 alert when merging 5c24fd6 into 0b702ba - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Oct 25, 2021

This pull request introduces 1 alert and fixes 3 when merging e49eeb6 into d972c21 - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2021

This pull request fixes 2 alerts when merging c546be7 into d972c21 - view on LGTM.com

fixed alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Oct 28, 2021

This pull request fixes 2 alerts when merging 32cda4e into 5197339 - view on LGTM.com

fixed alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2021

This pull request fixes 2 alerts when merging 4893369 into 021206d - view on LGTM.com

fixed alerts:

  • 2 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2021

This pull request fixes 1 alert when merging affd630 into d7ac28b - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@renukamanavalan renukamanavalan requested a review from a team November 19, 2021 17:19
def set_syslog_dest(duthost):
syslog_dir = "/run/mount/logs/"
res = duthost.shell("mkdir -p {}".format(syslog_dir))
assert res["rc"] == 0, "failed to create {}".format(syslog_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assert is unnecessary because when rc is not 0, the duthost.shell call will raise an exception unless argument ignore_module_errors=True is supplied.

duthost.copy(src=out_file, dest=duthost_fpath)

res = duthost.shell("ls -l {}".format(duthost_fpath))
assert res["rc"] == 0, "failed to create {}".format(duthost_fpath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary rc check.

assert res["rc"] == 0, "failed to create {}".format(duthost_fpath)

res = duthost.shell("systemctl restart rsyslog")
assert res["rc"] == 0, "failed to restart rsyslog"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary rc check.


# remove the file. So no impact upon reboot
res = duthost.shell("rm -f {}".format(duthost_fpath))
assert res["rc"] == 0, "failed to remove {}".format(duthost_fpath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary rc check.

def test_ro_disk(localhost, duthosts, enum_rand_one_per_hwsku_hostname, creds_all_duts, check_tacacs):
"""test tacacs rw user
"""
duthost = duthosts[enum_rand_one_per_hwsku_hostname]
skip_release(duthost, ["201911", "201811"])

# Enable failthrough to allow admin access
res = duthost.shell("config aaa authentication failthrough enable")
assert res["rc"] == 0, "failed to enable failthrough"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary rc check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants