librbd: add support for snapshot namespaces #11160

VictorDenisov · 2016-09-21T05:19:24Z

No description provided.

dillaman · 2016-09-21T13:13:22Z

src/cls/rbd/cls_rbd.cc

@@ -1488,6 +1489,34 @@ int get_snapshot_name(cls_method_context_t hctx, bufferlist *in, bufferlist *out
  return 0;
 }

+int get_snapshot_namespace(cls_method_context_t hctx, bufferlist *in, bufferlist *out)


Minor: add block comment describing expected in/out params

dillaman · 2016-09-21T13:13:38Z

src/cls/rbd/cls_rbd.cc

+
+  CLS_LOG(20, "get_snapshot_namespace snap_id=%llu", (unsigned long long)snap_id);
+
+  if (snap_id == CEPH_NOSNAP)


Minor: wrap in braces

dillaman · 2016-09-21T13:13:47Z

src/cls/rbd/cls_rbd.cc

+  string snapshot_key;
+  key_from_snap_id(snap_id, &snapshot_key);
+  int r = read_key(hctx, snapshot_key, &snap);
+  if (r < 0)


Minor: same here

dillaman · 2016-09-21T13:15:24Z

src/cls/rbd/cls_rbd.cc

+    return -EINVAL;
+  }
+
+  CLS_LOG(20, "get_snapshot_namespace snap_id=%llu", (unsigned long long)snap_id);


Minor: you can use PRIu64 to directly encode snap_id

dillaman · 2016-09-21T13:18:00Z

src/cls/rbd/cls_rbd_client.cc

-        ::encode(snap_id, bl2);
-        op->exec("rbd", "get_size", bl2);
+	::encode(snap_id, bl2);
+	op->exec("rbd", "get_snapshot_namespace", bl2);


Since this cls method won't exist on older OSDs, unfortunately, until we get to the point of only supporting Kraken+, you need to retrieve the snapshot namespaces as part of an individual OSD op in case it errors out with -EOPNOTSUPP (which you could then ignore)

I see. Too bad I didn't think about it.

dillaman · 2016-09-21T13:19:14Z

src/cls/rbd/cls_rbd_types.cc

+
+std::ostream& operator<<(std::ostream& os, const GroupSnapshotNamespace& ns) {
+  os << "[group]";
+  return os;


Minor: dump fields?

dillaman · 2016-09-21T13:21:18Z

src/cls/rbd/cls_rbd_types.h

+  string snapshot_id;
+
+  void encode(bufferlist& bl) const {
+    ::encode(group_pool, bl);


Minor: move the implementation into the cc file

dillaman · 2016-09-21T13:23:26Z

src/cls/rbd/cls_rbd_types.h

+
+inline void encode(const SnapshotNamespace &c, bufferlist &bl, uint64_t features=0)
+{
+  ENCODE_DUMP_PRE();


I don't think this is doing what you are expecting -- this for for creating the binary file dump records, which leads to the next comment ...

I'm not sure I understand this comment. Are you talking about ENCODE_DUMP_PRE(); specifically? I see that WRITE_CLASS_ENCODER is used in order to create encode and decode functions for MirrorImageStatus. WRITE_CLASS_ENCODER adds ENCODE_DUMP_PRE and ENCODE_DUMP post inside encode function it generates.

If you look at the code for ENCODE_DUMP_PRE -- most of the time it's a no-op. The only time it actually does something is within the dencoder test where it writes the binary encoding format out to a file. I think you were looking for ENCODE_START/ENCODE_FINISH which add version control.

dillaman · 2016-09-21T13:24:47Z

src/cls/rbd/cls_rbd_types.h

+
+  void encode(bufferlist& bl) const {}
+  void decode(bufferlist::iterator& it) {}
+


All the new types should have generate_test_instances and dump methods so that they can be registered with the dencoder test case.

... might be either to create a new SnapshotNamespace struct that just inherits from boost::variant. That would allow you to create the necessary generate_test_instances static member function for use with dencoder. Then you wouldn't need generate_test_instances for each individual type.

I tried to implement it using inheritance and couldn't reconcile all types of constructors that are provided by boost::variant. It looks like the solution is to have a wrapper type like NotifyMessage wraps Payload and EventEntry wraps Event type. Those wrappers implement all the required methods.

VictorDenisov · 2016-09-22T01:08:50Z

not ready yet. will update when done with addressing comments

dillaman · 2016-09-23T13:37:54Z

... all internal plumbing changes, no doc updates required

VictorDenisov · 2016-09-24T01:40:57Z

@dillaman could you please take a look if you like this version at all. In the mean time I'll look into what happened to the tests.

dillaman · 2016-09-25T20:35:36Z

src/cls/rbd/cls_rbd_types.cc

@@ -316,5 +317,138 @@ bool GroupSpec::is_valid() const {
  return (!group_id.empty()) && (pool_id != -1);
 }

+void GroupSnapshotNamespace::encode(bufferlist& bl) const {
+  ENCODE_START(1, 1, bl);


Minor: probably no need to encode another version here since it's already wrapped in a versioned struct

dillaman · 2016-09-25T20:36:12Z

src/cls/rbd/cls_rbd_types.cc

+}
+
+void SnapshotNamespaceEntry::generate_test_instances(std::list<SnapshotNamespaceEntry *> &o) {
+


Minor: append a UserSnapshotNamespace and GroupSnapshotNamespace example to the list

Should GroupSnapshotNamespace have any meaningful ids in it or any random values are enough?

Random but valid is fine -- it's to help ensure that the format isn't unintentionally broken by future changes.

dillaman · 2016-09-25T20:37:38Z

src/cls/rbd/cls_rbd_types.h

+
+};
+
+WRITE_CLASS_ENCODER(UserSnapshotNamespace);


Minor: since these will always be encoded via SnapshotNamespaceEntry, no need to create encode/decode free functions for these types.

dillaman · 2016-09-25T20:38:42Z

src/librbd/ImageCtx.h

@@ -230,6 +231,8 @@ namespace librbd {
    const SnapInfo* get_snap_info(librados::snap_t in_snap_id) const;
    int get_snap_name(librados::snap_t in_snap_id,
 		      std::string *out_snap_name) const;
+    int get_snap_namespace(librados::snap_t in_snap_id,
+			   cls::rbd::SnapshotNamespaceEntry *out_snap_namespace) const;


Minor: could directly use SnapshotNamespace variant since SnapshotNamespaceEntry is only used for encode/decode and requires an indirection. Perhaps rename SnapshotNamespaceEntry to SnapshotNamespaceOnDisk and hide its use outside of "cls/rbd"

dillaman · 2016-09-25T20:45:46Z

src/librbd/journal/Types.h

@@ -135,9 +136,10 @@ struct SnapCreateEvent : public SnapEventBase {

  SnapCreateEvent() {
  }
-  SnapCreateEvent(uint64_t op_tid, const std::string &snap_name)
-    : SnapEventBase(op_tid, snap_name) {
+  SnapCreateEvent(uint64_t op_tid, const std::string &snap_name, const cls::rbd::SnapshotNamespaceEntry &_snap_namespace)


Would there be a need to journal group snapshot create events? It seems like the group actions would be journaled via a group journal (eventually). If group snap create events are recorded with the image, they wouldn't be properly associated with a group and you wouldn't be able to remove them (perhaps). Therefore, perhaps this should wait until after group actions can be journaled and replayed.

but then I don't have any info about snapshots namespace and in case of replay I can't recreate the snapshot with required namespace. So, far all it does - keeps this information. Nobody in the code creates namespaces other than UserSnapshotNamespace. If I remove this then all replayed snapshots should be in UserSnapshotNamespace by default.

dillaman · 2016-10-12T20:23:45Z

src/librbd/image/RefreshRequest.cc

@@ -140,6 +140,11 @@ Context *RefreshRequest<I>::handle_v1_get_snapshots(int *result) {
    return m_on_finish;
  }

+  int n = m_snap_names.size();


Minor: could just use m_snap_namespaces = {m_snap_names.size(), cls::rbd::UserSnapshotNamespace()};

dillaman · 2016-10-12T20:24:17Z

src/librbd/image/RefreshRequest.cc

+    send_v2_get_mutable_metadata();
+    return nullptr;
+  } else if (*result == -EOPNOTSUPP) {
+    m_snap_namespaces.resize(m_snapc.snaps.size());


Minor: default to cls::rbd::UserSnapshotNamespace?

dillaman · 2016-10-12T20:33:09Z

src/tools/rbd_mirror/image_sync/SnapshotCopyRequest.cc

@@ -354,8 +368,23 @@ void SnapshotCopyRequest<I>::send_snap_protect() {
    librados::snap_t remote_snap_id = *snap_id_it;

    m_remote_image_ctx->snap_lock.get_read();
+


Minor: I don't think you'll need this for the unprotect/protect states since only user snapshots should be protected -- but I think you need to skip over non-user snapshots in the remove state machine for now.

VictorDenisov · 2016-10-14T00:12:03Z

@dillaman done. it looks like those test failures are unrelated. every time I push different tests sigfault.

dillaman · 2016-10-17T14:57:24Z

src/librbd/WatchNotifyTypes.cc

@@ -239,6 +240,26 @@ void SnapPayloadBase::dump(Formatter *f) const {
  f->dump_string("snap_name", snap_name);
 }

+void SnapCreatePayload::encode(bufferlist &bl) const {
+  ::encode(cls::rbd::SnapshotNamespaceOnDisk(snap_namespace), bl);


All additions need to be added to the end of the structure. Move the encode/decode of the namespace to the end of the function

VictorDenisov · 2016-10-17T17:45:25Z

@dillaman fixed.

dillaman · 2016-10-19T14:07:01Z

@VictorDenisov TestClsRbd failed: http://qa-proxy.ceph.com/teuthology/jdillaman-2016-10-18_21:31:07-rbd-wip-jd-testing---basic-smithi/483908/teuthology.log

VictorDenisov · 2016-10-19T19:58:38Z

ok. looking into it.

dillaman · 2016-10-20T00:49:05Z

src/cls/rbd/cls_rbd.cc

@@ -1678,7 +1678,8 @@ int snapshot_add(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
 	        (unsigned long long)snap_id.val);
 	return -EIO;
      }
-      if (snap_meta.name == old_meta.name || snap_meta.id == old_meta.id) {
+      if ((snap_meta.name == old_meta.name || snap_meta.id == old_meta.id)


The snap id should be unique regardless of the namespace -- so the check should be:

if (snap_meta.id == old_meta.id || (snap_meta.name == old_meta.name && snap_meta.snapshot_namespace == old_meta.snapshot_namespace)) {

VictorDenisov · 2016-10-20T19:08:47Z

done

dillaman · 2016-10-21T12:40:29Z

src/cls/rbd/cls_rbd.cc

@@ -1678,7 +1678,9 @@ int snapshot_add(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
 	        (unsigned long long)snap_id.val);
 	return -EIO;
      }
-      if (snap_meta.name == old_meta.name || snap_meta.id == old_meta.id) {
+      if ((snap_meta.name == old_meta.name
+	    && snap_meta.snapshot_namespace == old_meta.snapshot_namespace)


Minor: the conditionals should go at the end of the line and not the start of a new line:

if ((snap_meta.name == old_meta.name && snap_meta.snapshot_namespace == old_meta.snapshot_namespace) || snap_meta.id == old_meta.id) {

oops. sorry. too much haskell before this commit. fixed.

Signed-off-by: Victor Denisov <denisovenator@gmail.com>

VictorDenisov force-pushed the consistency_groups_namespaces branch from e99bcfd to 821794e Compare September 21, 2016 05:21

dillaman added feature rbd labels Sep 21, 2016

dillaman reviewed Sep 21, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch 2 times, most recently from 09e2514 to db5bf5d Compare September 22, 2016 01:07

dmick added the needs-doc label Sep 22, 2016

VictorDenisov force-pushed the consistency_groups_namespaces branch from 82e4fe1 to c32e3a5 Compare September 23, 2016 00:40

dillaman removed the needs-doc label Sep 23, 2016

VictorDenisov force-pushed the consistency_groups_namespaces branch 2 times, most recently from 4ab1798 to 0b7f89f Compare September 24, 2016 01:01

VictorDenisov force-pushed the consistency_groups_namespaces branch 3 times, most recently from 81ffdb0 to 142d4fc Compare September 25, 2016 03:52

dillaman reviewed Sep 25, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch from ac014b1 to f964026 Compare October 12, 2016 20:22

dillaman reviewed Oct 12, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch 5 times, most recently from db1b6c0 to c800c9c Compare October 13, 2016 23:35

VictorDenisov force-pushed the consistency_groups_namespaces branch from c800c9c to 34d0e90 Compare October 14, 2016 16:57

dillaman reviewed Oct 17, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch from 34d0e90 to 6689573 Compare October 17, 2016 16:52

dillaman added the wip-jason-testing label Oct 17, 2016

dillaman reviewed Oct 20, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch from e3dd0c7 to ca82f88 Compare October 20, 2016 07:16

dillaman reviewed Oct 21, 2016

View reviewed changes

VictorDenisov force-pushed the consistency_groups_namespaces branch 2 times, most recently from ccf117b to 7d4350b Compare October 21, 2016 21:46

VictorDenisov added 4 commits October 21, 2016 20:53

cls_rbd: Add snapshot namespaces data structures

ca31b7a

Signed-off-by: Victor Denisov <denisovenator@gmail.com>

librbd: By default create snapshots in UserNamespace

2174751

Signed-off-by: Victor Denisov <denisovenator@gmail.com>

rbd: Use user snapshot namespace in rbd_mirror for now

6788d93

Signed-off-by: Victor Denisov <denisovenator@gmail.com>

cls_rbd: Allow snapshots with the same name in different namespaces

dfc6316

Signed-off-by: Victor Denisov <denisovenator@gmail.com>

VictorDenisov force-pushed the consistency_groups_namespaces branch from 7d4350b to dfc6316 Compare October 22, 2016 03:53

dillaman changed the title ~~librbd: Consistency groups, snapshot namespaces~~ librbd: add support for snapshot namespaces Oct 24, 2016

dillaman merged commit e7da810 into ceph:master Oct 24, 2016


		CLS_LOG(20, "get_snapshot_namespace snap_id=%llu", (unsigned long long)snap_id);

		if (snap_id == CEPH_NOSNAP)


		void encode(bufferlist& bl) const {}
		void decode(bufferlist::iterator& it) {}

		}

		void SnapshotNamespaceEntry::generate_test_instances(std::list<SnapshotNamespaceEntry *> &o) {

		@@ -354,8 +368,23 @@ void SnapshotCopyRequest<I>::send_snap_protect() {
		librados::snap_t remote_snap_id = *snap_id_it;

		m_remote_image_ctx->snap_lock.get_read();


		};

		WRITE_CLASS_ENCODER(UserSnapshotNamespace);

librbd: add support for snapshot namespaces #11160

librbd: add support for snapshot namespaces #11160

Conversation

VictorDenisov commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dillaman Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorDenisov commented Sep 22, 2016

dillaman commented Sep 23, 2016

VictorDenisov commented Sep 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dillaman Sep 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorDenisov commented Oct 14, 2016

dillaman Oct 17, 2016 • edited

Choose a reason for hiding this comment

VictorDenisov commented Oct 17, 2016

dillaman commented Oct 19, 2016

VictorDenisov commented Oct 19, 2016

Choose a reason for hiding this comment

VictorDenisov commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dillaman Sep 21, 2016 •

edited

dillaman Sep 25, 2016 •

edited

dillaman Oct 17, 2016 •

edited