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

[ceph_mds] Capture alternative mds file path #3645

Merged
merged 1 commit into from
May 20, 2024

Conversation

jcastill
Copy link
Member

Newer versions of Ceph place information in
/var/lib/ceph//mds* instead of
/var/lib/ceph/mds/*, so we need to capture
the new path as well.

Related: RH: SUPDEV-154


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?

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

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

@arif-ali
Copy link
Member

@jcastill is it not worth adding to add_copy_spec below as well?

@jcastill
Copy link
Member Author

@jcastill is it not worth adding to add_copy_spec below as well?

oh wow, I completely missed that. I'll add it and push again, thank you @arif-ali !

@arif-ali
Copy link
Member

arif-ali commented May 17, 2024

@jcastill is it not worth adding to add_copy_spec below as well?

oh wow, I completely missed that. I'll add it and push again, thank you @arif-ali !

alternatively, we could just call super().setup() at the end and remove the copy_spec for those specific files, as that already does this. I'm not sure why we're not already doing that, when the Plugin class can do this for us already. Something to think about on other plugins too

@pmoravec
Copy link
Contributor

@jcastill is it not worth adding to add_copy_spec below as well?

oh wow, I completely missed that. I'll add it and push again, thank you @arif-ali !

alternatively, we could just call super().setup() at the end and remove the copy_spec for those specific files, as that already does this. I'm not sure why we're not already doing that, when the Plugin class can do this for us already. Something to think about on other plugins too

I was under the impression this is done automatically..? It imho makes sense to collect files by default, no?

@arif-ali
Copy link
Member

alternatively, we could just call super().setup() at the end and remove the copy_spec for those specific files, as that already does this. I'm not sure why we're not already doing that, when the Plugin class can do this for us already. Something to think about on other plugins too

I was under the impression this is done automatically..? It imho makes sense to collect files by default, no?

That was my original impression too, but the only place in the Plugin class we are is in the setup() function. Maybe we need some improvements here. I did a quick search for self.files in __init__py, we only find one scenario where we are collecting from that dict, and that is in the setup(), but because we always override it, we don't collect anything from the dict

@arif-ali
Copy link
Member

@pmoravec you have proved me wrong, just tested the same for the cron plugin, which only has /etc/crontab in files but nowhere else. So, this looks good to me :)

Newer versions of Ceph place information in
/var/lib/ceph/<fsid>/mds* instead of
/var/lib/ceph/mds/*, so we need to capture
the new path as well.

Related: RH: SUPDEV-154

Signed-off-by: Jose Castillo <jcastillo@redhat.com>
@jcastill jcastill force-pushed the jcastillo-add-ceph-mds-file-entry branch from 7e83bb5 to 0573184 Compare May 17, 2024 15:11
@pmoravec
Copy link
Contributor

@pmoravec you have proved me wrong, just tested the same for the cron plugin, which only has /etc/crontab in files but nowhere else. So, this looks good to me :)

But we add_copy_spec the glob /etc/cron* - when I comment it out, /etc/crontab stops to be collected.

add_default_collections method does (all?) such automatic collection from plugins triggers.

Since we touched a topic we are not certain about proper behaviour, I kicked off discussion on it in https://github.com/orgs/sosreport/discussions/3647 .

@arif-ali
Copy link
Member

@pmoravec you have proved me wrong, just tested the same for the cron plugin, which only has /etc/crontab in files but nowhere else. So, this looks good to me :)

But we add_copy_spec the glob /etc/cron* - when I comment it out, /etc/crontab stops to be collected.

Dunno why, but I overlooked that bit :), good catch

@arif-ali arif-ali merged commit ed04ba7 into sosreport:main May 20, 2024
39 checks passed
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