[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 1/4] Add logpicker into utils




Hi,

I have a few comments inlined. One of them is serious (the issue I discussed and complained about to you and msivak, the architect).

All in all it's an ack after the remaining problems are removed. Already looking forward to tell my first reporter to 'go run logpick and come back later'.

Ales

On 09/08/2010 01:22 PM, Tomas Mlcoch wrote:

+class LogPicker(object):
+
+    def send(self):
+        if not self.archive_obj:
+            raise LogPickerError("Object for sending hasn't been passed.")
+

No need to raise the exception here: in case the object is not properly configured at the time when send() is called, it is a programming error so let's just see the traceback.

+    def getlogs(self):
+
+        f = open(self.filename, 'w')
+
+        for miner in self.miners:
+            desc = "%s (%s)" % (miner._name, miner._description)
+            f.write(desc+"\n")
+            try:
+                miner(f).getlog()
+            except (LogMinerError) as e:
+                self._errprint("Warning: %s - %s" % (miner._name, e))
+                f.write("\n%s\n\n\n" % e)
+
+        f.close()
+
diff --git a/utils/log_picker/archiving.py b/utils/log_picker/archiving.py
new file mode 100644
index 0000000..753fc2b
--- /dev/null
+++ b/utils/log_picker/archiving.py
@@ -0,0 +1,117 @@
+import os
+import shutil
+import tempfile
+import tarfile
+import gzip
+
+
+class ArchivationError(Exception):
+    pass
+
+class NoFilesArchivationError(Exception):
+    pass

Perhaps make NoFilesArchivationError inherit from ArchivationError (so it's often enough to catch just one exception in an except: block)

+
+class ArchiveBaseClass(object):
+
+    _compression = False
+    _ext = ".ext"
+    _mimetype = ""
+
+    def __init__(self, *args, **kwargs):
+        self._tar_ext = ".tar"
+        pass
+
+    @property
+    def support_compression(self):
+        return self._compression
+
+    @property
+    def file_ext(self):
+        return self._ext
+
+    @property
+    def mimetype(self):
+        return self._mimetype
+
+    def _create_tmp_tar(self, filelist):
+        _, tmpfile = tempfile.mkstemp(suffix=self._tar_ext)
+        tar = tarfile.open(tmpfile, "w")
+        for name in filelist:
+            arcname = name.rsplit('/', 1)[-1]
+            tar.add(name, arcname=arcname)
+        tar.close()
+        return tmpfile
+
+    def create_archive(self, outfilename, filelist):
+        raise NotImplementedError()
+
+
+class tarArchive(ArchiveBaseClass):

Do we need to ship this class? I noticed the default is 'GZIP' and there's no option to switch to tar.

+
+    _compression = False
+    _ext = ".tar"
+    _mimetype = "application/x-tar"
+
+    def __init__(self, *args, **kwargs):
+        ArchiveBaseClass.__init__(self, args, kwargs)
+
+    def create_archive(self, outfilename, filelist):
+        if not filelist:
+            raise NoFilesArchivationError("No files to archive.")
+
+        size = 0
+        for file in filelist:
+            size += os.path.getsize(file)
+        if size<= 0:
+            raise NoFilesArchivationError("No files to archive.")
+
+        tmptar = self._create_tmp_tar(filelist)
+        shutil.move(tmptar, outfilename)
+        return outfilename
+
+
+class gzipArchive(ArchiveBaseClass):

Generaly in Anaconda we try to use uppercase letter at the start of a class name (whenever it won't make the class name look completely silly like "Iscsi").

(personally I also try to follow http://www.python.org/dev/peps/pep-0008/, but the Anaconda code is more random sometimes).


diff --git a/utils/log_picker/logmining.py b/utils/log_picker/logmining.py
new file mode 100644
index 0000000..61f300b
--- /dev/null
+++ b/utils/log_picker/logmining.py
@@ -0,0 +1,191 @@
+import os
+import shlex
+import time
+import subprocess
+
+
+class LogMinerError(Exception):
+    pass
+
+
+class LogMinerBaseClass(object):
+
+    _name = "name"
+    _description = "Description"
+    _prefer_separate_file = True
+
+    def __init__(self, logfile=None, *args, **kwargs):
+        self.logfile = logfile
+        self._used = False
+
+    def _write_separator(self):
+        self.logfile.write('\n\n')
+

this method name:
+    def _get_file(self, file):
+        self.get_files([file])
+
and this method name:
+    def _get_files(self, files):
+        if self._used:
+            self._write_separator()
+        self._used = True
+
+        for filename in files:
+            self.logfile.write('%s:\n' % filename)
+            try:
+                f = open(filename, 'r')
+            except (IOError) as e:
+                self.logfile.write("Exception while opening: %s\n" % e)
+                continue
+
+            self.logfile.writelines(f)
+            f.close()
are not too fortunate: the methods are not getting anything, have side effects and return None. Just '_write_file()' or similar?

+class AnacondaLogMiner(LogMinerBaseClass):
+
+    _name = "anaconda_log"
+    _description = "Log dumped from Anaconda."
+    _prefer_separate_file = True
+
+    def _action(self):
+        # Actual state of /tmp
+        old_state = set(os.listdir('/tmp'))
+
+        # Tell Anaconda to dump itself
+        proc = subprocess.Popen(shlex.split("killall -s SIGUSR2 anaconda"),
+                                stdout=subprocess.PIPE, stderr=subprocess.PIPE)

It would be much better if you used 'kill' with the pid from /var/run/anaconda.pid. Anaconda is forked into 3 or so processes but we only want the traceback from the main process.

+        proc.communicate()
+        #ret = os.system("killall -s SIGUSR2 anaconda")
+        if proc.returncode:
+            raise LogMinerError('Error while sending signal to Anaconda')
+
+        time.sleep(5)
+
+        # Check if new traceback file exists
+        new_state = set(os.listdir('/tmp'))
+        tbpfiles = list(new_state - old_state)
+
+        if not len(tbpfiles):
+            raise LogMinerError('Error: No anaconda traceback file exist')
+
+        for file in tbpfiles:
+            if file.startswith('anaconda-tb-'):
+                tbpfile_name = file
+                break
+        else:
+            raise LogMinerError('Error: No anaconda traceback file exist')
+
+        # Copy anaconda traceback log
+        self._get_file('/tmp/%s' % tbpfile_name)

This method is a root of all evil and I am worried we'll have to change it at some point. First it depends on the anaconda process still running and being responsive. Second it doesn't give me what I originally asked for: that is a copy of anaconda.log, storage.log, program.log etc. just as it lies in /tmp in the moment of the invocation, no other layer (meh, exception.py) included and no need to split the resulting file by hand for quick browsing.

On the other hand, you introduced a good measure of modularity and it should be simple enough to change this later (or even now!).

diff --git a/utils/logpicker b/utils/logpicker
+class ApplicationScope(object):
+    """Application configuration class."""
+
+    def __init__(self, parser_options={}):
+        # sender
+        self.sender = ""
+        self.bug_id = parser_options.bug_id or ""
+        self.bug_comment = parser_options.bug_comment or ""
+        self.bz_login = parser_options.bz_login or ""
+        self.bz_password = ""

You can also use None when you mean to say 'no value'.


+
+        # archivator
+        self.archivator = ""
+
+        # miners
+        if parser_options.l_all:
+            self.miners = MINERS

How about making this the default so we don't have to tell the reporters to use '-A' all the time? Are you worried there could be too many files?


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]