osd/SnapMapper: fix legacy key conversion in snapmapper class by mlausch · Pull Request #46908 · ceph/ceph

mlausch · 2022-06-30T12:40:17Z

Octopus modified the SnapMapper key format from

<LEGACY_MAPPING_PREFIX><snapid>_<shardid>_<hobject_t::to_str()>

to

<MAPPING_PREFIX><pool>_<snapid>_<shardid>_<hobject_t::to_str()>

When this change was introduced, 94ebe0e also introduced a conversion
with a crucial bug which essentially destroyed legacy keys by mapping them
to

<MAPPING_PREFIX><poolid>_<snapid>_

without the object-unique suffix. This commit fixes this conversion going
forward, but a fix for existing clusters still needs to be developed.

Unit test result:

[ RUN      ] SnapMapperTest.Legacy
 Converted: MAP_0000000000000008_.1_0000000000000000.E3826231.138.srumroypjftinm.gbdloxfphi.dzitxoz
 To:        SNA_0_0000000000000008_.1_0000000000000000.E3826231.138.srumroypjftinm.gbdloxfphi.dzitxoz 
 New key:   SNA_0_0000000000000008_.1_0000000000000000.E3826231.138.srumroypjftinm.gbdloxfphi.dzitxoz
[       OK ] SnapMapperTest.Legacy (0 ms)

Fixes: https://tracker.ceph.com/issues/56147
Signed-off-by: Manuel Lausch manuel.lausch@1und1.de
Signed-off-by: Matan Breizman mbreizma@redhat.com

Matan-B · 2022-07-03T12:55:46Z

Introduced here: 94ebe0e
We should also be able to convert already (Octopus) updated clusters since the conversion here only applies upon running Octopus for the first time. (e.g calling convert_legacy() when finding old structured SnapMapper key)

Matan-B

Please edit the commit and PR title. https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#commit-title

mlausch · 2022-07-04T09:34:29Z

Hi @Matan-B

The commit / PR title should be prefixed with osd: ... , or do I miss something else?

Do you think there could old structures remaining after successful conversion? As far as I see, after successful conversion the superblock will be updated.
Anyway. I fear more complex changes will exceed my coding skills. It would be nice If you or someone else could support me with this.

src/osd/SnapMapper.cc

Matan-B · 2022-07-05T12:10:45Z

Hi @Matan-B

The commit / PR title should be prefixed with osd: ... , or do I miss something else?

Yes, osd/SnapMapper:

Do you think there could old structures remaining after successful conversion? As far as I see, after successful conversion the superblock will be updated.

I'm afraid that clone objects created before upgrading may not be deleted correctly, as the tracker implies.
While this patch will result in successful conversion for future upgrades, already updated clusters should also be able to delete (old) clone objects.

Anyway. I fear more complex changes will exceed my coding skills. It would be nice If you or someone else could support me with this.

I'm looking into this and I'm available for any question! 👍

mlausch · 2022-07-05T13:01:08Z

I'm afraid that clone objects created before upgrading may not be deleted correctly, as the tracker implies.
While this patch will result in successful conversion for future upgrades, already updated clusters should also be able to delete (old) clone objects.

Ah yes, I see your point. Unfortunately the faulty conversion deleted the old MAP_ entries. So I'm not quite sure how to recover this.
And I think the "repair" should only run once if the error is present. Iterating over the whole omap, or something else, just in case, might slow down the startup.

Matan-B · 2022-07-11T13:35:55Z

jenkins test make check

Matan-B · 2022-07-11T15:01:30Z

Upgrade suite Teuthology runs:
https://gist.github.com/Matan-B/9913f9bf413f7805d7a52dac22a589c1

src/osd/SnapMapper.cc

rzarzynski · 2022-07-13T18:35:42Z

@athanatos: do the updated commits addresses the worry for having the fix in octopus?

athanatos · 2022-07-13T22:41:43Z

Squash all three commits to a single commit. In general (and particularly since we need to backport this) the commit message needs to do a good job of explaining what the commit does. Since it's a backport it also must include the Fixes line in the commit message itself. How about:

osd/SnapMapper: fix pacific legacy key conversion and introduce test

Pacific modified the SnapMapper key format from

  <LEGACY_MAPPING_PREFIX><snapid>_<shardid>_<hobject_t::to_str()>

to

  <MAPPING_PREFIX><pool>_<snapid>_<shardid>_<hobject_t::to_str()>

When this change was introduced, 94ebe0ea also introduced a conversion
with a crucial bug which essentially destroyed legacy keys by mapping them
to

  <MAPPING_PREFIX><poolid>_<snapid>_

without the object-unique suffix.  This commit fixes this conversion going
forward, but a fix for existing clusters still needs to be developed.

Fixes: https://tracker.ceph.com/issues/56147
Signed-off-by: Manuel Lausch <manuel.lausch@1und1.de>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>

edit: had the version wrong

athanatos

Pretty close, I suggested an explanatory comment, a name change, and the commit message needs to be fixed.

src/osd/SnapMapper.cc

src/test/test_snap_mapper.cc

athanatos · 2022-07-13T22:44:48Z

@neha-ojha Given that we'll want to backport this relatively quickly, can you look over it as well?

Octopus modified the SnapMapper key format from <LEGACY_MAPPING_PREFIX><snapid>_<shardid>_<hobject_t::to_str()> to <MAPPING_PREFIX><pool>_<snapid>_<shardid>_<hobject_t::to_str()> When this change was introduced, 94ebe0e also introduced a conversion with a crucial bug which essentially destroyed legacy keys by mapping them to <MAPPING_PREFIX><poolid>_<snapid>_ without the object-unique suffix. This commit fixes this conversion going forward, but a fix for existing clusters still needs to be developed. Fixes: https://tracker.ceph.com/issues/56147 Signed-off-by: Manuel Lausch <manuel.lausch@1und1.de> Signed-off-by: Matan Breizman <mbreizma@redhat.com>

neha-ojha

LGTM, I will run it through testing and we'll include the octopus backport of this patch in the final octopus point release, if testing goes fine.

neha-ojha · 2022-07-14T18:57:47Z

Upgrade suite Teuthology runs: https://gist.github.com/Matan-B/9913f9bf413f7805d7a52dac22a589c1

@Matan-B Please raise a separate PR for the teuthology test you added to catch this bug

ljflores · 2022-07-15T20:37:51Z

jenkins test windows

ljflores · 2022-07-15T20:42:21Z

https://pulpito.ceph.com/?branch=snapshot_key_conversion

Unrelated Failures:
1. https://tracker.ceph.com/issues/56575 -- new Tracker
2. https://tracker.ceph.com/issues/56384
3. https://tracker.ceph.com/issues/55853
4. https://tracker.ceph.com/issues/56573 -- new Tracker
5. https://tracker.ceph.com/issues/53789
6. https://tracker.ceph.com/issues/53939
7. https://tracker.ceph.com/issues/56574 -- new Tracker
8. https://tracker.ceph.com/issues/45423

Details:
1. test_cls_lock.sh: ClsLock.TestExclusiveEphemeralStealEphemeral fails from "method lock.get_info tried to update object but is not marked WR" - Ceph - RADOS
2. ceph/test.sh: check_response erasure-code didn't find erasure-code in output - Ceph - CephFS
3. test_cls_rgw.sh: failures in 'cls_rgw.index_list' and 'cls_rgw.index_list_delimited` - Ceph - RGW
4. test_cephadm.sh: KeyError: 'TYPE' - Ceph - Orchestrator
5. CommandFailedError (rados/test_python.sh): "RADOS object not found" causes test_rados.TestWatchNotify.test_aio_notify to fail - Ceph - RADOS
6. qa/tasks/rook times out: 'check osd count' reached maximum tries (90) after waiting for 900 seconds - Ceph - Rook
7. rados/valgrind-leaks: cluster [WRN] Health check failed: 2 osds down (OSD_DOWN)" in cluster log - Ceph - RADOS
8. api_tier_pp: [ FAILED ] LibRadosTwoPoolsPP.HitSetWrite - Ceph - RADOS

mlausch requested a review from a team as a code owner June 30, 2022 12:40

github-actions bot added the core label Jun 30, 2022

ronen-fr requested review from Matan-B and aclamk June 30, 2022 13:43

neha-ojha requested review from athanatos June 30, 2022 18:21

Matan-B reviewed Jul 3, 2022

View reviewed changes

Matan-B reviewed Jul 5, 2022

View reviewed changes

src/osd/SnapMapper.cc Show resolved Hide resolved

mlausch force-pushed the snapshot_key_conversion branch from 1c88cad to e45f478 Compare July 5, 2022 12:48

mlausch changed the title ~~RADOS: fix legacy key conversion in snapmapper class~~ osd/SnapMapper: fix legacy key conversion in snapmapper class Jul 5, 2022

Matan-B self-requested a review July 6, 2022 08:09

Matan-B approved these changes Jul 11, 2022

View reviewed changes

ifed01 self-requested a review July 11, 2022 15:47

athanatos requested changes Jul 11, 2022

View reviewed changes

src/osd/SnapMapper.cc Show resolved Hide resolved

github-actions bot added the tests label Jul 13, 2022

athanatos requested changes Jul 13, 2022

View reviewed changes

src/osd/SnapMapper.cc Show resolved Hide resolved

src/osd/SnapMapper.cc Outdated Show resolved Hide resolved

src/osd/SnapMapper.cc Show resolved Hide resolved

src/test/test_snap_mapper.cc Outdated Show resolved Hide resolved

athanatos requested review from neha-ojha and removed request for aclamk and ifed01 July 13, 2022 22:44

Matan-B force-pushed the snapshot_key_conversion branch from c4c9be4 to 66bea86 Compare July 14, 2022 08:14

Matan-B requested a review from athanatos July 14, 2022 08:18

athanatos approved these changes Jul 14, 2022

View reviewed changes

neha-ojha added the wip-neha-testing label Jul 14, 2022

neha-ojha approved these changes Jul 14, 2022

View reviewed changes

Matan-B mentioned this pull request Jul 14, 2022

octopus: osd/SnapMapper: fix pacific legacy key conversion and introduce test #47108

Merged

neha-ojha merged commit d471440 into ceph:main Jul 15, 2022

neha-ojha removed the wip-neha-testing label Jul 15, 2022

Conversation

mlausch commented Jun 30, 2022 • edited by Matan-B Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matan-B commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

mlausch commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Matan-B commented Jul 5, 2022

Uh oh!

mlausch commented Jul 5, 2022

Uh oh!

Matan-B commented Jul 11, 2022

Uh oh!

Matan-B commented Jul 11, 2022

Uh oh!

Uh oh!

rzarzynski commented Jul 13, 2022

Uh oh!

athanatos commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athanatos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athanatos commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neha-ojha left a comment

Choose a reason for hiding this comment

Uh oh!

neha-ojha commented Jul 14, 2022

Uh oh!

ljflores commented Jul 15, 2022

Uh oh!

ljflores commented Jul 15, 2022 • edited by neha-ojha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mlausch commented Jun 30, 2022 •

edited by Matan-B

Loading

Matan-B commented Jul 3, 2022 •

edited

Loading

mlausch commented Jul 4, 2022 •

edited

Loading

athanatos commented Jul 13, 2022 •

edited

Loading

athanatos commented Jul 13, 2022 •

edited

Loading

ljflores commented Jul 15, 2022 •

edited by neha-ojha

Loading