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

[sssd] Collect memory cache individual files #2462

Closed
wants to merge 1 commit into from

Conversation

elkoniu
Copy link
Contributor

@elkoniu elkoniu commented Mar 24, 2021

By default SSSD collects all memory cache files:

  • /var/lib/sss/mc/passwd
  • /var/lib/sss/mc/group
  • /var/lib/sss/mc/initgroups

They all are included in 25MB size limit for sosreport.
This commits add memory cache files one - by - one,
this way 25MB size limit will be aplied per file.


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?
  • If this commit closes an existing issue, is the line Closes: #ISSUENUMBER included in an independent line?
  • If this commit resolves an existing pull request, is the line Resolves: #PRNUMBER included in an independent line?

@elkoniu elkoniu force-pushed the memcache-size-limit branch 2 times, most recently from cc3e846 to eb452e9 Compare March 24, 2021 15:29
@elkoniu
Copy link
Contributor Author

elkoniu commented Mar 24, 2021

This PR is combination of both:
#2444
#2445

It extends logic from #2445 to affect memory cache files we use in SSSD daemon.

Comment on lines 38 to 40
self.add_copy_spec(glob("/var/lib/sss/mc/passwd"))
self.add_copy_spec(glob("/var/lib/sss/mc/group"))
self.add_copy_spec(glob("/var/lib/sss/mc/initgroups"))
Copy link
Member

Choose a reason for hiding this comment

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

Are these files or directories? If they're straight files there's no reason to glob here, and you can just pass the individual file names in a list in a single add_copy_spec() call. Each item in a list has the --log-size limitation applied to it individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this suggestion, I just updated the code. Please take a look and share you thoughts.

By default SSSD collects all memory cache files:
* /var/lib/sss/mc/passwd
* /var/lib/sss/mc/group
* /var/lib/sss/mc/initgroups

They all are included in 25MB size limit for sosreport.
This commits add memory cache files one - by - one,
this way 25MB size limit will be aplied per file.

Signed-off-by: Paweł Poławski <ppolawsk@redhat.com>
Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

Assuming each binary file is under 25MB sizelimit every time, ACK.

@pmoravec
Copy link
Contributor

The failed tests are irelevant to the PR; sssd plugin was inactivated in both of them.

@thalman
Copy link
Contributor

thalman commented Mar 25, 2021

@pmoravec, @elkoniu normally this should be OK, it is much better like this.

But I wonder if we can handle the situation when files are bigger due to some user's customization. I know there is this sizelimit=0 - that would pack everything. The price will be bigger sosreport. Can we somehow "not include" files if they do not fit the limit?

I'm afraid that people can conclude that memory cache is broken on the system just because it did not fit into sosreport. And I think that in this case it is better to pack nothing rather than tail.

Tom

@pmoravec
Copy link
Contributor

@pmoravec, @elkoniu normally this should be OK, it is much better like this.

But I wonder if we can handle the situation when files are bigger due to some user's customization. I know there is this sizelimit=0 - that would pack everything. The price will be bigger sosreport. Can we somehow "not include" files if they do not fit the limit?

I'm afraid that people can conclude that memory cache is broken on the system just because it did not fit into sosreport. And I think that in this case it is better to pack nothing rather than tail.

Tom

I understand the concern. But sos report does log something like:

2021-02-23 10:42:49,734 INFO: [plugin:rpm] collecting tail of '/var/lib/rpm/Packages' due to size limit
2021-02-23 10:42:50,117 INFO: [plugin:rpm] skipping '/var/lib/rpm/Conflictname' over size limit

into sos_logs/sos.log. That informs a user sufficiently - if they check the logs.

I would change the default sizelimit primarily to prevent potential lack of data collected. How often either file can be >25MB ? How often it could be over, say, 100MB (or anyhow huge that it would become impractical to collect it)? If we speak about very rare circumstances, maybe we can start with the default 25MB and optionally amend per future experience.

@alexey-tikhonov
Copy link

maybe we can start with the default 25MB and optionally amend per future experience.

+1

Default sizes are within (6mb..10mb) range.
The option to configure this size was introduced very recently and I expect this to be customized in extreme rare cases.

(Still "all or nothing" option would be nice too, because cache tail really doesn't make sense)

@thalman
Copy link
Contributor

thalman commented Mar 26, 2021

I'm fine with this PR as it is too.

Thanks for all the work.
Tom

@elkoniu
Copy link
Contributor Author

elkoniu commented Mar 26, 2021

@pmoravec I think that on SSSD side we agree to continue with this PR in current shape. We can continue with review process.

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.

Ack. Minor stylistic nitpick: sos-style for the list would have it like so:

self.add_copy_spec([
    'foo',
    'bar'
])

But I'll take care of that one during a batch merge later today, no worries.

@elkoniu
Copy link
Contributor Author

elkoniu commented Apr 6, 2021

Ack. Minor stylistic nitpick: sos-style for the list would have it like so:

self.add_copy_spec([
    'foo',
    'bar'
])

But I'll take care of that one during a batch merge later today, no worries.

Thank you for the fast merge and taking care of this sos-style mismatch :)

jjansky1 added a commit to jjansky1/sos that referenced this pull request Apr 6, 2021
By default SSSD collects all memory cache files:
* /var/lib/sss/mc/passwd
* /var/lib/sss/mc/group
* /var/lib/sss/mc/initgroups

They all are included in 25MB size limit for sosreport.
This commits add memory cache files one - by - one,
this way 25MB size limit will be aplied per file.

Related: sosreport#2462
Resolves: sosreport#2476

Signed-off-by: Jan Jansky <jjansky@redhat.com>
TurboTurtle pushed a commit that referenced this pull request Apr 7, 2021
By default SSSD collects all memory cache files:
* /var/lib/sss/mc/passwd
* /var/lib/sss/mc/group
* /var/lib/sss/mc/initgroups

They all are included in 25MB size limit for sosreport.
This commits add memory cache files one - by - one,
this way 25MB size limit will be aplied per file.

Related: #2462
Resolves: #2476

Signed-off-by: Jan Jansky <jjansky@redhat.com>
Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>
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.

None yet

5 participants