[Kimchi-devel] [PATCH 2/2] model/debugreports.py

[Aline Manera]

On 17/11/2015 11:54, mesmriti at linux.vnet.ibm.com wrote:
> From: Megha Smriti <mesmriti at linux.vnet.ibm.com>

Please, add an appropriated commit message explaining what this patch is 
for.

>
> ---
>   src/wok/plugins/gingerbase/model/debugreports.py | 103 ++++++++++++++---------
>   1 file changed, 61 insertions(+), 42 deletions(-)
>
> diff --git a/src/wok/plugins/gingerbase/model/debugreports.py b/src/wok/plugins/gingerbase/model/debugreports.py
> index 94ab7fe..9c325ef 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
> @@ -75,50 +75,19 @@ class DebugReportsModel(object):
>           def log_error(e):
>               log = logging.getLogger('Model')
>               log.warning('Exception in generating debug file: %s', e)
> -
>           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 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)
> -            wok_log.info(msg)
> -            shutil.move(reportFile, target)
> -            # Deleting md5
> -            msg = 'Deleting report md5 file: "%s"' % (md5_report_file)
> +            sosreport_target = os.path.join(path,
> +                                            name + report_file_extension)
> +            msg = 'Moving debug report file "%s" to "%s"' \
> +                  % (sosreport_file, sosreport_target)

Why do you need to move the file if you use the --tmp-dir to collect and 
save the sosreport in the right location?

>               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)

This is not needed anymore, right? As we are using the --tmp-dir from 
now on.

> +            delete_the_sosreport_md5_file(md5_report_file)
>               cb('OK', True)
>               return
>
> @@ -213,3 +182,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.
> +    """

This comment is not up to date

> +    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('_', '')

Do not change the user input without letting the user know about it.
As I said before, we should not allow user to enter underscore in the 
debug report name.

> +    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
> +                break

You don't need that code anymore while using --tmp-dir

The file will be <tmp-dir>/sosreport-<name>.tar.gz

> +    # 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