cephadm: Make path to cephadm binary unique by sebastian-philipp · Pull Request #39141 · ceph/ceph

sebastian-philipp · 2021-01-28T16:00:48Z

right now, an upgrade might overwrite the existing
binary which would force us to have the CLI stable. Thus the path to cephadm is not safe for us.

This PR changes the path to a location that depends on the content of cephadm and thus is safe for up and downgrades.

Required for #37495

Signed-off-by: Sebastian Wagner sebastian.wagner@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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

src/pybind/mgr/cephadm/module.py

right now, an upgrade might overwrite the existing binary which would force us to have the CLI stable. Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

sebastian-philipp · 2021-02-18T11:08:31Z

https://pulpito.ceph.com/swagner-2021-02-17_14:01:13-rados:cephadm-wip-swagner-testing-2021-02-16-1806-distro-basic-smithi/

sebastian-philipp · 2021-02-18T11:09:47Z

    @pytest.fixture
    def exporter():
        with mock.patch('cephadm.CephadmDaemon.daemon_path', _daemon_path()), \
           mock.patch('cephadm.CephadmDaemon.can_run', return_value=True), \
           mock.patch('cephadm.CephadmDaemon.run', _mock_run), \
           mock.patch('cephadm.CephadmDaemon._scrape_host_facts', _mock_scrape_host):
    
            ctx = cd.CephadmContext()
>           exporter = cd.CephadmDaemon(ctx, fsid='foobar', daemon_id='test')
E           TypeError: __init__() missing 1 required positional argument: 'binary'

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

src/cephadm/cephadm

jmolmo

Need to check that cephadm binary exists when we create a Cephadmdaemon

Also: * Enable the `test_can_run` unit test Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

sebastian-philipp · 2021-02-24T14:08:14Z

https://pulpito.ceph.com/swagner-2021-02-22_13:53:31-rados:cephadm-wip-swagner3-testing-2021-02-22-1135-distro-basic-smithi/

sebastian-philipp · 2021-02-24T14:08:38Z

ping @jmolmo

adk3798

Code generally looks good to me. I haven't looked much at the exporter code so these are more questions on things I'm unsure about than suggestions.

adk3798 · 2021-02-24T18:30:01Z

src/cephadm/cephadm

+    parser_exporter.add_argument(
+        '--binary',
+        type=str,
+        help='path to the cephadm binary')


What's the use case for this arg? If we're calling the exporter command directly from the command line then the binary to use would just be the one we're calling with, and in a call from the mgr to setup an exporter the only place the binary location is being used is in the CephadmDaemon unit_run function

ceph/src/cephadm/cephadm

Lines 6829 to 6838 in 4166eca

def unit_run(self):

return """set -e

{py3} {bin_path} exporter --fsid {fsid} --id {daemon_id} --port {port} &""".format(

py3 = shutil.which('python3'),

bin_path=self.binary_path,

fsid=self.fsid,

daemon_id=self.daemon_id,

port=self.port

)

which we aren't adjusting to use this arg.

What's the use case for this arg? If we're calling the exporter command directly from the command line then the binary to use would just be the one we're calling with.

There is no binary at this point. At least not when calling from cephadm. At least not until we have merged #39619 which depends on this very PR.

but we can probably avoid adding the --binary command argument indeed.

adk3798 · 2021-02-24T18:43:30Z

src/pybind/mgr/cephadm/services/cephadmservice.py

            "key": cfg.key,
-            "token": cfg.token
+            "token": cfg.token,
+            'binary': self.mgr.cephadm_binary_path,


I could be off, but it looks like we pass the config dict into the CephadmDaemon deploy_daemon_unit function

ceph/src/cephadm/cephadm

Lines 2497 to 2504 in 4166eca

if ctx.config_json == '-':

config_js = get_parm('-')

else:

config_js = get_parm(ctx.config_json)

assert isinstance(config_js, dict)

cephadm_exporter = CephadmDaemon(ctx, fsid, config_js['binary'], daemon_id, port)

cephadm_exporter.deploy_daemon_unit(config_js)

which then writes all these files out

ceph/src/cephadm/cephadm

Lines 6876 to 6879 in 4166eca

# Create the required config files in the daemons dir, with restricted permissions

for filename in config:

with open(os.open(os.path.join(self.daemon_path, filename), os.O_CREAT | os.O_WRONLY, mode=0o600), "w") as f:

f.write(config[filename])

should we avoid having it do so for the binary since we're already deploying it beforehand with _deploy_cephadm_binary?

wondering if it makes sense to combine this PR with #39619 and avoid this complication

src/pybind/mgr/cephadm/module.py

sebastian-philipp requested review from jmolmo and pcuzner January 28, 2021 16:00

sebastian-philipp requested a review from a team as a code owner January 28, 2021 16:00

github-actions bot added cephadm pybind labels Jan 28, 2021

sebastian-philipp mentioned this pull request Jan 28, 2021

cephadm: Get rid of injected_argv #37495

Closed

sebastian-philipp added wip-swagner3-testing needs-review and removed wip-swagner3-testing labels Jan 29, 2021

adk3798 reviewed Feb 10, 2021

View reviewed changes

src/pybind/mgr/cephadm/module.py Outdated Show resolved Hide resolved

sebastian-philipp removed the needs-review label Feb 16, 2021

sebastian-philipp added 2 commits February 16, 2021 13:12

cephadm: Make path to cephadm binary unique

d799683

right now, an upgrade might overwrite the existing binary which would force us to have the CLI stable. Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

mgr/cephadm: fix CephadmExporter deployment

9f43ee0

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

sebastian-philipp force-pushed the cephadmd-fix-unsafe-path branch from 3983233 to 9f43ee0 Compare February 16, 2021 12:22

sebastian-philipp added the wip-swagner-testing My Teuthology tests label Feb 16, 2021

sebastian-philipp removed the wip-swagner-testing My Teuthology tests label Feb 18, 2021

cephadm: Add binary= to cephadm pytest suite

c77a954

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

jmolmo reviewed Feb 18, 2021

View reviewed changes

src/cephadm/cephadm Show resolved Hide resolved

jmolmo requested changes Feb 18, 2021

View reviewed changes

cephadm: verify exporter's binary_path is a file

4166eca

Also: * Enable the `test_can_run` unit test Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

sebastian-philipp requested a review from jmolmo February 19, 2021 11:46

sebastian-philipp mentioned this pull request Feb 21, 2021

gravel: expose bootstrap progress aquarist-labs/aquarium#179

Merged

sebastian-philipp added the wip-swagner3-testing label Feb 22, 2021

sebastian-philipp mentioned this pull request Feb 22, 2021

cephadm: remove injected_args #39619

Merged

3 tasks

sebastian-philipp added needs-review and removed wip-swagner3-testing labels Feb 24, 2021

adk3798 reviewed Feb 24, 2021

View reviewed changes

sebastian-philipp closed this Feb 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephadm: Make path to cephadm binary unique#39141

cephadm: Make path to cephadm binary unique#39141
sebastian-philipp wants to merge 4 commits intoceph:masterfrom
sebastian-philipp:cephadmd-fix-unsafe-path

sebastian-philipp commented Jan 28, 2021 •

edited

Loading

Uh oh!

Uh oh!

sebastian-philipp commented Feb 18, 2021

Uh oh!

sebastian-philipp commented Feb 18, 2021

Uh oh!

Uh oh!

jmolmo left a comment

Uh oh!

sebastian-philipp commented Feb 24, 2021

Uh oh!

sebastian-philipp commented Feb 24, 2021

Uh oh!

adk3798 left a comment

Uh oh!

adk3798 Feb 24, 2021

Uh oh!

sebastian-philipp Feb 25, 2021

Uh oh!

adk3798 Feb 24, 2021

Uh oh!

sebastian-philipp Feb 25, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	def unit_run(self):

	return """set -e
	{py3} {bin_path} exporter --fsid {fsid} --id {daemon_id} --port {port} &""".format(
	py3 = shutil.which('python3'),
	bin_path=self.binary_path,
	fsid=self.fsid,
	daemon_id=self.daemon_id,
	port=self.port
	)

	if ctx.config_json == '-':
	config_js = get_parm('-')
	else:
	config_js = get_parm(ctx.config_json)
	assert isinstance(config_js, dict)

	cephadm_exporter = CephadmDaemon(ctx, fsid, config_js['binary'], daemon_id, port)
	cephadm_exporter.deploy_daemon_unit(config_js)

	# Create the required config files in the daemons dir, with restricted permissions
	for filename in config:
	with open(os.open(os.path.join(self.daemon_path, filename), os.O_CREAT \| os.O_WRONLY, mode=0o600), "w") as f:
	f.write(config[filename])

Conversation

sebastian-philipp commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

sebastian-philipp commented Feb 18, 2021

Uh oh!

sebastian-philipp commented Feb 18, 2021

Uh oh!

Uh oh!

jmolmo left a comment

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp commented Feb 24, 2021

Uh oh!

sebastian-philipp commented Feb 24, 2021

Uh oh!

adk3798 left a comment

Choose a reason for hiding this comment

Uh oh!

adk3798 Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

adk3798 Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

sebastian-philipp Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sebastian-philipp commented Jan 28, 2021 •

edited

Loading