Re: [Kimchi-devel] [PATCH] Dbginfo report generation

On 17/11/2015 20:05, mesmriti(a)linux.vnet.ibm.com wrote: > From: root <root(a)localhost.localdomain&gt; > > Signed-off-by: root <root(a)localhost.localdomain&gt; > --- > 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(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel