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

rbd: When Ceph cluster becomes full, should allow user to remove rbd … #12627

Merged
merged 3 commits into from Mar 3, 2017

Conversation

Projects
None yet
5 participants
@liupan1111
Contributor

liupan1111 commented Dec 22, 2016

…and snapshot in order to release storage space.

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

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Dec 22, 2016

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

@@ -1153,6 +1153,7 @@ namespace librados
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;}

This comment has been minimized.

@yangdongsheng

yangdongsheng Dec 23, 2016

Member

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

This comment has been minimized.

@liupan1111

liupan1111 Dec 26, 2016

Contributor

agree, will modify later, thanks.

This comment has been minimized.

@liupan1111

liupan1111 Dec 27, 2016

Contributor

done, thanks.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 1, 2017

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

@@ -110,7 +110,7 @@ int get_formatter(const boost::program_options::variables_map &vm,
void init_context();
int init(const std::string &pool_name, librados::Rados *rados,
librados::IoCtx *io_ctx);
librados::IoCtx *io_ctx, bool force = false);

This comment has been minimized.

@dillaman

dillaman Jan 4, 2017

Contributor

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.

This comment has been minimized.

@liupan1111

liupan1111 Jan 4, 2017

Contributor

@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.

@dillaman dillaman added the core label Jan 10, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 16, 2017

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

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 16, 2017

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

@@ -1848,7 +1848,8 @@ void PrimaryLogPG::do_op(OpRequestRef& op)
<< *m << dendl;
return;
}
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) {

This comment has been minimized.

@liewegas

liewegas Jan 16, 2017

Member

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

This comment has been minimized.

@liupan1111

liupan1111 Jan 17, 2017

Contributor

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

@@ -1154,6 +1154,9 @@ namespace librados
config_t cct();
void set_honor_osdmap_full();
void unset_honor_osdmap_full();

This comment has been minimized.

@liewegas

liewegas Jan 16, 2017

Member

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...

This comment has been minimized.

@liupan1111

liupan1111 Jan 17, 2017

Contributor

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.

This comment has been minimized.

@liewegas

liewegas Feb 17, 2017

Member

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!

This comment has been minimized.

@liewegas

liewegas Feb 17, 2017

Member

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

This comment has been minimized.

Member

liewegas commented Jan 17, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jan 17, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 23, 2017

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

This comment has been minimized.

Contributor

liupan1111 commented Jan 27, 2017

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

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jan 30, 2017

@liewegas Ping

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Feb 17, 2017

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

@dillaman

This comment has been minimized.

Contributor

dillaman commented Feb 17, 2017

@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.

@@ -730,7 +730,7 @@ void init_context() {
}
int init(const std::string &pool_name, librados::Rados *rados,
librados::IoCtx *io_ctx) {
librados::IoCtx *io_ctx, bool force) {

This comment has been minimized.

@liewegas

liewegas Feb 17, 2017

Member

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

This comment has been minimized.

@liupan1111

liupan1111 Feb 24, 2017

Contributor

yes, I will modify it.

liupan1111 added some commits Feb 26, 2017

librados: add interface to set osdmap fulll try flag
Signed-off-by: Pan Liu <liupan1111@gmail.com>
rbd: allow users to remove rbd image when Ceph cluster becomes full.
Signed-off-by: Pan Liu <liupan1111@gmail.com>
rbd: allow to remove snapshots of an image when Ceph cluster becomes …
…full.

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

This comment has been minimized.

Contributor

liupan1111 commented Feb 26, 2017

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

@liupan1111 liupan1111 requested a review from liewegas Feb 26, 2017

@dillaman

rbd changes lgtm

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

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment