Skip to content
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

osd: compact osd feature #16045

Merged
merged 3 commits into from Jul 15, 2017

Conversation

Projects
None yet
5 participants
@Liuchang0812
Copy link
Contributor

Liuchang0812 commented Jun 30, 2017

usage:

➜  build git:(wip-compact-osd-feature) ✗ ./bin/ceph daemon osd.2 compact
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
{
    "elapsed_time": "0.021056"
}

compact logging


2017-06-30 21:32:43.842428 7f0f3af7e700  4 rocksdb: [/home/liuchang/WorkSpace/ceph/src/rocksdb/db/db_impl_compaction_flush.cc:839] [default] Manual compaction starting
2017-06-30 21:32:43.842724 7f0f1cf3a700  3 rocksdb: [/home/liuchang/WorkSpace/ceph/src/rocksdb/db/db_impl.cc:447] ------- DUMPING STATS -------
2017-06-30 21:32:43.842733 7f0f1cf3a700  3 rocksdb: [/home/liuchang/WorkSpace/ceph/src/rocksdb/db/db_impl.cc:448]

todo list:

  • daemon command to compact
  • mon command to compact
  • support BlueStore
  • some test script
  • squash commits

@liewegas liewegas added the core label Jun 30, 2017

@@ -3207,12 +3223,15 @@ int OSD::shutdown()
cct->get_admin_socket()->unregister_command("dump_watchers");
cct->get_admin_socket()->unregister_command("dump_reservations");
cct->get_admin_socket()->unregister_command("get_latest_osdmap");
cct->get_admin_socket()->unregister_command("head");

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jun 30, 2017

Author Contributor

todo: it should be heap

@varadakari

This comment has been minimized.

Copy link
Contributor

varadakari commented Jul 1, 2017

Could you please compact all compact changes into one? There seem to be more commits for each line change, You could make them to couple of commits like all objectstore interface stuff into one commit and admin socket changes into one.

@varadakari

This comment has been minimized.

Copy link
Contributor

varadakari commented Jul 1, 2017

You can make adding unregister changes in one and cleanup and alignment into one more. Please clean them repush the changes.

@varadakari

This comment has been minimized.

Copy link
Contributor

varadakari commented Jul 1, 2017

Made changes only to filestore? Could please add the same interface to rest of the stores?

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 1, 2017

@varadakari thanks for reviewing, I will address what you comments.

Made changes only to filestore? Could please add the same interface to rest of the stores?

I will add the same interface to BlueStore & KVStore.

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch 2 times, most recently from ebf5d66 to c28eb8c Jul 2, 2017

@Liuchang0812 Liuchang0812 changed the title [DNM]Wip compact osd feature OSD: compact osd feature Jul 3, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 3, 2017

@varadakari I have squashed some commits. how about now? Would you mind taking a review? appreciated!

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 3, 2017

jenkins test this please

@@ -4336,6 +4336,11 @@ void FileStore::inject_mdata_error(const ghobject_t &oid) {
dout(10) << __FUNC__ << ": init error on " << oid << dendl;
mdata_error_set.insert(oid);
}

void FileStore::compact() {

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

You merge this to header file.

@@ -142,6 +142,12 @@ int DBObjectMap::check(std::ostream &out, bool repair)
return errors;
}

void DBObjectMap::compact()
{

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

You can merge this to header file. Just one line change.

@@ -3213,6 +3229,8 @@ int OSD::shutdown()
cct->get_admin_socket()->unregister_command("calc_objectstore_db_histogram");
cct->get_admin_socket()->unregister_command("flush_store_cache");
cct->get_admin_socket()->unregister_command("dump_pgstate_history");
//TODO: some commands is not unregistered

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

Could you please remove this comment?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

@Liuchang0812 this comment is not addressed.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

oh..sorry...i removed this in another commit[1093f82]...I will squash that commit.

} else if (admin_command == "compact") {
dout(1) << "triggering manual compaction" << dendl;
utime_t start = ceph_clock_now();
store->compact();

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

Is this a synchronous operation or async? If it is a sync operation, what is the impact to the front end IO's? If there is some impact, then the users should know about it as warning. Did you try this a live osd when writes are in progress?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 3, 2017

Author Contributor

It's a synchronous operation. leveldb/rocksdb's compaction has some impacts to user's request. Giving a warning to user is a better solution. how about :

 WARNING:  compaction possibly slows your requests.  

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

Yeah that looks good. Not sure if that results in a timeout on the application end, may be we need to add(if possible) saying you might observe timeouts, if we have a lot to compact on the backend.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 4, 2017

Author Contributor

@varadakari actually leveldb/rocksdb will compact automatically in background. maybe we should clarify it in documentation? I have added warn message in command description now.

@@ -3242,6 +3243,8 @@ int OSD::shutdown()
cct->get_admin_socket()->unregister_command("injectdataerr");
cct->get_admin_socket()->unregister_command("injectmdataerr");
cct->get_admin_socket()->unregister_command("set_recovery_delay");
cct->get_admin_socket()->unregister_command("trigger_scrub");
cct->get_admin_socket()->unregister_command("injectfull");

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

This commit looks good.

@@ -2381,6 +2381,11 @@ class BlueStore : public ObjectStore,
RWLock::WLocker l(debug_read_error_lock);
debug_mdata_error_objects.insert(o);
}
void compact() override {
assert(db);

This comment has been minimized.

Copy link
@varadakari

varadakari Jul 3, 2017

Contributor

Need to see the impact to front end IO's when compaction is progress, from mark's runs we have seen a slowdown in IO's when compaction is in progress. It is better to warn users when issuing the command. We shouldn't be seeing any timeouts because of this invocation.

@tchaikov tchaikov added the feature label Jul 3, 2017

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from c28eb8c to 0ca1ee2 Jul 4, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 4, 2017

jenkins test this please

1 similar comment
@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 4, 2017

jenkins test this please

@Liuchang0812 Liuchang0812 referenced this pull request Jul 4, 2017

Merged

mon, osd: misc fixes #16078

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 4, 2017

@varadakari updated and repushed.

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 6, 2017

@tchaikov tchaikov self-requested a review Jul 6, 2017

@liewegas liewegas changed the title OSD: compact osd feature osd: compact osd feature Jul 10, 2017

dout(1) << "triggering manual compaction" << dendl;
utime_t start = ceph_clock_now();
store->compact();
utime_t end = ceph_clock_now();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

might want to use ceph::coarse_mono_clock instead, as we are switching to the chrono library for timing.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

cool...I will do it

f->dump_stream("elapsed_time") << end;
f->close_section();
}
else {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

move else { into the previous line.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

Fixed!

"name=pgid,type=CephString ",
test_ops_hook,
"Trigger a scheduled scrub ");
"trigger_scrub",

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

could you drop the formatting-only change?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@@ -3213,6 +3229,8 @@ int OSD::shutdown()
cct->get_admin_socket()->unregister_command("calc_objectstore_db_histogram");
cct->get_admin_socket()->unregister_command("flush_store_cache");
cct->get_admin_socket()->unregister_command("dump_pgstate_history");
//TODO: some commands is not unregistered

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

@Liuchang0812 this comment is not addressed.

utime_t end = ceph_clock_now();
end -= start;
dout(1) << "finished manual compaction in " << end << " seconds" << dendl;
ss << "compacted omap in" << end << " seconds";

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

add a space after "in"

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@@ -6205,7 +6205,7 @@ COMMAND("dump_pg_recovery_stats", "dump pg recovery statistics",
"osd", "r", "cli,rest")
COMMAND("reset_pg_recovery_stats", "reset pg recovery statistics",
"osd", "rw", "cli,rest")
COMMAND("compact", "compact omap",
COMMAND("compact", "compact object store's omap. WARNING: Compaction probably slows your requests",

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

better off squashing this commit into the one which introduces this command. and please wrap at 79 chars.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done, I have squashed this commit.

@@ -2364,6 +2364,12 @@ function test_osd_tell_help_command()
expect_false ceph tell osd.100 help
}

function test_osd_command()

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 11, 2017

Contributor

s/test_osd_command/test_osd_compact/

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from 0ca1ee2 to e6683a9 Jul 12, 2017

@Liuchang0812
Copy link
Contributor Author

Liuchang0812 left a comment

updated and repushed

dout(1) << "triggering manual compaction" << dendl;
utime_t start = ceph_clock_now();
store->compact();
utime_t end = ceph_clock_now();

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

cool...I will do it

f->dump_stream("elapsed_time") << end;
f->close_section();
}
else {

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

Fixed!

"name=pgid,type=CephString ",
test_ops_hook,
"Trigger a scheduled scrub ");
"trigger_scrub",

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@@ -3213,6 +3229,8 @@ int OSD::shutdown()
cct->get_admin_socket()->unregister_command("calc_objectstore_db_histogram");
cct->get_admin_socket()->unregister_command("flush_store_cache");
cct->get_admin_socket()->unregister_command("dump_pgstate_history");
//TODO: some commands is not unregistered

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

oh..sorry...i removed this in another commit[1093f82]...I will squash that commit.

utime_t end = ceph_clock_now();
end -= start;
dout(1) << "finished manual compaction in " << end << " seconds" << dendl;
ss << "compacted omap in" << end << " seconds";

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@@ -2364,6 +2364,12 @@ function test_osd_tell_help_command()
expect_false ceph tell osd.100 help
}

function test_osd_command()

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done

@@ -6205,7 +6205,7 @@ COMMAND("dump_pg_recovery_stats", "dump pg recovery statistics",
"osd", "r", "cli,rest")
COMMAND("reset_pg_recovery_stats", "reset pg recovery statistics",
"osd", "rw", "cli,rest")
COMMAND("compact", "compact omap",
COMMAND("compact", "compact object store's omap. WARNING: Compaction probably slows your requests",

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

done, I have squashed this commit.

store->compact();
auto end = ceph::coarse_mono_clock::now();
std::chrono::duration<double> time_span = std::chrono::duration_cast<std::chrono::duration<double>>(end - start);
dout(1) << "finished manual compaction in " << time_span.count()<< " seconds" << dendl;

This comment has been minimized.

Copy link
@xiexingguo

xiexingguo Jul 12, 2017

Member

nit: space before <<

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

thank you xingguo!

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from e6683a9 to b0fdc19 Jul 12, 2017

@@ -160,6 +161,8 @@
#undef dout_prefix
#define dout_prefix _prefix(_dout, whoami, get_osdmap_epoch())

using namespace std::chrono;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 12, 2017

Contributor

this exposes lots of names like seconds,hours. i'd suggest just using the used types or keep using the chrono:: namespace.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 12, 2017

Author Contributor

Updated as following. furthermore, do we need to convert other ceph_now to ceph::coarse_mono_clock? I am happy to do this if needed.

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index b333b4e..c8f3485 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -161,7 +161,6 @@
 #undef dout_prefix
 #define dout_prefix _prefix(_dout, whoami, get_osdmap_epoch())
 
-using namespace std::chrono;
 
 const double OSD::OSD_TICK_INTERVAL = 1.0;
 
@@ -2213,7 +2212,7 @@ bool OSD::asok_command(string admin_command, cmdmap_t& cmdmap, string format,
     auto start = ceph::coarse_mono_clock::now();
     store->compact();
     auto end = ceph::coarse_mono_clock::now();
-    duration<double> time_span = duration_cast<duration<double>>(end - start);
+    chrono::duration<double> time_span = chrono::duration_cast<chrono::duration<double>>(end - start);
     dout(1) << "finished manual compaction in " << time_span.count() << " seconds" << dendl;
     f->open_object_section("compact_result");
     f->dump_float("elapsed_time", time_span.count());
@@ -6631,7 +6630,7 @@ void OSD::do_command(Connection *con, ceph_tid_t tid, vector<string>& cmd, buffe
     auto start = ceph::coarse_mono_clock::now();
     store->compact();
     auto end = ceph::coarse_mono_clock::now();
-    duration<double> time_span = duration_cast<duration<double>>(end - start);
+    chrono::duration<double> time_span = chrono::duration_cast<chrono::duration<double>>(end - start);
     dout(1) << "finished manual compaction in " << time_span.count() << " seconds" << dendl;
     ss << "compacted omap in " << time_span.count() << " seconds";
   }

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from b0fdc19 to e53dfc6 Jul 12, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jul 13, 2017

2017-07-12 14:25:06.004370 7f4da08d8700 -1 mgr init Error loading module 'restful': (2) No such file or directory

jenkins test this please

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from e53dfc6 to ada9103 Jul 13, 2017

@tchaikov
Copy link
Contributor

tchaikov left a comment

aside from the nits, lgtm

store->compact();
auto end = ceph::coarse_mono_clock::now();
chrono::duration<double> time_span = chrono::duration_cast<chrono::duration<double>>(end - start);
dout(1) << "finished manual compaction in " << time_span.count() << " seconds" << dendl;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 13, 2017

Contributor

wrap at char 79.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 13, 2017

Author Contributor

done

auto start = ceph::coarse_mono_clock::now();
store->compact();
auto end = ceph::coarse_mono_clock::now();
chrono::duration<double> time_span = chrono::duration_cast<chrono::duration<double>>(end - start);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Jul 13, 2017

Contributor

use auto if you please.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jul 13, 2017

Author Contributor

done

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from ada9103 to 1d1d085 Jul 13, 2017

@tchaikov tchaikov added the needs-qa label Jul 13, 2017

Liuchang0812 added some commits Jun 30, 2017

os: export compact interface in ObjectStore and ObjectMap
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
osd: new command compact via tell/daemon
user could manual compact OSD's omap as following:
1. ceph tell osd.id compact
2. ceph daemon osd.id compact
user's requests will be impacted during compaction.

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

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
test: add test of ceph compact command
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-compact-osd-feature branch from 1d1d085 to d1f24d0 Jul 13, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 13, 2017

retest this please. (jenkins slave issue)

@tchaikov

This comment has been minimized.

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jul 14, 2017

@varadakari is your concern addressed?

@liewegas shall we have this change in luminous? IIRC, i spotted cases where omap leveldb failed to compact itself before once in jewel.

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Jul 14, 2017

looks ok to me!

@tchaikov tchaikov merged commit 0cc6519 into ceph:master Jul 15, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@Liuchang0812 Liuchang0812 deleted the Liuchang0812:wip-compact-osd-feature branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.