rbd: When Ceph cluster becomes full, should allow user to remove rbd … by liupan1111 · Pull Request #12627 · ceph/ceph

liupan1111 · 2016-12-22T16:48:00Z

…and snapshot in order to release storage space.

Signed-off-by: Pan Liu pan.liu@istuary.com

liupan1111 · 2016-12-22T16:48:32Z

At this moment, the behavior is "hang the client".

yangdongsheng · 2016-12-23T01:49:31Z

src/include/rados/librados.hpp

    int get_object_pg_hash_position2(const std::string& oid, uint32_t *pg_hash_position);

    config_t cct();
+    IoCtxImpl *get_impl() {return io_ctx_impl;}


I don't think it's a good idea to expose implementation to user. Maybe adding an interface for what you want is better.

agree, will modify later, thanks.

done, thanks.

liupan1111 · 2017-01-01T10:25:45Z

@liewegas, @tchaikov, please help take a look, thanks!

dillaman · 2017-01-04T18:07:36Z

src/tools/rbd/Utils.h


 int init(const std::string &pool_name, librados::Rados *rados,
-         librados::IoCtx *io_ctx);
+         librados::IoCtx *io_ctx, bool force = false);


The parameter name of force doesn't really align (name-wise) with what it is actually doing behind the scenes which is to force-apply a special flag to the OSDs. Also, assuming there are no issues w/ the librados changes by others, I don't think this is the correct place to do this. Seems like you should be able to set this flag after opening the image for the remove actions so you don't need to put it in this common location.

@dillaman, Thank you for The review! I agree this change should better not be placed in Such a common place like init. I will modify it.

liupan1111 · 2017-01-16T09:53:11Z

@dillaman, I've commit a new one, please help take a look.

dillaman · 2017-01-16T16:48:08Z

The rbd changes look good to me -- a member of the core team needs to evaluate the changes to librados and the OSDs.

liewegas · 2017-01-16T16:52:40Z

src/osd/PrimaryLogPG.cc

  }
-  if (!(m->get_source().is_mds()) && osd->check_failsafe_full() && write_ordered) {
+  if (!(m->get_source().is_mds()) && osd->check_failsafe_full() && 
+      !m->has_flag(CEPH_OSD_FLAG_FULL_FORCE) && write_ordered) {


I don't think we want to touch the failsafe condition!

The ops of deleting an image will be discarded here if don’t touch the failsafe condition

liewegas · 2017-01-16T16:54:51Z

src/include/rados/librados.hpp


+    void set_honor_osdmap_full();
+    void unset_honor_osdmap_full();
+


Instead of exposing this, you can use the existing librados op flag:

src/include/rados/librados.hpp: OPERATION_FULL_TRY = LIBRADOS_OPERATION_FULL_TRY,

FULL_TRY makes the OSD assemble the transaction and proceed only if it does not result in a net increase in utilization. This should be true for deleting objects.

There's also a FORCE variant, but I don't think you should need it...

In “Objecter::_prepare_osd_op”, “CEPH_OSD_FLAG_FULL_FORCE” is set before sent op to an OSD, if “honor_osdmap_full” is false. When removing an image, several ops, like open, delete, watch, will be sent to the OSD, it’s difficult to set FULL_TRY flag for these ops one by one.

Hmm, in that case, the FULL_TRY flag can be set in the same place as the FULL_FORCE flag: Objecter::_prepare_osd_op(). So you can add {set,unset}_osdmap_full_try(), and set the flag in one place, and then use that.

I really think we should use FULL_TRY unless we find there is some reason that can't work... if that's the case we have a bigger problem!

It would still be far preferably to use the existing librados flags, though! Please try that appraoch first.. hopefully there is a central place wehre that can be done, or add an ImageCtx 'rados_flags' variable that is |'d in to every request.

liewegas · 2017-01-17T14:14:18Z

Right, but you shouldn't be reaching the failsafe condition. This is usually a much higher threshold than the osdmap cluster FULL flag that stops writes.. it's there to prevent some other process (errant backfill or something) from filling up an OSD. In theory it should never be triggered.

liewegas · 2017-01-17T14:14:58Z

In that case, let's do one patch that rneames honor_osdmap_full to set_full_force() (with opposite argument... false by default, true for enable), and then another patch that adds a new set_full_try().

liupan1111 · 2017-01-23T14:21:52Z

In that case, let's do one patch that rneames honor_osdmap_full to
set_full_force() (with opposite argument... false by default, true for
enable), and then another patch that adds a new set_full_try().

@liewegas, do you mean I need rename honor_osdmap_full or set_honor_osdmap_full()?

liupan1111 · 2017-01-27T20:28:59Z

@liewegas, this failsafe is not triggered by my modification... it will be called even without my changes...

liupan1111 · 2017-01-30T19:35:17Z

@liewegas Ping

liupan1111 · 2017-02-17T11:56:50Z

@liewegas, Could you help take a look? thanks.

dillaman · 2017-02-17T13:00:06Z

@liupan1111 I don't see any changes. I thought Sage was suggesting that you re-use the existing librados flag for allowing ops to proceed on full(?). Assuming that is the case, I think you would want to modify librbd to set the flag where needed when opening the image and set the flag on the individual ops issued on the remove / snap remove paths.

liewegas · 2017-02-17T14:31:12Z

src/tools/rbd/Utils.cc


 int init(const std::string &pool_name, librados::Rados *rados,
-         librados::IoCtx *io_ctx) {
+         librados::IoCtx *io_ctx, bool force) {


Like @dillaman said, this should also be a more descriptive name... bool force_full_try probably

yes, I will modify it.

Signed-off-by: Pan Liu <liupan1111@gmail.com>

…full. Signed-off-by: Pan Liu <liupan1111@gmail.com>

liupan1111 · 2017-02-26T00:29:09Z

@dillaman @liewegas , I've modified as your comment, please help take a look. Thanks!

dillaman

rbd changes lgtm

yuriw · 2017-03-03T00:59:58Z

RBD

http://pulpito.front.sepia.ceph.com:80/yuriw-2017-03-02_15:18:23-rbd-wip-yuri-testing_2017_3_2-distro-basic-smithi/

TestLibRBD.FlattenNoEmptyObjects failures unrelated, also on master per @jasondillaman

RADOS

http://pulpito.ceph.com/yuriw-2017-03-01_17:30:41-rados-wip-yuri-testing_2017_3_2-distro-basic-smithi/

http://pulpito.ceph.com/yuriw-2017-03-02_01:20:35-rados-wip-yuri-testing_2017_3_2---basic-smithi/

and reproduce

http://pulpito.ceph.com/yuriw-2017-03-02_01:56:48-rados-wip-yuri-testing_2017_3_2---basic-ovh/

same job passed on manual rerun !

and confirmed pass

http://pulpito.ceph.com/yuriw-2017-03-02_21:15:59-rados-wip-yuri-testing_2017_3_2-distro-basic-smithi/

yangdongsheng reviewed Dec 23, 2016

View reviewed changes

dillaman added common feature rbd labels Dec 28, 2016

dillaman reviewed Jan 4, 2017

View reviewed changes

dillaman added the core label Jan 10, 2017

liewegas requested changes Jan 16, 2017

View reviewed changes

liupan1111 mentioned this pull request Feb 1, 2017

ceph_test_objectstore: match clone_range src and dst offset #13211

Merged

liewegas reviewed Feb 17, 2017

View reviewed changes

Pan Liu added 3 commits February 26, 2017 08:20

librados: add interface to set osdmap fulll try flag

ad8d0e4

Signed-off-by: Pan Liu <liupan1111@gmail.com>

rbd: allow users to remove rbd image when Ceph cluster becomes full.

25eaf0a

Signed-off-by: Pan Liu <liupan1111@gmail.com>

rbd: allow to remove snapshots of an image when Ceph cluster becomes …

0daaa3c

…full. Signed-off-by: Pan Liu <liupan1111@gmail.com>

liupan1111 requested a review from liewegas February 26, 2017 00:30

dillaman reviewed Feb 27, 2017

View reviewed changes

liewegas approved these changes Feb 27, 2017

View reviewed changes

liewegas added the needs-qa label Feb 27, 2017

yuriw added the wip-yuri-testing label Mar 1, 2017

yuriw merged commit c4c7bce into ceph:master Mar 3, 2017

liupan1111 deleted the wip-fix-remove-when-full branch March 3, 2017 01:26

This was referenced Mar 27, 2017

rbd: when Ceph cluster is full, return -ENOSPC for creating image command. #14167

Closed

librados: set the flag CEPH_OSD_FLAG_FULL_TRY of Op in the right place. #14193

Merged

liupan1111 mentioned this pull request Aug 23, 2017

osd: allow FULL_TRY after failsafe #17177

Merged


		void set_honor_osdmap_full();
		void unset_honor_osdmap_full();

Conversation

liupan1111 commented Dec 22, 2016

Uh oh!

liupan1111 commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liupan1111 commented Jan 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liupan1111 commented Jan 16, 2017

Uh oh!

dillaman commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liewegas commented Jan 17, 2017 via email

Uh oh!

liewegas commented Jan 17, 2017 via email

Uh oh!

liupan1111 commented Jan 23, 2017

Uh oh!

liupan1111 commented Jan 27, 2017

Uh oh!

liupan1111 commented Jan 30, 2017

Uh oh!

liupan1111 commented Feb 17, 2017

Uh oh!

dillaman commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liupan1111 commented Feb 26, 2017

Uh oh!

dillaman left a comment

Choose a reason for hiding this comment

Uh oh!

yuriw commented Mar 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants