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.

Sign up for GitHub

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

Jump to bottom

[collect] Handle custom node and inherited configs with collections #3852

Merged
merged 1 commit into from
Dec 3, 2024
Merged

[collect] Handle custom node and inherited configs with collections #3852

merged 1 commit into from
Dec 3, 2024

Conversation

TrevorBenson
Copy link
Member

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 Packit-as-a-Service
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-3852
  • And now you can install the packages.

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

@TrevorBenson TrevorBenson force-pushed the sos-trevorbenson-collector-node-inherit-config branch 2 times, most recently from 8641897 to d6bc604 Compare November 26, 2024 07:12
jcastill
jcastill suggested changes Nov 26, 2024
View reviewed changes
sos/collector/transports/__init__.py Show resolved Hide resolved
sos/collector/transports/control_persist.py Outdated Show resolved Hide resolved
@TrevorBenson TrevorBenson force-pushed the sos-trevorbenson-collector-node-inherit-config branch from 24cdb35 to 4b56b74 Compare November 26, 2024 08:04
@arif-ali
Copy link
Member

I've restarted the 2 failed jobs, not sure why they are failing, and it's not the first time, I'll take that away as a separate issue

@TrevorBenson TrevorBenson marked this pull request as ready for review November 26, 2024 16:12
TurboTurtle
TurboTurtle reviewed Nov 26, 2024
View reviewed changes
sos/collector/__init__.py Outdated Show resolved Hide resolved
sos/collector/sosnode.py Outdated Show resolved Hide resolved
sos/collector/sosnode.py Outdated
Comment on lines 793 to 795
self.run_command(f"rm {remote_config} ; rmdir {self.tmpdir}",
use_shell=True,
need_root=True)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, same here. Very much not a fan of doing rms like this - the local sos installation on the remote node should take care of any cleanup. If we place the inherited config files in /tmp, then they would be both out of the way and cleaned up separately by long-standing behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll presume you mean leaving the inherited conf in the tmp directory and letting systemd-tmpfiles-clean.timer RHEL, or tmpfiles.d for Ubuntu, handle the cleanup on schedule?

It works for my use case, as long as we are comfortable that the file remains might (although probably uncommon) store plugin_options in the file for things like mysql.dbpass, postgresql.password, skydive.password?

@arif-ali
Copy link
Member

I've started building some packages for testing [1], so will do the testing soon

[1] https://launchpad.net/~arif-ali/+archive/ubuntu/sos-pr-3852

@arif-ali arif-ali added the Status/Needs Review This issue still needs a review from project members label Nov 28, 2024
@jcastill
Copy link
Member

@TrevorBenson did you test the oc transport already? I'm asking because I'm setting up an environment to test some other stuff and I can use it to test this PR as well.

arif-ali
arif-ali approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@arif-ali arif-ali left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tested both the of the new config options, and works as expected from the juju perspective. Thanks for your work on this

EDIT: don't forget to squash your commits, as ideally it is preferred to have one commit per PR

pmoravec
pmoravec reviewed Nov 29, 2024
View reviewed changes
sos/collector/sosnode.py Outdated Show resolved Hide resolved
@pmoravec
Copy link
Contributor

Just the 3 / 5 retry attempts are worth to fix, otherwise ACK.

TurboTurtle
TurboTurtle reviewed Nov 30, 2024
View reviewed changes
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.

Looks good to me overall. Just two super minor nits, and Pavel's comment on the 3 vs 5 attempts. FWIW, I think 3 attempts is sufficient.

sos/collector/transports/control_persist.py Outdated Show resolved Hide resolved
sos/collector/transports/juju.py Outdated Show resolved Hide resolved
sos/collector/transports/local.py Outdated Show resolved Hide resolved
@TrevorBenson
Copy link
Member Author

@TrevorBenson did you test the oc transport already? I'm asking because I'm setting up an environment to test some other stuff and I can use it to test this PR as well.

Nope. not yet. I'd need to reconfigure some things or get a spare system so would take me at least a week before I could do testing via oc.

@TrevorBenson
Copy link
Member Author

tested both the of the new config options, and works as expected from the juju perspective. Thanks for your work on this

EDIT: don't forget to squash your commits, as ideally it is preferred to have one commit per PR

No problem as I tend to squash/fixup everything, but sometimes I've seen the force push removing a line hide comments which appeared to come from a review of a specific commit vs. the entire file.

Once all reviewers are happy I'll get it into the single commit and force push. 👍🏼

TurboTurtle
TurboTurtle approved these changes Dec 2, 2024
View reviewed changes
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.

LGTM. Please just squash at least the 2 review commits.

@TurboTurtle TurboTurtle added Reviewed/Ready for Merge Has been reviewed, ready for merge Patch formatting and removed Status/Needs Review This issue still needs a review from project members labels Dec 2, 2024
@TrevorBenson TrevorBenson force-pushed the sos-trevorbenson-collector-node-inherit-config branch from 3d35b2f to 4fdbb38 Compare December 2, 2024 16:30
@TrevorBenson
Copy link
Member Author

TrevorBenson commented Dec 2, 2024 •
edited
Loading

LGTM. Please just squash at least the 2 review commits.

I've squashed into a single commit.

My initial tests were with control persist and all worked as expected. Once the checks are complete I will load from the updated copr repo for the saltstack transport test.

I believe @arif-ali will test, or already has tested, with juju & @jcastill will test with oc.

@arif-ali
Copy link
Member

arif-ali commented Dec 2, 2024

I believe @arif-ali will test, or already has tested, with juju & @jcastill will test with oc.

Yup, I already tested, and reported above from the juju perspective

@TrevorBenson
[collect] Node and Inherit config files
ff74a74 
Signed-off-by: Trevor Benson <trevor.benson@gmail.com>
@TrevorBenson TrevorBenson force-pushed the sos-trevorbenson-collector-node-inherit-config branch from 4fdbb38 to ff74a74 Compare December 2, 2024 18:20
@TrevorBenson
Copy link
Member Author

FYI I moved the arguments below --become and --case-id, before --cluster-type. They felt to easy to overlook when placed after the --upload options.

@TurboTurtle TurboTurtle merged commit 0ee02ea into sosreport:main Dec 3, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TurboTurtle TurboTurtle TurboTurtle approved these changes

@arif-ali arif-ali arif-ali approved these changes

@jcastill jcastill Awaiting requested review from jcastill

@pmoravec pmoravec Awaiting requested review from pmoravec

Assignees
No one assigned
Labels
Reviewed/Ready for Merge Has been reviewed, ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TrevorBenson @arif-ali @jcastill @pmoravec @TurboTurtle