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

[WIP] Improve CoreOS distribution parsing #53563

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
7 participants
@samdoran
Copy link
Member

samdoran commented Mar 8, 2019

SUMMARY
  • use /etc/lsb-release for CoreOS since it contains all the information we need and /etc/coreos/update.conf didn't have much at all
  • add explicit check to ensure the system is CoreOS
  • create fixtures of distribution data for easy reuse during tests
  • update ClearLinux tests to use these fixtures
  • add tests for CoreOS distribution parsing
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/facts/system/distribution.py

Improve CoreOS distribution parsing
- use /etc/lsb-release for CoreOS since it contains all the information we need and /etc/coreos/update.conf didn't have much at all
- add explicit check to ensure the system is CoreOS
- create fixtures of distribution data for easy reuse during tests
- update ClearLinux tests to use these fixtures
- add tests for CoreOS distribution parsing
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 8, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

test/units/module_utils/facts/system/distribution/test_parse_distribution_file_ClearLinux.py:11:0: relative-beyond-top-level Attempted relative import beyond top-level package
test/units/module_utils/facts/system/distribution/test_parse_distribution_file_Coreos.py:11:0: relative-beyond-top-level Attempted relative import beyond top-level package

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Mar 8, 2019

@Akasurde

This comment has been minimized.

Copy link
Member

Akasurde commented Mar 10, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 10, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

test/units/module_utils/facts/system/distribution/test_parse_distribution_file_ClearLinux.py:11:0: relative-beyond-top-level Attempted relative import beyond top-level package
test/units/module_utils/facts/system/distribution/test_parse_distribution_file_Coreos.py:11:0: relative-beyond-top-level Attempted relative import beyond top-level package

click here for bot help

@samdoran samdoran changed the title Improve CoreOS distribution parsing [WIP] Improve CoreOS distribution parsing Mar 11, 2019

@ansibot ansibot added the WIP label Mar 11, 2019

@samdoran

This comment has been minimized.

Copy link
Member Author

samdoran commented Mar 11, 2019

After discussing with the CoreOS folks a bit, I need to do some more work to get this correct. Notes from my discussion with them:

  • There are three CoreOS distributions:
    1. Container Linux by CoreOS
    2. Red Hat Enterprise Linux CoreOS (RHCOS)
    3. Fedora CoreOS (FCOS)
  • /etc/os-release is the file we should parse for distro information
  • The GROUP value from /usr/share/coreos/update.conf should be used for ansible_facts.release. The codename of a release is not useful.
  • RHCOS and FCOS should report ansible_facts.os_family as RedHat

This means we will have to parse multiple files and pass that in to parse_distribution_file_Coreos().

@ashcrow

This comment has been minimized.

Copy link

ashcrow commented Mar 11, 2019

@samdoran looks right to me. Please note that, at least on RHCOS, the update.conf file referenced above does not exist.

$ cat /usr/share/coreos/update.conf
cat: /usr/share/coreos/update.conf: No such file or directory
$ 
@samdoran

This comment has been minimized.

Copy link
Member Author

samdoran commented Mar 11, 2019

@ashcrow Thank you for the information. Is there something appropriate from /etc/os-release you can think would be a good fallback to use for the value of ansible_facts.release? That's the only piece of information we would get from update.conf.

On other platforms, this seems to be the value in parenthesis from PRETTY_NAME.

        "ansible_distribution": "Coreos",
        "ansible_distribution_file_parsed": true,
        "ansible_distribution_file_path": "/etc/lsb-release",
        "ansible_distribution_file_variety": "Coreos",
        "ansible_distribution_major_version": "1911",
        "ansible_distribution_release": "rhyolite",
        "ansible_distribution_version": "1911.5.0",
        "ansible_os_family": "Coreos",

        "ansible_distribution": "CentOS",
        "ansible_distribution_file_parsed": true,
        "ansible_distribution_file_path": "/etc/redhat-release",
        "ansible_distribution_file_variety": "RedHat",
        "ansible_distribution_major_version": "7",
        "ansible_distribution_release": "Core",
        "ansible_distribution_version": "7",
        "ansible_os_family": "RedHat",

        "ansible_distribution": "Debian",
        "ansible_distribution_file_parsed": true,
        "ansible_distribution_file_path": "/etc/os-release",
        "ansible_distribution_file_variety": "Debian",
        "ansible_distribution_major_version": "8",
        "ansible_distribution_release": "jessie",
        "ansible_distribution_version": "8",
        "ansible_os_family": "Debian",

        "ansible_distribution": "Fedora",
        "ansible_distribution_file_parsed": true,
        "ansible_distribution_file_path": "/etc/redhat-release",
        "ansible_distribution_file_variety": "RedHat",
        "ansible_distribution_major_version": "28",
        "ansible_distribution_release": "Twenty Eight",
        "ansible_distribution_version": "28",
        "ansible_os_family": "RedHat",

        "ansible_distribution": "RedHat",
        "ansible_distribution_file_parsed": true,
        "ansible_distribution_file_path": "/etc/redhat-release",
        "ansible_distribution_file_search_string": "Red Hat",
        "ansible_distribution_file_variety": "RedHat",
        "ansible_distribution_major_version": "7",
        "ansible_distribution_release": "Maipo",
        "ansible_distribution_version": "7

> ansible lab-coreos,lab-deb8,lab-fed28,lab-rhel7,lab-centos7 -a 'cat /etc/os-release'

lab-deb8 | CHANGED | rc=0 >>
PRETTY_NAME="Debian GNU/Linux 8 (jessie)"
NAME="Debian GNU/Linux"
VERSION_ID="8"
VERSION="8 (jessie)"
ID=debian
HOME_URL="http://www.debian.org/"
SUPPORT_URL="http://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

lab-coreos | CHANGED | rc=0 >>
NAME="Container Linux by CoreOS"
ID=coreos
VERSION=1911.5.0
VERSION_ID=1911.5.0
BUILD_ID=2018-12-15-2317
PRETTY_NAME="Container Linux by CoreOS 1911.5.0 (Rhyolite)"
ANSI_COLOR="38;5;75"
HOME_URL="https://coreos.com/"
BUG_REPORT_URL="https://issues.coreos.com"
COREOS_BOARD="amd64-usr"

lab-centos7 | CHANGED | rc=0 >>
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"

lab-rhel7 | CHANGED | rc=0 >>
NAME="Red Hat Enterprise Linux Server"
VERSION="7.5 (Maipo)"
ID="rhel"
ID_LIKE="fedora"
VARIANT="Server"
VARIANT_ID="server"
VERSION_ID="7.5"
PRETTY_NAME="Employee SKU"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:redhat:enterprise_linux:7.5:beta:server"
HOME_URL="https://www.redhat.com/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"

REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7"
REDHAT_BUGZILLA_PRODUCT_VERSION=7.5
REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux"
REDHAT_SUPPORT_PRODUCT_VERSION="7.5 Beta"

lab-fed28 | CHANGED | rc=0 >>
NAME=Fedora
VERSION="28 (Twenty Eight)"
ID=fedora
VERSION_ID=28
PLATFORM_ID="platform:f28"
PRETTY_NAME="Fedora 28 (Twenty Eight)"
ANSI_COLOR="0;34"
CPE_NAME="cpe:/o:fedoraproject:fedora:28"
HOME_URL="https://fedoraproject.org/"
SUPPORT_URL="https://fedoraproject.org/wiki/Communicating_and_getting_help"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=28
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=28
PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy"

That would be Rhyolite on the particular Container Linux instance I'm testing against, which we already determined isn't very useful.

@ashcrow

This comment has been minimized.

Copy link

ashcrow commented Mar 11, 2019

The best I can come up with would be to re-use the ID for RHCOS as we don't use release names ourselves. That would mean it would return rhcos.

@ashcrow

This comment has been minimized.

Copy link

ashcrow commented Mar 11, 2019

/cc @bgilbert

@ashcrow

This comment has been minimized.

Copy link

ashcrow commented Mar 11, 2019

@dustymabe

This comment has been minimized.

Copy link
Contributor

dustymabe commented Mar 11, 2019

For Fedora CoreOS here is where we decided what would go in /etc/os-release. This is what it looks like today:

NAME=Fedora
VERSION="29 (CoreOS preview)"
ID=fedora
VERSION_ID=29
VERSION_CODENAME=""
PLATFORM_ID="platform:f29"
PRETTY_NAME="Fedora 29 (CoreOS preview)"
ANSI_COLOR="0;34"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:29"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f29/system-adminii
strators-guide/"
SUPPORT_URL="https://fedoraproject.org/wiki/Communicating_and_getting_help"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=29
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=29
PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy"
VARIANT="CoreOS preview"
VARIANT_ID=coreos

It almost looks like rather than parsing PRETTY_NAME for that info we should be using a separate variable. The VERSION_CODENAME variable looks like a good idea but that appears unpopulated.

@samdoran

This comment has been minimized.

Copy link
Member Author

samdoran commented Mar 12, 2019

@dustymabe Thank you very much. This is very helpful.

@bgilbert

This comment has been minimized.

Copy link

bgilbert commented Mar 12, 2019

@samdoran /usr/share/coreos/update.conf is only relevant for Container Linux, and /etc/coreos/update.conf should also be checked for the GROUP (if that file exists), with the latter file winning in case of conflict. Users may sometimes override the GROUP in /etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.