Skip to content
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

mimic: ceph-detect-init: stop using platform.linux_distribution #21523

Merged
merged 1 commit into from Jul 18, 2018

Conversation

@smithfarm
Copy link
Contributor

commented Apr 19, 2018

Fixes: http://tracker.ceph.com/issues/18163
Signed-off-by: Nathan Cutler ncutler@suse.com

@smithfarm smithfarm requested review from tchaikov and rjfd Apr 19, 2018
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

Will need to mock the /etc/os-release file for the unit tests.

@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from 158b3d3 to b4b63b0 Apr 19, 2018
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

passes py3 tox tests locally

Obviously, this will need manual testing on all supported distros (will do with Docker)

@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from d8cb1cc to a67b483 Apr 19, 2018
logging.debug("platform_information: ",
"Error while opening %s : %s" % (file_name, err))
else:
linux_distro = platform.linux_distribution(

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 20, 2018

Contributor

have you considered using the distro package from pypi?

This comment has been minimized.

Copy link
@smithfarm

smithfarm Apr 20, 2018

Author Contributor

@tchaikov It seems like overkill given that:

  • all the Linux distros we're targeting have /os/release and are easily testable
  • use of distro would introduce a new runtime dependency
  • ceph-detect-init is deprecated in Mimic and will be dropped in Nautilus
@smithfarm smithfarm requested review from tserong and liewegas Apr 20, 2018
data = f.read()
linux_distro = (
_extract_from_os_release(data, 'ID'),
_extract_from_os_release(data, 'VERSION'),

This comment has been minimized.

Copy link
@tserong

tserong Apr 21, 2018

Contributor

It's possible for VERSION to not be present (it's not there in /etc/os-release on openSUSE Tumbleweed), which probably means the SUSE version of choose_init() will do the wrong thing. Maybe try using VERSION_ID instead. There's a bunch of example os-release files in https://github.com/ceph/ceph-deploy/pull/453/files if it helps. But otherwise, I'm happy with just parsing os-release rather than using distro from pypi for the reasons you've stated.

@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from a67b483 to b59c61d Apr 24, 2018
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

Quoting #21650

# cat /etc/os-release

NAME="openSUSE Tumbleweed"
# VERSION="20180424"
ID="opensuse-tumbleweed"
ID_LIKE="opensuse suse"
VERSION_ID="20180424"
PRETTY_NAME="openSUSE Tumbleweed"
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:opensuse:tumbleweed:20180424"
BUG_REPORT_URL="https://bugs.opensuse.org"
HOME_URL="https://www.opensuse.org/"
def choose_init():
"""Select a init system
Returns the name of a init system (upstart, sysvinit ...).
"""
if release and int(release.split('.')[0]) >= 7:
if release and _rh_major_version(release) >= 7:

This comment has been minimized.

Copy link
@tserong

tserong May 1, 2018

Contributor

_rh_major_version() probably isn't necessary since switching from VERSION to VERSION_ID (the latter should be only numbers, AFAIK)

@smithfarm smithfarm changed the base branch from master to mimic Jul 4, 2018
@smithfarm smithfarm added this to the mimic milestone Jul 4, 2018
@smithfarm smithfarm added the DNM label Jul 4, 2018
@smithfarm smithfarm changed the title ceph-detect-init: stop using platform.linux_distribution [DNM] ceph-detect-init: stop using platform.linux_distribution Jul 4, 2018
@smithfarm smithfarm changed the title [DNM] ceph-detect-init: stop using platform.linux_distribution [DNM] mimic: ceph-detect-init: stop using platform.linux_distribution Jul 4, 2018
@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch 4 times, most recently from 0b72da2 to 35673ae Jul 17, 2018
@smithfarm smithfarm removed the DNM label Jul 17, 2018
@smithfarm smithfarm changed the title [DNM] mimic: ceph-detect-init: stop using platform.linux_distribution mimic: ceph-detect-init: stop using platform.linux_distribution Jul 17, 2018
@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from 35673ae to ca57eb3 Jul 17, 2018
@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from ca57eb3 to 5311179 Jul 17, 2018
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

Ideally this would go through a ceph-disk suite before being merged.

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@tserong I addressed your review comment and added unit tests. Care to take another look?

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

Manual testing notes:

  1. unpatched, ceph-detect-init detects that it is running in docker and returns "none". I got around this by monkey-patching
  2. the heuristics for detecting systemd and upstart on Ubuntu/Debian are broken in docker containers (I guess that's why unpatched ceph-detect-init detects that it is running in a docker container and avoids running these heuristics)

Tested (on a real system) that ceph-detect-init with this patch correctly returns systemd in the following environment:

  • openSUSE Leap 42.3

Tested (using docker containers) that ceph-detect-init with this patch correctly returns systemd in the following environments:

  • CentOS 7
  • openSUSE Leap 15.0

Tested (using docker containers) that the platform_information sets distro and release correctly:

  • Ubuntu Xenial
  • Ubuntu Trusty
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

pep8 runtests: commands[0] | flake8 ceph_detect_init tests
tests/test_all.py:436:80: E501 line too long (84 > 79 characters)
tests/test_all.py:468:80: E501 line too long (81 > 79 characters)
tests/test_all.py:481:80: E501 line too long (90 > 79 characters)
tests/test_all.py:484:80: E501 line too long (81 > 79 characters)
This commit introduces a new function, _extract_from_os_release, which is
used (only on Linux) to extract the ID and VERSION_ID fields from the
/etc/os-release file. This is sufficient to positively determine the OS
and version on these systems, and enable ceph-detect-init to correctly
return systemd on them.

The commit includes unit tests asserting that _extract_from_os_release really
extracts the strings that we expect it to, using the contents of
/etc/os-release from a number of different operating systems targeted by Ceph.

This commit is not cherry-picked because ceph-detect-init has been dropped in
master.

Fixes: http://tracker.ceph.com/issues/18163
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm smithfarm force-pushed the smithfarm:wip-18163 branch from 5311179 to 12d94ca Jul 17, 2018
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

Unit tests should pass now, and I am satisfied it will work on all the distros mimic is targeting.

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

@tserong

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

@tserong I addressed your review comment and added unit tests. Care to take another look?

LGTM

@yuriw yuriw merged commit 9fb543b into ceph:mimic Jul 18, 2018
4 checks passed
4 checks passed
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
@smithfarm smithfarm deleted the smithfarm:wip-18163 branch Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.