Hi Aline,
Please find my comments inline and few suggestions.
Thanks & Regards,
Megha Smriti
On 17/11/2015 20:05,
mesmriti@linux.vnet.ibm.com
wrote:
From: root <root@localhost.localdomain>
Signed-off-by: root <root@localhost.localdomain>
---
src/wok/plugins/gingerbase/i18n.py | 4 +-
src/wok/plugins/gingerbase/model/debugreports.py | 212
++++++++++++++++++-----
2 files changed, 167 insertions(+), 49 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py
b/src/wok/plugins/gingerbase/i18n.py
index af75c70..6ff783b 100644
--- a/src/wok/plugins/gingerbase/i18n.py
+++ b/src/wok/plugins/gingerbase/i18n.py
@@ -36,7 +36,9 @@ messages = {
"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."),
-
+ "GGBDR0009E": _("Unable to create dbginfo report with
%(retcode)s. Details: %(err)s"),
+ "GGBDR0010E": _("Unable to compress the final debug
report tar file with %(retcode)s. Details: %(error)s"),
+ "GGBDR0011E": _("Unable to generate final debug report
%(name)s. Details: %(err)s"),
"GGBHOST0001E": _("Unable to shutdown host machine as
there are running virtual machines"),
"GGBHOST0002E": _("Unable to reboot host machine as
there are running virtual machines"),
"GGBHOST0003E": _("There may be virtual machines running
on the host"),
diff --git a/src/wok/plugins/gingerbase/model/debugreports.py
b/src/wok/plugins/gingerbase/model/debugreports.py
index 94ab7fe..927e173 100644
--- a/src/wok/plugins/gingerbase/model/debugreports.py
+++ b/src/wok/plugins/gingerbase/model/debugreports.py
@@ -19,13 +19,14 @@
# 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 platform
+import re
from wok.exception import InvalidParameter, NotFoundError,
OperationFailed
from wok.exception import WokException
@@ -71,54 +72,109 @@ class DebugReportsModel(object):
raise OperationFailed("GGBDR0002E")
@staticmethod
- def sosreport_generate(cb, name):
+ def
debugreport_generate(cb, name):
def log_error(e):
log = logging.getLogger('Model')
log.warning('Exception in generating debug file:
%s', e)
I know it is already there prior to your patch, but we should
use wok_log to log the errors.
This special wrapper can be removed.
I will incorporate this in my next patch
try:
- command = ['sosreport', '--batch', '--name=%s' %
name]
- output, error, retcode = run_command(command)
-
- if retcode != 0:
- raise OperationFailed("GGBDR0003E", {'name':
name,
- 'err':
retcode})
-
- # 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
- # Some error in sosreport happened
- if reportFile 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]
+ # Sosreport generation
+ sosreport_file = sosreport_collection(name)
+ md5_report_file = sosreport_file + '.md5'
+ report_file_extension = '.' +
sosreport_file.split('.', 1)[1]
+ #
If the platform is a system Z machine.
+ if platform.machine().startswith('s390'):
The debugreport_generate() function will be called only if the
system has the dbginfo command
So you don't need to rely in the system arch to decide on that.
So the 'if' statement above can be safely removed.
Will take care of this in my next patch.
+
path_debugreport = '/var/tmp/'
+ dbgreport_regex = '(\S+\s+)(' +
path_debugreport + \
+ 'DBGINFO-[\d+]{4}-[\d+]{2}'
\
+
'-[\d+]{2}-[\d+]{2}-[\d+]{2}' \
+
'-[\d+]{2}-\w+-\d+\S+)(\s+\S+)'
+ command = ['/usr/sbin/dbginfo.sh', '-d',
path_debugreport]
+ output, error, retcode = run_command(command)
+ if retcode != 0:
+ raise OperationFailed("GGBDR0009E",
+ {'retcode':
retcode, 'err': error})
+ output = output.splitlines()
+ dbginfo_report = None
+ for line in output:
+ line = line.strip()
+ n = re.match(dbgreport_regex, line)
+ if n:
+ dbginfo_report = n.groups()[1]
+ break
+ final_tar_report_name = name +
report_file_extension
+ if dbginfo_report is not None:
+ sosreport_tar = sosreport_file.split('/',
3)[3]
+ dbginfo_tar = dbginfo_report.split('/',
3)[3]
+ msg = 'Compressing the sosreport and
debug info files into ' \
+ 'final report file'
+ wok_log.info(msg)
+ # Compressing the sosreport and dbginfo
reports into one
+ # tar file
+ command = ['tar', '-cvzf', '%s' %
final_tar_report_name,
+ '-C', path_debugreport,
dbginfo_tar,
+ sosreport_tar]
You can use final_tar_report_name with the right debug report
location to avoid need to move the file after that.
Instead of the direct location compressing it
would be good to do in var/tmp and then moving
it to target directory
rather trying to do any operation on the target directory. In
some cases if developer
makes a mistake it is possible that previous
reports might get deleted . Please correct me if any other
suggestions.
+
output, error, retcode = run_command(command)
+ if retcode != 0:
+ raise OperationFailed("GGBDR0010E",
+ {'retcode':
retcode,
+ 'error':
error})
+
path = config.get_debugreports_path()
+ dbg_target = os.path.join(path,
+ name +
report_file_extension)
Use the dbg_target value while running the 'tar' command above.
So we don't need to move the file.
Same as my comment above.
+
# Moving final tar file to debugreports path
+ msg = 'Moving final debug report file
"%s" to "%s"' % \
+ (final_tar_report_name, dbg_target)
+ wok_log.info(msg)
+ shutil.move(final_tar_report_name,
dbg_target)
The above block can be removed.
+
# Deleting the sosreport md5 file
+
delete_the_sosreport_md5_file(md5_report_file)
+ # Deleting the dbingo report file
+ msg = 'Deleting the dbginfo file "%s" ' \
+ % dbginfo_report
+ wok_log.info(msg)
+ os.remove(dbginfo_report)
+ # Deleting the sosreport file
+ msg = 'Deleting the sosreport file "%s" '
% sosreport_file
+ wok_log.info(msg)
+ os.remove(sosreport_file)
+ wok_log.info('The debug report file has
been moved')
+ cb('OK', True)
+ return
+
+ except WokException as e:
+ log_error(e)
+ raise
+
+ except OSError as e:
+ log_error(e)
+ raise
+
+ except Exception, e:
+ # No need to call cb to update the task status
here.
+ # The task object will catch the exception raised
here
+ # and update the task status there
+ log_error(e)
+ raise OperationFailed("GGBDR0011E", {'name':
name, 'err': e})
+
+ @staticmethod
+ def sosreport_generate(cb, name):
+ def
log_error(e):
+ log = logging.getLogger('Model')
+ log.warning('Exception in generating debug file:
%s', e)
The same I commented before. We can use wok_log to log the
errors.
I will incorporate this in my next patch
+ try:
+ # Sosreport collection
+ sosreport_file = sosreport_collection(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)
+ 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)
- shutil.move(reportFile, target)
- # Deleting md5
- msg = 'Deleting report md5 file: "%s"' %
(md5_report_file)
- 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)
As we are using --tmp-dir option, we know exactly where the
sosreport will be saved. So we don't need to move the file
around.
When the sosreport is generated md5 file is generated along
with the sosreport. That is why we generate the sosreport
in a var/tmp dir and remove the md5 file and then move the
final file to the target location to avoid doing all the
operations
on the target directory.
+
delete_the_sosreport_md5_file(md5_report_file)
cb('OK', True)
return
@@ -142,17 +198,27 @@ class DebugReportsModel(object):
# Please add new possible debug report command here
# and implement the report generating function
# based on the new report command
- report_tools = ({'cmd': 'sosreport --help',
+ report_tools = ({'cmd': '/usr/sbin/dbginfo.sh
--help',
+ 'fn':
DebugReportsModel.debugreport_generate},
+ {'cmd': 'sosreport --help',
'fn':
DebugReportsModel.sosreport_generate},)
# check if the command can be found by shell one by
one
for helper_tool in report_tools:
try:
- retcode = subprocess.call(helper_tool['cmd'],
shell=True,
-
stdout=subprocess.PIPE,
-
stderr=subprocess.PIPE)
- if retcode == 0:
- return helper_tool['fn']
+
if 'cmd' == '/usr/sbin/dbginfo.sh --help':
+ retcode =
subprocess.call(helper_tool['cmd'], shell=True,
+
stdout=subprocess.PIPE,
+
stderr=subprocess.PIPE)
+ if retcode == 0:
+ return helper_tool['fn']
You don't need to a special condition to cover the dbginfo
command. The former code already does that.
I ll make the changes.
+
else:
+
+ retcode =
subprocess.call(helper_tool['cmd'], shell=True,
+
stdout=subprocess.PIPE,
+
stderr=subprocess.PIPE)
+ if retcode == 0:
+ return helper_tool['fn']
except Exception, e:
wok_log.info('Exception running command:
%s', e)
@@ -213,3 +279,53 @@ class DebugReportContentModel(object):
def lookup(self, name):
return self._debugreport.lookup(name)
+
+
+def delete_the_sosreport_md5_file(md5_file):
+ """
+ Deleting md5 file and displaying the contents of the
same.
+ """
+ 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)
+
+
+def sosreport_collection(name):
+ """
+ Code for the collection of sosreport n the path
+ /var/tmp as specified in the command.
+ """
+ path_sosreport = '/var/tmp/'
+ command = ['sosreport', '--batch', '--name=%s' % name,
+ '--tmp-dir=%s' % path_sosreport]
+ output, error, retcode = run_command(command)
+ if retcode != 0:
+ raise OperationFailed("GGBDR0003E", {'name': name,
+ 'err': retcode})
+
+ # 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
You can remove this comment.
+
sosreport_name = name.replace('_', '')
The same I commented before. We should not change the user input
without informing user about it.
So it is better to block underscore in the debug report name.
Please, update API.json accordingly.
I ll make the required changes.
+
sosreport_name_regex = '(\s+)(' + path_sosreport + \
+ ')(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
While using --tmp-dir you know exactly where the sosreport file
will be: <tmp-dir>-sosreport-<name>.tar.gz
So you can need to parse the command output.
I will take care of this in the next patch.
+
break
+ # Some error in sosreport happened
+ if sosreport_file is None:
+ wok_log.error('Debug report file not found. See
sosreport '
+ 'output for detail:\n%s', output)
+ raise OperationFailed('GGBDR0004E', {'name': name})
+ return sosreport_file