Skip to content

crimson/osd/pg_recovery: rework start_recovery_ops#62847

Merged
Matan-B merged 4 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-do_recovery
May 6, 2025
Merged

crimson/osd/pg_recovery: rework start_recovery_ops#62847
Matan-B merged 4 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-do_recovery

Conversation

@Matan-B
Copy link
Contributor

@Matan-B Matan-B commented Apr 16, 2025 •
edited
Loading

We had few confusions around the return value usage from start_recovery_ops.
Rewriting start_recovery_ops and changing the return type of do_recovery should help avoid future mistakes.

new logs should help with https://tracker.ceph.com/issues/70501#note-3

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

@Matan-B Matan-B added this to Crimson Apr 16, 2025
@Matan-B Matan-B moved this to In Progress in Crimson Apr 16, 2025
@Matan-B Matan-B force-pushed the wip-matanb-crimson-do_recovery branch from 47d2863 to fd93e9d Compare April 16, 2025 12:30
@Matan-B Matan-B changed the title crimson/osd/pg_recovery: rewrite start_recovery_ops crimson/osd/pg_recovery: rework start_recovery_ops Apr 16, 2025
@github-actions
Copy link

github-actions bot commented Apr 22, 2025

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@Matan-B Matan-B force-pushed the wip-matanb-crimson-do_recovery branch from fd93e9d to f167d60 Compare April 24, 2025 08:18
@Matan-B Matan-B marked this pull request as ready for review April 24, 2025 08:21
@Matan-B Matan-B requested a review from a team as a code owner April 24, 2025 08:21
@Matan-B Matan-B moved this from In Progress to Awaits review in Crimson Apr 24, 2025
@github-actions
Copy link

github-actions bot commented May 4, 2025

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Matan-B added 4 commits May 4, 2025 14:32
@Matan-B
crimson/osd/pg_recovery: rewrite start_recovery_ops
ce4e9aa 
We had few confusions around the return value from start_recovery_ops.
This commit is a groundwork for the return type change.

* Move to coroutines
* Update logging macro

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd: Convert do_recovery() from bool to stop_iteration
798a14a 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd/pg: Let PGListener use start_peering_event_operation
c94e604 
PG::start_peering_event_operation is a template function while
PGRecovery::pg is of PGRecoveryListener* type. We can't expose a template
function through the PGRecoveryListener interface since it must be
also virtual.
Instead, introduce start_peering_event_operation_listener which will act
as a wrapper to PG::start_peering_event_operation for PGRecovery to use
freely.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd: Logging fixes
314d2e7 
* Fix "failed to log message"
* PGRecovery move to new logging macro
* PGRecovery to print pg prefix as it's impossible to debug specific pg
  recovery ops without it.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B Matan-B force-pushed the wip-matanb-crimson-do_recovery branch from 2c27212 to 314d2e7 Compare May 4, 2025 14:33
amathuria
amathuria approved these changes May 5, 2025
View reviewed changes
Copy link
Contributor

@amathuria amathuria left a comment

Choose a reason for hiding this comment

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

LGTM!

@Matan-B Matan-B moved this from Awaits review to Needs QA in Crimson May 5, 2025
@Matan-B
Copy link
Contributor Author

Matan-B commented May 6, 2025

https://pulpito.ceph.com/matan-2025-05-05_09:22:41-crimson-rados-wip-matanb-crimson-only-do_recovery-distro-crimson-smithi/

8271011, 8270926, 8270941 - https://tracker.ceph.com/issues/70337
8271051 - https://tracker.ceph.com/issues/71204
8270931, 8270983 - slow ops 
8270935, 8270936 infra

@Matan-B Matan-B merged commit c308de9 into ceph:main May 6, 2025
12 checks passed
@Matan-B Matan-B moved this from Needs QA to Merged in Crimson May 6, 2025
@Matan-B Matan-B mentioned this pull request May 6, 2025
Merged
14 tasks
@Matan-B Matan-B moved this from Merged Pre Tentacle Freeze to Merged (Main) in Crimson May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amathuria amathuria amathuria approved these changes

@xxhdx1985126 xxhdx1985126 Awaiting requested review from xxhdx1985126

@athanatos athanatos Awaiting requested review from athanatos

@mohit84 mohit84 Awaiting requested review from mohit84

Assignees

No one assigned

Labels

crimson backport tentacle crimson wip-matan-testing

Projects

Crimson
Status: Merged (Main)

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Matan-B @amathuria