[00:12] smoser: shall I land https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/343122 ? [00:51] I'm for it as mentioned, I think we can do a followup with something dynamic if needed. [00:51] I'd like us to continue spending time on the CLI, so there will be ample opportunity if needed to touch this again and make it dynamic at that point. [00:51] as it is for now it's fast and correct and 'done' so I'm all for that [00:52] rharper: yeah. [00:53] we can borrow some simple regex cli --help parsing that landscape-api CLI did for the next iteration, it won't be too bad to pull in. [00:53] i consider myself more competant than one should be with shell scripting [00:53] and compgen stuff just makes me cringe [00:55] seen here is another option. but yeah uses compgen too https://bazaar.launchpad.net/~landscape/landscape/trunk/view/head:/canonical/landscape/api/client/etc/bash_completion.d/landscape-api [00:55] and yeah, I'm all for something simple if we could get away with it [00:57] smoser: sorry I'm late on the review comments of ifconfig->iproute2 stuff, still massaging unit tests for the switch to look before you leap (i.e. which('ip') elif which('ifconfig') ) [00:58] FTR, I agree with the approach to testing for the cmd before running it. instead of all the CalledProcessError handling [00:58] blackboxsw: yeah, i knew that that was going to be a pain for your tests [00:58] one way or another we shoudl decide "we are using 'ip'" and then use it [01:00] yeah right, instead of separate tests in each separate function (ip vs netstat or ip vs ifconfig) [01:00] yeah just a has_iproute2 would be easy, only one thing to mock then. [01:01] I might just make that change now, as it's too late to get this fixed and back to you for review tonight anyway [01:04] meh, n/m I retract that statement, as we also check which(netstat) and which(ifconfig) and have different error messages/failure paths to handle, so the consolidation doesn't buy much as we'd have to still handle ProcessExecutionErrors and log warnings if the 2nd command we run isn't present anyway [01:04] ahh well [01:05] freebsd uses ifconfig i believe ? [01:05] other than that i would be fine to assume that if you have netstat then you have ifconfig [01:05] they';re' both from net-tools [01:06] smoser: ok, pushing bash completion [01:10] blackboxsw: Yeah, regex parsing is one way though I think the parsers are relegated to other files, so we can't just grep through cmd/main.py; the recursive --help invokation may be more robust and we can do that at build-time so we have a static bash file. [01:31] +1 rharper we can create a ./tool to gen that static file from recursive --help invocations [01:31] good pt [01:32] could do it at build time. or just run it on a branch in progress prior to commit === pmcnabb_ is now known as pmcnabb [16:13] blackboxsw: have a minute ? [16:22] yes definitely [16:22] smoser: heading to hangout [17:11] rharper: smoser dropped Metric column from route info output to retain original behavior. Only differences in behavior to what currently exists in bionic when relying on ip instead of ifconfig/netstat are that we do the following: [17:11] 1. properly print full scope name for addrs as presented by ip [17:11] 2. drop trailing ':' in the device column [17:12] 3. print ipv6 addrs instead of '.' [17:12] 4. print an ipv6 routes table [17:12] smoser: rharper the diff on an ec2 instance: https://pastebin.ubuntu.com/p/KZWYPbCRvC/ [17:14] are we consistent with o urselves now ? [17:14] between ifconfig and ip [17:18] blackboxsw: my freebsd seems not working on server stack :-( [17:18] * blackboxsw is trying freebsd on ec2 [17:41] blackboxsw: can you run ec2 with ipv6 addr so we can see the diff? I didn;t think we displayed link-local ip6 addrs; and we don't split out v4 and v6 for netdev, but we do for route ? [17:43] rharper spinning one up now [17:43] blackboxsw: w.r.t the interface to the various parse output methods; as smoser has told me, if you don't want someone using the modules as an api, then you need to use _ prefix [17:44] otherwise, those methods in netinfo *are* an api, and that means callers could pass non-strings to you [17:44] +1, I should change that [17:44] if smoser doesn't mind, then I'm OK with not adding checks on the inputs, but it seems reasonable to me to set those as input=None, and then add if not input: input = "" [17:45] or something safer; [17:48] why set a default ? [17:48] they require input [17:52] one way of ensuring you don't blow up doing .split() on a non string for typical usage, None is more comment than passing in a dict; or raise valueerror on the wrong type [17:52] rharper: well, we really aren't providing an api. we're just not really doing that. [17:52] smoser: then it I think blackboxsw will end up marking the methods with _ [17:52] we should, and ideally would. but just really are not. [17:54] rharper: there is no reason to provide a default variable [17:54] changing [17:54] def netdev_info_iproute(ipaddr_out): [17:54] to [17:54] def netdev_info_iproute(ipaddr_out=None) [17:54] is contrary to useful [17:54] sure [17:54] raising a TypeError if ipaddr_out is not a string is fine. [17:55] yeah +1 not defaulting as we don't want users calling netdev_info_iproute(). it needs something to parse [17:55] TypeError I balked at because our code is the only caller and it stuffs in util.subp output which is a known quantity of a string type [17:56] right. [17:56] so that is why i didnt' feel we need it. [17:56] so it's a case where I didn't feel users could likely induce a failure path by presenting an invalid type [17:57] at this point we're not *really* providing an api. no one that is using cloud-init as a python library (at thsi point) is surprised by that :) [17:57] that isnt that we shouldnt. just that we're imature. [17:57] if we were, say, processing user-provided cloud-config information which we've loaded into a dict, then I'd be more concerned about checking types of data etc. [17:57] yeah, i largely agree with that. [17:57] but i'm also OK to just shove a _ on it too. [17:58] yeah I'll do that _ as I feel like for our own sanity it's nice to designate what we think should really have internal (to the module) callers [17:58] it's a good function/method documentation policy [18:07] smoser: thanks for landing those [18:20] rharper: ec2 w/ ipv6 address [18:20] https://pastebin.ubuntu.com/p/96qtpRpdxV/ [18:21] thx [18:21] so, the link-local is new, as is the lo in the routing v6 table [18:22] should we clean those up? unreachable seems unhelpful, on the fence on link-local ipv6 values, we're not showing link-local v4 stuff [18:22] link local is new ? [18:22] printing link-local values in the net dev info table [18:22] yeah, look at the 17.2 vs 18.2 [18:23] yeah it was previously just . [18:23] just absent I mean [18:43] blackboxsw: so what are you doing with all that ? [18:43] are we looking to make output identical to previous ? [18:45] smoser: rharper I just wanted to see if you guys had concerns about that behavior and if you think we should avoid printing link local info like line 29 of https://pastebin.ubuntu.com/p/96qtpRpdxV/ [18:46] and if line 27 and 30 should really be like 8 and 10 [18:46] do we want to continue to not process/report certain data, even though I fixed what I felt was a bug in the ommission [18:46] I dunno; my use of that table is mostly restricted to seeing what networking as applied, typically the ipv4/v6 addresses, I don't think for clouds many users will ahve a second instance in the same network where they could reach it via link local [18:47] i dont think line 29 is bad. [18:48] also the whole table line 40+ (ipv6 routes) is 'new' [18:48] and i'm fine with 27 and 30 too [18:49] i think they're generally improvements. [18:50] blackboxsw: for the ipv6 output [18:50] i think there is no reasno to show routes on interface lo [18:50] (they're not seen on ipv4) [18:52] +1 will drop them [18:52] for SRU fodder, would we want to drop the ipv6 route table altogether [18:52] ? [18:53] or it's just more output that users could take advantage of (not really change in existing functionality presented by other tables) [18:53] i think its fine. idont really believe any tools are likely scraping that. [18:53] cool sound good rharper ? [18:56] blackboxsw: yeah, I agree with smoser [19:11] blackboxsw: can i help you with anything ? [19:13] all good. just pushed all changes [19:14] was going to make deb one more time and test lxc and ec2 while waiting for ci [19:14] smoser: rharper all done [19:14] nice [19:14] will pastebin one more pass [19:14] rharper: i have updates to https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/343246 and responses to your comments [19:18] done smoser rharper https://pastebin.ubuntu.com/p/dK8JBmdpNk/ [19:18] looks good [19:19] I think we can land it and I'll put up a merge proposal for bionic if that sounds good? [19:21] https://jenkins.ubuntu.com/server/job/cloud-init-ci/1025/ in progress [19:33] * blackboxsw will be back in 30 mins [19:41] https://www.diffchecker.com/9sLtnCgT [20:04] blackboxsw: i put a patch on there. https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/342428 [20:35] waiting on ci [20:56] blackboxsw: marked approved [21:21] smoser: sorry power loss. I'm back up, landing and publishing [21:22] and cutting a bionic upload MP [21:29] smoser: it' [21:29] s up https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/343562