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