From 3d810c08848ede2a3c499d9773cb7aba2dc83ecc Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 26 Jun 2024 12:03:36 +0200 Subject: [PATCH 1/2] [PluginOpt] Support different parameter name Current `PluginOpt` code doesn't have the possibility of having a different name for the parameter and the reference used in the code. This would be the equivalent in arg parser to not having a `dest` argument available to us. This patch proposes a change to allow this using a new `PluginOpt` field called `param_name`. This new field will only be used when interfacing with users. In other words, when showing the options to users (eg: `list_plugins`), when parsing the command line arguments (eg: `_set_tunables`), and when reading the config file (this doesn't require code changes because it leverages the normal `_set_tunables` method). Signed-off-by: Gorka Eguileor --- sos/report/__init__.py | 16 ++++++++++------ sos/report/plugins/__init__.py | 15 ++++++++++++--- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/sos/report/__init__.py b/sos/report/__init__.py index 04cd994eaa..d6d40f399c 100644 --- a/sos/report/__init__.py +++ b/sos/report/__init__.py @@ -940,13 +940,17 @@ def _set_tunables(self): for plugname, plug in self.loaded_plugins: if plugname in opts: - for opt in opts[plugname]: - if opt not in plug.options: - self.soslog.error(f'no such option "{opt}" for ' + # Map from parameter names to internal option names + plug_param_map = {o.param_name: o.name + for o in plug.options.values()} + for param in opts[plugname]: + opt = plug_param_map.get(param) + if not opt: + self.soslog.error(f'no such option "{param}" for ' f'plugin ({plugname})') self._exit(1) try: - plug.options[opt].set_value(opts[plugname][opt]) + plug.options[opt].set_value(opts[plugname][param]) self.soslog.debug( f"Set {plugname} plugin option to " f"{plug.options[opt]}") @@ -1059,7 +1063,7 @@ def list_plugins(self): val = TIMEOUT_DEFAULT if opt.name == 'postproc': val = not self.opts.no_postproc - self.ui_log.info(f"{opt.name:<25} {val:<15} {opt.desc}") + self.ui_log.info(f"{opt.param_name:<25} {val:<15} {opt.desc}") self.ui_log.info("") self.ui_log.info(_("The following plugin options are available:")) @@ -1079,7 +1083,7 @@ def list_plugins(self): if tmpopt is None: tmpopt = 0 - self.ui_log.info(f" {f'{opt.plugin}.{opt.name}':<25} " + self.ui_log.info(f" {f'{opt.plugin}.{opt.param_name}':<25} " f"{tmpopt:<15} {opt.desc}") else: self.ui_log.info(_("No plugin options available.")) diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py index 191a96def9..be34cf7add 100644 --- a/sos/report/plugins/__init__.py +++ b/sos/report/plugins/__init__.py @@ -401,7 +401,8 @@ class PluginOpt(): to define options alongside their distro-specific classes in order to add support for user-controlled changes in Plugin behavior. - :param name: The name of the plugin option + :param name: The name of the plugin option as used by the code. + Equivalent to the `dest` in arg parser terminology. :type name: ``str`` :param default: The default value of the option @@ -417,9 +418,15 @@ class PluginOpt(): :param val_type: The type of object the option accepts for values. If not provided, auto-detect from the type of ``default`` :type val_type: A single type or a ``list`` of types + + :param param_name: The name of the parameter in the command line. Defaults + to ``name`` if unset. + :type param_name: ``str`` + """ name = '' + param_name = '' default = None enabled = False desc = '' @@ -429,8 +436,9 @@ class PluginOpt(): plugin = '' def __init__(self, name='undefined', default=None, desc='', long_desc='', - val_type=None): + val_type=None, param_name=None): self.name = name + self.param_name = param_name or name self.default = default self.desc = desc self.long_desc = long_desc @@ -445,6 +453,7 @@ def __init__(self, name='undefined', default=None, desc='', long_desc='', def __str__(self): items = [ f'name={self.name}', + f'param_name={self.param_name}', f'desc=\'{self.desc}\'', f'value={self.value}', f'default={self.default}' @@ -861,7 +870,7 @@ def display_plugin_help(cls, section): _def = "True/On" else: _def = "False/Off" - _ln = f"{' ':<4}{opt.name:<20}{_def:<30}{opt.desc:<20}" + _ln = f"{' ':<4}{opt.param_name:<20}{_def:<30}{opt.desc:<20}" optsec.add_text( textwrap.fill(_ln, width=TERMSIZE, subsequent_indent=opt_indent), From 8bc2ccb7c005c28ffab8aaf4e71d67c3cb25a893 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 21 Jun 2024 11:03:02 +0200 Subject: [PATCH 2/2] [plugins] maxhours and all_logs on all plugins This patch makes 2 options settable globally and per-plugin: `maxhours` and `all-logs`. The `maxhours` option maps to what the code used to call `maxage`, which has also been renamed to `maxhours` to be consistent. Examples: ``` sos report --all-logs --maxhours=1 sos report -k container_log.all-logs=on,container_log.maxhours=1 sos report --all-logs --maxhours=1 \ -k container_log.all-logs=off,container_log.maxhours=2 ``` This adds flexibility to what users want to retrieve. Signed-off-by: Gorka Eguileor --- sos/report/__init__.py | 18 +++++++- sos/report/plugins/__init__.py | 42 +++++++++++++------ sos/report/plugins/kdump.py | 4 +- .../options_tests/options_tests.py | 3 +- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/sos/report/__init__.py b/sos/report/__init__.py index d6d40f399c..f2e701c963 100644 --- a/sos/report/__init__.py +++ b/sos/report/__init__.py @@ -122,6 +122,7 @@ class SoSReport(SoSComponent): 'cmd_timeout': TIMEOUT_DEFAULT, 'profiles': [], 'since': None, + 'maxhours': None, 'verify': False, 'allow_system_changes': False, 'usernames': [], @@ -200,6 +201,12 @@ def add_parser_options(cls, parser): help="Escapes archived files older than date. " "This will also affect --all-logs. " "Format: YYYYMMDD[HHMMSS]") + report_grp.add_argument("--maxhours", action="store", + dest="maxhours", default=None, type=int, + help="Escapes archived files older (according " + "to `mktime`) than this many hours. This will " + "also affect --all-logs (sets the default for " + "all plugins)") report_grp.add_argument("--build", action="store_true", dest="build", default=False, help="preserve the temporary directory and do " @@ -904,6 +911,12 @@ def _set_all_options(self): opt.value = True def _set_tunables(self): + # Set plugin option's defaults to the global argument values + for pluginname, plugin in self.loaded_plugins: + for optname in plugin.options: + if hasattr(self.opts, optname): + plugin.options[optname].value = getattr(self.opts, optname) + if self.opts.plugopts: opts = {} for opt in self.opts.plugopts: @@ -1063,12 +1076,13 @@ def list_plugins(self): val = TIMEOUT_DEFAULT if opt.name == 'postproc': val = not self.opts.no_postproc - self.ui_log.info(f"{opt.param_name:<25} {val:<15} {opt.desc}") + self.ui_log.info( + f"{opt.param_name:<25} {val!s:<15} {opt.desc}") self.ui_log.info("") self.ui_log.info(_("The following plugin options are available:")) for opt in self.all_options: - if opt.name in ('timeout', 'postproc', 'cmd-timeout'): + if opt.name in _defaults: if opt.value == opt.default: continue # format option value based on its type (int or bool) diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py index be34cf7add..37d7f876e8 100644 --- a/sos/report/plugins/__init__.py +++ b/sos/report/plugins/__init__.py @@ -594,7 +594,19 @@ def __init__(self, commons): self.set_predicate(SoSPredicate(self)) def get_default_plugin_opts(self): + # The default of options that have a global argument counterpart with + # the same name must use the same default in both places. return { + 'maxhours': PluginOpt( + 'maxhours', default=None, val_type=int, + desc='Escapes archived files older (according to `mktime`) ' + 'than this many hours' + ), + 'all_logs': PluginOpt( + 'all_logs', default=False, val_type=bool, + param_name='all-logs', + desc='collect all available logs regardless of size', + ), 'timeout': PluginOpt( 'timeout', default=-1, val_type=int, desc='Timeout in seconds for plugin to finish all collections' @@ -1567,9 +1579,12 @@ def get_option(self, optionname, default=0): :returns: The value of `optionname` if found, else `default` """ + # These are Global options that can only be set globally. Options that + # can be set globally and per-plugin (those also returned by + # get_default_plugin_opts) are handled by the `_set_tunables` method. global_options = ( - 'all_logs', 'allow_system_changes', 'cmd_timeout', 'journal_size', - 'log_size', 'plugin_timeout', 'since', 'verify' + 'allow_system_changes', 'cmd_timeout', 'journal_size', 'log_size', + 'plugin_timeout', 'since', 'verify' ) if optionname in global_options: @@ -1636,7 +1651,7 @@ def generate_copyspec_tags(self): manifest_data['files_copied'] = matched_files self.manifest.files.append(manifest_data) - def add_copy_spec(self, copyspecs, sizelimit=None, maxage=None, + def add_copy_spec(self, copyspecs, sizelimit=None, maxhours=None, tailit=True, pred=None, tags=[], container=None): """Add a file, directory, or globs matching filepaths to the archive @@ -1647,9 +1662,9 @@ def add_copy_spec(self, copyspecs, sizelimit=None, maxage=None, to this size in MB :type sizelimit: ``int`` - :param maxage: Collect files with `mtime` not older than this many - hours - :type maxage: ``int`` + :param maxhours: Collect files with `mtime` not older than this many + hours + :type maxhours: ``int`` :param tailit: Should a file that exceeds `sizelimit` be tail'ed to fit the remaining space to meet `sizelimit` @@ -1679,9 +1694,10 @@ def add_copy_spec(self, copyspecs, sizelimit=None, maxage=None, sos will collect up to 25MB worth of files within `/etc/foo`, and will collect the last 25MB of `/etc/bar.conf`. """ - since = None - if self.get_option('since'): - since = self.get_option('since') + since = self.get_option('since') or None + + # User's plugin option argument takes precedence over plugin passed arg + maxhours = self.get_option('maxhours') or maxhours logarchive_pattern = re.compile(r'.*((\.(zip|gz|bz2|xz))|[-.][\d]+)$') configfile_pattern = re.compile(fr"^{self.path_join('etc')}/*") @@ -1798,8 +1814,8 @@ def getmtime(path): return 0 def time_filter(path): - """ When --since is passed, or maxage is coming from the - plugin, we need to filter out older files """ + """ When --since is passed, or maxhours is coming from the + plugin or its options, we need to filter out older files """ # skip config files or not-logarchive files from the filter if ((logarchive_pattern.search(path) is None) or @@ -1808,11 +1824,11 @@ def time_filter(path): filetime = getmtime(path) filedatetime = datetime.fromtimestamp(filetime) if ((since and filedatetime < since) or - (maxage and (time()-filetime < maxage*3600))): + (maxhours and (time()-filetime < maxhours*3600))): return False return True - if since or maxage: + if since or maxhours: files = list(filter(lambda f: time_filter(f), files)) files.sort(key=getmtime, reverse=True) diff --git a/sos/report/plugins/kdump.py b/sos/report/plugins/kdump.py index a1be9ea462..21205cd939 100644 --- a/sos/report/plugins/kdump.py +++ b/sos/report/plugins/kdump.py @@ -100,7 +100,7 @@ def setup(self): # collect the latest vmcore created in the last 24hrs <= 2GB if self.get_option("get-vm-core"): - self.add_copy_spec(f"{path}/*/vmcore", sizelimit=2048, maxage=24) + self.add_copy_spec(f"{path}/*/vmcore", sizelimit=2048, maxhours=24) class DebianKDump(KDump, DebianPlugin, UbuntuPlugin): @@ -178,6 +178,6 @@ def setup(self): # collect the latest vmcore created in the last 24hrs <= 2GB if self.get_option("get-vm-core"): - self.add_copy_spec(f"{path}/*/vmcore", sizelimit=2048, maxage=24) + self.add_copy_spec(f"{path}/*/vmcore", sizelimit=2048, maxhours=24) # vim: set et ts=4 sw=4 : diff --git a/tests/report_tests/options_tests/options_tests.py b/tests/report_tests/options_tests/options_tests.py index 15f0b57248..d40abfd5ee 100644 --- a/tests/report_tests/options_tests/options_tests.py +++ b/tests/report_tests/options_tests/options_tests.py @@ -28,11 +28,12 @@ def test_plugins_only_from_config(self): def test_plugopts_logged_from_config(self): self.assertSosLogContains( r"Set kernel plugin option to \(name=with-timer, " + r"param_name=with-timer, " r"desc='gather /proc/timer\* statistics', value=True, " r"default=False\)" ) self.assertSosLogContains( - r"Set kernel plugin option to \(name=trace, " + r"Set kernel plugin option to \(name=trace, param_name=trace, " "desc='gather /sys/kernel/debug/tracing/trace file', " r"value=True, default=False\)" )