Skip to content

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

Merged
sebastian-philipp merged 3 commits intoceph:masterfrom
mgfritch:cephadm-no-container-init
Feb 8, 2021
Merged

cephadm: run containers using --init by default#37764
sebastian-philipp merged 3 commits intoceph:masterfrom
mgfritch:cephadm-no-container-init

Conversation

@mgfritch
Copy link
Contributor

@mgfritch mgfritch commented Oct 23, 2020

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 mgfritch requested a review from a team as a code owner October 23, 2020 05:35
@mgfritch
Copy link
Contributor Author

mgfritch commented Oct 23, 2020

Alternate proposal to #37648 and #36822

This was referenced Oct 23, 2020
Closed
Closed
@ricardoasmarques
Copy link
Contributor

ricardoasmarques commented Oct 23, 2020 •
edited
Loading

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
Copy link
Contributor

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

@mgfritch
Copy link
Contributor Author

mgfritch commented Oct 23, 2020

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
Copy link
Contributor Author

mgfritch commented Oct 23, 2020 •
edited
Loading

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
Copy link
Member

l-mb commented Oct 23, 2020

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
Copy link
Contributor Author

mgfritch commented Oct 23, 2020

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
Copy link
Contributor

ricardoasmarques commented Oct 23, 2020

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 mgfritch added the DNM label Oct 27, 2020
@mgfritch mgfritch force-pushed the cephadm-no-container-init branch from 0387cfe to f2c4f04 Compare November 9, 2020 23:23
@mgfritch mgfritch removed the DNM label Nov 9, 2020
@mgfritch
Copy link
Contributor Author

mgfritch commented Nov 9, 2020

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
Copy link
Contributor Author

mgfritch commented Nov 9, 2020

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
Copy link
Contributor Author

mgfritch commented Nov 9, 2020

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 mgfritch requested review from a team, jmolmo, ricardoasmarques and tserong November 9, 2020 23:38
@mgfritch
Copy link
Contributor Author

mgfritch commented Nov 11, 2020

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
Copy link
Contributor

sebastian-philipp commented Nov 19, 2020

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

@github-actions
Copy link

github-actions bot commented Dec 16, 2020

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

@mgfritch
Copy link
Contributor Author

mgfritch commented Jan 21, 2021

blocked by #39007

@mgfritch mgfritch force-pushed the cephadm-no-container-init branch from f2c4f04 to 2d302ef Compare January 27, 2021 00:40
@mgfritch
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>
@mgfritch
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>
@mgfritch
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 mgfritch force-pushed the cephadm-no-container-init branch from 2d302ef to 5750589 Compare February 5, 2021 15:04
@mgfritch
Copy link
Contributor Author

mgfritch commented Feb 8, 2021

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

@sebastian-philipp sebastian-philipp merged commit f635555 into ceph:master Feb 8, 2021
@mgfritch mgfritch deleted the cephadm-no-container-init branch February 8, 2021 17:04
@sebastian-philipp sebastian-philipp mentioned this pull request Feb 9, 2021
Merged
liewegas added a commit to liewegas/ceph that referenced this pull request Feb 15, 2021
@liewegas
Revert "Merge pull request ceph#37764 from mgfritch/cephadm-no-contai…
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>
liewegas added a commit that referenced this pull request Feb 15, 2021
@liewegas
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>
jmolmo pushed a commit that referenced this pull request Feb 18, 2021
@liewegas @jmolmo
Revert "Merge pull request #37764 from mgfritch/cephadm-no-container-…
33c9453 
…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)
@mgfritch mgfritch mentioned this pull request Mar 8, 2021
Merged
3 tasks
myoungwon pushed a commit to myoungwon/ceph that referenced this pull request Mar 29, 2021
@liewegas @myoungwon
Revert "Merge pull request ceph#37764 from mgfritch/cephadm-no-contai…
178dd52 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmolmo jmolmo jmolmo approved these changes

@sebastian-philipp sebastian-philipp Awaiting requested review from sebastian-philipp

@ricardoasmarques ricardoasmarques Awaiting requested review from ricardoasmarques

@tserong tserong Awaiting requested review from tserong

+1 more reviewer

@jschmid1 jschmid1 jschmid1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm documentation needs-qa pybind wip-mgfritch-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mgfritch @ricardoasmarques @sebastian-philipp @l-mb @jmolmo @jschmid1 @tchaikov