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

ceph-detect-init: detect init system by poking the system #15043

Merged
merged 3 commits into from May 17, 2017

Conversation

Projects
None yet
3 participants
@tchaikov
Contributor

tchaikov commented May 11, 2017

@tchaikov tchaikov added the bug fix label May 11, 2017

@tchaikov tchaikov requested a review from May 11, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 11, 2017

Contributor

@dachary would you please take a look?

Contributor

tchaikov commented May 11, 2017

@dachary would you please take a look?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 11, 2017

This is tricky because we don't actually have integration tests for those platforms. It may be better to be very conservative and only add support for the missing operating system in the same way it was done before. IIRC a pitfall is that some operating system versions can have two init system installed at the same time. But ceph has only been tested on one of them. That being said, since we don't actually have teuthology tests to verify Ceph deploys and run, it's hard to tell what should work and what does not. Reason why I'd suggest a minimal change instead of a (sensible but uncertain) refactor. Does that make sense ?

ghost commented May 11, 2017

This is tricky because we don't actually have integration tests for those platforms. It may be better to be very conservative and only add support for the missing operating system in the same way it was done before. IIRC a pitfall is that some operating system versions can have two init system installed at the same time. But ceph has only been tested on one of them. That being said, since we don't actually have teuthology tests to verify Ceph deploys and run, it's hard to tell what should work and what does not. Reason why I'd suggest a minimal change instead of a (sensible but uncertain) refactor. Does that make sense ?

@gaudenz

This comment has been minimized.

Show comment
Hide comment
@gaudenz

gaudenz May 11, 2017

@dachary as Debian supports both sysvinit and systemd in at least jessie (current stable) and stretch (soon to be released new stable) the current logic which hardcodes one init system per Debian version just does not make sense. I much prefer the approach suggested in this PR.
The hardcoding would make at least some sense from Ceph's POV if it refused to run under sysvinit when installed on jessie or stretch, but the current packages still ship sysvinit scripts for these releases. And from a Debian POV this is a good thing as packages are supposed to still support sysvinit.

gaudenz commented May 11, 2017

@dachary as Debian supports both sysvinit and systemd in at least jessie (current stable) and stretch (soon to be released new stable) the current logic which hardcodes one init system per Debian version just does not make sense. I much prefer the approach suggested in this PR.
The hardcoding would make at least some sense from Ceph's POV if it refused to run under sysvinit when installed on jessie or stretch, but the current packages still ship sysvinit scripts for these releases. And from a Debian POV this is a good thing as packages are supposed to still support sysvinit.

@gaudenz

This comment has been minimized.

Show comment
Hide comment
@gaudenz

gaudenz May 11, 2017

@tchaikov Probably you should also remove the release name detection code in ceph_detect_init/init.py (lines 143-163) alltogether to not create any confusion as this is no longer used by your code.

gaudenz commented May 11, 2017

@tchaikov Probably you should also remove the release name detection code in ceph_detect_init/init.py (lines 143-163) alltogether to not create any confusion as this is no longer used by your code.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 11, 2017

Debian supports both sysvinit and systemd in at least jessie (current stable) and stretch (soon to be released new stable)

My point is that Ceph (not Debian) has only been tested with either sysvinit or systemd on Debian, not both. I'm not even sure where to look to find traces of the last integration tests regarding these.

ghost commented May 11, 2017

Debian supports both sysvinit and systemd in at least jessie (current stable) and stretch (soon to be released new stable)

My point is that Ceph (not Debian) has only been tested with either sysvinit or systemd on Debian, not both. I'm not even sure where to look to find traces of the last integration tests regarding these.

tchaikov added some commits May 11, 2017

ceph-detect-init: detect init system by poking the system not checkin…
…g its codename

ubuntu's codename is not strictly lexical ordered. and after Xenial,
Yakkity and Zesty, it will be Artful. so we can not just compare it with
'vivid' to tell if it is using systemd or upstart.

also, the current approach for debian only checks for squeeze and
wheezy. this is not flexible and per BTS#862075, it's wrong. so, we'd
better check the system by poking it instead of just looking at its
codename

---
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=862075

Fixes: http://tracker.ceph.com/issues/19884
Signed-off-by: Kefu Chai <kchai@redhat.com>
ceph-disk: remove unused methods
this is a follow-up of aac8971, which
replaced partx with partprobe. and we don't need to detect the platform
to decide if we need to use partx or not anymore with that commit
because we are dropping the support of distros w/o partprobe.

Fixes: http://tracker.ceph.com/issues/19884
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 12, 2017

Contributor

@dachary i think the problem is two folded with the precondition of: we want to be sure that Ceph behave on all supported distros/releases.

with this change, user could be using Ceph with untested combinations, like wheezy + systemd (systemd was introduced to wheezy as a technical preview) or jessie + sysvinit, and runs into unexpected issues. because we don't test Ceph on Debian or its derivative with these unknown combinations. so our user could run into unexpected problems. but what if he/she is able to take the risk, and really wants to use Ceph with the untested setting (jessie + sysvinit, for instance)? shall we error out in that case? IMHO, it would be better if we allow them to do it. for example, if rados is able to compile and run on OpenIndiana, shall we stop our user from using librados on that platform because we don't have any teuthology integration test for that combination?

when it comes to ceph-disk and ceph-detect-init, i think, probably we don't need to forbid the untested combinations. but instead, to put a warning in the README or release document or man page, so user is aware of the risk of experiencing undefined behavior.

in other words, if a user runs ceph-detect-init on jessie with only sysvinit installed, sysvinit is returned. if a user runs ceph-detect-init a system with both sysvinit and systemd installed, but only one of them is activated, we return the activated one, even if it is not tested in our integration test.

what do you think?

Contributor

tchaikov commented May 12, 2017

@dachary i think the problem is two folded with the precondition of: we want to be sure that Ceph behave on all supported distros/releases.

with this change, user could be using Ceph with untested combinations, like wheezy + systemd (systemd was introduced to wheezy as a technical preview) or jessie + sysvinit, and runs into unexpected issues. because we don't test Ceph on Debian or its derivative with these unknown combinations. so our user could run into unexpected problems. but what if he/she is able to take the risk, and really wants to use Ceph with the untested setting (jessie + sysvinit, for instance)? shall we error out in that case? IMHO, it would be better if we allow them to do it. for example, if rados is able to compile and run on OpenIndiana, shall we stop our user from using librados on that platform because we don't have any teuthology integration test for that combination?

when it comes to ceph-disk and ceph-detect-init, i think, probably we don't need to forbid the untested combinations. but instead, to put a warning in the README or release document or man page, so user is aware of the risk of experiencing undefined behavior.

in other words, if a user runs ceph-detect-init on jessie with only sysvinit installed, sysvinit is returned. if a user runs ceph-detect-init a system with both sysvinit and systemd installed, but only one of them is activated, we return the activated one, even if it is not tested in our integration test.

what do you think?

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 12, 2017

Contributor

changelog

  • remove not used code and tests

@gaudenz fixed and repushed

Contributor

tchaikov commented May 12, 2017

changelog

  • remove not used code and tests

@gaudenz fixed and repushed

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 12, 2017

@tchaikov this is a sane approach, good idea :-)

ghost commented May 12, 2017

@tchaikov this is a sane approach, good idea :-)

doc/8/man/ceph-detect-init: update with tested distro and init systems
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 12, 2017

Contributor

@dachary thanks =) i added a "Bugs" section in the related manpages.

Contributor

tchaikov commented May 12, 2017

@dachary thanks =) i added a "Bugs" section in the related manpages.

@ghost

ghost approved these changes May 12, 2017

@gaudenz

This comment has been minimized.

Show comment
Hide comment
@gaudenz

gaudenz May 12, 2017

@tchaikov @dachary Just as a note to anyone wanting to backport this to jewel: You also need at least the first part of commit e442f56 for the detection to work on Debian stretch. The Python platform detection code returns '' as the codename on Debian stretch.

gaudenz commented May 12, 2017

@tchaikov @dachary Just as a note to anyone wanting to backport this to jewel: You also need at least the first part of commit e442f56 for the detection to work on Debian stretch. The Python platform detection code returns '' as the codename on Debian stretch.

@gaudenz

This comment has been minimized.

Show comment
Hide comment
@gaudenz

gaudenz May 12, 2017

And of course it would be nice to have this backported to jewel as many people will still be running jewel at the point where they upgrade to stretch.

gaudenz commented May 12, 2017

And of course it would be nice to have this backported to jewel as many people will still be running jewel at the point where they upgrade to stretch.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 12, 2017

Contributor

@gaudenz thanks for the note, i marked the tracker ticket so the it can be backported after being resolved.

Contributor

tchaikov commented May 12, 2017

@gaudenz thanks for the note, i marked the tracker ticket so the it can be backported after being resolved.

@liewegas liewegas merged commit 570f576 into ceph:master May 17, 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

@tchaikov tchaikov deleted the tchaikov:wip-19884 branch May 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment