| === medberry is now known as Guest62131 | ||
| danMS | blackboxsw: i uploaded the collect-logs output for 1779207, lmk if you need anything else? | 18:23 |
|---|---|---|
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| 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:36 |
| * blackboxsw checks sles | 19:37 | |
| rharper | hrm, machine just looks wrong; not sure why we didn't catch that | 19:38 |
| 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:39 |
| 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:40 |
| rharper | in Xenial, it should be VERSION_CODENAME | 19:41 |
| rharper | and there is no such field in redhat/centos | 19:41 |
| rharper | yuck, so in centos/redhat PRETTY_NAME includes the codename, but ubuntu's PRETTY_NAME has Ubuntu 16.04 LTS | 19:44 |
| 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:45 |
| rharper | I wonder if they have a codename | 19:46 |
| blackboxsw | yeah I'm filing a bug, SRU-blocker :/ | 19:48 |
| 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:49 |
| blackboxsw | weird behavior on sles. | 19:50 |
| blackboxsw | platform.machine instead of codename or series | 19:50 |
| blackboxsw | it was my original review on that branch. I should've noted it :/ | 19:51 |
| rharper | blackboxsw: yeah =( | 19:57 |
| 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:58 |
| 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 | 19:59 |
| 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:08 |
| 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:14 |
| 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:15 |
| 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:16 |
| 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:17 |
| 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:18 |
| 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:19 |
| 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:21 |
| rharper | processing os-release on xenial results i getting x86_64 and no variant | 20:22 |
| rharper | centos is the same | 20:23 |
| rharper | platform.dist() tuple returns the 'codename' of the release | 20:23 |
| 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:24 |
| 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:25 | |
| 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:26 |
| 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:27 |
| 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:28 |
| rharper | the 'Core' in os-release in centos/redhat is part of the VERSION_ID but the value CODENAME is not set | 20:29 |
| 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:30 |
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| 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:35 |
| rharper | robjo: yes, that sounds right | 20:45 |
| robjo | blackboxsw: rharper one of you handling this or you want me to? | 20:47 |
| 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 | 20:48 |
| 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 | 21:25 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!