Skip to content

kv/RocksDBStore: Remove ability to bound WholeSpaceIterator#46095

Merged
yuriw merged 1 commit intoceph:masterfrom
aclamk:wip-aclamk-unbounded-wholespace-iterator
May 2, 2022
Merged

kv/RocksDBStore: Remove ability to bound WholeSpaceIterator#46095
yuriw merged 1 commit intoceph:masterfrom
aclamk:wip-aclamk-unbounded-wholespace-iterator

Conversation

@aclamk
Copy link
Contributor

@aclamk aclamk commented Apr 29, 2022 •
edited
Loading

Iterator-bounding feature is introduced to make RocksDB iterators limited, so they
would less likely traverse over tombstones.
This is used when listing keys in fixed range, for example OMAPS for specific object.

It is problematic when extending this logic to WholeSpaceIterator,
since prefix must be taken into account.

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

Replication:

  1. vstart.sh -l -n -o 'bluestore_rocksdb_cfs="M= P= L="' -o bluestore_rocksdb_cf=false (this are quincy settings)
  2. ceph_test_rados_api_aio_pp --gtest_filter=LibRadosAio.OmapPP

Contribution Guidelines

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

@github-actions github-actions bot added the core label Apr 29, 2022
@aclamk
kv/RocksDBStore: Remove feature to make WholeSpaceIterator based on b…
6b9b938 
…ounded iterator

Iterator-bounding feature is introduced to make RocksDB iterators limited, so they
would less likely traverse over tombstones.
This is used when listing keys in fixed range, for example OMAPS for specific object.

It is problematic when extending this logic to WholeSpaceIterator,
since prefix must be taken into account.

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

Signed-off-by: Adam Kupczyk <akupczyk@redhat.com>
@aclamk aclamk force-pushed the wip-aclamk-unbounded-wholespace-iterator branch from c0bdd4d to 6b9b938 Compare April 29, 2022 22:08
@github-actions github-actions bot added the tests label Apr 29, 2022
@aclamk aclamk force-pushed the wip-aclamk-unbounded-wholespace-iterator branch from 6b9b938 to 4983708 Compare April 29, 2022 22:29
@neha-ojha
Copy link
Member

neha-ojha commented Apr 29, 2022 •
edited
Loading

@aclamk please include the commits from #45963 in the pacific backport of this PR

@aclamk aclamk force-pushed the wip-aclamk-unbounded-wholespace-iterator branch from 4983708 to 6b9b938 Compare April 29, 2022 22:34
@aclamk aclamk mentioned this pull request Apr 29, 2022
Merged
14 tasks
idryomov
idryomov approved these changes May 1, 2022
View reviewed changes
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

This fixes it for my TestClsJournal upgrade reproducer as well, so +1.

However I'm concerned about the lack of a straightforward automated test here. Since you found a way to reproduce without involving the upgrade suite (which is cumbersome to run on custom branches for debugging and can be a bit challenging to interpret as well), should a new workunit that would stand up the cluster with

bluestore_rocksdb_cfs="M= P= L="
bluestore_rocksdb_cf=false

and run basic omap tests be added to the rados suite?

cfsnyder
cfsnyder approved these changes May 2, 2022
View reviewed changes
Copy link
Contributor

@cfsnyder cfsnyder left a comment

Choose a reason for hiding this comment

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

Nice catch @aclamk!

@aclamk
Copy link
Contributor Author

aclamk commented May 2, 2022 •
edited
Loading

@idryomov

basic omap tests be added to the rados suite?

I intent to add:

  1. intensive testing for bound iterators
  2. documentation what should be expected behavior when bound iterators are set (meaning of seek_to_first(), valid() etc)
    This was too much for this patch grade PR.

@idryomov
Copy link
Contributor

idryomov commented May 2, 2022

@aclamk Yuri reported the following failure in the tracker ticket. Not sure if it is related to the original bound iterators change, PTAL:

2022-04-30T17:28:28.770 INFO:tasks.workunit.client.1.smithi066.stdout:[ RUN      ] CmpOmap.cmp_vals_u64_invalid_default
2022-04-30T17:28:28.774 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:274: Failure
2022-04-30T17:28:28.775 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.775 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::EQ, {{key, input}}, def)
2022-04-30T17:28:28.775 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: 0
2022-04-30T17:28:28.776 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.778 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:275: Failure
2022-04-30T17:28:28.778 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::NE, {{key, input}}, def)
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: -125
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:276: Failure
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::GT, {{key, input}}, def)
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: -125
2022-04-30T17:28:28.784 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.785 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:277: Failure
2022-04-30T17:28:28.785 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.785 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::GTE, {{key, input}}, def)
2022-04-30T17:28:28.785 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: 0
2022-04-30T17:28:28.785 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:278: Failure
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::LT, {{key, input}}, def)
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: -125
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:/build/ceph-16.2.5/src/test/cls_cmpomap/test_cls_cmpomap.cc:279: Failure
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:Expected equality of these values:
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:  do_cmp_vals(oid, Mode::U64, Op::LTE, {{key, input}}, def)
2022-04-30T17:28:28.786 INFO:tasks.workunit.client.1.smithi066.stdout:    Which is: 0
2022-04-30T17:28:28.787 INFO:tasks.workunit.client.1.smithi066.stdout:  -5
2022-04-30T17:28:28.787 INFO:tasks.workunit.client.1.smithi066.stdout:[  FAILED  ] CmpOmap.cmp_vals_u64_invalid_default (10 ms)

http://pulpito.front.sepia.ceph.com/yuriw-2022-04-30_04:14:21-upgrade:pacific-p2p-pacific-16.2.8_RC1-distro-default-smithi/6816038

@cbodley
Copy link
Contributor

cbodley commented May 2, 2022

@idryomov that looks like https://tracker.ceph.com/issues/52590#note-5. there was an API-breaking bug fix in 16.2.6, so upgrades from 16.2.5 are expected to fail there

@idryomov
Copy link
Contributor

idryomov commented May 2, 2022

Oh, I see -- I have to confess to not searching the tracker for this one as the failure seemed to point in the same general direction at first glance. No further concerns from me then!

neha-ojha
neha-ojha approved these changes May 2, 2022
View reviewed changes
Copy link
Member

@neha-ojha neha-ojha left a comment

Choose a reason for hiding this comment

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

Test results look good - http://pulpito.front.sepia.ceph.com/?branch=wip-yuri3-testing-2022-04-29-1830

@yuriw yuriw merged commit 83041da into ceph:master May 2, 2022
@rzarzynski rzarzynski mentioned this pull request Feb 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@idryomov idryomov idryomov approved these changes

@cfsnyder cfsnyder cfsnyder approved these changes

@ifed01 ifed01 ifed01 approved these changes

@neha-ojha neha-ojha neha-ojha approved these changes

@markhpc markhpc Awaiting requested review from markhpc

@cbodley cbodley Awaiting requested review from cbodley

Assignees

No one assigned

Labels

bluestore common core needs-qa performance tests wip-yuri3-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@aclamk @neha-ojha @idryomov @cbodley @cfsnyder @ifed01 @markhpc @yuriw