=== medberry is now known as Guest62131
[18:23] <danMS> blackboxsw: i uploaded the collect-logs output for 1779207, lmk if you need anything else?
[19:32] <blackboxsw> thanks danMS !
[19:32] <blackboxsw> oops rharper: $ lxc exec test-xenial -- python3 -c 'from platform import dist; from cloudinit.util import get_linux_distro; print(dist()); print(get_linux_distro())'
[19:32] <blackboxsw> ('Ubuntu', '16.04', 'xenial')
[19:32] <blackboxsw> ('ubuntu', '16.04', 'x86_64')
[19:32] <blackboxsw> so cloud-init's new get_linux_distro is actually returning arch instead of series
[19:32] <rharper> eww
[19:32] <rharper> how do we not have unittest for that ?
[19:33] <blackboxsw> I think it's all mocked, but checking now
[19:33] <rharper> there's no reason for a mock
[19:33] <rharper> at least not under the util path
[19:33] <rharper> certainly for distro objects
[19:33]  * blackboxsw wonders what the impact would have been here. (and why it didn't show up as a problem)
[19:33] <rharper> but unittesting get_linux_distro shouldn't ; we could mock the read() results from parsing what os-release looks like
[19:34] <rharper> we only use release in one place
[19:34] <rharper> under ntp for ubuntu
[19:34] <rharper> generally, getting 'ubuntu' vs. 'redhat' is the variant that we key on
[19:34] <rharper> possible the opensusu/sles stuff for ntp could fail as well though I suspect the code works fine with the os-release in opensuse
[19:35] <blackboxsw> hrm all unit tests were using platform.machine() as the third entry in the tuple
[19:35] <rharper> which gives me x86_64
[19:35] <rharper> on my xenial machine
[19:35] <blackboxsw> which made an incorrect assumption about the orig output of platform.dist()
[19:36] <blackboxsw> yeah, that's a valid return, but platform.dist() on your system reports series as 3rd element
[19:36] <rharper> yes
[19:36] <rharper> platform.dist()
[19:36] <rharper> ('centos', '7.4.1708', 'Core')
[19:36] <rharper> that's even stranger
[19:36] <rharper> ah, that's its code name
[19:37]  * blackboxsw checks sles
[19:38] <rharper> hrm, machine just looks wrong; not sure why we didn't catch that
[19:39] <blackboxsw> hrm ('SuSE', '42.3', 'x86_64')
[19:39] <blackboxsw> on suse that is what it returns
[19:39] <rharper> right, that path looks like it's not expected to be taken
[19:40] <rharper> how do you get that path anyhow ?
[19:40] <rharper> oh I see, parsing os-release path
[19:40] <blackboxsw> sorry not following. right, we process /etc/os-release
[19:40] <rharper> I see it now; it's like we're missing the last tuple being read out of os-release
[19:41] <rharper> in Xenial, it should be VERSION_CODENAME
[19:41] <rharper> and there is no such field in redhat/centos
[19:44] <rharper> yuck, so in centos/redhat PRETTY_NAME includes the codename, but ubuntu's PRETTY_NAME has Ubuntu 16.04 LTS
[19:45] <rharper> so no single entry works to pick out element 3 of the platform.dist() tuple
[19:45] <blackboxsw> from sles https://pastebin.ubuntu.com/p/q89fsmwBVD/
[19:46] <rharper> I wonder if they have a codename
[19:48] <blackboxsw> yeah I'm filing a bug, SRU-blocker :/
[19:49] <blackboxsw> rharper: system_info()['dist'] series is used by cloudinit/distros/ubuntu.py for ntp client choices
[19:49] <blackboxsw> so it'll break on xenial
[19:49] <blackboxsw> coding up a fix :/
[19:50] <blackboxsw> weird behavior on sles.
[19:50] <blackboxsw> platform.machine instead of codename or series
[19:51] <blackboxsw> it was my original review on that branch. I should've noted it :/
[19:57] <rharper> blackboxsw: yeah =(
[19:58] <rharper> I think bringing in the major distro os-release files into the unittest to verify we pull out the bits we want is going to be needed
[19:59] <blackboxsw> yeah can add that in this pass
[19:59] <blackboxsw> thank lxc for ease of testing these envs
[19:59] <rharper> the ci tests with different distros could (should) verify we get what we want out of /etc/os-release ?
[19:59] <rharper> yeah
[20:08] <blackboxsw> ok filed https://bugs.launchpad.net/cloud-init/+bug/1780481
[20:08] <ubot5> Ubuntu bug 1780481 in cloud-init "ubuntu/centos/debian: get_linux_distro has different behavior than platform.dist" [Undecided,New]
[20:14] <blackboxsw> hrm rharper robjo this also won't work exactly as it currently does on opensuse 42.3,
[20:14] <blackboxsw> sles42:~ # python3 -c 'from platform import dist; print(dist());'
[20:14] <blackboxsw> ('SuSE', '42.3', 'x86_64')
[20:14] <blackboxsw> but per /etc/os/release: ID=opensuse
[20:15] <rharper> =(
[20:15] <blackboxsw> so cloudinit.util.get_linux_distro would return ('opensuse',...) instead of SuSE
[20:15] <rharper> shoudl we back this out?
[20:15] <rharper> we've not crossed the EOL for platform.dist() yet
[20:15] <rharper> blackboxsw: we're both out next week, but smoser is back
[20:15] <blackboxsw> I think it's worth backing it out as it doesn't seem to work on all release of any given distribution
[20:16] <robjo> in opensuse 42.3 cloud-init runs with python 2 and because .dist works the os-release is not read, or should not be read
[20:17] <robjo> backing it out will break SLES 15 which will be released on monday and is way more important to us than openSUSE
[20:17] <robjo> I am pretty certain I tested on openSUSE and things worked as expected
[20:17] <blackboxsw>  robjo I thought the get_linux_distro code sources /etc/os-release as primary option if file exists
[20:17] <robjo> let me double check
[20:18] <rharper> robjo: do you have a sles15 os-release handy while you're looking ?
[20:18] <rharper> while we're triaging the bug blackboxsw filed, best to collect as much details so we can see how best to sort it out;
[20:18] <rharper> robjo: we certainly don't want to break any distro
[20:18] <blackboxsw> robjo: trying to validate now
[20:19] <blackboxsw> sles42:~ # python -c 'from cloudinit.util import get_linux_distro; from platform import dist; print(dist()); print(get_linux_distro())'
[20:19] <blackboxsw> ('SuSE', '42.3', 'x86_64')
[20:19] <blackboxsw> ('opensuse', '42.3', 'x86_64')
[20:19] <blackboxsw> so here's the diff on opensuse 42.3
[20:19] <blackboxsw> it's like we need a mapping from ID -> python_dist_value
[20:21] <robjo> but this is used for variant definition and I handled all cases and added opensuse as needed
[20:21] <robjo> why is this popping up?
[20:22] <rharper> processing os-release on xenial results i getting x86_64 and no variant
[20:23] <rharper> centos is the same
[20:23] <rharper> platform.dist() tuple returns the 'codename'  of the release
[20:24] <blackboxsw> yeah, surfacing just platform.machine() in all distros doesn't match platform.dist() output :/
[20:24] <robjo> so os-release on xenial does not confirm to the freedesktop.org format of os-release?
[20:25] <robjo> s/confirm/conform/
[20:25]  * blackboxsw tries to find a spec on that.   I don't think it's /etc/os-release related that's the problem, it's platform.dist() reporting different content on different distros
[20:26] <rharper> reading os-release means that the third tuple is platform.machine()
[20:26] <rharper> that doesn't match on centos nor on ubuntu
[20:26] <rharper>  platform.dist()
[20:26] <rharper> ('centos', '7.4.1708', 'Core')
[20:26] <rharper> platform.dist()
[20:26] <rharper> ('Ubuntu', '16.04', 'xenial')
[20:26] <blackboxsw> on sles-based it seems platform.machine() as the 3rd element is reasonable. but centos/rhel/ubuntu/debian it breaks conformance as 3rd element is series
[20:26] <blackboxsw> series/flavor/codename
[20:27] <rharper> we don't use the variant much, but at least in the ntp path on Xenial, we are using it there
[20:27] <blackboxsw> yeah that's the only place in cloudinit that it is being used
[20:27] <rharper> I wonder why python on sles returns the machine ?
[20:28] <rharper> or why ubuntu and redhat/centos return the codename ?
[20:28] <blackboxsw> and why on opensuse it return 'SuSE' instead (which also isn't represented in /etc/os-release)
[20:28] <robjo> touche sample size of one :(, so platform.machine() needs to be replaced by whatever returns the flavor, I think that would be codename in os-release
[20:28] <rharper> yes
[20:28] <rharper> but
[20:28] <rharper> CODENAME is not present in all os-releases =(
[20:28] <rharper> what a PITA
[20:29] <rharper> the 'Core' in os-release in centos/redhat is part of the VERSION_ID but the value CODENAME is not set
[20:30] <blackboxsw> and Core for centos' os-release would work for debian
[20:30] <blackboxsw> PRETTY_NAME="Debian GNU/Linux 9 (stretch)"
[20:30] <blackboxsw> match the (<codename>) re
[20:30] <rharper> yeah, but we should try to get as many of these
[20:31] <blackboxsw> but codename doesn't matter for debian.... root@debStretch:~# python -c 'from platform import dist; print(dist());'
[20:31] <blackboxsw> ('debian', '9.4', '')
[20:31] <rharper> rolling our own sorta hurts here
[20:31] <rharper> lol
[20:31] <blackboxsw> yeah I think I can draw up a fairly simple fixup if we need to keep this changeset in. At least it should get us through on debian/ubuntu/centos....
[20:32] <rharper> we should get an os-release of SLES 15 from robjo if we can
[20:32] <blackboxsw> since robjo doesn't get affected by ('opensuse',...) instead of 'SuSE' we probably don't need a distro-release-lookup map to sort those differences
[20:32] <rharper> and the python2/python3 platform.dist() output as well
[20:32] <rharper> just for completeness sake
[20:32] <blackboxsw> +1 robjo if possible. I know you included snippets in unit tests
[20:33] <blackboxsw> in cloudinit/tests/test_util.py
[20:33] <blackboxsw> I can add an opensuse42.3  one too as I have that on hand
[20:34] <blackboxsw> then at least we document existing behavior and only resolve the mapping opensuse -> SuSE if need be
[20:34] <robjo> # cat /etc/os-release
[20:34] <robjo> NAME="SLES"
[20:34] <robjo> VERSION="15"
[20:34] <robjo> VERSION_ID="15"
[20:34] <robjo> PRETTY_NAME="SUSE Linux Enterprise Server 15"
[20:35] <robjo> ID="sles"
[20:35] <robjo> ID_LIKE="suse"
[20:35] <robjo> ANSI_COLOR="0;32"
[20:35] <robjo> CPE_NAME="cpe:/o:suse:sles:15"
[20:35] <robjo> SO we could do "if codename in os-release -> use it, else platform.machine()" that is is xenial sets codename ;)
[20:45] <rharper> robjo: yes, that sounds right
[20:47] <robjo> blackboxsw: rharper one of you handling this or you want me to?
[20:48] <blackboxsw> robjo: on it no worries I'll assign you as a reviewer on the brnach
[20:48] <robjo> I will not get to it until Monday, in the middle of pushing out EC2 images
[20:48] <blackboxsw> +1 robjo
[20:48] <robjo> OK, cool thanks
[20:48] <blackboxsw> no worries
[21:25] <blackboxsw> robjo: rharper branch pushed a branch for review whenever https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/349081
[21:25] <blackboxsw> I'll block cloud-init's SRU on this