mon: disable snap id allocation for fsmap pools by mchangir · Pull Request #47753 · ceph/ceph

mchangir · 2022-08-23T12:05:29Z

disallow monitor managed snaps to be created for pools attached to cephfs filesystems
disallow pools with snaps to be used in dir and file layouts
disallow pools with snaps to be used for fs creation

Fixes: https://tracker.ceph.com/issues/16745
Signed-off-by: Milind Changire mchangir@redhat.com

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

ajarr

I guess this PR is WIP/draft , and you'll add tests later.

ajarr · 2022-08-23T17:56:46Z

src/mon/OSDMonitor.cc


  const pg_pool_t *pool = osdmap.get_pg_pool(m->pool);

+  if (m->op == POOL_OP_CREATE_SNAP ||


Can you please explain in the commit message why we disallow pool level snaps for FS pools?

thinking about it more ... since pool level snap IDs are assigned by mons and FS level snap IDs are assigned by the MDS itself, I wonder how the snap IDs may clash causing problems ... which was the main driving factor for this PR
since MDS is never going to talk to the mon for manging FS snaps ... I think I may be lacking a bit of context

@gregsfortytwo could you elaborate on this please

The snapids are a shared resource and when you delete a snapshot, you insert it into the osdmap. Any overlap is bad since it would delete both...

@gregsfortytwo could you point me to the piece of code on the mds side which initiates the propagation of the deleted snapid to the osdmap

so @vshankar mentions that the reason for creating MDS managed snapshots was to implement a finer granularity (dir level) snaps rather than using the pool level snaps for the data pool in the fs

the collision happens since each write op to the data pool has to be handed a snap context (snapid) to write the data to and when pool level snaps are allowed along side MDS level snaps for the same pool, a "remove snap" from the mds could encounter a collision with pool level snap IDs and cause the unintentional removal of pool level data rather then dir level data as the MDS intended to remove

@mchangir You'd need to look at SnapServer.cc

ajarr · 2022-08-23T20:18:06Z

src/mds/Server.cc

+{
+  /* Since we don't allow to create pool level snaps for all pools associated
+   * with a CephFS, we also block a pool with pre-existing snaps to be attached
+   * to a CephFS.


Does that mean we want to prevent a pool that has pre-existing snaps to be used as a FS data pool using 'add_data_pool' or 'fs new' command? So we will want this check in "FileSystemCommandHandler::_check_pool() as well?

Yes; we'd need a check there as well. But I want others to comment on this specific issue.

I think that would be an ideal thing to do.

kotreshhr · 2022-08-24T05:35:35Z

src/mds/Server.cc

+   * to a CephFS.
+   */
+  if (const auto *p = osdmap.get_pg_pool(pool_id); p->snaps.size() > 0) {
+    dout(20) << "pool(" << pool_id << ") already has snaps; can't attach to fs" << dendl;


should this be derr ?

this is not a fatal error, so its not a good idea to tag it as ERR in the logs
in fact we are avoiding errors by blocking the addition of such pools to the fs

kotreshhr · 2022-08-24T05:37:58Z

src/mds/Server.cc

 }

 // XATTRS
+int Server::check_if_pool_is_empty(const OSDMap& osdmap, int64_t pool_id)


Is it better to rename the function as check_if_pool_has_snaps instead ?

renamed function to check_for_pool_snaps()

src/mds/Server.cc

vshankar · 2022-08-24T09:50:19Z

src/mds/Server.cc

+{
+  /* Since we don't allow to create pool level snaps for all pools associated
+   * with a CephFS, we also block a pool with pre-existing snaps to be attached
+   * to a CephFS.


I think that would be an ideal thing to do.

src/mon/OSDMonitor.cc

vshankar · 2022-08-30T05:08:58Z

@mchangir I guess this is ready for QA?

mchangir · 2022-08-30T05:16:55Z

@mchangir I guess this is ready for QA?

yes, its ready for QA

gregsfortytwo

Reviewed-by: Greg Farnum gfarnum@redhat.com

batrick

Good work!

vshankar · 2022-09-13T04:33:10Z

FYI, I will pick this for fs suite runs soon -- I am currently trying to merge pending PRs which are running tests, however, I'm frequently running into teuthology related issues.

rishabh-d-dave

IMHO it'll be great to add a note about this in CephFS docs.

rishabh-d-dave · 2022-11-18T14:53:53Z

qa/tasks/cephfs/test_snapshots.py

            pass
        self.delete_dir_and_snaps("accounts", new_limit + 1)
+
+    def test_disallow_monitor_managed_snaps_for_fs_pools(self):


Moving this test to a new class, say TestMonSnapsAndFsPools (subclass of TestSnapshots), would allow us to quickly test how monitor-managed snapshots are treated on FS pools conveniently.

added new class to test specifically for mon-managed snaps

rishabh-d-dave · 2022-11-18T14:56:22Z

src/mon/FSCommands.cc

  }

+  if (pool->has_snaps()) {
+    *ss << "pool(" << pool_id << ") already has snaps; can't attach to fs";


Would it be worth it to write "mon-managed snaps" instead of just "snaps" here for clarity of CephFS users?

fixed debug log to mention "mon-managed snaps"

rishabh-d-dave · 2022-11-18T15:01:37Z

Ceph testing infra is down for last few weeks, I'll put this PR through testing as soon as it's fine again.

kotreshhr · 2022-11-21T09:14:20Z

qa/tasks/cephfs/test_snapshots.py

+        self.assertEqual(ret, errno.EOPNOTSUPP)
+
+        # cleanup
+        self.fs.rados(["rmsnap", "snap3"], pool=test_pool_name)


nit: delete {test_pool_name}

@kotreshhr do you want me to replace {test_pool_name} with snap-test-pool ?

Nope, I meant cleanup is missing. The pool needs to deleted.

Isn't cleaning up redundant for since all FSs and related pools are deleted after every test case unconditionally?

added pool cleanup

Signed-off-by: Milind Changire <mchangir@redhat.com>

anthonyeleven · 2022-11-21T14:06:02Z

doc/dev/cephfs-snapshots.rst

 invisible, not throwing errors to the user.) If each FS gets its own
 pool things probably work, but this isn't tested and may not be true.
+
+.. Note:: To avoid snap id collision between mon-managed snapshots and file-system


There has been some contention about "filesystem" vs "file system" vs "file-system". The powers that be prefer "file system", so I suggest taking out the - in these three instances.

corrected to file system

anthonyeleven

hyphens

Signed-off-by: Milind Changire <mchangir@redhat.com>

rishabh-d-dave

https://tracker.ceph.com/projects/cephfs/wiki/Main#30-JAN-2023

rishabh-d-dave · 2023-01-30T06:30:57Z

jenkins test make check arm64

rishabh-d-dave · 2023-01-30T06:31:06Z

jenkins test windows

rishabh-d-dave · 2023-01-30T06:32:06Z

Since PRs from this testing batch are waiting on required CI jobs to be green, I'll wait on this one too for a couple of hours.

Requested change has been incorporated.

mchangir requested a review from a team as a code owner August 23, 2022 12:05

github-actions bot added cephfs Ceph File System core mon labels Aug 23, 2022

mchangir requested a review from a team August 23, 2022 12:06

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch 2 times, most recently from 320fa67 to afac3a1 Compare August 23, 2022 12:25

ajarr reviewed Aug 23, 2022

View reviewed changes

mchangir marked this pull request as draft August 24, 2022 03:07

kotreshhr reviewed Aug 24, 2022

View reviewed changes

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch from afac3a1 to 043c7a3 Compare August 24, 2022 06:05

vshankar reviewed Aug 24, 2022

View reviewed changes

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch from 043c7a3 to eb0f530 Compare August 24, 2022 11:51

gregsfortytwo reviewed Aug 24, 2022

View reviewed changes

src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch 3 times, most recently from 4cb8673 to 86e6af3 Compare August 30, 2022 03:14

github-actions bot added the tests label Aug 30, 2022

mchangir marked this pull request as ready for review August 30, 2022 03:15

gregsfortytwo approved these changes Sep 12, 2022

View reviewed changes

batrick approved these changes Sep 13, 2022

View reviewed changes

vshankar approved these changes Sep 13, 2022

View reviewed changes

vshankar added the wip-rishabh-testing Rishabh's testing label label Oct 17, 2022

rishabh-d-dave approved these changes Nov 18, 2022

View reviewed changes

kotreshhr reviewed Nov 21, 2022

View reviewed changes

mchangir added 3 commits November 21, 2022 15:33

osd: add utility function has_snaps()

87f969f

Signed-off-by: Milind Changire <mchangir@redhat.com>

mon: block monitor managed snaps for fs pools

23db15d

Signed-off-by: Milind Changire <mchangir@redhat.com>

qa: test snap id failures for fs pools

d8c55ed

Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch from 86e6af3 to d8c55ed Compare November 21, 2022 10:45

mchangir requested a review from a team as a code owner November 21, 2022 10:58

github-actions bot added the documentation label Nov 21, 2022

kotreshhr approved these changes Nov 21, 2022

View reviewed changes

anthonyeleven reviewed Nov 21, 2022

View reviewed changes

anthonyeleven previously requested changes Nov 21, 2022

View reviewed changes

doc/dev: add note about mon-managed and fs snaps

a81c0b3

Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir force-pushed the mon-disable-snap-id-allocation-for-fsmap-pools branch from 96915f1 to a81c0b3 Compare November 22, 2022 00:49

rishabh-d-dave reviewed Jan 30, 2023

View reviewed changes

rishabh-d-dave merged commit 44d7ab9 into ceph:main Jan 30, 2023

This was referenced Feb 9, 2023

pacific: mon: disable snap id allocation for fsmap pools #50050

Merged

quincy: mon: disable snap id allocation for fsmap pools #50090

Merged

rzarzynski mentioned this pull request May 4, 2023

mon: block osd pool mksnap for fs pools #51275

Merged

14 tasks


		const pg_pool_t *pool = osdmap.get_pg_pool(m->pool);

		if (m->op == POOL_OP_CREATE_SNAP \|\|

Conversation

mchangir commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

ajarr left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vshankar commented Aug 30, 2022

Uh oh!

mchangir commented Aug 30, 2022

Uh oh!

gregsfortytwo left a comment

Choose a reason for hiding this comment

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Sep 13, 2022

Uh oh!

rishabh-d-dave left a comment

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!

rishabh-d-dave commented Nov 18, 2022

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!

anthonyeleven left a comment

Choose a reason for hiding this comment

mchangir commented Aug 23, 2022 •

edited

Loading

rishabh-d-dave commented Jan 30, 2023 •

edited

Loading