<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hello Aline,<br>
<br>
Please find my comment inline. Let me know if questions.<br>
<br>
Thanks,<br>
Megha<br>
<br>
<div class="moz-cite-prefix">On 11/3/2015 9:21 PM, Aline Manera
wrote:<br>
</div>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
For the commit message, please, use 80 characters per line.<br>
The first line correspond to a generic message followed by a
description.<br>
<br>
</blockquote>
This I will take care in next patch.<br>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<br>
<div class="moz-cite-prefix">On 02/11/2015 10:03, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:mesmriti@linux.vnet.ibm.com"><a class="moz-txt-link-abbreviated" href="mailto:mesmriti@linux.vnet.ibm.com">mesmriti@linux.vnet.ibm.com</a></a>
wrote:<br>
</div>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap="">From: Megha Smriti <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:mesmriti@linux.vnet.ibm.com"><mesmriti@linux.vnet.ibm.com></a>
---
src/wok/plugins/gingerbase/i18n.py | 7 ++-
src/wok/plugins/gingerbase/model/debugreports.py | 67 +++++++++++++-----------
2 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py
index 8596f17..01573b0 100644
--- a/src/wok/plugins/gingerbase/i18n.py
+++ b/src/wok/plugins/gingerbase/i18n.py
@@ -29,14 +29,13 @@ messages = {
"GGBDR0001E": _("Debug report %(name)s does not exist"),
"GGBDR0002E": _("Debug report tool not found in system"),</pre>
</blockquote>
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap="">- "GGBDR0003E": _("Unable to create debug report %(name)s. Details: %(err)s."),
- "GGBDR0004E": _("Can not find any debug report with the given name %(name)s"),
- "GGBDR0005E": _("Unable to generate debug report %(name)s. Details: %(err)s"),
+ "GGBDR0003E": _("Unable to create sosreport report %(name)s. Details: %(err)s."),
+ "GGBDR0004E": _("Can not find any sosreport with the given name %(name)s"),
+ "GGBDR0005E": _("Unable to generate sosreport %(name)s. Details: %(err)s"),</pre>
</blockquote>
<br>
Please, keep using "debug report" instead of changing it to
"sosreport" as the sosreport is not the default debug report tool
in some distribution.<br>
The debug report feature may not necessarily linked with
sosreport. I know it is by now but the plan is to support more and
more distros in future.<br>
</blockquote>
<br>
<pre wrap="">Error message "GGBDR0003E" is used when sosreport command execution return code is non-zero.
This can also happen if the distribution does not have sosreport command for debug data collection.
As per my understanding, keeping sosreport in error message clearly point the error that sosreport command execution failed.
Keeping "debug report" instead of "sosreport" will be generic and will not clearly tell the reason of failure.
</pre>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap=""> "GGBDR0006E": _("You should give a name for the debug report file."),
"GGBDR0007E": _("Debug report name must be a string. Only letters, digits, underscore ('_') and "
"hyphen ('-') are allowed."),
"GGBDR0008E": _("The debug report with specified name \"%(name)s\" already exists. Please use another one."),
-
"GGBHOST0001E": _("Unable to shutdown host machine as there are running virtual machines"),
"GGBHOST0002E": _("Unable to reboot host machine as there are running virtual machines"),
"GGBHOST0005E": _("When specifying CPU topology, each element must be an integer greater than zero."),
diff --git a/src/wok/plugins/gingerbase/model/debugreports.py b/src/wok/plugins/gingerbase/model/debugreports.py
index 94ab7fe..0bb36fe 100644
--- a/src/wok/plugins/gingerbase/model/debugreports.py
+++ b/src/wok/plugins/gingerbase/model/debugreports.py
@@ -19,13 +19,13 @@
# License along with this library; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-import fnmatch
import glob
import logging
import os
import shutil
import subprocess
import time
+import re
from wok.exception import InvalidParameter, NotFoundError, OperationFailed
from wok.exception import WokException
@@ -71,7 +71,7 @@ class DebugReportsModel(object):
raise OperationFailed("GGBDR0002E")
</pre>
</blockquote>
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap=""> @staticmethod
- def sosreport_generate(cb, name):
+ def debugreport_generate(cb, name):</pre>
</blockquote>
<br>
This function represents which tool to use to generate the debug
report. Keep using "sosreport_generate"<br>
</blockquote>
As you mention in previous comment, debug report should be generic
as for some distro, sosreport is not the default debug report tool.<br>
So instead we can keep this method as "debugreport_generate" and
have seperate method to collect sosreport and any other way of
collection report as different method.<br>
And depending upon distro, we can call these methods inside
"debugreport_generate"<br>
e.g.<br>
<pre wrap="">def debugreport_generate(cb, name):
collect_sosreport(name)
if platform.machine().startswith('s390'):
collect_dbginfo(name)
Please let me know if it make sense to you.
</pre>
<br>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap=""> def log_error(e):
log = logging.getLogger('Model')
log.warning('Exception in generating debug file: %s', e)
@@ -87,38 +87,35 @@ class DebugReportsModel(object):
# SOSREPORT might create file in /tmp or /var/tmp
# FIXME: The right way should be passing the tar.xz file directory
# though the parameter '--tmp-dir', but it is failing in Fedora 20
- patterns = ['/tmp/sosreport-%s-*', '/var/tmp/sosreport-%s-*']
- reports = []
- reportFile = None
- for p in patterns:
- reports = reports + [f for f in glob.glob(p % name)]
- for f in reports:
- if not fnmatch.fnmatch(f, '*.md5'):
- reportFile = f
- break</pre>
</blockquote>
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap="">+ sosreport_name = name.replace('_', '')</pre>
</blockquote>
<br>
Why do you replace underscore? <br>
If underscore character is not allowed we should block user to use
and raise an appropriated error message instead of changing user's
input without any previous warning.<br>
</blockquote>
sosreport command allow to give name with underscore meaning I can
execute #sosreport --batch --name=test_sosreport<br>
However it generate the corresponding sosreport with name <font
face="monospace">sosreport-testsosreport-20151103134451.tar.xz<br>
As sosreport command allow _ and does not throw any excpetion, the
code should also able to handle this scenario.<br>
<br>
So we are not changing the name passed to sosreport command, we
still pass the name which we got from user.<br>
As explained above sosreport will generate the compress report
removing the underscore.<br>
So just for collecting the compressed file doing replace.<br>
<br>
</font>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap="">+ sosreport_name_regex = '(\s+)(\/\w+\/\w+\/|\/\w+\/)' \
+ '(sosreport-' +\
+ sosreport_name + '-\d+.tar.xz)'
+ sosreport_file = None
+ output = output.splitlines()
+ for line in output:
+ if line:
+ matched_name = re.match(sosreport_name_regex, line)
+ if matched_name:
+ path = matched_name.groups()[1]
+ fname = matched_name.groups()[2]
+ sosreport_file = path + fname
+ break</pre>
</blockquote>
<br>
It seems a lot processing to find the file path.<br>
<br>
I've just figured out the sosreport has an option "--tmp-dir" to
save the report in a custom directory. Seems it is the better
approach to follow here.<br>
We can use "--tmp-dir" to save the report in the file location
according to gingerbase configuration.<br>
<br>
For example: <br>
<pre wrap="">command = ['sosreport', '--batch', '--name=%s' % name, <b>'--tmp-dir=%s' % config.get_debugreports_path()</b>]</pre>
<br>
Here is the command I used to test: <font face="monospace">sosreport
--batch --name=alinetest --tmp-dir=/home/alinefm</font><br>
<br>
After that I got:<br>
<font face="monospace"><br>
alinefm@alinefm-ThinkPad-T440:~/mail-patches$ ls
/home/alinefm/sosreport-alinetest-20151103134451.tar.xz<br>
/home/alinefm/sosreport-aline-test-20151103134451.tar.xz</font><br>
<br>
Using the --tmp-dir we also eliminate the need to move the debug
report after its creation.<br>
</blockquote>
I will take care of this in my next patch for sosreport collection.<br>
<blockquote cite="mid:5638D803.20605@linux.vnet.ibm.com" type="cite">
<br>
<blockquote
cite="mid:1446465813-4068-2-git-send-email-mesmriti@linux.vnet.ibm.com"
type="cite">
<pre wrap=""> # Some error in sosreport happened
- if reportFile is None:
+ if sosreport_file is None:
wok_log.error('Debug report file not found. See sosreport '
'output for detail:\n%s', output)
- fname = (patterns[0] % name).split('/')[-1]
- raise OperationFailed('GGBDR0004E', {'name': fname})
-
- md5_report_file = reportFile + '.md5'
- report_file_extension = '.' + reportFile.split('.', 1)[1]
+ raise OperationFailed('GGBDR0004E', {'name': name})
+ md5_report_file = sosreport_file + '.md5'
+ report_file_extension = '.' + sosreport_file.split('.', 1)[1]
path = config.get_debugreports_path()
- target = os.path.join(path, name + report_file_extension)
- # Moving report
- msg = 'Moving debug report file "%s" to "%s"' % (reportFile,
- target)
- wok_log.info(msg)
- shutil.move(reportFile, target)
- # Deleting md5
- msg = 'Deleting report md5 file: "%s"' % (md5_report_file)
+ sosreport_target = os.path.join(path,
+ name + report_file_extension)
+ msg = 'Moving debug report file "%s" to "%s"' \
+ % (sosreport_file, sosreport_target)
wok_log.info(msg)
- with open(md5_report_file) as f:
- md5 = f.read().strip()
- wok_log.info('Md5 file content: "%s"', md5)
- os.remove(md5_report_file)
+ shutil.move(sosreport_file, sosreport_target)
+ delete_the_sosreport_md5_file(md5_report_file)
cb('OK', True)
return
@@ -143,7 +140,7 @@ class DebugReportsModel(object):
# and implement the report generating function
# based on the new report command
report_tools = ({'cmd': 'sosreport --help',
- 'fn': DebugReportsModel.sosreport_generate},)
+ 'fn': DebugReportsModel.debugreport_generate},)
# check if the command can be found by shell one by one
for helper_tool in report_tools:
@@ -213,3 +210,13 @@ class DebugReportContentModel(object):
def lookup(self, name):
return self._debugreport.lookup(name)
+
+
+def delete_the_sosreport_md5_file(md5_file):
+ # Deleting md5
+ msg = 'Deleting report md5 file: "%s"' % md5_file
+ wok_log.info(msg)
+ with open(md5_file) as f:
+ md5 = f.read().strip()
+ wok_log.info('Md5 file content: "%s"', md5)
+ os.remove(md5_file)
</pre>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>