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

[ptf_runner] Save ptf log to script executing host in case of failure #823

Merged
merged 2 commits into from
Apr 10, 2019
Merged

[ptf_runner] Save ptf log to script executing host in case of failure #823

merged 2 commits into from
Apr 10, 2019

Conversation

wangxin
Copy link
Collaborator

@wangxin wangxin commented Mar 8, 2019

Description of PR

Summary:
Fixes # (issue)

The PTF log and pcap files are useful for debugging in case of PTF
script failed. However, these files are in the PTF container and could
be lost when the PTF container is re-deployed.

This improvement is to save the log and pcap files to the script
executing host when the PTF script is failed.

Type of change

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

Approach

How did you do it?

When the PTF script failed, copy the PTF log and pcap files to sonic-mgmt container for later debugging.

How did you verify/test it?

Tested on Mellanox platform.

Any platform specific information?

No

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

Documentation

The PTF log and pcap files are useful for debugging in case of PTF
script failed. However, these files are in the PTF container and could
be lost when the PTF container is re-deployed.

This improvement is to save the log and pcap files to the script
executing host when the PTF script is failed.

Signed-off-by: Xin Wang <xinw@mellanox.com>
Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

how to align the ptf log with the test?
please also check the location of the log analyzer output and advise to have them all in the same location.
if this already done, great.

@wangxin
Copy link
Collaborator Author

wangxin commented Mar 18, 2019

@liat-grozovik
The ptf log files are saved to the same location of log analyzer on sonic-mgmt.

PTF log files save location:
aisble/test/<dut_hostname>/ptf/

Log analyzer output files save location:
ansible/test/<dut_hostname>/

To avoid overwritten, timestampe appended to the PTF logfiles while saving them to sonic-mgmt.

Copy link
Contributor

@volodymyrsamotiy volodymyrsamotiy left a comment

Choose a reason for hiding this comment

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

Since this PR changes default behavior, I am just thinking maybe it is better to have all new copy logic under some condition.
So we don't change default behavior and if some option is passed only then logs are copied to specified location.
For example something like: ansible-playbook <some_test>.yml ... -e save-ptf-log=</dest/location/>
@wangxin, what do you think?

@@ -51,5 +51,36 @@

- debug: var=out.stdout_lines

- name: Set default PTF log filename
set_fact:
ptf_log_file: "/root/ptf.log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here default PTF log file is set to /root/ptf.log, but all tests use /tmp/.
I believe it would be better to change this default to /tmp/ as well, so we have all logs in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is to specify where to get the PTF log and pcap files from the PTF container.

When PTF log file is not explicitly specified while calling ptf_runner.yml, PTF saves default log and pcap files to /root/ptf.log and /root/ptf.pcap on PTF container.

If explicit log file parameter is parsed from the PTF command line, this default value will be overwritten with custom log file location.

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

Let's save the logs in succesful runs too.

ansible/roles/test/tasks/ptf_runner.yml Outdated Show resolved Hide resolved
ansible/roles/test/tasks/ptf_runner.yml Outdated Show resolved Hide resolved
@wangxin
Copy link
Collaborator Author

wangxin commented Mar 19, 2019

@volodymyrsamotiy I agree that your proposal has two advantages:

  1. Keep default behavior unchanged
  2. Explicitly specify the location for saving log files.
    To do it this way, PTF logs are not saved to sonic-mgmt unless we explicitly add this argument while run the scripts.
    Since explicit is better than implicit. I'll make the change. Thanks!

The previous commit changed the default behavior. This change is to add
an option for specifying whether to save ptf log in case of failure.
For example: ansible-playbook <some_test>.yml ... -e save_ptf_log=yes
@wangxin
Copy link
Collaborator Author

wangxin commented Mar 21, 2019

@volodymyrsamotiy
I just committed a change adding an option for specifying whether to save ptf log in case of failure. For example: ansible-playbook <some_test>.yml ... -e save_ptf_log=yes
The save_ptf_log option only takes values like 'yes', 'no', 'true', 'false'. Destination folder of ptf log files cannot be spefied. The ptf log files are saved to the same location of the log analyzer result files. I think it's OK for us to follow the same practice. To be consistent with the format of other variables given in command line, I used underscore '_' instead of '-'.

@liat-grozovik
Copy link
Collaborator

@pavel-shirshov can you please review as well? if all good, we can move on and merge.

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

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

How can we know that target dir 'test/{{ inventory_hostname }}/ptf/' is valid?

@wangxin
Copy link
Collaborator Author

wangxin commented Apr 8, 2019

@pavel-shirshov
I used the same pattern that has been used by logAnalyzer. Result files of logAnalyzer are stored to test/{{ inventory_hostname }} in case of failure. Since this pattern has been proven by logAnalyzer, I keep using it for ptf_runner.

@lguohan lguohan merged commit 56c9a00 into sonic-net:master Apr 10, 2019
yxieca pushed a commit that referenced this pull request Apr 10, 2019
…#823)

* [ptf_runner] Save ptf log to script executing host in case of failure

The PTF log and pcap files are useful for debugging in case of PTF
script failed. However, these files are in the PTF container and could
be lost when the PTF container is re-deployed.

This improvement is to save the log and pcap files to the script
executing host when the PTF script is failed.

Signed-off-by: Xin Wang <xinw@mellanox.com>

* [ptf_runner] Add option for specifying whether to save ptf log

The previous commit changed the default behavior. This change is to add
an option for specifying whether to save ptf log in case of failure.
For example: ansible-playbook <some_test>.yml ... -e save_ptf_log=yes
@wangxin wangxin deleted the ptf-log-review branch May 24, 2019 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants