[Kimchi-devel] [PATCH] Dbginfo report generation
Chandra Shekhar Reddy Potula
chandra at linux.vnet.ibm.com
Wed Nov 25 17:36:03 UTC 2015
One suggestion on where the final tar report file should get generated
and deletion of files after the final tar creation,
I would prefer to choose /tmp location to do all these operations before
I copy it over to final target folder for the following reasons.
1. In case developer do mistake in coding, it should not impact the
files already exists in the final folder (ie.
/var/lib/gingerbase/debugreports/). May be due code errors it should not
accidentally delete other files exists
2. In general /tmp will be used for such operation before we get the
final report.
Regards
Chandra
On 23/11/15 11:47 PM, Aline Manera wrote:
>
>
> On 17/11/2015 20:05, mesmriti at linux.vnet.ibm.com wrote:
>> From: root <root at localhost.localdomain>
>>
>> Signed-off-by: root <root at 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.
>
>> 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.
>
>> + 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.
>
>> + 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.
>
>> + # 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.
>
>> + 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.
>
>> + 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.
>
>> + 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.
>
>> + 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.
>
>> + 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
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
More information about the Kimchi-devel
mailing list