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

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



Hi,
thank you for your comment.
I followed your advices.


----- "Ales Kozumplik" <akozumpl redhat com> wrote:

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

Exception has been removed.

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

Fixed.

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

You're right. I've removed all unused class.

> > +
> > +    _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).

I've fixed that.
 
> > 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?

Good point. I've changed names to _write_*.

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

Fixed.
 
> > +        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!).

I agree, I'll change it in future. At least for now I've added method for split anaconda log.

> > 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'.

Changed "" -> None

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

All reporters are now used by default.

Tomas


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