Add default setup() method about sos HOT 11 CLOSED

The pcp module on branch bmr-pcp-plugin has something along these lines:

from sos.plugins import Plugin, RedHatPlugin

class sar(Plugin, RedHatPlugin):
    """Collect PCP information
    """
    files = ('/var/log/pcp', '/var/lib/pcp/config')
    packages = ('pcp',)

    def setup(self):
        self.addCopySpecs(list(self.files))

from sos.

My only concern is the possible overloading of the meaning of files. Is it common to specify paths in files that you don't actually want copies of?

from sos.

There are a few - emc checks for /opt/Navisphere/bin, vmware /usr/bin/vmware, pxe /usr/sbin/pxeos, corosync /usr/bin/corosync, systemtap /usr/bin/stap, ds /opt/redhat-ds, systemd /usr/lib/systemd/systemd, cloudforms /usr/share/katello/script/katello-debug.

I'd almost say that most of these are candidates for a new 'binaries'|'executables' list - apart from /opt/redhat-ds and /opt/Navisphere/bin they are all binaries run by the modules. Those two could be easily changed to check for a specific binary in that path.

from sos.

We could also make addCopySpec skip binary files (on a switch perhaps) so that we never copy binaries unless we really mean to.

EDIT: this might be hard to do...

from sos.

Question is how to check for them reliably? Execute perms won't work as there are legitimate cases where we do collect files with this mode - initscripts for e.g. as well as other things that hook into the system by placing an executable file somewhere (cluster resource agents etc.).

I don't really want to get into sniffing files to type them so I'd rather separate these things out into a new list if we're going to pursue this.

from sos.

Yes, it involves guessing. A common way of doing it is to look for a null byte in the first K of the file. This provides false positives for UTF-16 files. It's messy and we should probably avoid it.

Growing a new binaries/executables member simplifies things wrt automagical copying but I think it could make plugin authorship a bit more confusing. I think this can be worked around with a review of a new plugin though.

from sos.

It'd probably be better to do it the way file does - check for the magic numbers of applicable binary formats (for us probably just ELF32/64 but if it needs to work cross platform more).

Still, I think it'd be OK with documentation for authors and review - programmers already need to differentiate between the two when calling addCopySpec*() or collectExtOutput().

from sos.

python-magic could provide a way to do this, but I don't think adding a dependency is something I'd like to argue for. I'm ok with separate lists.

We might want to come up with more descriptive names for each list to give a hint as to what's going on. Of course, all bets are off if you implement your own setup() without invoking super().

Perhaps we could have a copy_files member, that setup() consumes and a required_files (check_files?) that is used specifically for checkenabled. copy_files would be consumed in checkenabled as well. Just thinking out loud here...

from sos.

I like copy_files, or files_list, or something - I've been bitten at least once by redefining a superclass member in subclasses - things get Weird® very fast when you overwrite files, for e.g. :-)

In some ways I like _list suffixes - "executable" is already getting a bit wordy (whichever synonym you use.. command, binary, executable, program, ...) - I don't like "run_programs" and "check_programs" e.g.

Anyway, this isn't urgent so we can kick it around some more to get it right.

from sos.

Another idea:

check_for = ['/path/to/thing', '/path/to/other/bin/myexe']

and

copy_list = ['/path/to/config', '/path/to/log/foo']

I'm still not 100% if it's reasonable to assume that items in copy_list should be required for the plugin to run. It might be a toss up. In the best case it fits the most common need, in the worst it makes a situation where you have to override setup() and checkenabled() to get your plugin to run in an environment where some of your files you wish to copy don't exist, but you still want to continue.

from sos.

All the plugins that include binaries in their files lists currently define a setup() method so they'd be unaffected by this. I've pushed commit f1c643d which adds a default Plugin.setup() and converts the plugins that were previously just collecting their own file lists to use it.

It means a simple plugin can now be just:

class Anacron(Plugin, RedHatPlugin, DebianPlugin, UbuntuPlugin):
    plugin_name = 'anacron'
    files = ('/etc/anacrontab',)

from sos.