[PATCH 0/2] *** Sosreport accepts underscore and support for s390x platform ***
From: Megha Smriti <mesmriti@linux.vnet.ibm.com> #748: Sosreport accepts underscore in name #749: Added debug report for plaform s390x along with sosreport Megha Smriti (2): Modified code to parse the output of sosreport command using regular expression which gives the name and location of sosreport present in command output, instead of searching the sosreport file with regular pattern "sosreport-<name>*" in /tmp and /var/tmp Modified code to check platform and if its s390x then collect dbginfo tar along with sosreport and create new tar having both the above tar and move to /var/lib/kimchi/debugreports. Also added i18n error messages required for dbginfo. src/wok/plugins/gingerbase/i18n.py | 9 +- src/wok/plugins/gingerbase/model/debugreports.py | 135 +++++++++++++++++------ 2 files changed, 106 insertions(+), 38 deletions(-) -- 2.4.0
From: Megha Smriti <mesmriti@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"), "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): 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('_', '') + 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 # 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) -- 2.4.0
For the commit message, please, use 80 characters per line. The first line correspond to a generic message followed by a description. On 02/11/2015 10:03, mesmriti@linux.vnet.ibm.com wrote:
From: Megha Smriti <mesmriti@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.
"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"
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_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.
# 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)
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@linux.vnet.ibm.com wrote:
From: Megha Smriti<mesmriti@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)
On 04/11/2015 04:46, Megha Smriti wrote:
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@linux.vnet.ibm.com wrote:
From: Megha Smriti<mesmriti@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.
No! Prior to run the sosreport command, a verification is done. So the sosreport is only run when it is installed in the host.
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.
The 'details' information should care about it. My point is: Should the user know which tool kimchi uses to collect system data? How important is that for the user? How much does user know about debug report tools? Keep in mind Kimchi/Ginger is for entry level users.
"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.
There is a list of debug report tools and a validation command to figure out which tool to use. Because that we have a specific function to sosreport.
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.
Why? It is specific for sosreport. Why should we use it overall?
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.
OK. But to list all the debug reports, Kimchi list the files in the debug report directory. So if I understood correctly, user entries "test_sosreport" it will be convert to "test-sosreport" and on next time Kimchi lists the debug report files it will list "test-sosreport' instead of the original name "test_sosreport" It will make the user confused! So, my recommendation is block underscore from user input.
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)
From: Megha Smriti <mesmriti@linux.vnet.ibm.com> --- src/wok/plugins/gingerbase/i18n.py | 2 + src/wok/plugins/gingerbase/model/debugreports.py | 80 +++++++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 01573b0..e19b997 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -36,6 +36,8 @@ 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 tar the final debug report tar file with %(retcode)s. Details: %(error)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"), "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 0bb36fe..719e460 100644 --- a/src/wok/plugins/gingerbase/model/debugreports.py +++ b/src/wok/plugins/gingerbase/model/debugreports.py @@ -25,6 +25,7 @@ import os import shutil import subprocess import time +import platform import re from wok.exception import InvalidParameter, NotFoundError, OperationFailed @@ -108,16 +109,75 @@ class DebugReportsModel(object): raise OperationFailed('GGBDR0004E', {'name': 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 + # If the platform is a system Z machine. + if platform.machine().startswith('s390'): + dbgreport_regex = '(\S+\s+)(\/\w+\/\w+-[\d+]{4}-[\d+]{2}' \ + '-[\d+]{2}-[\d+]{2}-[\d+]{2}' \ + '-[\d+]{2}-\w+-\d+\S+)(\s+\S+)' + command = ['/usr/sbin/dbginfo.sh'] + output, error, retcode = run_command(command) + if retcode != 0: + raise OperationFailed("GGBDR0009E", + {'retcode': retcode, 'err': error}) + output = output.splitlines() + for line in output: + line = line.strip() + n = re.match(dbgreport_regex, line) + if n: + dbginfo_report = n.groups()[1] + break + pathis = '/var/tmp/' + file_extension_dbginfo = dbginfo_report.split('/', 2)[2] + target_report_file = os.path.join(pathis, + file_extension_dbginfo) + msg = 'Moving the "%s" to "%s"'\ + % (dbginfo_report, target_report_file) + shutil.move(dbginfo_report, target_report_file) + wok_log.info(msg) + 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('/', 2)[2] + msg = 'Zipping the sosreport and debug info files into ' \ + 'final report file' + wok_log.info(msg) + command = ['tar', '-cvzf', '%s' % final_tar_report_name, + '-C', '/var/tmp/', 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() + dbg_target = os.path.join(path, + name + report_file_extension) + # Moving report + 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) + delete_the_sosreport_md5_file(md5_report_file) + msg = 'Deleting the dbginfo file "%s" ' \ + % target_report_file + wok_log.info(msg) + os.remove(target_report_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 + else: + 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 except WokException as e: log_error(e) -- 2.4.0
The same I commented regarding the commit message. On 02/11/2015 10:03, mesmriti@linux.vnet.ibm.com wrote:
From: Megha Smriti <mesmriti@linux.vnet.ibm.com>
--- src/wok/plugins/gingerbase/i18n.py | 2 + src/wok/plugins/gingerbase/model/debugreports.py | 80 +++++++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 01573b0..e19b997 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -36,6 +36,8 @@ 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 tar the final debug report tar file with %(retcode)s. Details: %(error)s"),
"Unable to compress the final debug report data due error %(retcode)s. Details: %(error)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"), "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 0bb36fe..719e460 100644 --- a/src/wok/plugins/gingerbase/model/debugreports.py +++ b/src/wok/plugins/gingerbase/model/debugreports.py @@ -25,6 +25,7 @@ import os import shutil import subprocess import time +import platform import re
from wok.exception import InvalidParameter, NotFoundError, OperationFailed @@ -108,16 +109,75 @@ class DebugReportsModel(object): raise OperationFailed('GGBDR0004E', {'name': 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 + # If the platform is a system Z machine. + if platform.machine().startswith('s390'):
I suggest to create a new function to handle the sosreport generation for s390x. Just to make the code easier to read.
+ dbgreport_regex = '(\S+\s+)(\/\w+\/\w+-[\d+]{4}-[\d+]{2}' \ + '-[\d+]{2}-[\d+]{2}-[\d+]{2}' \ + '-[\d+]{2}-\w+-\d+\S+)(\s+\S+)' + command = ['/usr/sbin/dbginfo.sh']
Is there a parameter like '--tmp-dir' from sosreport to use with dbginfo.sh? What about the report name? Is it auto generated?
+ output, error, retcode = run_command(command) + if retcode != 0: + raise OperationFailed("GGBDR0009E", + {'retcode': retcode, 'err': error}) + output = output.splitlines() + for line in output: + line = line.strip() + n = re.match(dbgreport_regex, line) + if n: + dbginfo_report = n.groups()[1] + break
+ pathis = '/var/tmp/' + file_extension_dbginfo = dbginfo_report.split('/', 2)[2] + target_report_file = os.path.join(pathis, + file_extension_dbginfo) + msg = 'Moving the "%s" to "%s"'\ + % (dbginfo_report, target_report_file) + shutil.move(dbginfo_report, target_report_file)
So the report is generated somewhere and moved to /var/tmp. Is that correct? Why do you need to move it to /var/tmp?
+ wok_log.info(msg)
+ final_tar_report_name = name + report_file_extension
In the code below you are creating the tar.gz file. So why are you using an extension variable? It should correspond to the tar command you are using below.
+ if dbginfo_report is not None: + sosreport_tar = sosreport_file.split('/', 3)[3] + dbginfo_tar = dbginfo_report.split('/', 2)[2] + msg = 'Zipping the sosreport and debug info files into ' \ + 'final report file' + wok_log.info(msg) + command = ['tar', '-cvzf', '%s' % final_tar_report_name, + '-C', '/var/tmp/', dbginfo_tar, sosreport_tar]
Are you mixing sosreport with dbginfo? Is sosreport still present in s390x systems? Is the debug report a collection of sosreport + dbginfo? Sorry, I am newbie with s390x tools, so it'd be good if you explain what you are going to do here.
+ 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) + # Moving report + 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) + delete_the_sosreport_md5_file(md5_report_file) + msg = 'Deleting the dbginfo file "%s" ' \ + % target_report_file + wok_log.info(msg) + os.remove(target_report_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
+ else: + 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
except WokException as e: log_error(e)
Hello Aline, Please find my comment inline. Once I will get confirmation about the method separation mention in my previous mail [PATCH 1/2]. I will send you the next version patch. Thanks, Megha On 11/3/2015 10:49 PM, Aline Manera wrote:
The same I commented regarding the commit message.
On 02/11/2015 10:03, mesmriti@linux.vnet.ibm.com wrote:
From: Megha Smriti <mesmriti@linux.vnet.ibm.com>
--- src/wok/plugins/gingerbase/i18n.py | 2 + src/wok/plugins/gingerbase/model/debugreports.py | 80 +++++++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 01573b0..e19b997 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -36,6 +36,8 @@ 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 tar the final debug report tar file with %(retcode)s. Details: %(error)s"),
"Unable to compress the final debug report data due error %(retcode)s. Details: %(error)s"
Will take care of this in next patch version.
"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 0bb36fe..719e460 100644 --- a/src/wok/plugins/gingerbase/model/debugreports.py +++ b/src/wok/plugins/gingerbase/model/debugreports.py @@ -25,6 +25,7 @@ import os import shutil import subprocess import time +import platform import re
from wok.exception import InvalidParameter, NotFoundError, OperationFailed @@ -108,16 +109,75 @@ class DebugReportsModel(object): raise OperationFailed('GGBDR0004E', {'name': 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 + # If the platform is a system Z machine. + if platform.machine().startswith('s390'):
I suggest to create a new function to handle the sosreport generation for s390x. Just to make the code easier to read.
Please let me know you input regarding the separation of function mention in my previous mail [PATCH 1/2].
+ dbgreport_regex = '(\S+\s+)(\/\w+\/\w+-[\d+]{4}-[\d+]{2}' \ + '-[\d+]{2}-[\d+]{2}-[\d+]{2}' \ + '-[\d+]{2}-\w+-\d+\S+)(\s+\S+)' + command = ['/usr/sbin/dbginfo.sh']
Is there a parameter like '--tmp-dir' from sosreport to use with dbginfo.sh?
I will use that option to generate the dbginfo at config path.
What about the report name? Is it auto generated?
Yes, the name is auto generated with format DBGINFO-*.tgz.
+ output, error, retcode = run_command(command) + if retcode != 0: + raise OperationFailed("GGBDR0009E", + {'retcode': retcode, 'err': error}) + output = output.splitlines() + for line in output: + line = line.strip() + n = re.match(dbgreport_regex, line) + if n: + dbginfo_report = n.groups()[1] + break
+ pathis = '/var/tmp/' + file_extension_dbginfo = dbginfo_report.split('/', 2)[2] + target_report_file = os.path.join(pathis, + file_extension_dbginfo) + msg = 'Moving the "%s" to "%s"'\ + % (dbginfo_report, target_report_file) + shutil.move(dbginfo_report, target_report_file)
So the report is generated somewhere and moved to /var/tmp. Is that correct? Why do you need to move it to /var/tmp?
Since sosreport was getting generated at this path and we though to compress both together if platform is s390x. Let me investigate if it is good idea to keep both tar separately in config path or should be compressed into one file.
+ wok_log.info(msg)
+ final_tar_report_name = name + report_file_extension
In the code below you are creating the tar.gz file. So why are you using an extension variable? It should correspond to the tar command you are using below.
This was done for moving the combined (sosreport compressed file+ dbginfo compressed file).
+ if dbginfo_report is not None: + sosreport_tar = sosreport_file.split('/', 3)[3] + dbginfo_tar = dbginfo_report.split('/', 2)[2] + msg = 'Zipping the sosreport and debug info files into ' \ + 'final report file' + wok_log.info(msg) + command = ['tar', '-cvzf', '%s' % final_tar_report_name, + '-C', '/var/tmp/', dbginfo_tar, sosreport_tar]
Are you mixing sosreport with dbginfo? Is sosreport still present in s390x systems? Is the debug report a collection of sosreport + dbginfo?
Yes, I will investigate if it is good idea to keep both the report separately or compressed together.
Sorry, I am newbie with s390x tools, so it'd be good if you explain what you are going to do here.
That is fine. I will add comments in my next patch explaining what it is doing.
+ 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) + # Moving report + 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) + delete_the_sosreport_md5_file(md5_report_file) + msg = 'Deleting the dbginfo file "%s" ' \ + % target_report_file + wok_log.info(msg) + os.remove(target_report_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
+ else: + 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
except WokException as e: log_error(e)
Thanks for the answers, Megha! I know have a better background to provide feedbacks and suggestions. 1) I'd say to keep compressing the sosreport + dbginfo into one tar.gz file Otherwise we would need to update the UI and I don't think it is a good idea right now. 2) For the implementation, I'd suggest to keep the function very well self contained, ie, the sosreport function will only deal with sosreport, dbginfo will only deal with that command and so. So update get_system_report_tool() to use dbginfo. As the final result would be dbginfo + sosreport @staticmethod def get_system_report_tool(): # Please add new possible debug report command here # and implement the report generating function # based on the new report command report_tools = (*{'cmd': '/usr/sbin/dbginfo.sh --help', 'fn': DebugReportsModel.dbginfo_generate},* {'cmd': 'sosreport --help', 'fn': DebugReportsModel.sosreport_generate},) Create a new function ddginfo_generate: @staticmethod def dbginfo_generate(cb, name): # first of all let's generate the sosreport DebugReportsModel.sosreport_generate(cb, name) # After it, generated the dbginfo tar file <do all the work here> # Merge sosreport + dbginfo <...> And it is done. That way we make sure to have the functions self contained. It is better for maintenance. Let me know your thoughts on it. Regards, Aline Manera On 04/11/2015 05:08, Megha Smriti wrote:
Hello Aline,
Please find my comment inline. Once I will get confirmation about the method separation mention in my previous mail [PATCH 1/2]. I will send you the next version patch.
Thanks, Megha
On 11/3/2015 10:49 PM, Aline Manera wrote:
The same I commented regarding the commit message.
On 02/11/2015 10:03, mesmriti@linux.vnet.ibm.com wrote:
From: Megha Smriti <mesmriti@linux.vnet.ibm.com>
--- src/wok/plugins/gingerbase/i18n.py | 2 + src/wok/plugins/gingerbase/model/debugreports.py | 80 +++++++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 01573b0..e19b997 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -36,6 +36,8 @@ 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 tar the final debug report tar file with %(retcode)s. Details: %(error)s"),
"Unable to compress the final debug report data due error %(retcode)s. Details: %(error)s"
Will take care of this in next patch version.
"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 0bb36fe..719e460 100644 --- a/src/wok/plugins/gingerbase/model/debugreports.py +++ b/src/wok/plugins/gingerbase/model/debugreports.py @@ -25,6 +25,7 @@ import os import shutil import subprocess import time +import platform import re
from wok.exception import InvalidParameter, NotFoundError, OperationFailed @@ -108,16 +109,75 @@ class DebugReportsModel(object): raise OperationFailed('GGBDR0004E', {'name': 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 + # If the platform is a system Z machine. + if platform.machine().startswith('s390'):
I suggest to create a new function to handle the sosreport generation for s390x. Just to make the code easier to read.
Please let me know you input regarding the separation of function mention in my previous mail [PATCH 1/2].
+ dbgreport_regex = '(\S+\s+)(\/\w+\/\w+-[\d+]{4}-[\d+]{2}' \ + '-[\d+]{2}-[\d+]{2}-[\d+]{2}' \ + '-[\d+]{2}-\w+-\d+\S+)(\s+\S+)' + command = ['/usr/sbin/dbginfo.sh']
Is there a parameter like '--tmp-dir' from sosreport to use with dbginfo.sh?
I will use that option to generate the dbginfo at config path.
What about the report name? Is it auto generated?
Yes, the name is auto generated with format DBGINFO-*.tgz.
+ output, error, retcode = run_command(command) + if retcode != 0: + raise OperationFailed("GGBDR0009E", + {'retcode': retcode, 'err': error}) + output = output.splitlines() + for line in output: + line = line.strip() + n = re.match(dbgreport_regex, line) + if n: + dbginfo_report = n.groups()[1] + break
+ pathis = '/var/tmp/' + file_extension_dbginfo = dbginfo_report.split('/', 2)[2] + target_report_file = os.path.join(pathis, + file_extension_dbginfo) + msg = 'Moving the "%s" to "%s"'\ + % (dbginfo_report, target_report_file) + shutil.move(dbginfo_report, target_report_file)
So the report is generated somewhere and moved to /var/tmp. Is that correct? Why do you need to move it to /var/tmp?
Since sosreport was getting generated at this path and we though to compress both together if platform is s390x. Let me investigate if it is good idea to keep both tar separately in config path or should be compressed into one file.
+ wok_log.info(msg)
+ final_tar_report_name = name + report_file_extension
In the code below you are creating the tar.gz file. So why are you using an extension variable? It should correspond to the tar command you are using below.
This was done for moving the combined (sosreport compressed file+ dbginfo compressed file).
+ if dbginfo_report is not None: + sosreport_tar = sosreport_file.split('/', 3)[3] + dbginfo_tar = dbginfo_report.split('/', 2)[2] + msg = 'Zipping the sosreport and debug info files into ' \ + 'final report file' + wok_log.info(msg) + command = ['tar', '-cvzf', '%s' % final_tar_report_name, + '-C', '/var/tmp/', dbginfo_tar, sosreport_tar]
Are you mixing sosreport with dbginfo? Is sosreport still present in s390x systems? Is the debug report a collection of sosreport + dbginfo?
Yes, I will investigate if it is good idea to keep both the report separately or compressed together.
Sorry, I am newbie with s390x tools, so it'd be good if you explain what you are going to do here.
That is fine. I will add comments in my next patch explaining what it is doing.
+ 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) + # Moving report + 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) + delete_the_sosreport_md5_file(md5_report_file) + msg = 'Deleting the dbginfo file "%s" ' \ + % target_report_file + wok_log.info(msg) + os.remove(target_report_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
+ else: + 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
except WokException as e: log_error(e)
participants (3)
-
Aline Manera -
Megha Smriti -
mesmriti＠linux.vnet.ibm.com