reinstate ceph cluster_snap support

This patch brings back and updates (for dumpling) the code originally introduced to support ?ceph osd cluster_snap <snap>?, that was disabled and partially removed before cuttlefish.

Some minimal testing appears to indicate this even works: the modified mon actually generated an osdmap with the cluster_snap request, and starting a modified osd that was down and letting it catch up caused the osd to take the requested snapshot. I see no reason why it wouldn't have taken it if it was up and running, so... Why was this feature disabled in the first place?

Signed-off-by Alexandre Oliva <[email protected]>

---
 src/mon/MonCommands.h |    6 ++++--
 src/mon/OSDMonitor.cc |   11 +++++++----
 src/osd/OSD.cc        |   13 +++++++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

src/mon/MonCommands.h +4 -2

index 8e9c2bb..225c687 100644
@@ -431,8 +431,10 @@ COMMAND("osd set " \
 COMMAND("osd unset " \
 	"name=key,type=CephChoices,strings=pause|noup|nodown|noout|noin|nobackfill|norecover|noscrub|nodeep-scrub", \
 	"unset <key>", "osd", "rw", "cli,rest")
-COMMAND("osd cluster_snap", "take cluster snapshot (disabled)", \
-	"osd", "r", "")
+COMMAND("osd cluster_snap " \
+	"name=snap,type=CephString", \
+	"take cluster snapshot",	\
+	"osd", "r", "cli")
 COMMAND("osd down " \
 	"type=CephString,name=ids,n=N", \
 	"set osd(s) <id> [<id>...] down", "osd", "rw", "cli,rest")

src/mon/OSDMonitor.cc +7 -4

index 07022ae..9bf9511 100644
@@ -3099,10 +3099,13 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
       return prepare_unset_flag(m, CEPH_OSDMAP_NODEEP_SCRUB);
 
   } else if (prefix == "osd cluster_snap") {
-    // ** DISABLE THIS FOR NOW **
-    ss << "cluster snapshot currently disabled (broken implementation)";
-    // ** DISABLE THIS FOR NOW **
-
+    string snap;
+    cmd_getval(g_ceph_context, cmdmap, "snap", snap);
+    pending_inc.cluster_snapshot = snap;
+    ss << "creating cluster snap " << snap;
+    getline(ss, rs);
+    wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
+    return true;
   } else if (prefix == "osd down" ||
 	     prefix == "osd out" ||
 	     prefix == "osd in" ||

src/osd/OSD.cc +13 -0

index 1a77dae..e41a6b3 100644
@@ -5022,6 +5022,19 @@ void OSD::handle_osd_map(MOSDMap *m)
     assert(0 == "MOSDMap lied about what maps it had?");
   }
 
+  // check for cluster snapshots
+  for (epoch_t cur = superblock.current_epoch + 1; cur <= m->get_last(); cur++) {
+    OSDMapRef newmap = get_map(cur);
+    string cluster_snap = newmap->get_cluster_snapshot();
+    if (cluster_snap.length() == 0)
+      continue;
+
+    dout(0) << "creating cluster snapshot '" << cluster_snap << "'" << dendl;
+    int r = store->snapshot(cluster_snap);
+    if (r)
+      dout(0) << "failed to create cluster snapshot: " << cpp_strerror(r) << dendl;
+  }
+
   if (superblock.oldest_map) {
     int num = 0;
     epoch_t min(

Signature · 9 lines

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

Quoted 2 lines · Sage Weil

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

That depends on what's expected from the feature, actually.

One use is to roll back a single osd, and for that, the feature works just fine. Of course, for that one doesn't need the multi-osd snapshots to be mutually consistent, but it's still convenient to be able to take a global snapshot with a single command.

Another use is to roll back the entire cluster to an earlier state, and for that, you *probably* want to roll back the monitors too, although it doesn't seem like this is strictly necessary, unless some significant configuration changes occurrend in the cluster since the snapshot was taken, and you want to roll back those too.

In my experience, rolling back only osds has worked just fine, with the exception of cases in which the snapshot is much too old, and the mons have already expired osdmaps after the last one the osd got when the snapshot was taken. For this one case, I have a patch that enables the osd to rejoin the cluster in spite of the expired osdmaps, which has always worked for me, but I understand there may be exceptional cluster reconfigurations in which this wouldn't have worked.

As for snapshotting monitors... I suppose the way to go is to start a store.db dump in background, instead of taking a btrfs snapshot, since the store.db is not created as a subvolume. That said, it would make some sense to make it so, to make it trivially snapshottable.

Anyway, I found a problem in the earlier patch: when I added a new disk to my cluster this morning, it tried to iterate over osdmaps that were not available (e.g. the long-gone osdmap 1), and crashed.

Here's a fixed version, that makes sure we don't start the iteration before m->get_first().

Hi,

Quoted 33 lines · Alexandre Oliva

Quoted 2 lines · Sage Weil

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

That depends on what's expected from the feature, actually.

One use is to roll back a single osd, and for that, the feature works just fine. Of course, for that one doesn't need the multi-osd snapshots to be mutually consistent, but it's still convenient to be able to take a global snapshot with a single command.

Another use is to roll back the entire cluster to an earlier state, and for that, you *probably* want to roll back the monitors too, although it doesn't seem like this is strictly necessary, unless some significant configuration changes occurrend in the cluster since the snapshot was taken, and you want to roll back those too.

In my experience, rolling back only osds has worked just fine, with the exception of cases in which the snapshot is much too old, and the mons have already expired osdmaps after the last one the osd got when the snapshot was taken. For this one case, I have a patch that enables the osd to rejoin the cluster in spite of the expired osdmaps, which has always worked for me, but I understand there may be exceptional cluster reconfigurations in which this wouldn't have worked.

As for snapshotting monitors... I suppose the way to go is to start a store.db dump in background, instead of taking a btrfs snapshot, since the store.db is not created as a subvolume. That said, it would make some sense to make it so, to make it trivially snapshottable.

Quoted 6 lines

Anyway, I found a problem in the earlier patch: when I added a new disk to my cluster this morning, it tried to iterate over osdmaps that were not available (e.g. the long-gone osdmap 1), and crashed.

Here's a fixed version, that makes sure we don't start the iteration before m->get_first().

In principle, we can add this back in. I think it needs a few changes, though.

First, FileStore::snapshot() needs to pause and drain the workqueue before taking the snapshot, similar to what is done with the sync sequence. Otherwise it isn't a transactionally consistent snapshot and may tear some update. Because it is draining the work queue, it *might* also need to drop some locks, but I'm hopeful that that isn't necessary.

Second, the call in handle_osd_map() should probably go in the loop a bit further down that is consuming maps. It probably won't matter most of the time, but I'm paranoid about corner conditions. It also avoids iterating over the new OSDMaps multiple times in the common case where there is no cluster_snap (a minor win).

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

Quoted 58 lines · Sage Weil

Hi,

Quoted 33 lines · Alexandre Oliva

Quoted 2 lines · Sage Weil

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

That depends on what's expected from the feature, actually.

One use is to roll back a single osd, and for that, the feature works just fine. Of course, for that one doesn't need the multi-osd snapshots to be mutually consistent, but it's still convenient to be able to take a global snapshot with a single command.

Another use is to roll back the entire cluster to an earlier state, and for that, you *probably* want to roll back the monitors too, although it doesn't seem like this is strictly necessary, unless some significant configuration changes occurrend in the cluster since the snapshot was taken, and you want to roll back those too.

In my experience, rolling back only osds has worked just fine, with the exception of cases in which the snapshot is much too old, and the mons have already expired osdmaps after the last one the osd got when the snapshot was taken. For this one case, I have a patch that enables the osd to rejoin the cluster in spite of the expired osdmaps, which has always worked for me, but I understand there may be exceptional cluster reconfigurations in which this wouldn't have worked.

As for snapshotting monitors... I suppose the way to go is to start a store.db dump in background, instead of taking a btrfs snapshot, since the store.db is not created as a subvolume. That said, it would make some sense to make it so, to make it trivially snapshottable.

Quoted 6 lines

Anyway, I found a problem in the earlier patch: when I added a new disk to my cluster this morning, it tried to iterate over osdmaps that were not available (e.g. the long-gone osdmap 1), and crashed.

Here's a fixed version, that makes sure we don't start the iteration before m->get_first().

In principle, we can add this back in. I think it needs a few changes, though.

First, FileStore::snapshot() needs to pause and drain the workqueue before taking the snapshot, similar to what is done with the sync sequence. Otherwise it isn't a transactionally consistent snapshot and may tear some update. Because it is draining the work queue, it *might* also need to drop some locks, but I'm hopeful that that isn't necessary.

Second, the call in handle_osd_map() should probably go in the loop a bit further down that is consuming maps. It probably won't matter most of the time, but I'm paranoid about corner conditions. It also avoids iterating over the new OSDMaps multiple times in the common case where there is no cluster_snap (a minor win).

Quoted 7 lines

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

I think we also need to snapshot the osd journal

Hi,

Quoted 11 lines · Alexandre Oliva

Quoted 2 lines · Sage Weil

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

That depends on what's expected from the feature, actually.

One use is to roll back a single osd, and for that, the feature works just fine. Of course, for that one doesn't need the multi-osd snapshots to be mutually consistent, but it's still convenient to be able to take a global snapshot with a single command.

In principle, we can add this back in. I think it needs a few changes, though.

First, FileStore::snapshot() needs to pause and drain the workqueue before taking the snapshot, similar to what is done with the sync sequence. Otherwise it isn't a transactionally consistent snapshot and may tear some update. Because it is draining the work queue, it *might* also need to drop some locks, but I'm hopeful that that isn't necessary.

Second, the call in handle_osd_map() should probably go in the loop a bit further down that is consuming maps. It probably won't matter most of the time, but I'm paranoid about corner conditions. It also avoids iterating over the new OSDMaps multiple times in the common case where there is no cluster_snap (a minor win).

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

Quoted 6 lines

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

I haven't looked into this yet.

Quoted 6 lines · Alexandre Oliva

Quoted 6 lines

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

Quoted 1 line

I haven't looked into this yet.

None of these are particularly appealing; (1) wastes disk space and cpu cycles; (2) relies on leveldb internal implementation details such as the fact that files are never modified after they're first closed, and
(3) requires a btrfs subvol for the store.db. My favorite choice would be 3, but can we just fail mon snaps when this requirement is not met?

Quoted 14 lines · Sage Weil

Hi,

Quoted 11 lines · Alexandre Oliva

Quoted 2 lines · Sage Weil

FWIW Alexandre, this feature was never really complete. For it to work, we also need to snapshot the monitors, and roll them back as well.

That depends on what's expected from the feature, actually.

One use is to roll back a single osd, and for that, the feature works just fine. Of course, for that one doesn't need the multi-osd snapshots to be mutually consistent, but it's still convenient to be able to take a global snapshot with a single command.

Quoted 8 lines

In principle, we can add this back in. I think it needs a few changes, though.

First, FileStore::snapshot() needs to pause and drain the workqueue before taking the snapshot, similar to what is done with the sync sequence. Otherwise it isn't a transactionally consistent snapshot and may tear some update. Because it is draining the work queue, it *might* also need to drop some locks, but I'm hopeful that that isn't necessary.

Hmm... I don't quite get this. The Filestore implementation of snapshot already performs a sync_and_flush before calling the backend's create_checkpoint. Shouldn't that be enough? FWIW, the code I brought in from argonaut didn't do any such thing; it did drop locks, but that doesn't seem to be necessary any more:

Quoted 8 lines · Alexandre Oliva

Quoted 6 lines · Alexandre Oliva

Quoted 6 lines

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

Quoted 7 lines

Quoted 1 line

I haven't looked into this yet.

None of these are particularly appealing; (1) wastes disk space and cpu cycles; (2) relies on leveldb internal implementation details such as the fact that files are never modified after they're first closed, and
(3) requires a btrfs subvol for the store.db. My favorite choice would be 3, but can we just fail mon snaps when this requirement is not met?

Another aspect that needs to be considered is whether to take a snapshot of the leader only, or of all monitors in the quorum. The fact that the snapshot operation may take a while to complete (particularly (1)), and monitors may not make progress while taking the snapshot (which might cause the client and other monitors to assume other monitors have failed), make the whole thing quite more complex than what I'd have hoped for.

Another point that may affect the decision is the amount of information in store.db that may have to be retained. E.g., if it's just a small amount of information, creating a separate database makes far more sense than taking a complete copy of the entire database, and it might even make sense for the leader to include the full snapshot data in the snapshot-taking message shared with other monitors, so that they all take exactly the same snapshot, even if they're not in the quorum and receive the update at a later time. Of course this wouldn't work if the amount of snapshotted monitor data was more than reasonable for a monitor message.

Anyway, this is probably more than what I'd be able to undertake myself, at least in part because, although I can see one place to add the snapshot-taking code to the leader (assuming it's ok to take the snapshot just before or right after all monitors agree on it), I have no idea of where to plug the snapshot-taking behavior into peon and recovering monitors. Absent a two-phase protocol, it seems to me that all monitors ought to take snapshots tentatively when they issue or acknowledge the snapshot-taking proposal, so as to make sure that if it succeeds we'll have a quorum of snapshots, but if the proposal doesn't succeed at first, I don't know how to deal with retries (overwrite existing snapshots? discard the snapshot when its proposal fails?) or cancellation (say, the client doesn't get confirmation from the leader, the leader changes, it retries that some times, and eventually it gives up, but some monitors have already tentatively taken the snapshot in the mean time).

(you've never been getting a real point-in-time snapshot, although as long as you didn't use external communication channels you could at least be sure it contained a causal cut).

And of course that's nothing compared to snapshotting the monitors, as you've noticed

— but making it actually be a cluster snapshot (instead of something you could basically do by taking a btrfs snapshot yourself)

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

Quoted 6 lines · Sage Weil

Finally, eventually we should make this do a checkpoint on the mons too. We can add the osd snapping back in first, but before this can/should really be used the mons need to be snapshotted as well. Probably that's just adding in a snapshot() method to MonitorStore.h and doing either a leveldb snap or making a full copy of store.db... I forget what leveldb is capable of here.

I suppose it might be a bit too late for Giant, but I finally got 'round to implementing this. I attach the patch that implements it, to be applied on top of the updated version of the patch I posted before, also attached.

I have a backport to Firefly too, if there's interest.

I have tested both methods: btrfs snapshotting of store.db (I've manually turned store.db into a btrfs subvolume), and creating a new db with all (prefix,key,value) triples. I'm undecided about inserting multiple transaction commits for the latter case; the mon mem use grew up a lot as it was, and in a few tests the snapshotting ran twice, but in the end a dump of all the data in the database created by btrfs snapshotting was identical to that created by explicit copying. So, the former is preferred, since it's so incredibly more efficient. I also considered hardlinking all files in store.db into a separate tree, but I didn't like the idea of coding that in C+-, :-) and I figured it might not work with other db backends, and maybe even not be guaranteed to work with leveldb. It's probably not worth much more effort.

Quoted 4 lines · Alexandre Oliva

I have tested both methods: btrfs snapshotting of store.db (I've manually turned store.db into a btrfs subvolume), and creating a new db with all (prefix,key,value) triples. I'm undecided about inserting multiple transaction commits for the latter case; the mon mem use grew

Quoted 8 lines

up a lot as it was, and in a few tests the snapshotting ran twice, but in the end a dump of all the data in the database created by btrfs snapshotting was identical to that created by explicit copying. So, the former is preferred, since it's so incredibly more efficient. I also considered hardlinking all files in store.db into a separate tree, but I didn't like the idea of coding that in C+-, :-) and I figured it might not work with other db backends, and maybe even not be guaranteed to work with leveldb. It's probably not worth much more effort.

This looks pretty reasonable!

I think we definitely need to limit the size of the transaction when doing the snap. The attached patch seems to try to do it all in one go, which is not going to work for large clusters. Either re-use an existing tunable like the sync chunk size or add a new one?

Quoted 15 lines · Sage Weil

Quoted 4 lines · Alexandre Oliva

I have tested both methods: btrfs snapshotting of store.db (I've manually turned store.db into a btrfs subvolume), and creating a new db with all (prefix,key,value) triples. I'm undecided about inserting multiple transaction commits for the latter case; the mon mem use grew

Quoted 8 lines

up a lot as it was, and in a few tests the snapshotting ran twice, but in the end a dump of all the data in the database created by btrfs snapshotting was identical to that created by explicit copying. So, the former is preferred, since it's so incredibly more efficient. I also considered hardlinking all files in store.db into a separate tree, but I didn't like the idea of coding that in C+-, :-) and I figured it might not work with other db backends, and maybe even not be guaranteed to work with leveldb. It's probably not worth much more effort.

Quoted 6 lines

This looks pretty reasonable!

I think we definitely need to limit the size of the transaction when doing the snap. The attached patch seems to try to do it all in one go, which is not going to work for large clusters. Either re-use an existing tunable like the sync chunk size or add a new one?

Ok, I changed it so that it reuses the same tunable used for mon sync transaction sizes. Tested on top of 0.87 (flawless upgrade, whee!), with both btrfs snapshots and leveldb copying. I noticed slight differences between the databases at each mon with the leveldb copying, presumably having to do with at least one round of monitor-internal retrying as the first copy in each mon took too long, but each copy appeared to be consistent, and their aggregate is presumably usable to get the cluster rolled back to an earlier consistent state.

I wish there was a better way to avoid the retry; though. I'm thinking maybe not performing the copy when the snapshot dir already exists (like we do for osd snapshots), but I'm not sure this would guarantee a consistent monitor quorum snapshot. But then, I'm not sure the current

approach does either. Plus, if we were to fail because the dir already existed, we'd need to somehow collect the success/fail status of the first try so that we don't misreport a failure just because the internal retry failed, and then, we'd need to distinguish an internal retry that ought to pass if the first attempt passed from a user-commanded attempt to created a new snapshot with the same name, which should ideally fail if any cluster component fails to take the snapshot. Not that we currently send any success/failure status back...

If we stick with re-copying (rather than dropping the request on the floor if the dir exists), I might improve the overwriting from ?remove all entries, then add them all back? to ?compare entries in source and destination, and copy only those that changed?. I have code to do just that, that I could borrow from a leveldbcmp programlet I wrote, but it would take some significant rewriting to refit it into the KeyValueDB interface, so I didn't jump through this hoop. Let me know if it is desirable, and I'll try to schedule some time to work on it.

Another issue I'm somewhat unhappy about is that, while we perform the copying (which can take a few tens of seconds), the cluster comes to a halt because the mons won't make progress. I wish we could do this copying in background, but I have no clue as to how to go about making the operation proceed asynchronously while returning a success at the end, rather than, say, because we successfully *started* the copying. I could use a clue or two to do better than that ;-)

Another nit I'm slightly unhappy about is that the command is still ?ceph osd cluster_snap <tag>?, whereas it now snapshots mons too, and it creates directories named clustersnap_<tag>, without the underscore separating cluster and snap as in the command. I'd like to spell them out the same. Any preferences?

Here's the patch I've tested, that currently builds upon the other patch upthread, that reinstates osd snapshotting.