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

[openshift] Capture ls long listing for /mnt #3910

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jmagrini
Copy link

This plugin is used to collect Openshift LSO details. This expands the
ceph_osd plugin since storage nodes by default are not setup for
ceph access. When gathering data from an OpenShift node when
LSO is in use, we currently do not collect the symlink data location
for LSO, which is always under /mnt. Many times this directory has
pointers/symlinks to paths instead of devices.


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

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

This plugin is used to collect Openshift LSO details. This expands
the ceph_osd plugin since storage nodes by default are not setup for
ceph access. When gathering data from an OpenShift node when LSO is in use,
we currently do not collect the symlink data location for LSO,
which is always under /mnt. Many times this directory has
pointers/symlinks to paths instead of devices.

Signed-off-by: Jon Magrini <[email protected]>
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-3910
  • And now you can install the packages.

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

Removed unused import 
Removed whitespace per guidelines
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.

Couple notes. I'm honestly surprised to find out we don't capture this listing already. I'm ok with the new plugin but I'm also wondering if it'd be useful to collect this by default for everyone and move it into the filesys plugin instead - thoughts @pmoravec @arif-ali ?

@@ -0,0 +1,39 @@
# openshift_lso.py
Copy link
Member

Choose a reason for hiding this comment

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

Drop this top header line, please

Comment on lines 37 to 39
self.add_cmd_output([
'ls -lanR /mnt'
])
Copy link
Member

Choose a reason for hiding this comment

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

We have a standardized wrapper for dir listings so that stuff those types of collections are consistent across plugins - add_dir_listing(path).

@arif-ali
Copy link
Member

Couple notes. I'm honestly surprised to find out we don't capture this listing already. I'm ok with the new plugin but I'm also wondering if it'd be useful to collect this by default for everyone and move it into the filesys plugin instead - thoughts @pmoravec @arif-ali ?

I'm not keen to collect this by default, it's like collecting directory listing of /. You could have users actually mounting filesystem in /mnt/<mount point>, so if we automatically collect directory of /mnt for every sos, that we'll see the listing of all files. Now, if this is a big filesystem with lots of content, that could take a very long time to collect that data too.

As an example, I already do this on my laptop, it took 13 seconds and the file would have 863198 lines.

So from my perspective is a -1 to include it by default in filesys plugin, unless there is a good case to collect this info

@arif-ali
Copy link
Member

@jmagrini please check the pylint and flake8 tests, and resolve those 2, when you resolve the above issues that have been mentioned.

So, this plugin will run whenever there is ceph-osd on a RedHat node, even if there is no OpenShift installed it will still give you the listing for /mnt, Personally, I don't get it, but maybe I don't know Openshift enough

Further remove whitespace
Replace add_cmd_output with add_dir_listing
Pylint rated at 10/10

Signed-off-by: Jon Magrini <[email protected]>
@pmoravec
Copy link
Contributor

pmoravec commented Jan 24, 2025

I forgot to do page refresh, some changes are needed. WIP on writing them..

Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

Please merge the four commits into one.

Ensure the final commit has DCO (Signed-off-by: Jon Magrini <[email protected]> in commit message) - older 2 commits lack this hence failing DCO test.

Fix the flake8/pylint formatting issues:

sos/report/plugins/openshift_lso.py:13:1: E302 expected 2 blank lines, found 1
sos/report/plugins/openshift_lso.py:26:1: W293 blank line contains whitespace
sos/report/plugins/openshift_lso.py:28:1: W293 blank line contains whitespace
sos/report/plugins/openshift_lso.py:34:1: W293 blank line contains whitespace

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.

4 participants