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

-------- Forwarded Message -------- Subject: Re: [PATCH] Dbginfo report generation Date: Wed, 25 Nov 2015 11:27:20 +0530 From: Megha Smriti <mesmriti(a)linux.vnet.ibm.com&gt; To: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; Hi Aline, Please find my comments inline and few suggestions. Thanks & Regards, Megha Smriti 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. Since this code was exixting prior to my patch, I am planned to let it be as it is though I

> 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. 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. Will be working according to chandra's suggestions. > + # 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 as I commented above. > + 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. I will do it in the next patch > + 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