New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librbd: support to list snapshot time stamp #12817

Merged
merged 4 commits into from Jan 30, 2017

Conversation

Projects
None yet
2 participants
@liupan1111
Contributor

liupan1111 commented Jan 7, 2017

When use "rbd snap ls XXX" command, it will list snap info, such as: snap_id, snap_name, and snap_size. But for a snapshot, the time when it is created is very important info, and the user could refer to this time stamp to mount/umount/delete this snapshot.

Fixes: http://tracker.ceph.com/issues/808

@@ -85,11 +86,12 @@ struct cls_rbd_snap {
::encode(parent, bl);
::encode(protection_status, bl);
::encode(flags, bl);
::encode(stamp, bl);

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

All new fields need to go at the end of the structure

This comment has been minimized.

@liupan1111

liupan1111 Jan 9, 2017

Contributor

agree.

@@ -4742,6 +4770,7 @@ CLS_INIT(rbd)
cls_method_handle_t h_get_data_pool;
cls_method_handle_t h_get_snapshot_name;
cls_method_handle_t h_get_snapshot_namespace;
cls_method_handle_t h_get_time_stamp;

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

This is very generically named -- perhaps h_snapshot_get_timestamp

This comment has been minimized.

@liupan1111

liupan1111 Jan 9, 2017

Contributor

agree.

@@ -4829,6 +4858,9 @@ CLS_INIT(rbd)
cls_register_cxx_method(h_class, "get_snapshot_name",
CLS_METHOD_RD,
get_snapshot_name, &h_get_snapshot_name);
cls_register_cxx_method(h_class, "get_snapshot_time_stamp",

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

Nit: "timestamp" is one word, so no underscore between "time" and "stamp"

This comment has been minimized.

@liupan1111

liupan1111 Jan 9, 2017

Contributor

agree.

@@ -537,6 +537,8 @@ namespace librbd {
op->exec("rbd", "get_parent", bl3);
::encode(snap_id, bl4);
op->exec("rbd", "get_protection_status", bl4);
::encode(snap_id, bl5);

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

Since this is a new function -- you need to query it as a separate step. Otherwise, newer librbds talking to older OSDs would fail the entire transaction.

@@ -23,6 +23,7 @@
#include "../rados/buffer.h"
#include "../rados/librados.hpp"
#include "librbd.h"
#include "../utime.h"

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

This is not a public API header

@@ -38,6 +39,7 @@ namespace librbd {
uint64_t id;
uint64_t size;
std::string name;
utime_t stamp;

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

This would break the ABI of the existing interface since you've increased the size of the structure. Clients compiled against an older librbd but linked against the newer ones would experience memory corruption.

This comment has been minimized.

@liupan1111

liupan1111 Jan 9, 2017

Contributor

Right... so could you give me some suggestion? I still believe the timestamp of a snapshot is very import to be listed.

This comment has been minimized.

@dillaman

dillaman Jan 10, 2017

Contributor

The only real solution would be to create a new snap_get_timestamp API method to retrieve the timestamp by snapshot name.

This comment has been minimized.

@liupan1111

liupan1111 Jan 10, 2017

Contributor

yes... So in this way, a new struct should also be made,which contains snapshot id, and timestamp. Is it ok for you?

This comment has been minimized.

@dillaman

dillaman Jan 10, 2017

Contributor

I think that is overkill at this point -- when we eventually create a new librbd v2 API that is not backwards compatible, we can look at updating the structs. You just need an API method that returns the timestamp (if populated) via an out param pointer -- if the snapshot doesn't have a timestamp, it should return -ENOENT.

This comment has been minimized.

@liupan1111

liupan1111 Jan 11, 2017

Contributor

agree. I will modify it. Thank you!

image_ctx.add_snap(m_snap_name, m_snap_namespace, m_snap_id, m_size, m_parent_info,
RBD_PROTECTION_STATUS_UNPROTECTED, 0);
RBD_PROTECTION_STATUS_UNPROTECTED, 0, snap_time);

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

Why can't the cls_rbd method inject the timestamp -- no need to pass it from librbd

@@ -1455,6 +1455,34 @@ int get_snapshot_name(cls_method_context_t hctx, bufferlist *in, bufferlist *out
return 0;
}
int get_snapshot_time_stamp(cls_method_context_t hctx, bufferlist *in, bufferlist *out)

This comment has been minimized.

@dillaman

dillaman Jan 8, 2017

Contributor

Nit: snapshot_get_timestamp

This comment has been minimized.

@liupan1111

liupan1111 Jan 9, 2017

Contributor

agree.

This comment has been minimized.

@liupan1111

liupan1111 Jan 11, 2017

Contributor

get_snapshot_timestamp, not snapshot_get_timestamp, right?

This comment has been minimized.

@dillaman

dillaman Jan 11, 2017

Contributor

Trying to get all new functions in the "_" convention so that similar functions are grouped together. There are existing "snapshot_XYZ" functions that this should be grouped with.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 12, 2017

@dillaman , I modify a new version as your request, please help take a look. There are still some test cases have compilation error because of api changes, let's first make agreement on the main changes, and I will give a separate commit for these test cases. Thank you for your review!

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 12, 2017

retest this please

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 16, 2017

@liupan1111 There is a compile failure w/ the current code

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 19, 2017

@liupan1111 Note: you can probably reference http://tracker.ceph.com/issues/808 in your commit message

try {
::decode(snap_id, iter);
} catch (const buffer::error &err) {
return -EINVAL;

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: spacing

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

thanks.

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

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: braces around if clause

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

thanks.

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

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: braces around if clause

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

thanks.

@@ -65,6 +65,7 @@ struct cls_rbd_snap {
uint8_t protection_status;
cls_rbd_parent parent;
uint64_t flags;
utime_t timestamp;

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: default initialize to zero

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

right... missed the initialization in construction list

@@ -119,6 +119,13 @@ namespace librbd {
std::vector<uint64_t> *sizes,
std::vector<parent_info> *parents,
std::vector<uint8_t> *protection_statuses);
void snap_timestamp_list_start(librados::ObjectReadOperation *op,

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: can you rename these two methods to snapshot_timestamp_list_start and snapshot_timestamp_list_start?

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

I just follow the style of "snap_namespace_list"... But I agree, snapshot_XX is better, let me change it.

@@ -293,6 +293,7 @@ class CEPH_RBD_API Image
int snap_rename(const char *srcname, const char *dstname);
int snap_get_limit(uint64_t *limit);
int snap_set_limit(uint64_t limit);
int snap_get_timestamp(uint64_t snap_id, std::string *timestamp);

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

The API should provide either a timespec * out parameter or a double * out parameter, not a string. You also need to add this to the C and Python APIs

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

agree! string will truely puzzle the user. I prefer timespec*

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

ok, I will also add c and python api for this change.

@@ -250,7 +250,7 @@ namespace librbd {
cls::rbd::SnapshotNamespace in_snap_namespace,
librados::snap_t id,
uint64_t in_size, parent_info parent,
uint8_t protection_status, uint64_t flags);
uint8_t protection_status, uint64_t flags, utime_t timestamp = utime_t());

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: don't default the param

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

This is the issue we really need discussion... this func "add_snap" will be called in two places:

  1. for snap list command, for example, if there are 3 snapshots existed for an image, add_snap will be called one by one. And this is the case what we do in this pr.
  2. create snap command. It will also call this add_snap, to add new snap shot info into image context. However, this new added snap shot info won't be used in any place. The list command will alway fetch it from backend each time... So I add a default value for timestamp for this "create snap" case. In this way, I don't need to regolden the test cases. Another way is: don't call this add_snap when create snapshot, since new snap shot info won't be used.

What is your opinion? forgive my pool english expression...

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

I found this default value will cause compilation failure of:
MOCK_METHOD7(add_snap, void(std::string in_snap_name, librados::snap_t id,
uint64_t in_size, parent_info parent,
uint8_t protection_status, uint64_t flags, utime_t stamp));

Since I can not add default value for this stamp.

So I prefer delete the add_snap func call when create snap

This comment has been minimized.

@dillaman

dillaman Jan 23, 2017

Contributor

(2) call add_snap w/ ceph_clock_now

This comment has been minimized.

@liupan1111

liupan1111 Jan 23, 2017

Contributor

agree.

SnapInfo(std::string _name, const cls::rbd::SnapshotNamespace &_snap_namespace,
uint64_t _size, parent_info _parent,
uint8_t _protection_status, uint64_t _flags)
uint8_t _protection_status, uint64_t _flags, utime_t _timestamp = utime_t())

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Nit: don't default the param

@@ -167,6 +169,9 @@ class RefreshRequest {
void send_v2_get_snap_namespaces();
Context *handle_v2_get_snap_namespaces(int *result);
void send_v2_get_snap_timestamps();

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Please update the state diagram

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

yes, thanks.

if (f) {
f->open_object_section("snapshot");
f->dump_unsigned("id", s->id);
f->dump_string("name", s->name);
f->dump_unsigned("size", s->size);
f->dump_string("timestamp", timestamp);

This comment has been minimized.

@dillaman

dillaman Jan 19, 2017

Contributor

Make sure you aren't going to dump "0" time for old snapshots -- just output nothing when the timestamp is zeroed.

This comment has been minimized.

@liupan1111

liupan1111 Jan 22, 2017

Contributor

so do you prefer to add a "if (timestamp != utime_t)" before dump timestamp?

This comment has been minimized.

@dillaman

dillaman Jan 23, 2017

Contributor

Probably better to dump an empty string in both cases (Formatter and TableFormatter). We just don't want to see something like "1970-01-01T00:00:00Z" or "0".

This comment has been minimized.

@liupan1111

liupan1111 Jan 25, 2017

Contributor

agree.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 19, 2017

@liupan1111 Please also add unit test coverage in "test_cls_rbd.cc" and "test_librbd.cc"

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 22, 2017

@liupan1111 There is a compile failure w/ the current code

Sorry for late. Some other works... This failure is caused by test cases compilation, let me fix it. Thanks.

@liupan1111 liupan1111 added the bug fix label Jan 22, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 22, 2017

@liupan1111 Note: you can probably reference http://tracker.ceph.com/issues/808 in your commit message

Sure, done.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 25, 2017

@liupan1111 You should rebase against master and not merge master into your branch: "git rebase origin/master"

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 25, 2017

@dillaman er... this merge is caused because github reminds me there is a conflict, and I resolved it by clicking the link in github. You are right, I will remember to do this rebase next time.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 25, 2017

@dillaman, the compile is ok now, but seems there is a segfault in jenkins result... I will look into it.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 26, 2017

@dillaman Compile pass, and regression ok, please help take a look. Thanks for your patient!

@dillaman

@liupan1111 Basically down to nit comments

@@ -147,6 +147,11 @@ Context *RefreshRequest<I>::handle_v1_get_snapshots(int *result) {
m_snap_names.size(),
cls::rbd::UserSnapshotNamespace());
m_snap_timestamps = std::vector
<utime_t>(

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: you can move the template param up to the previous line and fix the indentation below

This comment has been minimized.

@liupan1111

liupan1111 Jan 26, 2017

Contributor

cool

@@ -594,18 +592,17 @@ namespace librbd {
protection_statuses);
}
void snap_timestamp_list_start(librados::ObjectReadOperation *op,
void snapshot_timestamp_list_start(librados::ObjectReadOperation *op,
const std::vector<snapid_t> &ids)

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

bufferlist bl;
::encode(snap_id, bl);
op->exec("rbd", "get_snapshot_timestamp", bl);
}
}
int snap_timestamp_list_finish(bufferlist::iterator *it,
int snapshot_timestamp_list_finish(bufferlist::iterator *it,
const std::vector<snapid_t> &ids,

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

@@ -622,19 +619,17 @@ namespace librbd {
return 0;
}
void snap_namespace_list_start(librados::ObjectReadOperation *op,
void snapshot_namespace_list_start(librados::ObjectReadOperation *op,
const std::vector<snapid_t> &ids)

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

}
}
int snap_namespace_list_finish(bufferlist::iterator *it,
int snapshot_namespace_list_finish(bufferlist::iterator *it,
const std::vector<snapid_t> &ids,

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

cls::rbd::SnapshotNamespace in_snap_namespace,
librados::snap_t id,
uint64_t in_size, parent_info parent,
uint8_t protection_status, uint64_t flags));
uint64_t in_size, parent_info parent,

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

This comment has been minimized.

@liupan1111

liupan1111 Jan 26, 2017

Contributor

thx

int snapshot_timestamp_list(librados::IoCtx *ioctx, const std::string &oid,
const std::vector<snapid_t> &ids,

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

time_t tt = timestamp.tv_sec;
char* tt_str = ctime(&tt);
string tt_str = "";
if(timestamp.tv_sec != 0) {

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

This comment has been minimized.

@liupan1111

liupan1111 Jan 26, 2017

Contributor

thx

@@ -994,6 +994,11 @@ TEST_F(TestLibRBD, TestCreateLsDeleteSnap)
int test_get_snapshot_timestamp(rbd_image_t image, uint64_t snap_id)
{
const char *c = getenv("RBD_FEATURES");

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

You should just put "REQUIRE_FORMAT_V2();``` at the top of the test case

This comment has been minimized.

@liupan1111

liupan1111 Jan 26, 2017

Contributor

agree!

@@ -198,6 +198,16 @@ class TestMockImageRefreshRequest : public TestMockFixture {
}
}
void expect_snap_timestamp_list(MockRefreshImageCtx &mock_image_ctx, int r) {
auto &expect = EXPECT_CALL(get_mock_io_ctx(mock_image_ctx.md_ctx),
exec(mock_image_ctx.header_oid, _, StrEq("rbd"), StrEq("get_snapshot_timestamp"), _, _, _));

This comment has been minimized.

@dillaman

dillaman Jan 26, 2017

Contributor

Nit: spacing

This comment has been minimized.

@liupan1111

liupan1111 Jan 26, 2017

Contributor

thanks

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 26, 2017

@liupan1111 Please squash your commits down

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 26, 2017

Please squash your commits down

Sure, thanks!

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 27, 2017

@dillaman I have fixed the spacing issues, and squash commits into three, please help take a look.

m_snap_names.size(),
cls::rbd::UserSnapshotNamespace());
m_snap_timestamps = std::vector<utime_t>(
m_snap_names.size(),

This comment has been minimized.

@dillaman

dillaman Jan 27, 2017

Contributor

Nit: move this to the previous line

This comment has been minimized.

@liupan1111

liupan1111 Jan 27, 2017

Contributor

ok

@dillaman dillaman added wip-jason-testing and removed bug fix labels Jan 27, 2017

@@ -35,18 +35,28 @@ int do_list_snaps(librbd::Image& image, Formatter *f)
t.define_column("SNAPID", TextTable::RIGHT, TextTable::RIGHT);
t.define_column("NAME", TextTable::LEFT, TextTable::LEFT);
t.define_column("SIZE", TextTable::RIGHT, TextTable::RIGHT);
t.define_column("TIMESTAMP", TextTable::RIGHT, TextTable::RIGHT);

This comment has been minimized.

@dillaman

dillaman Jan 27, 2017

Contributor

Nit: you should use TextTable::LEFT here since the value is not a number

This comment has been minimized.

@liupan1111

liupan1111 Jan 27, 2017

Contributor

agree.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 27, 2017

@dillaman

This comment has been minimized.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 27, 2017

@dillaman ok... I will fix them. Could you give me some clue about how to run these cases? I didn't run them before...

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 28, 2017

@dillaman I've fixed the python test case, and it passed in my local by using "nosetests -v test_rbd.py".

But for formatted-output.t, it makes me puzzled:

  1. I read the the log you give to me, and found the fail reason is because the output has been changed: add timestamp. And it is easy for me to add it. However, different from snap name, size... it is impossilble to compare the timestamp... which is always changed for each run ...
    2017-01-27T20:48:09.246 INFO:tasks.cram.client.0.smithi068.stdout:+ "timestamp": "Fri Jan 27 20:47:51 2017\n"

  2. I don't know how to run this case to verify, could you give me some help?

Thanks!

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 28, 2017

@liupan1111 If you look at the existing test case, there are places that use a wildcard to match any value (like a changing timestamp) [1]. The easiest way to run it is to "make install" your branch, get a cluster running, and run the cram test "cram -v -- /path/to/formatted-output.t"

[1] https://github.com/ceph/ceph/blob/master/src/test/cli-integration/rbd/formatted-output.t#L788

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 28, 2017

@dillaman I regoldened formatted-output.t, and it passed in My local run with cram. please take a look.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 30, 2017

@liupan1111 Hopefully one last nit: can you update your commit subjects to include "librbd: " or "test: " as the prefix? Thanks.

liupan1111 added some commits Jan 27, 2017

librbd: Add timestamp metadata when creating rbd snapshot
Signed-off-by: Pan Liu <pan.liu@istuary.com>
librbd: support to list snapshot time stamp
Fixes: http://tracker.ceph.com/issues/808
Signed-off-by: Pan Liu <pan.liu@istuary.com>
test: Add unit test for getting rbd snapshot timestamp info
Signed-off-by: Pan Liu <pan.liu@istuary.com>
test: regolden formatted-output.t for timestamp metadata of rbd snapshot
Signed-off-by: Pan Liu <pan.liu@istuary.com>
@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 30, 2017

@dillaman done. Please take a look. Thanks.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 30, 2017

there is testcase TestJournalTrimer failed in jenkins, But this case Seems not relate to My change. And I only amend commits messages.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 30, 2017

@dillaman dillaman merged commit f56126a into ceph:master Jan 30, 2017

2 of 3 checks passed

default Build finished.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@liupan1111 liupan1111 deleted the liupan1111:wip-list-snapshot-time branch Jan 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment