1:46 a.m.
Hello Aline,
Please find my comment inline. Let me know if questions.
Thanks,
Megha
On 11/3/2015 9:21 PM, Aline Manera wrote:
For the commit message, please, use 80 characters per line.
The first line correspond to a generic message followed by a description.
This I will take care in next patch.
On 02/11/2015 10:03, mesmriti(a)linux.vnet.ibm.com wrote:
> From: Megha Smriti<mesmriti(a)linux.vnet.ibm.com>
>
> ---
> src/wok/plugins/gingerbase/i18n.py | 7 ++-
> src/wok/plugins/gingerbase/model/debugreports.py | 67 +++++++++++++-----------
> 2 files changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py
> index 8596f17..01573b0 100644
> --- a/src/wok/plugins/gingerbase/i18n.py
> +++ b/src/wok/plugins/gingerbase/i18n.py
> @@ -29,14 +29,13 @@ messages = {
>
> "GGBDR0001E": _("Debug report %(name)s does not exist"),
> "GGBDR0002E": _("Debug report tool not found in system"),
> - "GGBDR0003E": _("Unable to create debug report %(name)s. Details:
%(err)s."),
> - "GGBDR0004E": _("Can not find any debug report with the given
name %(name)s"),
> - "GGBDR0005E": _("Unable to generate debug report %(name)s.
Details: %(err)s"),
> + "GGBDR0003E": _("Unable to create sosreport report %(name)s.
Details: %(err)s."),
> + "GGBDR0004E": _("Can not find any sosreport with the given name
%(name)s"),
> + "GGBDR0005E": _("Unable to generate sosreport %(name)s. Details:
%(err)s"),
Please, keep using "debug report" instead of changing it to
"sosreport" as the sosreport is not the default debug report tool in
some distribution.
The debug report feature may not necessarily linked with sosreport. I
know it is by now but the plan is to support more and more distros in
future.
Error message "GGBDR0003E" is used when sosreport command execution return code
is non-zero.
This can also happen if the distribution does not have sosreport command for debug data
collection.
As per my understanding, keeping sosreport in error message clearly point the error that
sosreport command execution failed.
Keeping "debug report" instead of "sosreport" will be generic and will
not clearly tell the reason of failure.
> "GGBDR0006E": _("You should give a name for the debug report
file."),
> "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."),
> -
> "GGBHOST0001E": _("Unable to shutdown host machine as there are
running virtual machines"),
> "GGBHOST0002E": _("Unable to reboot host machine as there are
running virtual machines"),
> "GGBHOST0005E": _("When specifying CPU topology, each element
must be an integer greater than zero."),
> diff --git a/src/wok/plugins/gingerbase/model/debugreports.py
b/src/wok/plugins/gingerbase/model/debugreports.py
> index 94ab7fe..0bb36fe 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,7 +71,7 @@ class DebugReportsModel(object):
> raise OperationFailed("GGBDR0002E")
> @staticmethod
> - def sosreport_generate(cb, name):
> + def debugreport_generate(cb, name):
This function represents which tool to use to generate the debug
report. Keep using "sosreport_generate"
As you mention in previous
comment, debug report should be generic as
for some distro, sosreport is not the default debug report tool.
So instead we can keep this method as "debugreport_generate" and have
seperate method to collect sosreport and any other way of collection
report as different method.
And depending upon distro, we can call these methods inside
"debugreport_generate"
e.g.
def debugreport_generate(cb, name):
collect_sosreport(name)
if platform.machine().startswith('s390'):
collect_dbginfo(name)
Please let me know if it make sense to you.
> def log_error(e):
> log = logging.getLogger('Model')
> log.warning('Exception in generating debug file: %s', e)
> @@ -87,38 +87,35 @@ class DebugReportsModel(object):
> # 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
> + sosreport_name = name.replace('_', '')
Why do you replace underscore?
If underscore character is not allowed we should block user to use and
raise an appropriated error message instead of changing user's input
without any previous warning.
sosreport command allow to give name with underscore
meaning I can
execute #sosreport --batch --name=test_sosreport
However it generate the corresponding sosreport with name
sosreport-testsosreport-20151103134451.tar.xz
As sosreport command allow _ and does not throw any excpetion, the code
should also able to handle this scenario.
So we are not changing the name passed to sosreport command, we still
pass the name which we got from user.
As explained above sosreport will generate the compress report removing
the underscore.
So just for collecting the compressed file doing replace.
> + sosreport_name_regex = '(\s+)(\/\w+\/\w+\/|\/\w+\/)' \
> + '(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
It seems a lot processing to find the file path.
I've just figured out the sosreport has an option "--tmp-dir" to save
the report in a custom directory. Seems it is the better approach to
follow here.
We can use "--tmp-dir" to save the report in the file location
according to gingerbase configuration.
For example:
command = ['sosreport', '--batch', '--name=%s' %
name,*'--tmp-dir=%s' % config.get_debugreports_path()*]
Here is the command I used to test: sosreport --batch --name=alinetest
--tmp-dir=/home/alinefm
After that I got:
alinefm@alinefm-ThinkPad-T440:~/mail-patches$ ls
/home/alinefm/sosreport-alinetest-20151103134451.tar.xz
/home/alinefm/sosreport-aline-test-20151103134451.tar.xz
Using the --tmp-dir we also eliminate the need to move the debug
report after its creation.
I will take care of this in my next patch for sosreport
collection.
> # Some error in sosreport happened
> - if reportFile is None:
> + if sosreport_file 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]
> + raise OperationFailed('GGBDR0004E', {'name': 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)
> 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)
> + delete_the_sosreport_md5_file(md5_report_file)
> cb('OK', True)
> return
>
> @@ -143,7 +140,7 @@ class DebugReportsModel(object):
> # and implement the report generating function
> # based on the new report command
> report_tools = ({'cmd': 'sosreport --help',
> - 'fn': DebugReportsModel.sosreport_generate},)
> + 'fn': DebugReportsModel.debugreport_generate},)
>
> # check if the command can be found by shell one by one
> for helper_tool in report_tools:
> @@ -213,3 +210,13 @@ class DebugReportContentModel(object):
>
> def lookup(self, name):
> return self._debugreport.lookup(name)
> +
> +
> +def delete_the_sosreport_md5_file(md5_file):
> + # Deleting md5
> + 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)