Skip to content

OSD: PG stat is not synchronized between osds after deep-scrub#57582

Merged
sajibreadd merged 1 commit intoceph:mainfrom
sajibreadd:wip-66059
Oct 7, 2024
Merged

OSD: PG stat is not synchronized between osds after deep-scrub#57582
sajibreadd merged 1 commit intoceph:mainfrom
sajibreadd:wip-66059

Conversation

@sajibreadd
Copy link
Member

@sajibreadd sajibreadd commented May 20, 2024

pg stat are not synced between osds after deep-scrub. So if primary osd is killed, next primary osd has wrong stats. Reason behind it is PeeringState::proc_primary_info does not process or update any pg stats of secondary osds after deep-scrub.

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

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

@sajibreadd sajibreadd requested a review from a team as a code owner May 20, 2024 23:32
@github-actions github-actions bot added the core label May 20, 2024
@athanatos
Copy link
Contributor

@ronen-fr Can you take a look?

@athanatos athanatos requested a review from ronen-fr May 21, 2024 00:24
@ronen-fr
Copy link
Contributor

@ronen-fr Can you take a look?

@athanatos - I'll be back near a laptop only on May 30. But I'm glad we now have an explanation for this bug.

@rzarzynski
Copy link
Contributor

@ronen-fr: just a friendly mention to keep this on your radar.

dirty_big_info = true;
}
info.stats.stats.sum = oinfo.stats.stats.sum;
pl->publish_stats_to_osd();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand how publish_stats_to_osd(), which starts with

void PG::publish_stats_to_osd()
{
if (!is_primary())
return;

has any effect when called after line 3031 above

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, we don't need to publish it immediately(as publishing works only for primary osd). So whenever primary osd is down and secondary osd is taking control over at that time it will automatically publish the stats from the recovery_state. I did some adjustment in the code, such that the old codes are intact.

Copy link
Contributor

@ronen-fr ronen-fr left a comment

Choose a reason for hiding this comment

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

pls see my question in the comment

@ronen-fr ronen-fr self-requested a review June 4, 2024 15:42
Copy link
Contributor

@ronen-fr ronen-fr left a comment

Choose a reason for hiding this comment

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

LGTM

@sajibreadd
Copy link
Member Author

jenkins test make check

@sajibreadd
Copy link
Member Author

jenkins test make check arm64

Copy link
Member

@ljflores ljflores left a comment

Choose a reason for hiding this comment

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

@sajibreadd, the commit should have src/osd: in front of the title.

@ljflores
Copy link
Member

ljflores commented Jul 23, 2024

Hi @sajibreadd, this PR was tested in teuthology, and I found a few regressions. PTAL when you can.

/a/yuriw-2024-07-17_13:32:02-rados-wip-yuri12-testing-2024-07-16-1122-distro-default-smithi/7806002

2024-07-18T08:43:37.166 INFO:tasks.workunit.client.0.smithi046.stderr://home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:345: kill_daemons:  shopt -q -o xtrace
2024-07-18T08:43:37.166 INFO:tasks.workunit.client.0.smithi046.stderr://home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:345: kill_daemons:  echo true
2024-07-18T08:43:37.166 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:345: kill_daemons:  local trace=true
2024-07-18T08:43:37.166 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:346: kill_daemons:  true
2024-07-18T08:43:37.166 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:346: kill_daemons:  shopt -u -o xtrace
2024-07-18T08:43:42.480 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/ceph-helpers.sh:362: kill_daemons:  return 0
2024-07-18T08:43:42.480 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/osd-backfill/osd-backfill-space.sh:614: TEST_backfill_grow:  grep -q 'num_bytes mismatch' td/osd-backfill-space/osd.0.log td/osd-backfill-space/osd.1.log td/osd-backfill-space/osd.2.log
2024-07-18T08:43:42.485 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/osd-backfill/osd-backfill-space.sh:614: TEST_backfill_grow:  return 1
2024-07-18T08:43:42.485 INFO:tasks.workunit.client.0.smithi046.stderr:/home/ubuntu/cephtest/clone.client.0/qa/standalone/osd-backfill/osd-backfill-space.sh:38: run:  return 1

/a/yuriw-2024-07-17_13:32:02-rados-wip-yuri12-testing-2024-07-16-1122-distro-default-smithi/7805995

2024-07-18T08:12:37.463 INFO:tasks.ceph.osd.10.smithi090.stderr:2024-07-18T08:12:37.461+0000 7f1e40d07640 -1 log_channel(cluster) log [ERR] : 3.18 scrub : stat mismatch, got 3/2 objects, 1/1 clones, 3/2 dirty, 0/0 omap, 0/0 pinned, 0/0 hit_set_archive, 1/1 whiteouts, 4571570/4571570 bytes, 0/0 manifest objects, 0/0 hit_set_archive bytes.
2024-07-18T08:12:37.463 INFO:tasks.ceph.osd.10.smithi090.stderr:2024-07-18T08:12:37.461+0000 7f1e40d07640 -1 log_channel(cluster) log [ERR] : 3.18 scrub 1 errors

See this link for the full test run:
https://pulpito.ceph.com/yuriw-2024-07-17_13:32:02-rados-wip-yuri12-testing-2024-07-16-1122-distro-default-smithi/

See this link for example successful test runs (w/o your PR included):
https://pulpito.ceph.com/?branch=wip-yuri12-testing-2024-07-19-2220

Testing ref: https://tracker.ceph.com/issues/66706

@ljflores ljflores requested a review from ronen-fr July 23, 2024 16:25
@ljflores
Copy link
Member

@yuriw please do not merge this until the author can take another look.

@ljflores ljflores added the DNM label Jul 23, 2024
@sajibreadd
Copy link
Member Author

Probably could be the reason, as I info.stats.stats.sum = oinfo.stats.stats.sum only instead of full stats.

…primary osd is killed, next primary osd has wrong stats. Reason behind it is PeeringState::proc_primary_info does not process or update any pg stats.

Fixes: https://tracker.ceph.com/issues/66059
Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>
@sajibreadd
Copy link
Member Author

jenkins test api

@sajibreadd
Copy link
Member Author

@yuriw @ljflores Do I need to do anything from my side now? I updated the changes.

@ifed01
Copy link
Contributor

ifed01 commented Aug 12, 2024

@ljflores - can we have another QA round, please?

@ifed01 ifed01 removed the TESTED label Aug 12, 2024
@ljflores
Copy link
Member

@sajibreadd yeah this will go into a future batch.

@ronen-fr
Copy link
Contributor

@sajibreadd , @ljflores : tests continue to fail in the same pattern you have mentioned above.
See for example https://pulpito.ceph.com/rfriedma-2024-10-12_07:06:36-rados:thrash-wip-rf-store2-steps-distro-default-smithi/7944300/

Seems we made a mistake in approving this PR.
I'll try to analyze. @sajibreadd - I'd appreciate if you too take a look

@sajibreadd
Copy link
Member Author

sajibreadd commented Oct 14, 2024

Seems we made a mistake in approving this PR. I'll try to analyze. @sajibreadd - I'd appreciate if you too take a look

Sure

@sajibreadd
Copy link
Member Author

@ronen-fr Should we call publish_stats_to_osd after updating info.stats = oinfo.stats?

@sajibreadd
Copy link
Member Author

@ronen-fr Should we call publish_stats_to_osd after updating info.stats = oinfo.stats?

Ignore this comment, publish_stats_to_osd is only for primary osd.

@sajibreadd sajibreadd deleted the wip-66059 branch March 16, 2026 08:23
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.

8 participants