crimson/osd: bring support for client blocklisting by rzarzynski · Pull Request #47637 · ceph/ceph

rzarzynski · 2022-08-16T14:37:40Z

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)
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

athanatos · 2022-08-16T23:50:46Z

src/crimson/osd/pg.cc

+void PG::check_blocklisted_watchers()
+{
+  logger().debug("{}", __func__);
+  shard_services.for_each_obc([this] (ObjectContextRef obc) {


This is going to iterate over object contexts for other PGs as well. The PG needs a set of its own objects with live watches -- there shouldn't in general be all that many.

How about skipping a watch on PG mismatch?

Iterating over all live object contexts on the shard to find the very small subset with a live watcher for this pg still strikes me as inefficient and complicated.

I see. Perhaps we could flip the loops.

Nope, we don't have a separate registry of watchers. The main difference with the classical is it uses a per-PG (not per-shard) registry of object contexts:

void PrimaryLogPG::check_blocklisted_watchers() { dout(20) << "PrimaryLogPG::check_blocklisted_watchers for pg " << get_pgid() << dendl; pair<hobject_t, ObjectContextRef> i; while (object_contexts.get_next(i.first, &i)) check_blocklisted_obc_watchers(i.second); }

Duplicating the information looks even worse. HOWEVER:

using lru_set_t = boost::intrusive::set< base_t, lru_set_option_t, boost::intrusive::key_of_value<VToKWrapped> >; lru_set_t lru_set;

Maybe, by changing the key derivation function, we could have OBCs sorted / prefixed by PG-derived part.

Do we really need the per-shard aggregation of OBCs?

Probably not, tbh. We can just let each PG have its own.

src/common/intrusive_lru.h

rzarzynski · 2022-10-07T12:19:10Z

jenkins retest this please

github-actions · 2022-10-07T17:56:21Z

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

rzarzynski · 2022-10-07T18:26:02Z

@athanatos: would you mind another look?

src/crimson/osd/pg.cc

src/crimson/osd/watch.h

athanatos · 2022-10-07T18:47:46Z

Incidentally, if you re-request review it'll pop back up on my github query for PRs waiting on a review from me.

Matan-B · 2022-11-08T10:10:30Z

Ping

athanatos · 2022-11-09T03:52:48Z

Looks like comments haven't been addressed?

rzarzynski · 2023-01-11T19:20:00Z

The pushed revision addresses just the minors; the iteration thing requires further discussion.

This patch moves the OBC registry from ShardServices (which is basicaly a gateway to a bunch of PGs) into PG itself. Dividing OBCs by PG (they truly belong to) minimizes the space to search when e.g. checking whether a client is blocked or not. Therefore, this commit is enabler of more performant PR ceph#47637. The changeset draws the assumption that OBC registry and all its users live on the same CPU core. It looks valid to me. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

github-actions · 2023-02-14T08:24:21Z

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

athanatos · 2023-02-15T04:00:05Z

I think we're waiting on tests here. A crimson-rados suite should suffice. Normally the change in common/ would require a rados suite, but testing infrastructure is backed up and the added method has no users in classic so there isn't any real risk.

github-actions · 2023-03-07T18:13:52Z

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

Matan-B · 2023-04-02T08:26:17Z

@rzarzynski, Let's get this merged for rbd_lock_and_fence to pass in suite runs.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

`PG` directly aggregates both `OSDMap` and `PeeringState` which maintains its own `OSDMap` instance. This duplication leads to issues as the `PG::osdmap` never gets updated. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski · 2023-04-27T16:07:47Z

@Matan-B: rebased. Let's finish this PR!

rzarzynski · 2023-04-30T18:43:15Z

The following tests FAILED:
	  2 - setup-venv-for-mgr (Failed)
	  4 - run-tox-mgr (Not Run)
	 34 - run-rbd-unit-tests-127.sh (Timeout)

rzarzynski · 2023-04-30T18:43:31Z

jenkins test make check

Matan-B · 2023-05-01T08:47:48Z

Once make check is up again, this PR can be merged based on:

https://pulpito.ceph.com/matan-2023-04-30_16:46:54-crimson-rados-wip-matanb-crimson-only-testing-30.4-distro-crimson-smithi/

13/19 passed.
Dead jobs:
7258784 - infra
7258785 - infra
Failed, known:
7258780 - https://tracker.ceph.com/issues/59165
7258781 - New test (tracked qa/suites/crimson-rados/rbd: Add new rbd image format api tests #50457)
7258792 - rbd_lock_and_fence
7258796 - rbd_python_api_tests_old_format

rzarzynski · 2023-05-01T09:16:49Z

The following tests FAILED:
	  2 - setup-venv-for-mgr (Failed)
	  4 - run-tox-mgr (Not Run)

rzarzynski · 2023-05-01T09:17:01Z

jenkins test make check

Matan-B · 2023-05-02T07:32:40Z

jenkins test make check

rzarzynski added the feature label Aug 16, 2022

rzarzynski requested a review from a team as a code owner August 16, 2022 14:37

github-actions bot added common crimson labels Aug 16, 2022

athanatos requested changes Aug 16, 2022

View reviewed changes

rzarzynski force-pushed the wip-crimson-blocklist branch from f38adc7 to 845692a Compare August 19, 2022 17:09

rzarzynski requested a review from athanatos August 19, 2022 17:09

rzarzynski force-pushed the wip-crimson-blocklist branch from 845692a to fa61dbb Compare August 23, 2022 09:52

rzarzynski requested a review from a team as a code owner August 23, 2022 09:52

github-actions bot added core tests labels Aug 23, 2022

rzarzynski force-pushed the wip-crimson-blocklist branch from fa61dbb to a6f870f Compare August 23, 2022 10:02

rzarzynski commented Aug 23, 2022

View reviewed changes

src/common/intrusive_lru.h Outdated Show resolved Hide resolved

rzarzynski mentioned this pull request Aug 29, 2022

crimson/osd/client_request: Support client blocklisting #47846

Closed

14 tasks

github-actions bot added the needs-rebase label Oct 7, 2022

rzarzynski force-pushed the wip-crimson-blocklist branch 2 times, most recently from 211bbaf to 471b386 Compare October 7, 2022 18:23

github-actions bot removed the needs-rebase label Oct 7, 2022

athanatos reviewed Oct 7, 2022

View reviewed changes

src/crimson/osd/pg.cc Outdated Show resolved Hide resolved

athanatos reviewed Oct 7, 2022

View reviewed changes

src/crimson/osd/watch.h Outdated Show resolved Hide resolved

rzarzynski force-pushed the wip-crimson-blocklist branch from 471b386 to 08157f7 Compare January 11, 2023 19:15

rzarzynski mentioned this pull request Jan 12, 2023

crimson/osd: make the ObjectContextRegistry per-PG #49729

Merged

14 tasks

github-actions bot added the needs-rebase label Feb 14, 2023

rzarzynski force-pushed the wip-crimson-blocklist branch from 08157f7 to 654eadf Compare February 14, 2023 14:32

github-actions bot removed the needs-rebase label Feb 14, 2023

rzarzynski requested review from Matan-B and athanatos February 14, 2023 18:47

athanatos approved these changes Feb 15, 2023

View reviewed changes

github-actions bot added the needs-rebase label Mar 7, 2023

rzarzynski added 3 commits April 27, 2023 15:27

crimson/osd: check for blocklisted clients

03b6c21

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

crimson/osd: implement watcher blocklisting

24098bf

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

crimson/osd: drop the OSDMap caching from PG

d8655c0

`PG` directly aggregates both `OSDMap` and `PeeringState` which maintains its own `OSDMap` instance. This duplication leads to issues as the `PG::osdmap` never gets updated. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

rzarzynski force-pushed the wip-crimson-blocklist branch from 654eadf to d8655c0 Compare April 27, 2023 16:06

github-actions bot removed the needs-rebase label Apr 27, 2023

Matan-B approved these changes Apr 30, 2023

View reviewed changes

rzarzynski merged commit d8f1402 into ceph:main May 2, 2023

Matan-B mentioned this pull request May 22, 2023

reef: crimson/osd: bring support for client blocklisting #51675

Merged

Conversation

rzarzynski commented Aug 16, 2022

Contribution Guidelines

Checklist

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!

Uh oh!

rzarzynski commented Oct 7, 2022

Uh oh!

github-actions bot commented Oct 7, 2022

Uh oh!

rzarzynski commented Oct 7, 2022

Uh oh!

Uh oh!

Uh oh!

athanatos commented Oct 7, 2022

Uh oh!

Matan-B commented Nov 8, 2022

Uh oh!

athanatos commented Nov 9, 2022

Uh oh!

rzarzynski commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

athanatos commented Feb 15, 2023

Uh oh!

github-actions bot commented Mar 7, 2023

Uh oh!

Matan-B commented Apr 2, 2023

Uh oh!

rzarzynski commented Apr 27, 2023

Uh oh!

rzarzynski commented Apr 30, 2023

Uh oh!

rzarzynski commented Apr 30, 2023

Uh oh!

Matan-B commented May 1, 2023

Uh oh!

rzarzynski commented May 1, 2023

Uh oh!

rzarzynski commented May 1, 2023

Uh oh!

Matan-B commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rzarzynski commented Jan 11, 2023 •

edited

Loading