[Plugin] Standardize directory listings with new method #3664

TurboTurtle · 2024-06-10T20:27:34Z

This commit adds a new add_dir_listing() wrapper method to standardize the collection of directory content listings across plugins. By default this will use the ls command, and will use tree if the tree parameter is set to True.

This wrapper is not intended to replace directory listing that are handlded by other components with their own ls functionality (e.g. npm ls in the npm plugin). Additionally this does not replace the direct calling of ls when used with Plugin.exec_cmd() to take some action based on the output of the ls command.

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 · 2024-06-10T20:31:30Z

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

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

sos/report/plugins/vdsm.py

arif-ali

One nit, there is some inconsistency where we have used -R previously and hence stuff with the new default or recursive=True. But we've had newer updates here now where some places where we didn't have -R before we are now doing recursive and some we aren't.

Happy though, if this was all intended

sos/report/plugins/aap_gateway.py

sos/report/plugins/grub2.py

pmoravec · 2024-06-12T15:32:52Z

sos/report/plugins/aap_controller.py

+            '/var/lib/awx',
+            '/var/lib/awx/venv',
+            '/etc/tower',
+        ])


Missing recursive=False?

(should recursive have default value True or False? I would expect False since that follows default ls behaviour and I would expect it as a human-default thinking of "list directory" as well; also since the patch replaces 38 instances of recursive listings and 32 non-recursive ones, there is no need to have such strong preference to the "recursive is far more used default")

(should recursive have default value True or False? I would expect False since that follows default ls behaviour and I would expect it as a human-default thinking of "list directory" as well; also since the patch replaces 38 instances of recursive listings and 32 non-recursive ones, there is no need to have such strong preference to the "recursive is far more used default")

When I was first reviewing the PR I was thinking the same, and missed the recursive=True as the default in the function definition, and had put in ~10 comments before abandoning it. I would agree with @pmoravec here, I think having recursive=False should be default and we use recursive=True where needed

sos/report/plugins/apparmor.py

TurboTurtle · 2024-06-16T20:52:23Z

I've swapped the default to recursive=False and re-aligned things as needed. I believe I hit all of them, but a second pair of eyes to keep me honest is appreciated.

sos/report/plugins/aap_eda.py

sos/report/plugins/aap_hub.py

sos/report/plugins/ds.py

sos/report/plugins/filesys.py

sos/report/plugins/ovn_central.py

sos/report/plugins/grub2.py

TurboTurtle · 2024-06-21T21:19:52Z

@arif-ali - updated and I dropped the duplicate collection from grub2.

arif-ali

Hopefully last review, missed a couple where -d is being used, where it only gives the information of that dir, and doesn't actually list the contents of the directory. It does give the same info at the top, with . for the directory, instead of the directory name.

arif-ali · 2024-06-21T22:43:11Z

sos/report/plugins/filesys.py

@@ -108,6 +108,6 @@ class RedHatFilesys(Filesys, RedHatPlugin):

    def setup(self):
        super().setup()
-        self.add_cmd_output("ls -ltradZ /tmp")
+        self.add_dir_listing('/tmp')


what this did before, and what it will give now are 2 distinct things, not sure how we best want to do that

These options are being removed: -d to list just directory like:

$ ls -ltradZ /tmp drwxrwxrwt. 25 root root system_u:object_r:tmp_t:s0 640 Jun 22 14:44 /tmp $

and not the directory content. -t and -r to sort reversely by time is irelevant with -d option.

Calling self.add_dir_listing('/tmp') would collect same info via:

$ ls -alhZ /tmp total 44K drwxrwxrwt. 25 root root system_u:object_r:tmp_t:s0 640 Jun 22 14:44 . ..

line, but will also et details about files under /tmp. I doubt the content of the directory is important to collect at all, moreover listing the dir can be undesired ("show me your temporary filenames" has some Big Brother vibes).

We requested the current info in #1327 and that is reasonable request. Content of the directory is irelevant. So I would vote for keeping the current ls command (maybe strip redundant options to ls -ladZ /tmp?), or for some equivalent (stat?).

Ack, will move this one back to ls directly. a also is meaningless with d, so this will become ls -ldZ /tmp.

arif-ali · 2024-06-21T22:46:34Z

sos/report/plugins/vdsm.py

-        ])
+
+        self.add_dir_listing(
+            ['/etc/vdsm', '/rhev/data-center'],


same here, the -ldZ gives only the details of the directory, and no listing

arif-ali · 2024-06-21T22:47:31Z

sos/report/plugins/ssh.py

@@ -84,6 +84,6 @@ def user_ssh_files_permissions(self):
        for user in users_data:
            home_dir = self.path_join(user.pw_dir, '.ssh')
            if self.path_isdir(home_dir):


is it worth removing this check now, as you're checking if the path exists in add_dir_listing()

TurboTurtle · 2024-06-21T23:14:34Z

Hopefully last review, missed a couple where -d is being used, where it only gives the information of that dir, and doesn't actually list the contents of the directory. It does give the same info at the top, with . for the directory, instead of the directory name.

I think we can accept that difference. For the vdsm ones, RHV is an EOL product that won't be getting updated further, and I wasn't aware of any automation around those collections when I left RH, and I seriously doubt anything has been added for it since.

For /tmp, I think falling back on the . listing should be fine, but I'll defer to @pmoravec on that one.

is it worth removing this check now, as you're checking if the path exists in add_dir_listing()

Oh, good eye. I'll drop that check in the next update, pending Pavel's thoughts on /tmp.

pmoravec · 2024-06-22T12:14:58Z

sos/report/plugins/aap_gateway.py

+        self.add_cmd_output("aap-gateway-manage list_services")
+        self.add_dir_listing("/etc/ansible-automation-platform/",
+                             recursive=True)


We will additionally collect all recursive subdirs of /etc/ansible-automation-platform/ but I doubt that will cause an issue (like, speculating, some fiel in nested dir has some secret name we should not collect - but I doubt that would ever happen). ACK

pmoravec · 2024-06-22T12:18:32Z

I see one skipped placefor the new method:

sos/report/plugins/openstack_neutron.py:        self.add_cmd_output("ls -laZR /var/lib/neutron/lock")

pmoravec · 2024-06-22T13:17:09Z

sos/report/plugins/insights.py

+        self.add_dir_listing(["/etc/rhsm", "/sys/kernel", "/var/lib/sss"],
+                             recursive=True)


We stop adding tags here. Also not sure if -n is important or not (like -l, but list numeric user and group IDs).

I am checking this internally..

pmoravec · 2024-06-22T13:44:37Z

sos/report/plugins/libraries.py

-                self.add_cmd_output(f"ls -lanH {' '.join(dirs)}",
-                                    suggest_filename="ld_so_cache")
+                self.add_dir_listing(
+                    f"{' '.join(dirs)}",
+                    suggest_filename='ld_so_cache'
+                )


We will stop printing UIDs/GIDs here (dropping -n option), no idea if / how much it can matter. I expect no big deal - can @rmetrich as author of ff74942 confirm?

pmoravec · 2024-06-22T13:49:18Z

sos/report/plugins/tftpserver.py

-        self.add_cmd_output("ls -lanR /tftpboot")
-        self.add_cmd_output('ls -lanR /srv/tftp')
-
+        self.add_dir_listing(['/tftpboot', '/sr/tftp'], recursive=True)


Also removing -n option, again no idea if it matters or not. I am ok with removal, if somebody will miss it, we can add it back.

rmetrich · 2024-06-22T15:56:33Z

No need for UID indeed.

On Saturday, June 22, 2024, Pavel Moravec ***@***.***> wrote: @pmoravec commented on this pull request. ________________________________ In sos/report/plugins/tftpserver.py: > @@ -21,8 +21,6 @@ class TftpServer(Plugin, IndependentPlugin): packages = ('tftp-server',) def setup(self): - self.add_cmd_output("ls -lanR /tftpboot") - self.add_cmd_output('ls -lanR /srv/tftp') - + self.add_dir_listing(['/tftpboot', '/sr/tftp'], recursive=True) Also removing -n option, again no idea if it matters or not. I am ok with

removal, if somebody will miss it, we can add it back.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.<

https://ci3.googleusercontent.com/meips/ADKq_NYRNj593dWT-krSyFYtzfnYZKe-fvAhVHIXOIEOJ-RTFsNGfa4R6X0ECQb0MvAMktdU2_-EGvgQQCEB4gI-mONCYkdxeCkRPVx0X_Iy7_LJl_-uB3_OIAdYY1piXdplGfnVgtZIKMrQAkRjn3I0IR6AlHhOqXotskNNv0Gb-HpD4WpsV5op0LXN5Q0EuGGoYpHJrdN-Y6vMwlqDrb9omt-wuC8bv_cAPfSeofW41bHsNllh8zdYVrQxMnB3eWFeOQ=s0-d-e1-ft#https://github.com/notifications/beacon/AAI4C42GBNSRF7DRYOS6XBLZIV6HFA5CNFSM6AAAAABJC7342WWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT7F52HY.gif>Message ID: ***@***.***>

…

-- Renaud Out of working hours Sent from my phone, sorry for the typos...

This commit adds a new `add_dir_listing()` wrapper method to standardize the collection of directory content listings across plugins. By default this will use the `ls` command, and will use `tree` if the `tree` parameter is set to `True`. This wrapper is not intended to replace directory listing that are handlded by other components with their own `ls` functionality (e.g. `npm ls` in the `npm` plugin). Additionally this does not replace the direct calling of `ls` when used with `Plugin.exec_cmd()` to take some action based on the output of the `ls` command. Signed-off-by: Jake Hunsaker <jacob.r.hunsaker@gmail.com>

TurboTurtle · 2024-06-25T20:54:30Z

@pmoravec any other remaining concerns?

pmoravec

ACK, sorry for a delay.

arif-ali reviewed Jun 10, 2024

View reviewed changes

sos/report/plugins/vdsm.py Outdated Show resolved Hide resolved

TurboTurtle force-pushed the dir-listing-methods branch from 3005544 to 0bfb87d Compare June 10, 2024 22:15

arif-ali reviewed Jun 11, 2024

View reviewed changes

sos/report/plugins/aap_gateway.py Outdated Show resolved Hide resolved

sos/report/plugins/grub2.py Outdated Show resolved Hide resolved

pmoravec reviewed Jun 12, 2024

View reviewed changes

sos/report/plugins/apparmor.py Show resolved Hide resolved

TurboTurtle force-pushed the dir-listing-methods branch from 0bfb87d to 845ba3f Compare June 16, 2024 20:51

arif-ali reviewed Jun 17, 2024

View reviewed changes

sos/report/plugins/grub2.py Outdated Show resolved Hide resolved

TurboTurtle force-pushed the dir-listing-methods branch 2 times, most recently from 34da14d to c674473 Compare June 21, 2024 21:19

arif-ali reviewed Jun 21, 2024

View reviewed changes

pmoravec reviewed Jun 22, 2024

View reviewed changes

TurboTurtle force-pushed the dir-listing-methods branch from c674473 to c4957b9 Compare June 22, 2024 20:07

arif-ali approved these changes Jun 24, 2024

View reviewed changes

pmoravec approved these changes Jun 26, 2024

View reviewed changes

arif-ali merged commit b7d4e30 into sosreport:main Jun 26, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Plugin] Standardize directory listings with new method #3664

[Plugin] Standardize directory listings with new method #3664

TurboTurtle commented Jun 10, 2024

packit-as-a-service bot commented Jun 10, 2024

arif-ali left a comment

pmoravec Jun 12, 2024

arif-ali Jun 13, 2024

TurboTurtle commented Jun 16, 2024

TurboTurtle commented Jun 21, 2024

arif-ali left a comment

arif-ali Jun 21, 2024

pmoravec Jun 22, 2024

TurboTurtle Jun 22, 2024

arif-ali Jun 21, 2024

arif-ali Jun 21, 2024

TurboTurtle commented Jun 21, 2024

pmoravec Jun 22, 2024 •

edited

Loading

pmoravec commented Jun 22, 2024

pmoravec Jun 22, 2024

pmoravec Jun 22, 2024

pmoravec Jun 22, 2024

rmetrich commented Jun 22, 2024 via email

TurboTurtle commented Jun 25, 2024

pmoravec left a comment

		self.add_dir_listing(["/etc/rhsm", "/sys/kernel", "/var/lib/sss"],
		recursive=True)

[Plugin] Standardize directory listings with new method #3664

[Plugin] Standardize directory listings with new method #3664

Conversation

TurboTurtle commented Jun 10, 2024

packit-as-a-service bot commented Jun 10, 2024

arif-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurboTurtle commented Jun 16, 2024

TurboTurtle commented Jun 21, 2024

arif-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurboTurtle commented Jun 21, 2024

pmoravec Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

pmoravec commented Jun 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetrich commented Jun 22, 2024 via email

TurboTurtle commented Jun 25, 2024

pmoravec left a comment

Choose a reason for hiding this comment

pmoravec Jun 22, 2024 •

edited

Loading