debian: recursively adjust permissions of /var/lib/ceph/crash by Aequitosh · Pull Request #55917 · ceph/ceph

Aequitosh · 2024-03-04T10:32:47Z

A rather recent PR made ceph-crash run as ceph user instead of root¹. However, because /var/lib/ceph/crash/posted belongs to root, ceph-crash cannot actually post any crash logs now.

This commit fixes this by recursively updating the permissions of /var/lib/ceph/crash, which ensures that all files and directories used by ceph-crash.service are actually owned by the user configured for Ceph.

The previously existing loop has also been replaced by an invocation of find | xargs.

Fixes: https://tracker.ceph.com/issues/64548

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Further Notes

I'm not 100% sure if this is the best way to fix this issue, so please let me know if you prefer a different solution. This patch is originally from our mailing list² - we've decided that it's what works best for us, due to find | xargs being a bit more fine-grained than, let's say, something like for FILE in /var/lib/ceph/**/*; do ...; done.

Also, should this get merged (in whatever form), I'll also gladly provide backports for Reef and Quincy.

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*' to ceph:ceph (in our case), but misses out on the contents of '/var/lib/ceph/crash'. This patch therefore also recursively updates the permissions of '/var/lib/ceph/crash'. The change was also proposed upstream [0]. [0]: ceph/ceph#55917 Signed-off-by: Max Carrara <m.carrara@proxmox.com> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*' to ceph:ceph (in our case), but misses out on the contents of '/var/lib/ceph/crash'. This patch therefore also recursively updates the permissions of '/var/lib/ceph/crash'. The change was also proposed upstream [0]. [0]: ceph/ceph#55917 Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*' to ceph:ceph (in our case), but misses out on the contents of '/var/lib/ceph/crash'. This patch therefore also recursively updates the permissions of '/var/lib/ceph/crash'. The change was also proposed upstream [0]. [0]: ceph/ceph#55917 Signed-off-by: Max Carrara <m.carrara@proxmox.com> (cherry picked from commit d52158f) Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

frittentheke · 2024-04-02T14:20:20Z

@tchaikov could this please be backported to at least Reef to be fixed with the next point release?

Aequitosh · 2024-04-03T08:09:38Z

@tchaikov could this please be backported to at least Reef to be fixed with the next point release?

I can also provide backports for Reef and Quincy if you don't mind - either once this gets merged or whenever you wish.

tchaikov · 2024-04-03T08:26:31Z

@tchaikov could this please be backported to at least Reef to be fixed with the next point release?

@frittentheke hi Christian, i think so. since https://tracker.ceph.com/issues/64548 includes Reef in its "Backport" field.

and sorry for the latency, gentlemen. i will try to squeeze more time for a proper review asap.

@tchaikov could this please be backported to at least Reef to be fixed with the next point release?

I can also provide backports for Reef and Quincy if you don't mind - either once this gets merged or whenever you wish.

better off waiting until this PR gets merged, so that we can reference it with the correct sha1 in the backport PR.

frittentheke · 2024-04-03T08:49:57Z

@frittentheke hi Christian, i think so. since https://tracker.ceph.com/issues/64548 includes Reef in its "Backport" field.
and sorry for the latency, gentlemen. i will try to squeeze more time for a proper review asap.

Thanks @tchaikov for the quick update!

While we certainly adjusted this on our clusters already, I am just really afraid this causes less crash reports to come in from other installs out there. Less crash reports, means potentially more undiscovered bugs. Selfish me 🙈

frittentheke · 2024-04-14T18:31:22Z

debian/ceph-base.postinst

 	[ -x /sbin/start ] && start ceph-all || :

-        # adjust file and directory permissions
-	for DIR in /var/lib/ceph/* ; do


Why not just add the missing /var/lib/ceph/crash folder here? The new code treats this folder individually.

IIUC, what we need to do is to fix the permissions of the directories listed by debian/ceph-base.dirs, right? if that's the case, i concur with @frittentheke , that probably we can just add var/lib/ceph/crash here, like

for DIR in /var/lib/ceph/* /var/lib/ceph/crash/posted; do if ! dpkg-statoverride --list $DIR >/dev/null then chown $SERVER_USER:$SERVER_GROUP $DIR fi done

please note, i am listing /var/lib/ceph/crash/posted here, as /var/lib/ceph/crash has been covered by /var/lib/ceph/*.

Why not just add the missing /var/lib/ceph/crash folder here? The new code treats this folder individually.

That's actually a very fair point. In some iteration of this patch I ended up using the find+xargs approach, as that seemed just more logical at the time. (IIRC it was a globbing thing first that then became redundant anyway; the find+xargs combo remained anyway.)

for DIR in /var/lib/ceph/* /var/lib/ceph/crash/posted; do if ! dpkg-statoverride --list $DIR >/dev/null then chown $SERVER_USER:$SERVER_GROUP $DIR fi done

please note, i am listing /var/lib/ceph/crash/posted here, as /var/lib/ceph/crash has been covered by /var/lib/ceph/*.

While this does obviously chown the crash dir, it doesn't update the already existing entries in crash/posted that might've been created as root prior to #48713. That's why I chose to treat /var/lib/ceph/crash/ separately.

frittentheke · 2024-04-14T18:34:12Z

debian/ceph-base.postinst

+
+	# adjust file and directory permissions
+	find /var/lib/ceph -mindepth 1 -maxdepth 1 -print0 \
+	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"


Why restrict find to maxdepth 1 here and not let it recurse to cover all the files?
Or maybe do a chown -r to ensure all dirs and files below are owned by cheph:ceph?
This avoids the need to deal with /var/lib/ceph/crash in yet another find + xargs.

I wasn't 100% sure what's appropriate here - I simply wanted to preserve the original behavior, as I wasn't certain whether something deep inside /var/lib/ceph would perhaps require different ownership (at some point).

frittentheke · 2024-04-14T18:36:44Z

@tchaikov sorry to nag about this again, but I just noticed 18.2.3 QA phase already started (https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/KRC33HNRBCZC5Z2GUDN5DQO43I2RJBJJ/).

This PR is "just" about fixing some file permissions and we are likely discussion implementations styles.
But FWIW, I left a few lines of review myself.

tchaikov

i'd suggest just add /var/lib/ceph/crash to /var/lib/ceph/*, without adding the find commands. simpler this way.

tchaikov · 2024-04-15T02:08:45Z

debian/ceph-base.postinst

 	[ -x /sbin/start ] && start ceph-all || :

-        # adjust file and directory permissions
-	for DIR in /var/lib/ceph/* ; do


IIUC, what we need to do is to fix the permissions of the directories listed by debian/ceph-base.dirs, right? if that's the case, i concur with @frittentheke , that probably we can just add var/lib/ceph/crash here, like

for DIR in /var/lib/ceph/* /var/lib/ceph/crash/posted; do if ! dpkg-statoverride --list $DIR >/dev/null then chown $SERVER_USER:$SERVER_GROUP $DIR fi done

please note, i am listing /var/lib/ceph/crash/posted here, as /var/lib/ceph/crash has been covered by /var/lib/ceph/*.

tchaikov · 2024-04-15T02:12:03Z

@tchaikov sorry to nag about this again, but I just noticed 18.2.3 QA phase already started (https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/KRC33HNRBCZC5Z2GUDN5DQO43I2RJBJJ/).

This PR is "just" about fixing some file permissions and we are likely discussion implementations styles. But FWIW, I left a few lines of review myself.

@frittentheke my apologies. indeed.

frittentheke · 2024-04-16T08:42:03Z

@frittentheke my apologies. indeed.

No need @tchaikov ... I am still just a selfish dude looking for other installations' crash reports to make my Ceph installations more stable ;-)

Speaking of Reef ... any remote chance this change can still make it into 18.2.3 ?

ljflores · 2024-04-18T20:48:27Z

I should actually add the milestone to the Reef backport when it's ready. Instead, I've added the "needs-reef-backport" label.

Aequitosh · 2024-04-22T06:35:56Z

@frittentheke @tchaikov Thanks for the reviews, much appreciated! I left some more comments inline.

What's now obvious to me is that the original loop can just stay - the find+xargs replacement isn't necessary.

However, I'd personally suggest to do something like chown -R /var/lib/ceph/crash/*, as some entries in /var/lib/ceph/crash/posted might still belong to root e.g. - entries that were posted prior to #48713.

I'll update the PR accordingly, if you don't object.

frittentheke · 2024-04-22T07:15:42Z

However, I'd personally suggest to do something like chown -R /var/lib/ceph/crash/*, as some entries in /var/lib/ceph/crash/posted might still belong to root e.g. - entries that were posted prior to #48713.

I'll update the PR accordingly, if you don't object.

FWIW, I like the idea to "ensure" (and potentially fix) ownership also for existing files.

frittentheke · 2024-04-26T13:01:03Z

@Aequitosh @tchaikov could I nag you once more to finalize the discussion on how to implement the chown ?

tchaikov · 2024-04-26T13:17:27Z

@frittentheke @tchaikov Thanks for the reviews, much appreciated! I left some more comments inline.

What's now obvious to me is that the original loop can just stay - the find+xargs replacement isn't necessary.

However, I'd personally suggest to do something like chown -R /var/lib/ceph/crash/*, as some entries in /var/lib/ceph/crash/posted might still belong to root e.g. - entries that were posted prior to #48713.

I'll update the PR accordingly, if you don't object.

@Aequitosh hi Max, please go ahead. i didn't take the "existing files" use case into consideration. but if that case exists, then, yeah, i concur with you.

A rather recent PR made ceph-crash run as "ceph" user instead of root [0]. However, because /var/lib/ceph/crash/posted belongs to root, ceph-crash cannot actually post any crash logs now. This commit fixes this by recursively updating the permissions of '/var/lib/ceph/crash', which ensures that all files and directories used by 'ceph-crash.service' are actually owned by the user configured for Ceph. This also accounts for existing installations. Additionally, quote interpolated variables and use curly braces [1]. [0]: ceph#48713 [1]: https://www.shellcheck.net/wiki/SC2086 Fixes: https://tracker.ceph.com/issues/64548 Signed-off-by: Max Carrara <m.carrara@proxmox.com>

Aequitosh · 2024-04-30T15:51:04Z

Updated the PR. See the commit message for details.

Basically did as I described, but also quoted variables where they're interpolated. Also added curly braces there.

I highly doubt that globbing & word splitting are ever going to be issues, but still wanted to include it for good measure. (Though, should that not be desired, I can quickly update the PR again.)

tchaikov

lgtm

frittentheke · 2024-06-24T19:09:30Z

Is this fix on track to be merged?

ljflores · 2024-07-03T21:24:59Z

@frittentheke I'm not sure what happened with the testing. It will be included in the next main batch.

pereman2

https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues66151 rados approved

debian: recursively adjust permissions of /var/lib/ceph/crash Reviewed-by: Pere Diaz Bou <pdiazbou@redhat.com> Reviewed-by: Kefu Chai <tchaikov@gmail.com>

github-actions bot added the build/ops label Mar 4, 2024

tchaikov self-requested a review March 21, 2024 04:06

frittentheke reviewed Apr 14, 2024

View reviewed changes

tchaikov reviewed Apr 15, 2024

View reviewed changes

ljflores added the core label Apr 18, 2024

ljflores modified the milestones: 18.2.14, v18.2.4 Apr 18, 2024

ljflores added the needs-review label Apr 18, 2024

yuriw added wip-yuri7-testing and removed wip-yuri7-testing labels Apr 18, 2024

yuriw modified the milestone: v18.2.4 Apr 18, 2024

ljflores removed this from the v18.2.4 milestone Apr 18, 2024

ljflores added the needs-reef-backport label Apr 18, 2024

Aequitosh force-pushed the fix-ceph-crash-permissions branch from 76df6f5 to 97a1ec4 Compare April 30, 2024 15:45

tchaikov approved these changes May 9, 2024

View reviewed changes

tchaikov added needs-qa and removed needs-review labels May 9, 2024

ljflores added the wip-yuri7-testing label May 20, 2024

yuriw removed the wip-yuri7-testing label Jun 2, 2024

pereman2 approved these changes Jul 8, 2024

View reviewed changes

tchaikov merged commit 85d4bc8 into ceph:main Jul 8, 2024

This was referenced Jul 8, 2024

reef: debian: recursively adjust permissions of /var/lib/ceph/crash #58458

Merged

quincy: debian: recursively adjust permissions of /var/lib/ceph/crash #58459

Closed

Conversation

Aequitosh commented Mar 4, 2024

Checklist

Further Notes

Footnotes

Uh oh!

frittentheke commented Apr 2, 2024

Uh oh!

Aequitosh commented Apr 3, 2024

Uh oh!

tchaikov commented Apr 3, 2024

Uh oh!

frittentheke commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frittentheke Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaikov Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

Aequitosh Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

frittentheke Apr 14, 2024

Choose a reason for hiding this comment

Uh oh!

Aequitosh Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

frittentheke commented Apr 14, 2024

Uh oh!

tchaikov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaikov Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

tchaikov commented Apr 15, 2024

Uh oh!

frittentheke commented Apr 16, 2024

Uh oh!

ljflores commented Apr 18, 2024

Uh oh!

Aequitosh commented Apr 22, 2024

Uh oh!

frittentheke commented Apr 22, 2024

Uh oh!

frittentheke commented Apr 26, 2024

Uh oh!

tchaikov commented Apr 26, 2024

Uh oh!

Aequitosh commented Apr 30, 2024

Uh oh!

tchaikov left a comment

Choose a reason for hiding this comment

Uh oh!

frittentheke commented Jun 24, 2024

Uh oh!

ljflores commented Jul 3, 2024

Uh oh!

pereman2 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frittentheke commented Apr 3, 2024 •

edited

Loading

frittentheke Apr 14, 2024 •

edited

Loading

tchaikov left a comment •

edited

Loading