[Kimchi-devel] Fwd: Re: [PATCH] Dbginfo report generation
Aline Manera
alinefm at linux.vnet.ibm.com
Thu Nov 26 17:27:36 UTC 2015
Hi Megha,
I am not sure if your email was truncated but I don't see your comments.
On 25/11/2015 16:46, Megha Smriti wrote:
>
>
>
> -------- Forwarded Message --------
> Subject: Re: [PATCH] Dbginfo report generation
> Date: Wed, 25 Nov 2015 11:27:20 +0530
> From: Megha Smriti <mesmriti at linux.vnet.ibm.com>
> To: Aline Manera <alinefm at linux.vnet.ibm.com>
>
>
>
>
> Hi Aline,
>
> Please find my comments inline and few suggestions.
>
> Thanks & Regards,
> Megha Smriti
>
> 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.
>
> 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
>
>
>
>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20151126/c7d77197/attachment.html>
More information about the Kimchi-devel
mailing list