mon/AuthMonitor: log when parsing caps fails by rishabh-d-dave · Pull Request #51004 · ceph/ceph

rishabh-d-dave · 2023-04-11T10:07:31Z

The commit originally belongs to PR #41779. These log entries were very useful while debugging PR #41779 for parsing failures. So it's worth to get this patch merged.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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)
- Minor change (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

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

AuthMonitor::valid_caps() checks whether or not caps are valid by checking if parsing caps is successful. The respective parsing methods print a error message to stderr when parsing fails but none print a log message. Let's add a log entry when parsing fails are caps are known to be invalid. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2023-04-11T15:40:11Z

@ljflores Please pick PR for QA run.

rishabh-d-dave · 2023-04-11T15:40:40Z

jenkins test make check arm64

rishabh-d-dave · 2023-04-11T15:41:07Z

https://jenkins.ceph.com/job/ceph-pull-requests-arm64/39844/

rishabh-d-dave · 2023-04-11T15:42:02Z

jenkins test windows

vshankar · 2023-04-12T04:30:05Z

src/mon/AuthMonitor.cc

  if (type == "mon") {
    MonCap moncap;
    if (!moncap.parse(caps, out)) {
+      dout(20) << "Parsing MON caps failed. MON cap: " << caps << dendl;


out would generally have the failure error string returned back to the CLI - were those not verbose enough for debugging. Esp. since the message logged here are pretty generic.

It's better when logs too have an entry about failed parsing.

IMO, it'll be a bit more helpful if the log message has details about what failed. WDYT?

Okay. The needs the iterator that parsing uses. And that is internal to classes that parse.

IMO, unless we additionally log the reason for parsing failure, this change does not add anything useful to debug.

It's not useless because it's impossible to tell that parsing failed just by looking at logs. Such a message helps one to jump to a relevant part of the log and start investigating for the cause of failure from there.

Otherwise, logs is not very helpful as reader can't tell which messages were logged before the parsing failure.

I'll leave it upto @rzarzynski to take a call for this.

It's not useless because it's impossible to tell that parsing failed just by looking at logs

I agree that a developer not-always has access to CLI's output; having the info in one place (in the log) helps, IMHO.

ljflores · 2023-05-08T21:29:00Z

Rados suite results: http://pulpito.front.sepia.ceph.com/?branch=wip-yuri7-testing-2023-04-19-1343

Failures, unrelated:
1. https://tracker.ceph.com/issues/57755
2. https://tracker.ceph.com/issues/58946
3. https://tracker.ceph.com/issues/49888
4. https://tracker.ceph.com/issues/59380
5. https://tracker.ceph.com/issues/57754
6. https://tracker.ceph.com/issues/55347
7. https://tracker.ceph.com/issues/49287

Details:
1. task/test_orch_cli: test_cephfs_mirror times out - Ceph - Orchestrator
2. cephadm: KeyError: 'osdspec_affinity' - Ceph - Mgr - Dashboard
3. rados/singleton: radosbench.py: teuthology.exceptions.MaxWhileTries: reached maximum tries (3650) after waiting for 21900 seconds - Ceph - RADOS
4. rados/singleton-nomsgr: test failing from "Health check failed: 1 full osd(s) (OSD_FULL)" and "Health check failed: 1 filesystem is offline (MDS_ALL_DOWN)" - Ceph - RGW
5. test_envlibrados_for_rocksdb.sh: update-alternatives: error: alternative path /usr/bin/gcc-11 doesn't exist - Ceph - RADOS
6. SELinux Denials during cephadm/workunits/test_cephadm - Ceph - Orchestrator
7. podman: setting cgroup config for procHooks process caused: Unit libpod-$hash.scope not found - Ceph - Orchestrator

kotreshhr · 2023-05-24T17:59:41Z

jenkins test windows

rzarzynski · 2023-05-24T19:49:46Z

src/mon/AuthMonitor.cc

  if (type == "mon") {
    MonCap moncap;
    if (!moncap.parse(caps, out)) {
+      dout(20) << "Parsing MON caps failed. MON cap: " << caps << dendl;


It's not useless because it's impossible to tell that parsing failed just by looking at logs

I agree that a developer not-always has access to CLI's output; having the info in one place (in the log) helps, IMHO.

rishabh-d-dave requested a review from a team as a code owner April 11, 2023 10:07

rishabh-d-dave added the mon label Apr 11, 2023

rishabh-d-dave requested a review from vshankar April 11, 2023 10:07

github-actions bot added the core label Apr 11, 2023

rishabh-d-dave changed the title ~~src/mon: add log & error messages to AuthMonitor::valid_caps()~~ mon: add log & error messages to AuthMonitor::valid_caps() Apr 11, 2023

rishabh-d-dave force-pushed the authmon-invalid-caps branch from f8173de to b5807d0 Compare April 11, 2023 10:14

rishabh-d-dave changed the title ~~mon: add log & error messages to AuthMonitor::valid_caps()~~ mon/AuthMonitor: log when parsing caps fail Apr 11, 2023

rishabh-d-dave changed the title ~~mon/AuthMonitor: log when parsing caps fail~~ mon/AuthMonitor: log when parsing caps fails Apr 11, 2023

rishabh-d-dave force-pushed the authmon-invalid-caps branch from b5807d0 to 3fcbb0b Compare April 11, 2023 10:26

batrick approved these changes Apr 11, 2023

View reviewed changes

rishabh-d-dave added the needs-qa label Apr 11, 2023

vshankar reviewed Apr 12, 2023

View reviewed changes

yuriw added the wip-yuri7-testing label Apr 18, 2023

rishabh-d-dave mentioned this pull request Apr 20, 2023

mon,osd,mds: allow fs authorize to update caps #41779

Merged

4 tasks

rzarzynski approved these changes May 24, 2023

View reviewed changes

yuriw merged commit 140a8f3 into ceph:main May 25, 2023

Conversation

rishabh-d-dave commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

rishabh-d-dave commented Apr 11, 2023

Uh oh!

rishabh-d-dave commented Apr 11, 2023

Uh oh!

rishabh-d-dave commented Apr 11, 2023

Uh oh!

rishabh-d-dave commented Apr 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljflores commented May 8, 2023

Uh oh!

kotreshhr commented May 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rishabh-d-dave commented Apr 11, 2023 •

edited

Loading