osd, test: drop the pre-octopus keys conversion of SnapMapper by rzarzynski · Pull Request #60855 · ceph/ceph

rzarzynski · 2024-11-27T00:44:09Z

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)
- 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
jenkins test rook e2e

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

Matan-B

We can also cleanup removed_snap_ and removed_epoch_ which were replaced by purged_snap_ and purged_snap_ after octopus.

    if (osdmap.require_osd_release < ceph_release_t::octopus &&
	tmp.require_osd_release >= ceph_release_t::octopus) {
      dout(10) << __func__ << " first octopus+ epoch" << dendl;

...



      // clean out the old removed_snap_ and removed_epoch keys
      // ('`' is ASCII '_' + 1)
      t->erase_range(OSD_SNAP_PREFIX, "removed_snap_", "removed_snap`");
      t->erase_range(OSD_SNAP_PREFIX, "removed_epoch_", "removed_epoch`");

Matan-B · 2024-11-28T15:34:12Z

src/osd/SnapMapper.cc

 using code_t = Scrub::SnapMapReaderI::result_t::code_t;


-const string SnapMapper::LEGACY_MAPPING_PREFIX = "MAP_";


Before removing MAP_, removed_snap_,removed_epoch_ I think that we should note somewhere that these keys were used before octopus in case someone tries to understand where they came from.

Makes sense.

Makes sense.

Do we want to leave that as a comment or in a dedicated doc note?

Matan-B · 2024-11-28T15:36:59Z

src/osd/OSD.cc

-	goto out;
+      derr << __func__ << " snap_mapper upgrade from pre-octopus"
+	   << " is no longer supported" << dendl;
+      ceph_abort();


Matan-B · 2024-11-28T15:40:53Z

src/osd/SnapMapper.cc

-    + "_" + object_suffix;
-}
-
-int SnapMapper::convert_legacy(


What do you think about leaving these helpers as legacy to be used as a tool when clusters, for some reason, still have the old legacy keys stored? (e.g failure in conversion)

Removal of the legacy is caused by its dependency on the ObjectStore::get_omap_iterator() which is getting dropped. Switching to omap_iterate() is problematic due to lack of viable testing – the code should be already dead.

I think it's better to explicitly lack the code than provide barely tested new variant.

rzarzynski · 2025-01-14T17:40:56Z

src/osd/OSD.cc

-	goto out;
+      derr << __func__ << " snap_mapper upgrade from pre-octopus"
+	   << " is no longer supported" << dendl;
+      ceph_abort();


We should have an offline tool (an extension to COT?) to rebuilt the SnapMapper database (as a follow-up).

Follow-up tracker: https://tracker.ceph.com/issues/69565
Should we paste it near the abort as a comment?

Matan-B

lgtm, left two comments.
One question would be, whether we want to block this until the COT extension is there or do we consider this as a follow-up.

IMO, given that this PR is a possible dependency for #60890 I'm ok with a future follow up.

Matan-B · 2025-01-16T15:30:05Z

src/osd/SnapMapper.cc

 using code_t = Scrub::SnapMapReaderI::result_t::code_t;


-const string SnapMapper::LEGACY_MAPPING_PREFIX = "MAP_";


Makes sense.

Do we want to leave that as a comment or in a dedicated doc note?

Matan-B · 2025-01-16T15:36:39Z

src/osd/OSD.cc

-	goto out;
+      derr << __func__ << " snap_mapper upgrade from pre-octopus"
+	   << " is no longer supported" << dendl;
+      ceph_abort();


Follow-up tracker: https://tracker.ceph.com/issues/69565
Should we paste it near the abort as a comment?

rzarzynski · 2025-01-16T18:54:34Z

Do we want to leave that as a comment or in a dedicated doc note?

I think that a comment might be easier-to-find than a doc.

One question would be, whether we want to block this until the COT extension is there or do we consider this as a follow-up.

Follow-up seems preferable to not block squeezing the old get_omap_iterator() out. Yet, I'm looking into adding the feature to COT.

rzarzynski · 2025-02-12T15:40:09Z

jenkins test make check

NitzanMordhai · 2025-02-26T14:46:48Z

rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues70009

NitzanMordhai · 2025-02-26T14:47:11Z

jenkins test make check arm64

yuriw · 2025-02-26T16:06:00Z

@rzarzynski pls merge when all checks passed
ref: https://tracker.ceph.com/issues/70009

github-actions · 2025-04-03T12:46:53Z

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

rzarzynski · 2025-04-07T22:24:16Z

Merged altogether with #60890.

osd, test: drop the pre-octopus keys conversion of SnapMapper

9b28373

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

rzarzynski added the cleanup label Nov 27, 2024

rzarzynski requested review from Matan-B and athanatos November 27, 2024 00:44

rzarzynski requested a review from a team as a code owner November 27, 2024 00:44

github-actions bot added core tests labels Nov 27, 2024

Matan-B reviewed Nov 28, 2024

View reviewed changes

rzarzynski mentioned this pull request Dec 12, 2024

osd, os: drop the OmapIterator in favor of omap_iterate. Remove ObjectMap #60890

Merged

14 tasks

rzarzynski commented Jan 14, 2025

View reviewed changes

Matan-B approved these changes Jan 16, 2025

View reviewed changes

rzarzynski added the needs-qa label Feb 12, 2025

yuriw added the wip-yuri5-testing label Feb 17, 2025

yuriw added TESTED ready-to-merge and removed wip-yuri5-testing labels Feb 26, 2025

github-actions bot added the needs-rebase label Apr 3, 2025

rzarzynski closed this Apr 7, 2025

		using code_t = Scrub::SnapMapReaderI::result_t::code_t;


		const string SnapMapper::LEGACY_MAPPING_PREFIX = "MAP_";

Conversation

rzarzynski commented Nov 27, 2024

Contribution Guidelines

Checklist

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

Matan-B Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

rzarzynski Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

Matan-B Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzarzynski Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

rzarzynski Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzarzynski commented Jan 16, 2025

Uh oh!

rzarzynski commented Feb 12, 2025

Uh oh!

NitzanMordhai commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NitzanMordhai commented Feb 26, 2025

Uh oh!

yuriw commented Feb 26, 2025

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

rzarzynski commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Matan-B Jan 16, 2025 •

edited

Loading

Matan-B Nov 28, 2024 •

edited

Loading

Matan-B Jan 16, 2025 •

edited

Loading

Matan-B Jan 16, 2025 •

edited

Loading

Matan-B Jan 16, 2025 •

edited

Loading

NitzanMordhai commented Feb 26, 2025 •

edited

Loading