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

rbdmap: consider /etc/ceph/rbdmap when unmapping images #13361

Merged
merged 2 commits into from Feb 22, 2017

Conversation

Projects
None yet
3 participants
@ddiss
Contributor

ddiss commented Feb 10, 2017

As reported at http://tracker.ceph.com/issues/18884 , "rbdmap unmap" currently unmounts / unmaps all mapped RBD images, rather than just the images listed in RBDMAPFILE (/etc/ceph/rbdmap).

I'm unsure whether I should keep the existing behaviour and add the RBDMAP_UNMAP_ALL=no parameter, or just drop the old behaviour all together, given that it wasn't documented.
Feedback here would be very much appreciated.

@smithfarm smithfarm requested a review from dillaman Feb 12, 2017

logger -p "daemon.warning" -t rbdmap "No $RBDMAPFILE found."
exit 0
fi

This comment has been minimized.

@smithfarm

This comment has been minimized.

@smithfarm

smithfarm Feb 12, 2017

Contributor

See #8222 for background. Maybe the test & workunit have outlived their purpose and can be dropped?

logger -p "daemon.warning" -t rbdmap "No $RBDMAPFILE found."
exit 0
fi

This comment has been minimized.

@smithfarm

smithfarm Feb 12, 2017

Contributor

oh, now I see that you didn't remove this bit of code, but rather generalized it so it triggers for both map and unmap

@dillaman

Lots of duplicated code that could be potentially consolidated into a few helper methods to be re-used between map/unmap_all/unmap_file methods

# all locally mapped RBD images. Configuring "no" here overrides this
# behaviour so that only RBD images specified in /etc/ceph/rbdmap are
# unmounted and unmapped.
RBDMAP_UNMAP_ALL=yes

This comment has been minimized.

@dillaman

dillaman Feb 13, 2017

Contributor

Nit: if this is the default, perhaps leave it commented out as an example of the param

This comment has been minimized.

@ddiss

ddiss Feb 14, 2017

Contributor

@dillaman thanks for the feedback.

I didn't try to factor anything out in this round, as I was (and still am) unsure whether the existing unmap behaviour should be kept, or completely dropped. My preference would probably be to just drop it, unless someone thinks that there are users relying on the unmap-all-by-default behaviour.

If you think it's worth keeping the existing behaviour, then I'm happy

This comment has been minimized.

@dillaman

dillaman Feb 14, 2017

Contributor

It's probably a good question to raise to the ceph-users mailing list. My assumption is that most users that use this script use the associated service/systemd unit file that maps on startup and unmaps on shutdown. In that case, much like mapping file systems specified in /etc/fstab on startup and unmapping all mounted file systems on shutdown, I would expect this script, in the shutdown case to unmap all mapped RBD devices.

This comment has been minimized.

@ddiss

ddiss Feb 14, 2017

Contributor

Okay, will do that - thanks again.

@ddiss

This comment has been minimized.

Contributor

ddiss commented Feb 16, 2017

New version takes into account feedback from @dillaman :

  • factor out common unmount / unmap functionality
  • Leave new sysconfig parameter commented out
@dillaman

This comment has been minimized.

Contributor

dillaman commented Feb 16, 2017

@ddiss As an alternative middle-ground approach (non-binding suggestion), you could introduce a "unmap-all" command that the systemd script invokes and the existing "unmap" command would only unmap the configured images.

@ddiss

This comment has been minimized.

Contributor

ddiss commented Feb 16, 2017

@dillaman : hmm, yeah I think I'd prefer that idea, seems more intuitive for users... new round coming

ddiss added some commits Feb 10, 2017

rbdmap: unmap RBDMAPFILE images unless called with unmap-all
When called with a "map" parameter, the rbdmap script iterates the list
of images present in RBDMAPFILE (/etc/ceph/rbdmap), and maps each entry.
When called with "unmap", rbdmap currently iterates *all* mapped RBD
images and unmaps each one, regardless of whether it's listed in the
RBDMAPFILE or not.

This commit adds functionality such that only RBD images listed in the
configuration file are unmapped. This behaviour is the new default for
"rbdmap unmap". A new "unmap-all" parameter is added to offer the old
unmap-all-rbd-images behaviour, which is used by the systemd service.

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

Signed-off-by: David Disseldorp <ddiss@suse.de>
doc: update description of rbdmap unmap[-all] behaviour
Fixes: http://tracker.ceph.com/issues/18884

Signed-off-by: David Disseldorp <ddiss@suse.de>
@ddiss

This comment has been minimized.

Contributor

ddiss commented Feb 16, 2017

This new version follows @dillaman 's suggestion of adding an unmap-all parameter, which is used by the systemd service, and continues to unmount+unmap all RBD images regardless of whether or not they're listed in the config file. The behaviour of "rbdmap unmap" is changed such that it now respects the config file contents.

@dillaman

lgtm

@dillaman dillaman merged commit e0143b4 into ceph:master Feb 22, 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