Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugopts from config file not mentioned in "effective options" debug log #2663

Closed
pmoravec opened this issue Aug 30, 2021 · 5 comments
Closed
Milestone

Comments

@pmoravec
Copy link
Contributor

Currently, sos report prints a debug:

[sos.report:setup] effective options now: --batch --build --config-file /tmp/sos.conf --only-plugins qpid -vvv

before the config file is evaluated. We dont know what particular options were really used at the end.

Shall not we (maybe rename the "effective"? and ..) add a debug similar to above that will print options after all options processing is done, incl. config file?

Reproducer:

echo "[report]" > /tmp/sos.conf
echo "only-plugins = qpid" >> /tmp/sos.conf
python3 bin/sos report --conf /tmp/sos.conf -v -v -v --batch --build | grep ffect
[sos.report:setup] effective options now: --batch --build --config-file /tmp/sos.conf --only-plugins qpid -vvv

(or have the default /etc/sos/sos.conf of the same content)

@pmoravec
Copy link
Contributor Author

I apologize for confusion - the bug is in not printing plugin options only, the above example shows a correct behaviour. Proper reproducer:

echo "[plugin_options]" > /tmp/sos.conf; echo "filesys.lsof = on" >> /tmp/sos.conf
python3 bin/sos report --conf /tmp/sos.conf -v -v -v --batch --build | grep effect
[sos.report:setup] effective options now: --batch --build --config-file /tmp/sos.conf -vvv

(same happens when updating the default /etc/sos/sos.conf file - other sections are shown in the effective options", but plugin_options` not)

@pmoravec pmoravec changed the title Add debug log for "effective options" after config file is evaluated plugopts from config file not mentioned in "effective options" debug log Aug 30, 2021
@TurboTurtle
Copy link
Member

Two findings:

  1. This is caused by https://github.com/sosreport/sos/blob/main/sos/options.py#L103 - we need to instead hand a private copy of this value over here, otherwise we update the arg_default value when we later set the plugin options at https://github.com/sosreport/sos/blob/main/sos/options.py#L236

This can be solved by using self.arg_defaults.get(arg) on line 103. Easy enough.

  1. However, if we have a plugin option set in the config file and we pass -k plugin.opt=foo on the cmdline, the config file value is overwritten. This is inline with how we handle options generally right now, but I wonder if this is actually desired for plugin options specifically? Should we continue to overwrite, or should the cmdline options be appended (since we allow multiple -k invocations on the cmdline)?

@pmoravec
Copy link
Contributor Author

  1. Whatever I tried on line 103 there, I got no improvement :( .

  2. Good catch. That is a separate issue where I would lean to "append" behaviour (if needed, one can use -k plug.opt=Default to overwrite the config file setting). But it can be subjective and I am not against the current implementation.

@TurboTurtle
Copy link
Member

Ah, ok - looks like I had a separate change in my local branch that actually resolved it. If in SoSOptions.__init__() you add self.plugopts = [], you should see it log properly. Again this appears to be caused by the update_from_conf() method actually updating arg_defaults for plugopts from the conf file, thus causing argify().has_value() to detect that it hasn't been changed.

$ git diff upstream/main 
diff --git a/sos/options.py b/sos/options.py
index a014a022..3b660939 100644
--- a/sos/options.py
+++ b/sos/options.py
@@ -105,6 +105,7 @@ class SoSOptions():
         for arg in kwargs.keys():
             self.arg_names.append(arg)
             setattr(self, arg, kwargs[arg])
+        self.plugopts = []
 
     @classmethod
     def from_args(cls, args, arg_defaults={}):

$ grep -C2 kernel /etc/sos/sos.conf
[plugin_options]
#rpm.rpmva = off
kernel.with-timer = on


$ sudo ./bin/sos report -o kernel -v --batch

$ grep effectiv sos_logs/sos.log 
2021-08-31 04:53:37,974 INFO: [sos.report:setup] effective options now: --batch --only-plugins kernel --plugopts kernel.with-timer=on -v

That being said, I don't think this is really the best fix for this on its face, but I am struggling to find a better approach.

@pmoravec
Copy link
Contributor Author

Hmm.. that ignores cmdline options: try in your case "-k kernel.trace=yes":

# grep -C2 kernel /etc/sos/sos.conf

[plugin_options]
kernel.with-timer = on
# python3 bin/sos report -o kernel --batch -vvv -k kernel.trace=yes | grep effect
[sos.report:setup] effective options now: --batch --only-plugins kernel --plugopts kernel.with-timer=on -vvv
#

TurboTurtle added a commit to TurboTurtle/sos that referenced this issue Aug 31, 2021
First, provide a special-case handling for plugin options specified in
sos.conf in `SoSOptions.to_args().has_value()` that allows for plugin
options to be included in the "effective options now" log message.

Second, move the logging of said message (and thus the merging of
preset options, if used), to being _prior_ to the loading of plugin
options.

Combined, plugin options specified in sos.conf will now be logged
properly and this logging will occur before we set (and log the setting
of) those options.

Resolves: sosreport#2663

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>
TurboTurtle added a commit to TurboTurtle/sos that referenced this issue Aug 31, 2021
First, provide a special-case handling for plugin options specified in
sos.conf in `SoSOptions.to_args().has_value()` that allows for plugin
options to be included in the "effective options now" log message.

Second, move the logging of said message (and thus the merging of
preset options, if used), to being _prior_ to the loading of plugin
options.

Combined, plugin options specified in sos.conf will now be logged
properly and this logging will occur before we set (and log the setting
of) those options.

Resolves: sosreport#2663

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>
@TurboTurtle TurboTurtle added this to the 4.3 milestone Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants