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

[common] handle all_logs when snap package is installed #3684

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

arif-ali
Copy link
Member

@arif-ali arif-ali commented Jun 22, 2024

Add a new flag to add_cmd_output to suggest a snap_cmd, so that sos
does not direct the command directly to a file.

Resolves: #3683


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?

@arif-ali
Copy link
Member Author

Putting this as an idea, but, maybe we can do something in check_enabled instead to simplify this

Copy link

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-3684
  • And now you can install the packages.

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

@dnegreira
Copy link
Contributor

I like the idea overall, but I think the logic could be improved a bit, left some comments on it

sos/report/plugins/__init__.py Outdated Show resolved Hide resolved
sos/report/plugins/grafana.py Outdated Show resolved Hide resolved
@arif-ali arif-ali force-pushed the sos-arif-snap-cmd-all-logs branch 2 times, most recently from 8546e2d to 08050c2 Compare June 25, 2024 11:17
@arif-ali
Copy link
Member Author

Put in 2 ideas

  1. first commit still has some legs, and we standardise how we check for snaps, and just use one variable in plugins. Added option to disable to_file as well. The disabling is not ideal, as you want to ensure we don't use too much of the memory
  2. Second commit removes this, and we export the command to STDOUT and grab it via cat , this works for snap confinement case as well, which I have tested locally.

if we decide the second option is better, then we could split this PR into 2, and have one for standardising is_snap_installed() and the other to resolve the confinement issue using cat

thoughts?

@arif-ali arif-ali changed the title [snap,common] If snap don't use to_file [common] handle all_logs when snap package is installed Jun 27, 2024
@arif-ali arif-ali marked this pull request as ready for review June 27, 2024 10:52
@arif-ali
Copy link
Member Author

arif-ali commented Jun 27, 2024

alternatively, we can the change below (after the recent PR) but that defeats the purpose of the original commit of having commands outputting to file with all_logs

diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py
index 74ade1fb..2009a2cf 100644
--- a/sos/report/plugins/__init__.py
+++ b/sos/report/plugins/__init__.py
@@ -1990,7 +1990,8 @@ class Plugin():
             kwargs['priority'] = 10
         if 'changes' not in kwargs:
             kwargs['changes'] = False
-        if self.get_option('all_logs') or kwargs['sizelimit'] == 0:
+        if not self.is_snap and (self.get_option('all_logs') or
+                                 kwargs['sizelimit'] == 0):
             kwargs['to_file'] = True
         soscmd = SoSCommand(**kwargs)
         self._log_debug("packed command: " + soscmd.__str__())

@TurboTurtle
Copy link
Member

I'm not a big fan of inserting a cat execution like this before every command as a test/gate of being able to write out to a file, we should be able to do things more programmatically here.

I'm onboard with your first idea, allowing the plugin to disable/override/etc any enabling of to_file that would otherwise occur.

@arif-ali arif-ali force-pushed the sos-arif-snap-cmd-all-logs branch 3 times, most recently from c8737aa to 1ff241e Compare August 8, 2024 17:17
Add a new flag to add_cmd_output to suggest a snap_cmd, so that sos
does not direct the command directly to a file.

Resolves: sosreport#3683

Signed-off-by: Arif Ali <arif.ali@canonical.com>
Comment on lines -2010 to +2011
if self.get_option('all_logs') or kwargs['sizelimit'] == 0:
if (not getattr(SoSCommand(**kwargs), "snap_cmd", False) and
(self.get_option('all_logs') or kwargs['sizelimit'] == 0)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the impact of the snap_cmd option will be "dont store directly to file even when all_logs is set"? That is independent on snap packaging, so I suggest changing option name.

E.g. why not to use to_file directly? We would just have to change condition at https://github.com/sosreport/sos/blob/main/sos/report/plugins/__init__.py#L2010 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use to_file directly to be set false for snap specific, but we can't afford to have to_file to be true if the command is being run is a snap command. We'd need a different test here for this then, if we drop the snap_cmd paramater. Ultimately to_file is False by default, so somehow we need to detect is the command being run is a snap or not, maybe check where the command is coming from, i.e. /snap/bin.

We don't want to do a blanket all commands to be not run to file, as journalctl wouldn't be snapped, and hence can write to file.

So, we could have for example

sunbeam cluster list <--snap
journalctl <-- not snap

now, the latter command, we can run with to_file=True, but the former we can't

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. One part of my concern was also to the parameter name itself, trying to keep Plugin independent on packaging implementation (same concernt I would raise against, say, is_rpm parameter name). But I cant figure out some generic enough name (force_to_file? too long..) that I would suggest to use.

So ACK from me.

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned on IRC, I have some concerns around maintainability and the potential for snap_cmd=True to be marked for commands that aren't coming from a snap in a given installation - but for the state of sos today at least, that doesn't appear to be an actual potential issue.

Ack for the current PR, but we'll want to keep this in mind going forward so that we don't drown ourselves in a sort of "snap_cmd dance" for more generalized plugins.

@TurboTurtle TurboTurtle merged commit db95a3e into sosreport:main Aug 13, 2024
32 checks passed
@arif-ali arif-ali deleted the sos-arif-snap-cmd-all-logs branch October 2, 2024 13:46
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

Successfully merging this pull request may close these issues.

[snaps] when using --all-logs some command outputs will not be collected
4 participants