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

[ssh] Stop collecting 'ls' information from non-local home dirs #3807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcastill
Copy link
Member

Commands like 'ls -laZ' can take some time to run in remote filesystems, for example NFS. When you have hundreds of them, sos can take a long time to walk through it, and could be even worse if we use the option 'recursive'.
For ssh home dirs, we don't need the listing, so in this patch we detect the type of filesystem and run the command only on local ones.

Related: RHEL-22389, SUPDEV-156


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 protected]?
  • 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?

@jcastill
Copy link
Member Author

This is a different approach to what I was trying to solve in #3490 and I opened a new PR to keep both approaches separated.
In this case, we'll only skip these homedirs that are not mounted locally. In the other PR, I was dancing between disable the option completely or discarding all homedirs completely.

I tried to find if we gather this info in other places, but I could only find the function _get_devices_by_fstype() that I think is slightly different. Would it make sense to create a _get_mounted_fstypes() or similar in sos/report/init.py and call that from the ssh (and other) plugins?

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

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

@pmoravec
Copy link
Contributor

pmoravec commented Oct 14, 2024

The change makes sense; maybe worth having an option to keep current behaviour..? (no idea how useful it would be, though).

Will this PR make #3490 obsolete?

@jcastill
Copy link
Member Author

The change makes sense; maybe worth having an option to keep current behaviour..? (no idea how useful it would be, though).

I thought about that, but I keep being told that the reward for getting this info is not worth the slowness running sos that we get when going through NFS filesystems

Will this make #3490 obsolete?

Yes, I'll keep open only the one that you all agree it's a better approach.

# Read the home paths of users in the system and check the ~/.ssh dirs
for user in users_data:
if user.pw_dir in fs_mount_info and \
fs_mount_info[user.pw_dir] in non_local_fs:
continue
Copy link
Member

Choose a reason for hiding this comment

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

We should log something so that end users aren't left wondering why this wasn't collected when previously they may have been expecting it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What verbosity level? Info? (or debug? isnt it too hidden..?)

Copy link
Member Author

Choose a reason for hiding this comment

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

With warn I get this:

 Setting up archive ...
 Setting up plugins ...
[plugin:ssh] Skipping capture in /home/jcastill because it's a remote directory
 Running plugins. Please wait ...

  Starting 1/1   ssh             [Running: ssh]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and I dont know if that is not too "vocal". I think info verbosity is the best, but I am ok with any.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, in the specific scenario of tens or hundreds of NFS mounted homedirs, the output would be huge.

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 verbosity should not be warning, right..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. I'm pushing a change with info now

@jcastill jcastill force-pushed the jcastillo-capture-ssh-userconfs-only-local branch 2 times, most recently from dea08ef to 7eef06b Compare October 15, 2024 06:44
Commands like 'ls -laZ' can take some time to run in remote filesystems,
for example NFS. When you have hundreds of them, sos can take a long
time to walk through it, and could be even worse if we use the option
'recursive'.
For ssh home dirs, we don't need the listing, so in this patch we detect
the type of filesystem and run the command only on local ones.

Related: RHEL-22389, SUPDEV-156

Signed-off-by: Jose Castillo <[email protected]>
@jcastill jcastill force-pushed the jcastillo-capture-ssh-userconfs-only-local branch from 7eef06b to dd8f61d Compare October 15, 2024 13:09
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.

3 participants