cephadm: Add shell '--mount' option to mount host file or directory by ricardoasmarques · Pull Request #34773 · ceph/ceph

ricardoasmarques · 2020-04-27T14:24:26Z

The Problem:

I have a file on my host, that I would like to use to set a "config-key":

master:/etc/ceph # head -1 /tmp/ceph-salt-ssh-id_rsa
-----BEGIN RSA PRIVATE KEY-----

But when I try to use that file, I get an error because the file is not accessible inside the container:

master:/etc/ceph # cephadm shell -- ceph config-key set mgr/cephadm/ssh_identity_key -i /tmp/ceph-salt-ssh-id_rsa
...
Can't open input file /tmp/ceph-salt-ssh-id_rsa: [Errno 2] No such file or directory: '/tmp/c

Proposed Solution

Add a new --mount option to cephadm shell, that allows to mount a host file or directory:

master:~ # cephadm shell --help
...
  --mount MOUNT, -m MOUNT
                        file or directory path that will be mounted in
                        container /mnt
...

master:~ # cephadm shell --mount /tmp/ceph-salt-ssh-id_rsa -- ceph config-key set mgr/cephadm/ssh_identity_key -i /mnt/tmp/ceph-salt-ssh-id_rsa
...
set mgr/cephadm/ssh_identity_key

Fixes: https://tracker.ceph.com/issues/45284

Signed-off-by: Ricardo Marques rimarques@suse.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard backend
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

mgfritch

I really want to avoid adding an additional argument, but I don't see a safe way to pass secrets (via the environment, cmdline, or otherwise)

matthewoliver

I needed this exact option today to modify the ceph crushmap in ses7. As you need the compiled crushmap in the cephadm shell container to be able to run crushtool on it. When I applied these changes:

master:~ # ceph osd getcrushmap -o cm.compiled
master:~ # cephadm shell --mount ./
[ceph: root@master /]# crushtool -d /mnt/cm.compiled -o /mnt/cm.txt
master:~ # ls
bin  cm.compiled  cm.txt

It worked perfectly. Big +1 from me.

ricardoasmarques · 2020-04-29T07:56:53Z

@mgfritch @matthewoliver I've updated this PR to always mount the file or directory under /mnt, without relying on the args.mount:

-        mounts[pathify(args.mount)] = '/mnt/{}:z'.format(args.mount)
+        mount = pathify(args.mount)
+        filename = ''
+        if os.path.isfile(mount):
+            _, filename = os.path.split(mount)
+        mounts[mount] = '/mnt/{}:z'.format(filename)

sebastian-philipp · 2020-04-29T10:01:20Z

I don't have a good answer here. Maybe we can by default also mount $PWD?

ricardoasmarques · 2020-04-29T10:48:59Z

I don't have a good answer here. Maybe we can by default also mount $PWD?

@sebastian-philipp I've updated this PR to mount $PWD when no --mount option is specified. Also, I've now documented the --mount behavior on install.rst.

matthewoliver

Nice, I like this alot more. Now we don't have to assume the --mount option to make changes to things, only if you want to mount something other then cwd. Making it much easier to use.

Run it and you still have access to your current directory.

One thought. When I run cephadm shell it always dumps me in / I wonder if we should get it to dump us in /mnt now that it'll always be mounted. It'll make workflows easier maybe?

Though this could be a follow up patch, kind out of scope.

Maybe something like this will do it:

diff --git a/src/cephadm/cephadm b/src/cephadm/cephadm
index 23c838e79b..56bdcfcdd2 100755
--- a/src/cephadm/cephadm
+++ b/src/cephadm/cephadm
@@ -2767,6 +2767,7 @@ def command_shell():
             '-it',
             '-e', 'LANG=C',
             '-e', "PS1=%s" % CUSTOM_PS1,
+            '-w', '/mnt',
         ]
         if args.fsid:
             home = os.path.join(args.data_dir, args.fsid, 'home')

Of course making it something like ~/ might also be as useful. Anyway just a NIT not a blocker.

UPDATE: Just tested that ^^ locally and it worked for me.

@sebastian-philipp

I don't have a good answer here. Maybe we can by default also mount $PWD? > > @sebastian-philipp I've updated this PR to mount $PWD when no --mount option is specified. Also, I've now documented the --mount behavior on install.rst. Are we sure we want to default mount $PWD rather than forcing this option to be explicit? This could leave random files laying around in random places on the container host ...

mgfritch · 2020-04-30T02:53:49Z

I don't have a good answer here. Maybe we can by default also mount $PWD?

@sebastian-philipp I've updated this PR to mount $PWD when no --mount option is specified. Also, I've now documented the --mount behavior on install.rst.

Are we sure we want to default mount $PWD rather than forcing this option to be explicit? This could leave random files laying around in random places on the container host ...

trociny · 2020-04-30T12:20:41Z

I am also not very happy about default mount $PWD.

If this form does not look very convenient:

cephadm shell --mount /tmp/ceph-salt-ssh-id_rsa -- ceph config-key set mgr/cephadm/ssh_identity_key -i /mnt/tmp/ceph-salt-ssh-id_rsa

Then may be we can a provide a possibility to specify this like:

cephadm shell -- ceph config-key set mgr/cephadm/ssh_identity_key -i host://mnt/tmp/ceph-salt-ssh-id_rsa

Which would be preparsed by the cephadm and converted to the first form automatically?

ricardoasmarques · 2020-04-30T13:24:46Z

I've updated this PR to NOT mount $PWD by default as this is the less intrusive approach.

mgfritch

lgtm!

mgfritch · 2020-05-01T00:33:54Z

src/cephadm/cephadm

+        filename = ''
+        if os.path.isfile(mount):
+            _, filename = os.path.split(mount)
+        mounts[mount] = '/mnt/{}:z'.format(filename)


Suggested change

filename = ''

if os.path.isfile(mount):

_, filename = os.path.split(mount)

mounts[mount] = '/mnt/{}:z'.format(filename)

mounts[mount] = '/mnt/{}:z'.format(os.path.basename(mount))

this doesn't work with symlinks, char/block devs, etc.

maybe just use the basename instead?

Fixes: https://tracker.ceph.com/issues/45284 Signed-off-by: Ricardo Marques <rimarques@suse.com>

ricardoasmarques · 2020-05-04T14:37:24Z

@sebastian-philipp rebased

mgfritch · 2020-05-05T02:09:30Z

http://pulpito.ceph.com/mgfritch-2020-05-05_02:07:06-rados-wip-mgfritch-testing-2020-05-04-1622-distro-basic-smithi/

sebastian-philipp · 2020-05-05T08:56:29Z

http://pulpito.ceph.com/mgfritch-2020-05-05_02:07:06-rados-wip-mgfritch-testing-2020-05-04-1622-distro-basic-smithi/

https://tracker.ceph.com/issues/45335

ricardoasmarques added the cephadm label Apr 27, 2020

ricardoasmarques requested review from jschmid1, mgfritch and sebastian-philipp April 27, 2020 14:24

ricardoasmarques requested a review from a team as a code owner April 27, 2020 14:24

ricardoasmarques changed the title ~~cephadm: Add '--mount' option to mount host file or directory~~ cephadm: Add shell '--mount' option to mount host file or directory Apr 27, 2020

ricardoasmarques mentioned this pull request Apr 27, 2020

[WIP] Do not install "ceph-common" ceph/ceph-salt#201

Closed

mgfritch added the wip-mgfritch-testing label Apr 27, 2020

mgfritch previously approved these changes Apr 27, 2020

View reviewed changes

matthewoliver approved these changes Apr 29, 2020

View reviewed changes

ricardoasmarques force-pushed the cephadm-shell-mount branch from 2de2034 to fbbd091 Compare April 29, 2020 07:51

sebastian-philipp added the needs-doc label Apr 29, 2020

ricardoasmarques force-pushed the cephadm-shell-mount branch from fbbd091 to e79be56 Compare April 29, 2020 10:46

ricardoasmarques removed the needs-doc label Apr 29, 2020

ricardoasmarques force-pushed the cephadm-shell-mount branch from e79be56 to 543bfe2 Compare April 29, 2020 16:56

matthewoliver approved these changes Apr 29, 2020

View reviewed changes

mgfritch self-requested a review April 30, 2020 02:43

ricardoasmarques force-pushed the cephadm-shell-mount branch 2 times, most recently from bf220f3 to d3e92c0 Compare April 30, 2020 13:21

sebastian-philipp approved these changes Apr 30, 2020

View reviewed changes

sebastian-philipp added the needs-qa label Apr 30, 2020

mgfritch approved these changes May 1, 2020

View reviewed changes

sebastian-philipp added the needs-rebase label May 4, 2020

cephadm: Add '--mount' option to mount host file or directory

71c58f1

Fixes: https://tracker.ceph.com/issues/45284 Signed-off-by: Ricardo Marques <rimarques@suse.com>

ricardoasmarques force-pushed the cephadm-shell-mount branch from d3e92c0 to 71c58f1 Compare May 4, 2020 14:36

ricardoasmarques removed the needs-rebase label May 4, 2020

sebastian-philipp merged commit d347db2 into ceph:master May 5, 2020

sebastian-philipp mentioned this pull request May 21, 2020

octopus: cephadm: Batch backport May (2) #35188

Merged

trociny mentioned this pull request Jun 2, 2020

Runnin cephadm shell for every command may be too slow SUSE/supportutils-plugin-ses#36

Open

Conversation

ricardoasmarques commented Apr 27, 2020

The Problem:

Proposed Solution

Checklist

Uh oh!

mgfritch left a comment

Choose a reason for hiding this comment

Uh oh!

matthewoliver left a comment

Choose a reason for hiding this comment

Uh oh!

ricardoasmarques commented Apr 29, 2020

Uh oh!

sebastian-philipp commented Apr 29, 2020

Uh oh!

ricardoasmarques commented Apr 29, 2020

Uh oh!

matthewoliver left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgfritch commented Apr 30, 2020

Uh oh!

trociny commented Apr 30, 2020

Uh oh!

ricardoasmarques commented Apr 30, 2020

Uh oh!

mgfritch left a comment

Choose a reason for hiding this comment

Uh oh!

mgfritch May 1, 2020

Choose a reason for hiding this comment

Uh oh!

ricardoasmarques commented May 4, 2020

Uh oh!

mgfritch commented May 5, 2020

Uh oh!

sebastian-philipp commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matthewoliver left a comment •

edited

Loading