rbd: add data pool support to trash purge #21247

MahatiC · 2018-04-04T15:37:58Z

Checks implicitly for datapool utilization of all images and removes
respective pool's trashed images until threshold value is hit.

Fixes: http://tracker.ceph.com/issues/22872
Signed-off-by: Mahati Chamarthy mahati.chamarthy@intel.com

batrick · 2018-04-04T19:55:17Z

retest this please

MahatiC · 2018-04-05T09:15:16Z

retest this please

MahatiC · 2018-04-06T03:52:09Z

retest this please
hmm, make check shows failure in run-cli-tests. But that test passes in my env though.

tchaikov · 2018-04-07T09:46:13Z

retest this please

tchaikov · 2018-04-07T09:50:36Z

src/test/cli/rbd/help.t

@@ -1624,6 +1624,7 @@
  usage: rbd trash purge [--pool <pool>] [--no-progress] 
                         [--expired-before <expired-before>] 
                         [--threshold <threshold>] 
+                         [--data-pool <data-pool>]


/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/cli/rbd/help.t: failed --- /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/cli/rbd/help.t +++ /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/cli/rbd/help.t.err @@ -1626,8 +1626,7 @@ rbd help trash purge usage: rbd trash purge [--pool <pool>] [--no-progress] [--expired-before <expired-before>] - [--threshold <threshold>] - [--data-pool <data-pool>] + [--threshold <threshold>] [--data-pool <data-pool>] <pool-name> Remove all expired images from trash.

you might need to move "[--data-pool <data-pool>" up to the previous line.

dillaman

I would think it should just be implicit. It could loop through all the trashed images whose deferment end time is in the past and compute a set of (data) pools in-use by the images. For each pool in the set, delete trashed images in that (data) pool until that pool's threshold is met.

MahatiC · 2018-04-10T18:26:28Z

@dillaman ack. I wasn't alerted with Ceph tracker comment.

MahatiC · 2018-04-19T04:37:59Z

@dillaman rbd trash purge seems to accept only one pool. And will only have access to trashed images (whose metadata is stored) in that particular pool. For example:

rbd ls rbdpool
image1, image2, image3

datapool1 has {image1, otherimage1}
datapool2 has {image2, otherimage2, otherimage3}

rbd trash mv rbdpool/image1
rbd trash mv rbdpool/image2

On excuting: rbd trash purge rbdpool --threshold '0.02'

do you mean purge trashed images in rbdpool until datapool1 & datapool2 both have used % of 2? In the above example datapool1 & datapool2 have images from a pool other than rbdpool. Not sure I understand when you say remove images from data pool.

dillaman · 2018-04-19T12:22:03Z

@MahatiC

rbd trash purge seems to accept only one pool. And will only have access to trashed images (whose metadata is stored) in that particular pool. For example:

Yup -- the pool where the image headers are stored.

do you mean purge trashed images in rbdpool until datapool1 & datapool2 both have used % of 2?

Yes

In the above example datapool1 & datapool2 have images from a pool other than rbdpool. Not sure I understand when you say remove images from data pool.

Remove images associated w/ datapool1 / datapool2 as necessary from "rbdpool" until you hit the threshold.

MahatiC · 2018-04-27T03:31:08Z

@dillaman review please

dillaman · 2018-05-02T16:22:26Z

src/tools/rbd/action/Trash.cc

+          data_pool = "<missing data pool " + stringify(data_pool_id) + ">";
+        } else {
+          data_pool = data_io_ctx.get_pool_name();
+          datapools[data_pool];


Nit: unnecessary

dillaman · 2018-05-02T16:22:57Z

src/tools/rbd/action/Trash.cc

-                           (pool_percent_used - threshold));
-
-    librbd::Image curr_img;
+    std::string data_pool;


Nit: move to tightest scope

dillaman · 2018-05-02T16:23:17Z

src/tools/rbd/action/Trash.cc

+        librados::IoCtx data_io_ctx;
+        r = rados.ioctx_create2(data_pool_id, data_io_ctx);
+        if (r < 0) {
+          data_pool = "<missing data pool " + stringify(data_pool_id) + ">";


Nit: unnecessary

dillaman · 2018-05-02T16:25:00Z

src/tools/rbd/action/Trash.cc

+        }
+      }
+    }
+    datapools[pool_name]; 


Nit: awkward -- might be clearer if you use datapools[pool_name] = {} or datapools.insert({pool_name}, {}})

dillaman · 2018-05-02T16:25:48Z

src/tools/rbd/action/Trash.cc

+      }
+    }
+    datapools[pool_name]; 
+    uint64_t bytes_to_free;


Nit: initialize your variable

dillaman · 2018-05-02T16:26:11Z

src/tools/rbd/action/Trash.cc

+    for(uint8_t i = 0; i < arr.size(); ++i) {
+      json_spirit::mObject obj = arr[i].get_obj();
+      std::string name = obj.find("name")->second.get_str();
+      std::map<std::string, std::vector<std::string>>::iterator img = datapools.find(name);


Nit: auto img = ...

dillaman · 2018-05-02T16:56:42Z

src/tools/rbd/action/Trash.cc

+
+        if(arr[i].get_obj()["name"] == pool_name){
+          librbd::Image curr_img;
+          for (const auto& entry : trash_entries) {


Seems to be duplicating the code below. If you added the trash image ids for the base pool to datapools[pool_name] above, you could then just iterate through the image ids in the datapools[arr[i].get_obj()["name"]] to compute which images to remove.

MahatiC · 2018-05-09T07:27:27Z

@dillaman Thanks, updated.

since it compiles now

dillaman · 2018-05-10T19:31:27Z

src/tools/rbd/action/Trash.cc

+        data_pool = data_io_ctx.get_pool_name();
+        datapools[data_pool].push_back(entry.id.c_str());
+      }
+      datapools[pool_name].push_back(entry.id.c_str());


On second thought -- I guess my suggestion would double-count an image's space in both its data pool and base pool. Therefore, the disk usage check for determining which images to delete should only be made in the data pool. Therefore, perhaps this should just be the else branch.

Okay. With your prev suggestion I reasoned that technically all images belong to the base pool anyway and should be fine if we delete (datapool) images from basepool (where datapool's threshold is what is required and basepool is higher). But not double-counting makes it easier to differentiate and the (trash purge) behavior more predictable. Will modify.

dillaman · 2018-05-10T19:32:11Z

src/tools/rbd/action/Trash.cc

+      if (data_pool_id != io_ctx.get_id()) {
+        librados::Rados rados(io_ctx);
+        librados::IoCtx data_io_ctx;
+        r = rados.ioctx_create2(data_pool_id, data_io_ctx);


Nit: log error message to std::cerr and skip image if ```r < 0````

dillaman · 2018-05-15T13:21:50Z

src/tools/rbd/action/Trash.cc

+        data_pool = data_io_ctx.get_pool_name();
+        datapools[data_pool].push_back(entry.id.c_str());
+      }
+      else {


Nit: } else { on same line

dillaman · 2018-05-15T13:21:57Z

src/tools/rbd/action/Trash.cc

+        datapools[data_pool].push_back(entry.id.c_str());
+      }
+      else {
+      datapools[pool_name].push_back(entry.id.c_str());


Nit: indentation

checks implicitly for datapool utilization of all images and removes respective pool's trashed images until threshold value is hit. Fixes: http://tracker.ceph.com/issues/22872 Signed-off-by: Mahati Chamarthy <mahati.chamarthy@intel.com>

dillaman

@MahatiC Can you add "Fixes: http://tracker.ceph.com/issues/22872" to your commit message?

dillaman added feature rbd labels Apr 4, 2018

tchaikov previously requested changes Apr 7, 2018

View reviewed changes

MahatiC force-pushed the wip-rbd-datapool branch 2 times, most recently from 2b93a77 to 9c48700 Compare April 9, 2018 10:17

dillaman reviewed Apr 9, 2018

View reviewed changes

MahatiC force-pushed the wip-rbd-datapool branch 4 times, most recently from 1b3da49 to 6be4f7f Compare April 26, 2018 16:31

dillaman changed the title ~~rbd trash: add data pool support~~ rbd: add data pool support to trash purge May 2, 2018

dillaman reviewed May 2, 2018

View reviewed changes

MahatiC force-pushed the wip-rbd-datapool branch from 6be4f7f to 41b457f Compare May 9, 2018 04:30

dillaman reviewed May 10, 2018

View reviewed changes

MahatiC force-pushed the wip-rbd-datapool branch from 41b457f to 7e76377 Compare May 14, 2018 09:27

dillaman reviewed May 15, 2018

View reviewed changes

MahatiC force-pushed the wip-rbd-datapool branch from 7e76377 to 3fb45fa Compare May 16, 2018 05:31

rbd trash: add data pool support

00d5c18

checks implicitly for datapool utilization of all images and removes respective pool's trashed images until threshold value is hit. Fixes: http://tracker.ceph.com/issues/22872 Signed-off-by: Mahati Chamarthy <mahati.chamarthy@intel.com>

dillaman approved these changes May 16, 2018

View reviewed changes

dillaman added the needs-qa label May 16, 2018

MahatiC force-pushed the wip-rbd-datapool branch from 3fb45fa to 00d5c18 Compare May 16, 2018 13:01

dillaman added the wip-jason-testing label May 31, 2018

dillaman merged commit 1ae9dca into ceph:master Jun 4, 2018

MahatiC deleted the wip-rbd-datapool branch June 5, 2018 03:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: add data pool support to trash purge #21247

rbd: add data pool support to trash purge #21247

MahatiC commented Apr 4, 2018 •

edited

Loading

batrick commented Apr 4, 2018

MahatiC commented Apr 5, 2018

MahatiC commented Apr 6, 2018 •

edited

Loading

tchaikov commented Apr 7, 2018

tchaikov Apr 7, 2018

dillaman left a comment

MahatiC commented Apr 10, 2018

MahatiC commented Apr 19, 2018 •

edited

Loading

dillaman commented Apr 19, 2018

MahatiC commented Apr 27, 2018

dillaman May 2, 2018

dillaman May 2, 2018

dillaman May 2, 2018

dillaman May 2, 2018

dillaman May 2, 2018

dillaman May 2, 2018

dillaman May 2, 2018

MahatiC commented May 9, 2018

dillaman May 10, 2018

MahatiC May 11, 2018

MahatiC May 15, 2018

dillaman May 10, 2018

MahatiC May 15, 2018

dillaman May 15, 2018

MahatiC May 16, 2018

dillaman May 15, 2018

MahatiC May 16, 2018

dillaman left a comment

rbd: add data pool support to trash purge #21247

rbd: add data pool support to trash purge #21247

Conversation

MahatiC commented Apr 4, 2018 • edited Loading

batrick commented Apr 4, 2018

MahatiC commented Apr 5, 2018

MahatiC commented Apr 6, 2018 • edited Loading

tchaikov commented Apr 7, 2018

Choose a reason for hiding this comment

dillaman left a comment

Choose a reason for hiding this comment

MahatiC commented Apr 10, 2018

MahatiC commented Apr 19, 2018 • edited Loading

dillaman commented Apr 19, 2018

MahatiC commented Apr 27, 2018

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

MahatiC commented May 9, 2018

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

Choose a reason for hiding this comment

dillaman left a comment

Choose a reason for hiding this comment

MahatiC commented Apr 4, 2018 •

edited

Loading

MahatiC commented Apr 6, 2018 •

edited

Loading

MahatiC commented Apr 19, 2018 •

edited

Loading