Skip to content

common/LogEntry: Add log level to str helper for fmt::formatter<LogEntry>#55455

Merged
yuriw merged 1 commit intoceph:mainfrom
sseshasa:wip-fix-cluster-log-level-str
Mar 12, 2024
Merged

common/LogEntry: Add log level to str helper for fmt::formatter<LogEntry>#55455
yuriw merged 1 commit intoceph:mainfrom
sseshasa:wip-fix-cluster-log-level-str

Conversation

@sseshasa
Copy link
Contributor

@sseshasa sseshasa commented Feb 5, 2024

The Ceph cluster logs were missing the string equivalent [INF|WRN|ERR|DBG] representation of the 'prio' field. This was broken since the introduction of commit 2901943 of PR: #47830. This probably caused false positives in teuthology testing and particularly for those tests that check for cluster badness by parsing the cluster logs.

The fix involves adding a static helper function to the LogEntry struct. This function returns the string appropriate representation of the log level similar to the operator<<() for LogEntry.

Signed-off-by: Sridhar Seshasayee sseshasa@redhat.com
Fixes: https://tracker.ceph.com/issues/64314

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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)
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
  • jenkins test rook e2e

@ronen-fr
Copy link
Contributor

ronen-fr commented Feb 5, 2024

Looks OK, apart from one question: aren't there scripts in Teuthology that grep for WRN in the logs?
I am worried that before the change - only cluster warns/errors would trigger a failure, but that would change after this change.

-- update: as explained to me in the conf call - that's not a bug, but the desired result.
LGTM

@sseshasa sseshasa force-pushed the wip-fix-cluster-log-level-str branch from 49b0aa5 to 9d7446c Compare February 5, 2024 16:57
…try>

The Ceph cluster logs were missing the string equivalent [INF|WRN|ERR|DBG]
representation of the 'prio' field. This was broken since the introduction
of commit 2901943 of
PR: ceph#47830. This probably caused false
positives in teuthology testing and particularly for those tests that check
for cluster badness by parsing the cluster logs.

The fix involves adding a static helper function to the LogEntry struct.
This function returns the string appropriate representation of the log
level similar to the operator<<() for LogEntry.

Fixes: https://tracker.ceph.com/issues/64314
Signed-off-by: Sridhar Seshasayee <sseshasa@redhat.com>
@sseshasa sseshasa force-pushed the wip-fix-cluster-log-level-str branch from 9d7446c to 0bd01b9 Compare February 5, 2024 17:00
@sseshasa
Copy link
Contributor Author

sseshasa commented Feb 6, 2024

jenkins test api

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm

@ljflores
Copy link
Member

@sseshasa do you also want this in one of Yuri's batches?

@sseshasa
Copy link
Contributor Author

@sseshasa do you also want this in one of Yuri's batches?

@ljflores Yes, I'd like to include this PR along with the following in the same batch:

  1. common/LogEntry: Reintroduce human readable log level for clog messages #49730 and
  2. qa: Add benign cluster warning from ec-inconsistent-hinfo test to ignorelist #55764

@ljflores
Copy link
Member

ljflores commented Mar 4, 2024

Noted. It'll go in the next main batch along with the other PRs you linked.

@sseshasa
Copy link
Contributor Author

@vshankar
Copy link
Contributor

Sorry to comment on a merged PR, but this change should have been run through fs suite @sseshasa. We probably have test case fallouts due to this. See - https://tracker.ceph.com/issues/65020

@sseshasa
Copy link
Contributor Author

Sorry to comment on a merged PR, but this change should have been run through fs suite @sseshasa. We probably have test case fallouts due to this. See - https://tracker.ceph.com/issues/65020

Yes, that was a miss. As suspected, the rados suite run did result in the creation of multiple new trackers due to this PR. Please see https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrellocomcQFtoIRXE1971-wip-yuri4-testing-2024-03-05-0854 for a list of new trackers raised. I suspect there could be common failures between rados and cephfs suites and those trackers can be used to track the fixes to alleviate the task a bit. For failures unique to fs suite, new trackers can be raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants