[plugins] maxage and all_logs on all plugins

[Akrog]

This patch makes 2 options settable globally and per-plugin: maxage and all_logs.

Examples:

sos report --all-logs --maxage=1
sos report -k container_log.all-logs=on,container_log.maxage=1
sos report --all-logs --maxage=1 -k container_log.all-logs=off,container_log.maxage=2

This adds flexibility to what users want to retrieve.

---

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

• Is the commit message split over multiple lines and hard-wrapped at 72 characters?
• Is the subject and message clear and concise?
• Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
• Does the commit contain a Signed-off-by: First Lastname email@example.com?
• Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
• Are all passwords or private data gathered by this PR obfuscated?

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo yum install -y dnf-plugins-core on RHEL 8
• sudo dnf install -y dnf-plugins-core on Fedora
• dnf copr enable packit/sosreport-sos-3681
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[pmoravec]

Shall not we unify the code here? Both manipulations perform the same at the end, the unification would improve clarity of code.

[Akrog]

Sorry I don't understand what you mean by "unify the code".

The kdump plugin passes the maxage argument, so this code preserves existing behavior 1. Otherwise this would become a breaking change.

[pmoravec]

Would it be clear for users the option takes values in hours? Esp. if --since is primarily in days (default expected format is a day, optionally time)? Should not we rename it to, I dont know, --maxhours? (that sounds badly to me, I like the --maxage more).

I can easily be wrong here. I really dont know what users' perception would be here.

[Akrog]

I like the suggested change to maxhours, as it has the benefit of including the unit in the name and not force people to look at the description to figure it out.

I'll make the change.

[pmoravec]

I meant that

        since = None
        if self.get_option('since'):
            since = self.get_option('since')
        
        # User's plugin option argument takes precedence over plugin passed arg
        maxhours = self.get_option('maxhours') or maxhours

applies same logical change to either since or maxhours option. For the sake of code coherence, we should use same construction in either assignment, so let have there either:

        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

or:

        since = None
        if self.get_option('since'):
            since = self.get_option('since')
        
        # User's plugin option argument takes precedence over plugin passed arg
        if self.get_option('maxhours'):
            maxhours = self.get_option('maxhours')

The first construction is more concise, the second more explicit. I like the first more but 2nd is fully OK for sure.

[TurboTurtle]

We recently standardized on using hyphens for plugin options, to match the "top level" options which are all hyphens. So this will need to be all-logs (though the internal dest naming will of course still be all_logs).

[Akrog]

I'm new to this, but I assumed looking at the kernel plugin with-timer definition and how it is accessed as with-timer and not with_timer that there is no translation on the PluginOpt.

I looked a bit into the get_option and PluginOpt and didn't find any translation mechanism so I went with the solution that required fewer code changes.

But it is true that it would be weird to call it --all-logs only to have to use all_logs in the plugins.

I'll see if I can use all-logs everywhere.

[Akrog]

Done

[TurboTurtle]

In a quick test, this change works for maxhours, but does not work for all_logs - the plugin only gets the global value, which is due to this in Plugin:

sos/sos/report/plugins/__init__.py

Lines 1544 to 1564 in 7b0d5a4

	def get_option(self, optionname, default=0):
	"""Retrieve the value of the requested option, searching in order:
	parameters passed from the command line, set via `set_option()`, or the
	global_plugin_options dict.
	`optionname` may be iterable, in which case this function will return
	the first match.
	:param optionname: The name of the option to retrieve the value of
	:type optionname: ``str``
	:param default: Optionally provide a default value to return if no
	option matching `optionname` is found. Default 0
	:returns: The value of `optionname` if found, else `default`
	"""
	global_options = (
	'all_logs', 'allow_system_changes', 'cmd_timeout', 'journal_size',
	'log_size', 'plugin_timeout', 'since', 'verify'
	)

[Akrog]

In a quick test, this change works for maxhours, but does not work for all_logs - the plugin only gets the global value, which is due to this in Plugin:

sos/sos/report/plugins/__init__.py

Lines 1544 to 1564 in 7b0d5a4

	def get_option(self, optionname, default=0):
	"""Retrieve the value of the requested option, searching in order:
	parameters passed from the command line, set via `set_option()`, or the
	global_plugin_options dict.
	`optionname` may be iterable, in which case this function will return
	the first match.
	:param optionname: The name of the option to retrieve the value of
	:type optionname: ``str``
	:param default: Optionally provide a default value to return if no
	option matching `optionname` is found. Default 0
	:returns: The value of `optionname` if found, else `default`
	"""
	global_options = (
	'all_logs', 'allow_system_changes', 'cmd_timeout', 'journal_size',
	'log_size', 'plugin_timeout', 'since', 'verify'
	)

Done, I implemented it in what I consider the safest way, and it also adds a missing feature: different variable and user facing parameter name for plugin options.

Alternatively the all-logs command line argument could be changed to save the parameter in all-logs instead of all_logs and all code changed to access it with getattr instead of using .all_logs.

[TurboTurtle]

Sorry for the silence here, cycling around on it now.

The all-logs vs all_logs convention is due to python variable identifiers not allowing hyphen characters. We aligned to using hyphens in the user-facing CLI because that's typically what users expect, and it is the prevailing syntax for argparse and what it does automatically for name conversions.

Internally, developers just need to know that hyphens become underscores when accessing the associated variables, which is the case for any project using argparse and directly accessing the parsed options.

So, we're good with the original intent of the PR - expanding all-logs and maxage/maxhours to all plugins on a potentially individual basis, but we don't need to mess around with internal references to option names.

[pmoravec]

I am not sure of the sttaus of this PR - are we waiting for a change from author, or for a review? (I plan to review the current PR within a day or two, though it would be great if PR is rebased to current main - there are conflicts..)

[pmoravec]

I like it. Maybe worth adding a (report_)test? (I can come up with something if preferred)