Hi Aline, Please find my comments below. Regards, Megha > Subject: Re: [Kimchi-devel] [PATCH] Fixed the comments for > debugreports in report tool and removed the check for platform s390x > and introduced wok_log errors in log_errors. > Date: Mon, 30 Nov 2015 12:03:46 -0200 > From: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; > To: mesmriti(a)linux.vnet.ibm.com, Kimchi Devel <kimchi-devel(a)ovirt.org&gt; > > > > Almost there! =) > > Some comments below > > On 19/11/2015 02:56,mesmriti@linux.vnet.ibm.com wrote: > > From: root&lt;root(a)localhost.localdomain&gt; > > > > Signed-off-by:mesmriti@linux.vnet.ibm.com

> > --- > > src/wok/plugins/gingerbase/i18n.py | 4 +- > > src/wok/plugins/gingerbase/model/debugreports.py | 181 ++++++++++++++++++----- > > 2 files changed, 143 insertions(+), 42 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..c6b60e8 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,54 +71,107 @@ 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) > > + wok_log = logging.getLogger('Model') > > + wok_log.warning('Exception in generating debug file: %s', e) > > > > try: > > - command = ['sosreport', '--batch', '--name=%s' % name] > > + # 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. > > + 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("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 > > + 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 > > As you know where dbg report will be created, you can change the > dbgreport_regex to be a string pattern and use glob. > > import glob > > file = glob.glob(dgbreport_pattern) > if len(file) == 0: > # error [Megha:] Will incorporate this in my next patch. > I included I more detailed example on how use glob below. > > > - # 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] > > + final_tar_report_name = name + report_file_extension > > + 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] > > + output, error, retcode = run_command(command) > > + if retcode != 0: > > + raise OperationFailed("GGBDR0010E", > > + {'retcode': retcode, > > + 'error': error}) > > 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) > > + dbg_target = os.path.join(path, > > + name + report_file_extension) > > + # 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) > > + # 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) > > - shutil.move(reportFile, target) > > - # Deleting md5 > > - msg = 'Deleting report md5 file: "%s"' % (md5_report_file) > > + os.remove(dbginfo_report) > > + # Deleting the sosreport file > > + msg = 'Deleting the sosreport file "%s" ' % sosreport_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) > > + 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 > > + > > Please, raise an appropriated exception. Otherwise, the user will not be > able to understand what went wrong. > [Megha]: As far as exceptions are concerned These are the exception I think can be thrown back from the try block and it is also similar to what is done in sosreport. Please correct me if I any suggestions. > > + 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): > > + wok_log = logging.getLogger('Model') > > + wok_log.warning('Exception in generating debug file: %s', e) > > + 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() > > + 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(sosreport_file, sosreport_target) > > + delete_the_sosreport_md5_file(md5_report_file) > > cb('OK', True) > > return > > > > @@ -142,7 +195,9 @@ 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 > > @@ -213,3 +268,47 @@ 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_name_regex = '(\s+)(' + path_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: > > + sosreport_file = matched_name.groups()[1] > > + 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 > > You know the sosreport file was created under /var/tmp so you don't need > to query the command output to know the file was created or not. > > If you want to confirm the file was created you can use 'glob' > > import glob > > sosreport_pattern = path_sosreport + 'sosreport-' + name + '.tar.xz' > sosreport_file = glob.glob(sosreport_pattern) > > if len(sosreport_file) == 0: > raise OperationFailed() > > return sosreport_file[0] > > [Megha]: Will incorporate in my next patch > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > > > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel