Skip to content

crimson/common/errorator: rework aborts error handlers#62699

Merged
Matan-B merged 15 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-ignore-abort-v2
Apr 23, 2025
Merged

crimson/common/errorator: rework aborts error handlers#62699
Matan-B merged 15 commits intoceph:mainfrom
Matan-B:wip-matanb-crimson-ignore-abort-v2

Conversation

@Matan-B
Copy link
Contributor

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

This came up during: https://tracker.ceph.com/issues/69406#note-25
Where an "assert_all" was called but didn't cause an abort.

This PR is a rework of errorator aborting paths with fixes and some cleanups.

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 requested a review from a team as a code owner April 7, 2025 09:53
@Matan-B Matan-B mentioned this pull request Apr 7, 2025
Merged
14 tasks
@Matan-B Matan-B requested a review from rzarzynski April 7, 2025 09:54
Matan-B
Matan-B commented Apr 7, 2025
View reviewed changes
@Matan-B Matan-B added this to Crimson Apr 7, 2025
@Matan-B Matan-B moved this to In Progress in Crimson Apr 7, 2025
@Matan-B Matan-B changed the title [WIP] test/crimson/test_errorator: ignore assert_all crimson/common/errorator: rework aborts error handlers Apr 8, 2025
@Matan-B Matan-B force-pushed the wip-matanb-crimson-ignore-abort-v2 branch 2 times, most recently from 380740b to abea813 Compare April 8, 2025 13:30
@Matan-B Matan-B moved this from In Progress to Awaits review in Crimson Apr 8, 2025
liu-chunmei
liu-chunmei approved these changes Apr 9, 2025
View reviewed changes
Copy link
Contributor

@liu-chunmei liu-chunmei left a comment

Choose a reason for hiding this comment

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

LGTM.

@liu-chunmei
Copy link
Contributor

liu-chunmei commented Apr 9, 2025

jenkins test make check

@liu-chunmei
Copy link
Contributor

liu-chunmei commented Apr 9, 2025

jenkins test make check arm64

@liu-chunmei
Copy link
Contributor

liu-chunmei commented Apr 9, 2025

jenkins test make check

@Matan-B Matan-B force-pushed the wip-matanb-crimson-ignore-abort-v2 branch 2 times, most recently from 6c98eda to a87f5a6 Compare April 10, 2025 15:12
@Matan-B Matan-B requested a review from liu-chunmei April 10, 2025 15:43
@Matan-B Matan-B force-pushed the wip-matanb-crimson-ignore-abort-v2 branch from a87f5a6 to d09f560 Compare April 10, 2025 15:51
@Matan-B Matan-B moved this from Awaits review to Needs QA in Crimson Apr 14, 2025
@Matan-B Matan-B mentioned this pull request Apr 17, 2025
Closed
14 tasks
xxhdx1985126 and others added 15 commits April 22, 2025 15:11
@xxhdx1985126 @Matan-B
crimson/common/errorator: fix skipped aborts
91c1a51 
We should also invoke the errfunc (which aborts) when the
return type is no_touch_error_marker.
Added comments explaining:
* why it's forbidden to return void
* why std::is_same_v<return_t, no_touch_error_marker> is checked

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: move exception_comment
e3ad98c 
move the comment to where __cxa_exception_type is used
to keep handle() comments shorter.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: introduce take_exception_from_future
3bbbf0c 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: add TODO
be9d904 
There are few TODOs around errorator code which might be worth looking
into: https://tracker.ceph.com/issues/70875

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: Always check exception type
bb717e2 
We shouldn't bypass this check in the is_same_v<return_t, no_touch_error_marker>
case.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: print abort message when possible
78f1306 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
test/crimson/test_errorator: ignore assert_all
3f41030 
This came up during: https://tracker.ceph.com/issues/69406#note-25
Where an "assert_all" was called but didn't cause an abort.
Added "ignore_assert_all" to showcase this scenario along with any
other case where we are expected to abort.
The tests could be used to verify errorator's aborting behavior.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/ertr: assert_all informs about error being handled that way
2342382 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
@Matan-B
crimson/common/errorator: cleanup ErrorT::handler call
5900fb6 
call error_t::handle without decalring handler and invoking it later on

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: Cleanup assert_all pre_assert
f5d5f88 
Not used

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: allow assert_all to accept c_str()
27b232e 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/osd: Verbose assert_all aborts
7207975 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson/common/errorator: assert_failure to print error
d2a748e 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimosn/common/errorator: cleanup assert_failure pre_assert
9b5d418 
Any usage should be replaced with a message that supports priniting the
error.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
crimson: remove any assert_failure pre assert usages
a697a18 
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@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-ignore-abort-v2 branch from dfd1d08 to a697a18 Compare April 22, 2025 15:26
@Matan-B
Copy link
Contributor Author

Matan-B commented Apr 23, 2025

jenkins test make check arm64

@Matan-B
Copy link
Contributor Author

Matan-B commented Apr 23, 2025

Merging based on https://pulpito.ceph.com/matan-2025-04-23_07:18:01-crimson-rados-wip-matanb-crimson-ignore-abort-v2-distro-crimson-smithi/

@Matan-B
Copy link
Contributor Author

Matan-B commented Apr 23, 2025

jenkins test make check arm64

@Matan-B
Copy link
Contributor Author

Matan-B commented Apr 23, 2025

Unreleated:
30 - run-rbd-unit-tests-0.sh (Failed

@Matan-B Matan-B merged commit 7e91398 into ceph:main Apr 23, 2025
12 checks passed
@Matan-B Matan-B moved this from Needs QA to Merged in Crimson Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyx1231st cyx1231st cyx1231st approved these changes

@liu-chunmei liu-chunmei Awaiting requested review from liu-chunmei

@rzarzynski rzarzynski Awaiting requested review from rzarzynski

@xxhdx1985126 xxhdx1985126 Awaiting requested review from xxhdx1985126

Assignees

No one assigned

Labels

build/ops crimson ready-to-merge TESTED tests wip-matan-testing

Projects

Crimson
Status: Merged (Pre Tentacle Freeze)

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Matan-B @liu-chunmei @athanatos @rzarzynski @xxhdx1985126 @cyx1231st