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

[memcached] Collect from memcached using the tool #3654

Merged
merged 1 commit into from
Jun 16, 2024

Conversation

arif-ali
Copy link
Member

@arif-ali arif-ali commented Jun 6, 2024


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

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

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, though it seems strange to me that memcached-tool wouldn't be in $PATH when included as a packaged executable (whereas if you were installing from source, totally understandable).

@arif-ali
Copy link
Member Author

arif-ali commented Jun 6, 2024

LGTM, though it seems strange to me that memcached-tool wouldn't be in $PATH when included as a packaged executable (whereas if you were installing from source, totally understandable).

It's in the path for rocky8/rocky9 (which I just tested) which I am presume will be the case for RH based systems but it is not for ubuntu. So, happy to change that for the RedHatPlugin if you prefer

@arif-ali arif-ali force-pushed the sos-arif-memcached-updates branch from ea3d153 to 2a2df04 Compare June 6, 2024 17:31
Signed-off-by: Arif Ali <arif.ali@canonical.com>
@arif-ali arif-ali force-pushed the sos-arif-memcached-updates branch from 2a2df04 to 0fdab4e Compare June 6, 2024 17:31
@TurboTurtle
Copy link
Member

LGTM, though it seems strange to me that memcached-tool wouldn't be in $PATH when included as a packaged executable (whereas if you were installing from source, totally understandable).

It's in the path for rocky8/rocky9 (which I just tested) which I am presume will be the case for RH based systems but it is not for ubuntu. So, happy to change that for the RedHatPlugin if you prefer

Didn't need to change, was just making an observation that I am surprised that the Ubuntu packaging wouldn't drop it in $PATH.

Either way though, this looks good to me.

@arif-ali
Copy link
Member Author

arif-ali commented Jun 9, 2024

Didn't need to change, was just making an observation that I am surprised that the Ubuntu packaging wouldn't drop it in $PATH.

Either way though, this looks good to me.

It looked more elegant, and less code.

Reading up on it, apparently it can cause a pause on the cache gathering/setting if this tool is run, so that could be the reason for not adding it to the $PATH. Maybe a warning is at hand here, wdyt?

@TurboTurtle
Copy link
Member

If it can cause an interruption to normal operation I think we'd ideally gate it behind a plugin option, but there isn't much else this plugin collects and I doubt that the general use case for support folks would be well served without this info. Maybe a warning would suffice, let's dig in a little bit.

Is there any indication on how likely a pause or how long the pause could be (full duration of the command, or is it very brief at the beginning of execution), or for that matter is it any time this command is run, or only for specific sub commands like dump?

@arif-ali
Copy link
Member Author

arif-ali commented Jun 15, 2024

Reading up on this if we run stat sizes then there could be issues as per https://github.com/memcached/memcached/wiki/Commands#stats-sizes. As we're not passing that command, we should be ok.

So I think, this should be good to go

@TurboTurtle TurboTurtle merged commit 606bd81 into sosreport:main Jun 16, 2024
32 checks passed
@arif-ali arif-ali deleted the sos-arif-memcached-updates branch October 2, 2024 13:47
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.

2 participants