cephadm: run containers using `--init` by default #37764

mgfritch · 2020-10-23T05:35:23Z

generally all ceph containers need an init process to both reap any
zombie pids and/or peform signal handling (e.g. coredumps, etc.)

Removes the --container-init sub-command argument in favor of a global --no-container-init flag.

Signed-off-by: Michael Fritch mfritch@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

mgfritch · 2020-10-23T05:36:21Z

Alternate proposal to #37648 and #36822

ricardoasmarques · 2020-10-23T10:28:03Z

Any chance we can keep both options and just change the default behavior, so we guarantee backward compatibility in the options syntax?

I mean:

cephadm bootstrap (will now use "init", exactly the same as using bootstrap --container-init)
cephadm bootstrap --container-init (still works, it uses "init", exactly the same as default bootstrap without container init option)
cephadm bootstrap --no-container-init (do not use "init")

This way, any ceph-salt version will be able to deploy any ceph version, otherwise some ceph-salt (older) versions will fail with "--container-init is not a valid option".

sebastian-philipp · 2020-10-23T11:26:16Z

So the problem with this is how do we handle distros that have a broken - - in it gracefully? How do we make sure bootstrap still works for them?

mgfritch · 2020-10-23T13:51:54Z

Any chance we can keep both options and just change the default behavior, so we guarantee backward compatibility in the options syntax?

How long would we want to keep around backward compat? I'm unsure if it's worth it given the feature has only existed for a relatively short time ...

This way, any ceph-salt version will be able to deploy any ceph version, otherwise some ceph-salt (older) versions will fail with "--container-init is not a valid option".

We unfortunately don't have a versioned cephadm, but maybe a release note or errata about updating ceph-salt would be sufficient?

mgfritch · 2020-10-23T13:54:30Z

So the problem with this is how do we handle distros that have a broken - - in it gracefully? How do we make sure bootstrap still works for them?

I might suggest those distros are either unsuitable for cephadm or should be updated - but if someone would like to run cephadm in a less optimal way, the --no-container-init flag could be passed during bootstrap ..

l-mb · 2020-10-23T14:01:18Z

Any chance we can keep both options and just change the default behavior, so we guarantee backward compatibility in the options syntax?

I think that's required. I like this, but we shouldn't break old execution syntax if we can avoid it.

mgfritch · 2020-10-23T15:04:38Z

Any chance we can keep both options and just change the default behavior, so we guarantee backward compatibility in the options syntax?

I think that's required. I like this, but we shouldn't break old execution syntax if we can avoid it.

I think the only major consumer at the moment is ceph-salt, but point taken. Would that also imply we should preserve this behavior for cephadm adopt and cephadm deploy too?

If we add backwards compat,

$ cephadm --no-container-init bootstrap --container-init --mon-ip 192.168.1.1

would ignore the global option and create containers with an init, but that's maybe expected?

ricardoasmarques · 2020-10-23T15:08:54Z

cephadm --no-container-init bootstrap --container-init --mon-ip 192.168.1.1

In this case, I would expect an error saying that --no-container-init and --container-init are mutually exclusive.

mgfritch · 2020-11-09T23:26:29Z

cephadm --no-container-init bootstrap --container-init --mon-ip 192.168.1.1

In this case, I would expect an error saying that --no-container-init and --container-init are mutually exclusive.

@ricardoasmarques I had to add some awkward handling due to the global/subcommand args, but they should now be mutually exclusive

$ cephadm --no-container-init bootstrap --container-init
usage: cephadm [-h] [--image IMAGE] [--docker] [--data-dir DATA_DIR] [--log-dir LOG_DIR] [--logrotate-dir LOGROTATE_DIR] [--unit-dir UNIT_DIR] [--verbose] [--timeout TIMEOUT] [--retry RETRY] [--env ENV] [--no-container-init]
               {version,pull,inspect-image,ls,list-networks,adopt,rm-daemon,rm-cluster,run,shell,enter,ceph-volume,unit,logs,bootstrap,deploy,check-host,prepare-host,add-repo,rm-repo,install,registry-login,gather-facts} ...
cephadm: error: argument --container-init: not allowed with argument --no-container-init

mgfritch · 2020-11-09T23:36:22Z

Any chance we can keep both options and just change the default behavior, so we guarantee backward compatibility in the options syntax?

I think that's required. I like this, but we shouldn't break old execution syntax if we can avoid it.

@l-mb old execution syntax has been preserved, but hidden/removed from the man page and command help usage.

# container_init=True
$ cephadm bootstrap

# container_init=True (noop)
$ cephadm bootstrap --container-init

# container_init=False
$ cephadm --no-container-init bootstrap

# error, args are mutually exclusive
$ cephadm --no-container-init bootstrap  --container-init

mgfritch · 2020-11-09T23:37:21Z

So the problem with this is how do we handle distros that have a broken - - in it gracefully? How do we make sure bootstrap still works for them?

@sebastian-philipp this PR depends on podman >= 2.0 (#37922)

mgfritch · 2020-11-11T16:48:21Z

https://pulpito.ceph.com/mgfritch-2020-11-10_01:16:20-rados:cephadm-wip-mgfritch-testing-2020-11-09-1644-distro-basic-smithi/

Failure:
https://tracker.ceph.com/issues/48171

sebastian-philipp · 2020-11-19T15:07:15Z

DNM, till we have sorted out https://tracker.ceph.com/issues/48171

github-actions · 2020-12-16T17:27:13Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

mgfritch · 2021-01-21T18:04:24Z

blocked by #39007

adopt command allows for the `--container-init` flag Signed-off-by: Michael Fritch <mfritch@suse.com>

generally all ceph containers need an init process to both reap any zombie pids and/or perform signal handling (e.g. coredumps, etc.) Signed-off-by: Michael Fritch <mfritch@suse.com>

allow for the `--init` flag to be passed during `cephadm shell` Signed-off-by: Michael Fritch <mfritch@suse.com>

mgfritch · 2021-02-08T15:15:30Z

https://pulpito.ceph.com/mgfritch-2021-02-06_22:19:46-rados:cephadm-wip-mgfritch-testing-2021-02-06-1400-distro-basic-smithi/

e2e failures:
https://tracker.ceph.com/issues/49205

…ner-init" This reverts commit f635555, reversing changes made to d4d3d17. This PR seems to be (indirectly?) responsible for https://tracker.ceph.com/issues/49237 Also, it was causing the rados.py task's follow-up step to wait for snap trimming to fail: it would time out a 'ceph osd dump --format=json' command. :/ Signed-off-by: Sage Weil <sage@newdream.net>

* refs/pull/39482/head: Revert "Merge pull request #37764 from mgfritch/cephadm-no-container-init" Reviewed-by: Josh Durgin <jdurgin@redhat.com>

…init" This reverts commit f635555, reversing changes made to d4d3d17. This PR seems to be (indirectly?) responsible for https://tracker.ceph.com/issues/49237 Also, it was causing the rados.py task's follow-up step to wait for snap trimming to fail: it would time out a 'ceph osd dump --format=json' command. :/ Signed-off-by: Sage Weil <sage@newdream.net> (cherry picked from commit c896292)

…ner-init" This reverts commit f635555, reversing changes made to d4d3d17. This PR seems to be (indirectly?) responsible for https://tracker.ceph.com/issues/49237 Also, it was causing the rados.py task's follow-up step to wait for snap trimming to fail: it would time out a 'ceph osd dump --format=json' command. :/ Signed-off-by: Sage Weil <sage@newdream.net>

mgfritch added the cephadm label Oct 23, 2020

mgfritch requested review from sebastian-philipp and jschmid1 October 23, 2020 05:35

mgfritch requested a review from a team as a code owner October 23, 2020 05:35

This was referenced Oct 23, 2020

Drop --container-init flag during cephadm adopt SUSE/DeepSea#1859

Closed

Drop the container-init flag during bootstrap ceph/ceph-salt#441

Closed

jschmid1 approved these changes Oct 23, 2020

View reviewed changes

tchaikov added the wip-kefu-testing label Oct 23, 2020

tchaikov removed the wip-kefu-testing label Oct 24, 2020

mgfritch added the DNM label Oct 27, 2020

mgfritch force-pushed the cephadm-no-container-init branch from 0387cfe to f2c4f04 Compare November 9, 2020 23:23

mgfritch removed the DNM label Nov 9, 2020

mgfritch requested review from ricardoasmarques, tserong, jmolmo and a team November 9, 2020 23:38

mgfritch added the wip-mgfritch-testing label Nov 9, 2020

mgfritch removed the wip-mgfritch-testing label Nov 11, 2020

sebastian-philipp added the DNM label Nov 19, 2020

github-actions bot added the needs-rebase label Dec 16, 2020

mgfritch force-pushed the cephadm-no-container-init branch from f2c4f04 to 2d302ef Compare January 27, 2021 00:40

github-actions bot added documentation pybind and removed needs-rebase labels Jan 27, 2021

jmolmo approved these changes Jan 27, 2021

View reviewed changes

mgfritch added 3 commits February 5, 2021 08:03

doc/man/cephadm: add missing --container-init flag

d89fae5

adopt command allows for the `--container-init` flag Signed-off-by: Michael Fritch <mfritch@suse.com>

cephadm: run containers using --init by default

9ac2ab8

generally all ceph containers need an init process to both reap any zombie pids and/or perform signal handling (e.g. coredumps, etc.) Signed-off-by: Michael Fritch <mfritch@suse.com>

cephadm: extend --container-init to the shell command

5750589

allow for the `--init` flag to be passed during `cephadm shell` Signed-off-by: Michael Fritch <mfritch@suse.com>

mgfritch force-pushed the cephadm-no-container-init branch from 2d302ef to 5750589 Compare February 5, 2021 15:04

mgfritch added needs-qa wip-mgfritch-testing and removed DNM labels Feb 5, 2021

sebastian-philipp merged commit f635555 into ceph:master Feb 8, 2021

mgfritch deleted the cephadm-no-container-init branch February 8, 2021 17:04

sebastian-philipp mentioned this pull request Feb 9, 2021

pacific: cephadm backport February (1) #39378

Merged

liewegas added a commit that referenced this pull request Feb 15, 2021

Merge PR #39482 into master

9200b1e

* refs/pull/39482/head: Revert "Merge pull request #37764 from mgfritch/cephadm-no-container-init" Reviewed-by: Josh Durgin <jdurgin@redhat.com>

mgfritch mentioned this pull request Mar 8, 2021

octopus: cephadm: run containers using --init #39914

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephadm: run containers using `--init` by default #37764

cephadm: run containers using `--init` by default #37764

mgfritch commented Oct 23, 2020

mgfritch commented Oct 23, 2020

ricardoasmarques commented Oct 23, 2020 •

edited

sebastian-philipp commented Oct 23, 2020 via email •

edited by mgfritch

mgfritch commented Oct 23, 2020

mgfritch commented Oct 23, 2020 •

edited

l-mb commented Oct 23, 2020

mgfritch commented Oct 23, 2020

ricardoasmarques commented Oct 23, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 11, 2020

sebastian-philipp commented Nov 19, 2020

github-actions bot commented Dec 16, 2020

mgfritch commented Jan 21, 2021

mgfritch commented Feb 8, 2021

cephadm: run containers using --init by default #37764

cephadm: run containers using --init by default #37764

Conversation

mgfritch commented Oct 23, 2020

Checklist

mgfritch commented Oct 23, 2020

ricardoasmarques commented Oct 23, 2020 • edited

sebastian-philipp commented Oct 23, 2020 via email • edited by mgfritch

mgfritch commented Oct 23, 2020

mgfritch commented Oct 23, 2020 • edited

l-mb commented Oct 23, 2020

mgfritch commented Oct 23, 2020

ricardoasmarques commented Oct 23, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 9, 2020

mgfritch commented Nov 11, 2020

sebastian-philipp commented Nov 19, 2020

github-actions bot commented Dec 16, 2020

mgfritch commented Jan 21, 2021

mgfritch commented Feb 8, 2021

cephadm: run containers using `--init` by default #37764

cephadm: run containers using `--init` by default #37764

ricardoasmarques commented Oct 23, 2020 •

edited

sebastian-philipp commented Oct 23, 2020 via email •

edited by mgfritch

mgfritch commented Oct 23, 2020 •

edited