Skip to content

[openstack_edpm] Collect /var/lib/openstack on EDPM nodes#4328

Merged
TurboTurtle merged 1 commit into
sosreport:mainfrom
rabi:config_layout
Jun 4, 2026
Merged

[openstack_edpm] Collect /var/lib/openstack on EDPM nodes#4328
TurboTurtle merged 1 commit into
sosreport:mainfrom
rabi:config_layout

Conversation

@rabi

@rabi rabi commented May 15, 2026

Copy link
Copy Markdown
Contributor

Collect the new EDPM OpenStack layout from /var/lib/openstack while excluding certs, cacerts, and ssh-privatekey data. Keep legacy pre-FR5 collection paths for backward compatibility.

Closes: OSPRH-30140


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

Copy link
Copy Markdown

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo dnf install -y 'dnf*-command(copr)'
  • dnf copr enable packit/sosreport-sos-4328
  • And now you can install the packages.

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

@pmoravec

Copy link
Copy Markdown
Contributor

So far we have followed the never-written-approach "each .py file in sos/report/plugins stands for a dedicated plugin" - that is broken by the openstack_common.py. I understand the rationale here, just unsure if it cant have some consequences.

Can't this break e.g. sos report -l or sos help sos.report.plugins or similar?

@rabi

rabi commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

Can't this break e.g. sos report -l or sos help sos.report.plugins or similar?

I don't see any issue with those commands; openstack_common is not being shown as a plugin. That said, if we don't want to create this kind of aberration from the unwritten convention, and are okay with a bit of duplication, I can change it. Let me know.

Comment thread sos/report/plugins/openstack_edpm.py Outdated
Comment thread sos/report/plugins/openstack_edpm.py Outdated
Comment thread sos/report/plugins/openstack_edpm.py Outdated
@rabi rabi force-pushed the config_layout branch 2 times, most recently from fa67e23 to 2e59800 Compare May 18, 2026 06:29
@pmoravec

Copy link
Copy Markdown
Contributor

Indeed, both plugins list and help subcommands work well.

IMHO code duplication is yet bit worse than this approach (that I see as not ideal but acceptable). Potential better ideas:

  • have the mask_openstack_ini_secrets method declared in openstack_edpm.py and include it from openstack_nova.py (or vice versa)
  • comment in code in the openstack_common.py that there is no plugin definition, just an auxiliary method for other Openstack classes

@rabi rabi force-pushed the config_layout branch from 2e59800 to f97c60f Compare May 18, 2026 06:31
@pmoravec pmoravec added Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Status/Needs Review This issue still needs a review from project members Kind/report Changes to the report component labels May 18, 2026

@TurboTurtle TurboTurtle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not onboard with having a non-plugin in report.plugins.

This could, perhaps, be made a more generic postproc_ini_keys() (or whatever) as part of the base Plugin class however? And then any Plugin could call self.postproc_init_keys([list_of_keys]) as part of their postproc().

ini-style files are not exactly uncommon, and I image there are other plugins today that could potentially use something like this.

@rabi

rabi commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

I'm not onboard with having a non-plugin in report.plugins.

I had checked the loader behavior and this does not break plugin discovery or help/list output. I don't think a generic base Plugin interface as you suggested is a clear fit here, since the masking is still fairly OpenStack-specific and a simple postproc_ini_keys() interface would not cover the connection-URL masking part; if that still has to be handled in the plugins by overriding, we are not really reducing duplication. If we want to keep report.plugins strictly limited to plugin modules, I can remove openstack_common.py and duplicate the masking logic in the affected OpenStack plugins. Let me know. We need this merged quickly as we're not able to support customers with sosreports.

@pmoravec

Copy link
Copy Markdown
Contributor

Is the

  • have the mask_openstack_ini_secrets method declared in openstack_edpm.py and include it from openstack_nova.py (or vice versa)

idea acceptable for both of you?

@rabi

rabi commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Is the

  • have the mask_openstack_ini_secrets method declared in openstack_edpm.py and include it from openstack_nova.py (or vice versa)

idea acceptable for both of you?

It would make one plugin module import another plugin module which would create a code-level dependency between peer plugins. If we want to avoid openstack_common.py, I’d rather duplicate the small masking logic in the affected OpenStack plugins.

@rabi

rabi commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

idea acceptable for both of you?

Folks, let me know how to proceed. If that's what want we want I can do it.

@pmoravec

pmoravec commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Neither option is ideal. So I am suggesting yet another: offload such "code common to multiple plugins but not constituting a plugin per se" to some other place - either to parent sos/report/plugins/__init__.py file, or to sos/utilities.py or to say sos/misc.py.

I am fine with either this "offloading elsewhere" approach, or code duplication, or "import one method from another plugin". Neither is ideal, all those is acceptable to me.

@rabi rabi force-pushed the config_layout branch from f97c60f to 18c0fa3 Compare June 4, 2026 03:15
@rabi

rabi commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

method from another plugin"

I am fine with either this "offloading elsewhere" approach, or code duplication, or "import one method from another plugin". Neither is ideal, all those is acceptable to me.

I duplicated the masking code in the plugins so we can get this merged and unblock customer support. This keeps the plugins self-contained, and I do not think plugin-to-plugin dependencies are a good pattern here. I also do not think sos/utilities.py is the right place for plugin-specific shared functionality. We can revisit a more general approach later if needed.

@rabi rabi requested a review from TurboTurtle June 4, 2026 03:26

@TurboTurtle TurboTurtle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand in a technical sense what this structure change does, but I don't see the benefit to going with that design pattern versus the much simpler approach that was already present and is used across all other plugins.

If there is a tangible benefit to moving to partials over the existing (and in my opinion cleaner) design, please elaborate.

I'd prefer we solve the issue of not replicating ini-style obfuscation at the onset here, but if there is a pressing need then I don't have a problem with duplicating the postproc() code in two plugins until we get something hammered out otherwise. That said, we're not going to rush through for the sake of it and our long established patterns should be maintained unless and until there is sufficient cause to break that pattern.

Comment on lines 154 to 176

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see why we're making this change? We don't actually change anything in openstack_nova, and the postproc() method here is just being put behind a level of indirection for....a reason that I'm not seeing.

Comment thread sos/report/plugins/openstack_edpm.py Outdated
Comment on lines +68 to +73
mask_openstack_ini_secrets(
lambda regex, subst: self.do_path_regex_sub(
r"/var/lib/openstack/.*", regex, subst
)
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here. I don't follow the benefit of moving this to a partial like this.

@rabi

rabi commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Well, the main benefit for me was readability rather than a behavioral change. It is still part of postproc(), but using mask_openstack_ini_secrets() makes the intent explicit and keeps the regex details out of the main flow. I also thought it could be a step toward moving this into a more appropriate shared location later. That said, the benefit here is fairly small, and if the preference is to keep the simpler inline style for consistency, I’m fine with that.

Collect the new EDPM OpenStack layout from /var/lib/openstack while
excluding certs, cacerts, and ssh-privatekey data. Keep legacy pre-FR5
collection paths for backward compatibility.

Also, fix the EDPM `services` definition to use a one-item tuple so
enabled service reporting renders the full service name correctly.

Change-Id: I0d00c7b8d288dd6c2a327c9589c979b218a7e01e
Signed-off-by: rabi <ramishra@redhat.com>
@rabi rabi force-pushed the config_layout branch from 18c0fa3 to d719e83 Compare June 4, 2026 15:48
@TurboTurtle TurboTurtle removed the Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer label Jun 4, 2026
@TurboTurtle TurboTurtle added Reviewed/Ready for Merge Has been reviewed, ready for merge Kind/Obfuscation Fixes an Obfuscation issue Kind/Collection New or updated command or file collection and removed Status/Needs Review This issue still needs a review from project members labels Jun 4, 2026
@TurboTurtle TurboTurtle merged commit b4bd64e into sosreport:main Jun 4, 2026
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Kind/Collection New or updated command or file collection Kind/Obfuscation Fixes an Obfuscation issue Kind/report Changes to the report component Reviewed/Ready for Merge Has been reviewed, ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants