| blackboxsw | rharper: just pushed followup on https://github.com/canonical/cloud-init/pull/114 with doc updates and renamed ds config option to ``apply_network_config`` which aligns with both openstack and azure config option for getting stuff from imds | 18:17 |
|---|---|---|
| blackboxsw | s/stuff/network config/ | 18:17 |
| blackboxsw | https://paste.ubuntu.com/p/DyvY7vjFyN/ is the successful network config for multi-nics + secondary-ips | 18:18 |
| blackboxsw | sorry multi-nic only, will add secondary ips to that too (and some ipv6) for a more full success route test | 18:18 |
| rharper | blackboxsw: ok | 18:53 |
| Odd_Bloke | rharper: blackboxsw: powersj: Just pushed up my revisions to the code review doc: https://github.com/canonical/cloud-init/pull/160/ | 19:42 |
| rharper | k | 19:42 |
| blackboxsw | sweet Odd_Bloke will grab that next. wrapping up two other cloud-init pr reviews | 19:43 |
| blackboxsw | rharper: anything else in progress you are working related to initramfs ? https://github.com/canonical/cloud-init/pull/238 | 19:43 |
| blackboxsw | or is that branch ready for review | 19:43 |
| rharper | what's 238 ? | 19:43 |
| rharper | sorry | 19:44 |
| rharper | heads down in reviewing your 114 | 19:44 |
| rharper | the initramfs.netplan one needs review from xnox/vorlon before we move forward | 19:44 |
| blackboxsw | np just wondering if you are expecting more work on your #238 (I'll not review it until I get through OddBloke's 160) | 19:44 |
| blackboxsw | ok thanks rharper | 19:44 |
| rharper | blackboxsw: ok, finished review 114 | 19:45 |
| powersj | Odd_Bloke, approved | 19:59 |
| Odd_Bloke | Thanks! | 20:00 |
| blackboxsw | onto 160 now. closed out https://github.com/canonical/cloud-init/pull/75 | 20:01 |
| blackboxsw | updated FFe bug with secondary nic & secondary ip address context https://bugs.launchpad.net/ubuntu/+source/cloud-init/+bug/1866930 | 20:02 |
| ubot5 | Ubuntu bug 1866930 in cloud-init (Ubuntu) "[FFe] ec2 add support for configuring secondary NICs and secondary ipv4 and ipv6 addresses" [High,In progress] | 20:02 |
| blackboxsw | Odd_Bloke: approved with nits https://github.com/canonical/cloud-init/pull/160#pullrequestreview-374601125 | 20:28 |
| blackboxsw | if you want the nits take 'em otherwise merge at will | 20:28 |
| Odd_Bloke | blackboxsw: Well, I'm going to want to ping community folks for feedback, just giving core folks a head start. :) | 20:36 |
| powersj | blackboxsw, did you get another review on ec2 branch? | 20:37 |
| blackboxsw | powersj: rharper: said he's heads down on it | 20:37 |
| blackboxsw | right now | 20:37 |
| rharper | blackboxsw: I finished up 30 minutes ago .. .did you push up again ? | 20:37 |
| rharper | I left like 20 comments | 20:37 |
| blackboxsw | powersj: rharper I also validated upgrade path + reboot doesn't rewrite network config with multiple nics if former net config only had eth0 rendered. | 20:38 |
| blackboxsw | rharper: no didn't see what reviewing OddBloke's but done there. | 20:38 |
| blackboxsw | will address comments | 20:38 |
| powersj | rharper, thanks | 20:38 |
| rharper | np | 20:38 |
| blackboxsw | rharper: so primary interface discovery on ec2 is really related to find_fallback_nic_on_linux() not truly what ec2 mentioned as device-number: 0 https://github.com/canonical/cloud-init/pull/114/files#r392430307 | 21:19 |
| blackboxsw | as in, there is nothing currently in ec2 net config that truly checks device-number. but I guess you were saying we should document this correlation | 21:21 |
| blackboxsw | ultimately rharper do you think the route-metric should be set to 100 * nic_metadata['device-number'] +1 and we could drop the nic_idx counter variable? | 21:24 |
| rharper | blackboxsw: I would be shocked if those don't align | 21:24 |
| blackboxsw | rharper: I'm almost positive they align, but the code itself doesn't *do* that is what I was raising | 21:24 |
| blackboxsw | but maybe it should :) | 21:24 |
| rharper | I feel like it should too, but I'm somewhat worried... | 21:25 |
| rharper | let me reread the ec2 docs | 21:25 |
| rharper | blackboxsw: it says, device-index number corresponds to eth<number> ; I wonder if that matches 'ifindex' value | 21:30 |
| blackboxsw | rharper: ssh ubuntu@18.191.148.254 | 21:31 |
| blackboxsw | has 2 nics | 21:31 |
| rharper | hrm, I wonder why they're called ethX instead of en (predictable names) | 21:32 |
| rharper | no, ifindex does not relate | 21:32 |
| rharper | that's xen, I wonder what nitro ones look like | 21:33 |
| rharper | blackboxsw: well, maybe we can do work on device-id later then | 21:34 |
| rharper | I'd rather be sure than change it now | 21:34 |
| blackboxsw | rharper: +1 | 21:35 |
| blackboxsw | waiting on that. | 21:35 |
| blackboxsw | rharper: clarification for you https://github.com/canonical/cloud-init/pull/114/files#r392493895 | 21:36 |
| blackboxsw | if we are doing primary nic, it can have secondary addrs so we still need the apply_network_config check before adding secondary ips | 21:36 |
| blackboxsw | sound ok? (that's current behavior of cloud-init everywhere anyway) primary nic, no secondary ips | 21:37 |
| blackboxsw | ahh interesting. and I think I have something to fix then | 21:39 |
| blackboxsw | https://github.com/canonical/cloud-init/pull/114/files#r392419963 | 21:39 |
| blackboxsw | ok I saw our unittest which showed rendered v2 routes that were not under the specific address. | 21:39 |
| rharper | routes are per interface in v2 | 21:40 |
| rharper | there is no top-level routes in v2 | 21:41 |
| rharper | that will not validate via netplan generate | 21:41 |
| rharper | blackboxsw: on apply_network_config; if False, primary gets dhcp4 only, no secondary ; and dhcp6 instead of dhcp4 if ipv6s is non-empty, Yes? do we do dual-stack v4 + v6 (dhcp4/dhcp6) as well (and all of this is with apply_network_config: False) | 21:42 |
| rharper | if apply_network_config=True ;; then all of the things everywhere | 21:42 |
| blackboxsw | 1. on apply_network_config; if False, primary gets dhcp4 only, no secondary ; YES | 21:44 |
| blackboxsw | 2. and dhcp6 instead of dhcp4 if ipv6s is non-empty, Yes? No. it gets both dhcp4 and dhcp6 | 21:44 |
| blackboxsw | we bring up dhcp4 always on primary because it is needed anyway to talk to the imds | 21:45 |
| blackboxsw | so we know it works because we couldn't have getting IMDS networkconfig otherwise | 21:45 |
| blackboxsw | 3. do we do dual stack ipv4/v6 when apply_network_config == false : YES | 21:46 |
| blackboxsw | just no secondary ips in that case (or secondary nics) | 21:46 |
| blackboxsw | rharper: ^ sorry forgot to address me response | 21:47 |
| blackboxsw | *my8 | 21:47 |
| rharper | ok, on False, agreed, forgot about v4 for imds | 21:47 |
| blackboxsw | just pushed all I have. scrubbing rharper's review comments to make sure I addressed them all | 21:50 |
| blackboxsw | ahh right subnet route ordering is wonky https://github.com/canonical/cloud-init/pull/114/files#r392419963 | 21:51 |
| rharper | blackboxsw: one more suggestion | 22:03 |
| rharper | in convert, I think if we separate out the logic into two parts, apply_network_config==False (this is the primary nic dhcp4 + optionally v6, no additional ips) ; and then apply_network_config==True, and this just configures *all* the nics with all features per metadata | 22:04 |
| rharper | it's easier to see the path that older relases will take, and only the apply_network_config==True path will loop over all interfaces and enable all of the things. | 22:05 |
| blackboxsw | ahh rharper no prob. that makes sense and simplifies the for loop for all | 22:06 |
| blackboxsw | adding now | 22:08 |
| blackboxsw | rharper: just pushed that separation of logic | 22:29 |
| blackboxsw | 1f35018da2abd253561da82bdc7f577eb1058cb2 | 22:29 |
| blackboxsw | I know we're running out of daylight | 22:29 |
| blackboxsw | :/ | 22:29 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!