Merge ~darren-birkett/cloud-init:fix-infiniband-1847114 into cloud-init:master

Proposed by Darren Birkett on 2019-10-07
Status: Merged
Approved by: Ryan Harper on 2019-10-21
Approved revision: 4a94332920192b36e83d5ea71ddacc37f00c6a4e
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~darren-birkett/cloud-init:fix-infiniband-1847114
Merge into: cloud-init:master
Diff against target: 95 lines (+35/-6)
3 files modified
cloudinit/net/eni.py (+5/-4)
cloudinit/net/sysconfig.py (+2/-1)
tests/unittests/test_net.py (+28/-1)
Reviewer Review Type Date Requested Status
Ryan Harper 2019-10-07 Approve on 2019-10-21
Server Team CI bot continuous-integration Approve on 2019-10-21
Review via email: mp+373759@code.launchpad.net

Commit message

net: enable infiniband support in eni and sysconfig renderers

Commit e7b0e5f72 added support for configuring infiniband devices by
adding a new infiniband 'type'. This commit updates eni and sysconfig
renderers to consume this new type and configure infiniband devices
correctly.

LP: #1847114

To post a comment you must log in.
Ryan Harper (raharper) wrote :

Thanks for looking at this. I'd like to get this plumbed into the unittests for network generation ( tests/unittests/test_net.py ); I suggest adding this to the 'all' scenario, update the yaml to include the IB interface:

@@ -1456,6 +1476,12 @@ pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
                   vlan_id: 200
                   subnets:
                       - type: dhcp4
+ # An Infiniband
+ - type: infiniband
+ name: ib0
+ mac_address: 00:11:22:33:44:56
+ subnets:
+ - type: dhcp4

And then fix up the expected eni and sysconfig output; lastly this will need an update to handle the convertion of v1 with type: infiniband to v2 where the type: infiniband will be converted to an ethernet type.

review: Needs Fixing
Darren Birkett (darren-birkett) wrote :

Hi Ryan,

Thanks for your comments. Would you mind expanding on your last comment regarding the conversion between type 1 and type 2? I have added the tests as you suggested, and the sysconfig test is failing because it is expecting 'Type=Ethernet' but has 'Type=InfiniBand' (which we want). I'm happy to add this, just I'm not quite sure what the ask is here and need a bit of guidance.

a706916... by Darren Birkett on 2019-10-08

Add tests for patch to eni to support infiniband

4a94332... by Darren Birkett on 2019-10-08

Ensure sysconfig writes HWADDR when infiniband

Darren Birkett (darren-birkett) wrote :

I've pushed some additional commits to the branch - does this work, or do I need to do something else for the Type?

Also, I had to fix part of the sysconfig.py to ensure the mac addr is written (in order to pass the tests I added). I know it's not directly related to the bug I was fixing, but it is related to the tests I'm adding. Hope it's OK to include here.

Ryan Harper (raharper) wrote :

Thanks for adding the test. "type 2" refers to network-config version:2 which is known as netplan.
There's a netplan section in that same test and a netplan renderer. However, I was wondering of the full IB hardware address is required, or would we instead only render the subset of the address that's the same size/format as MAC ?

I know that netplan won't recognize the full hardware address so we currently couldn't support IB in netplan.

review: Needs Information
Darren Birkett (darren-birkett) wrote :

Hi Ryan. I don't mind working on a patch to support ib in netplan, but it seems like that should probably sit in its own separate patch rather than trying to lump it in with this fix specifically for eni?

I can raise a bug for netplan support and self-assign if you agree? In which case, the unittests for this patchset are passing for eni and sysconfig now so it should be good to go.

Ryan Harper (raharper) wrote :

Well, we've touched two of the three renderers to fix network config rendering for IB devices; it feels to me the core issue "ib network config isn't rendered correct", is true for all renderers; I think it would be fine to put it all in one place.

I'd like to first confirm if we can use the 8-octet portion of the IB hardware address instead of the full IB hardware address.

Depending on the answer to this query will determine if we can pull netplan support into this branch or not. If we must use the full hardware address, then we'll need to file a bug against netplan to have it add support for parsing the full-length MAC; as now netplan complains if you feed it a MAC address that's not in the standard 8-octet format (where as eni and sysconfig, AFAICT ignore format the value).

Darren Birkett (darren-birkett) wrote :

Hi Ryan,

I've tested a netplan config with the truncated mac addr (as generated in the current cloud-init code: mac = mac[36:-14] + mac[51:]) from an ib interface, and netplan doesn't seem able to match the device so doesn't configure it properly. I also tried using the final 6 bytes of the long ib mac, and then tried with the first 6 bytes. None of them match the device.

Example not working-

---
network:
    version: 2
    ethernets:
        ib0:
            addresses:
            - 192.168.207.26/24
            mtu: 9000
            match:
                macaddress: a0:00:02:20:fe:80
            set-name: ib0
---

Removing the mac matching completely from the config allows netplan to apply the config and bring the device up, at the risk of not matching the correct device at some point depending on how the device gets enumerated.

Example working-

---
network:
    version: 2
    ethernets:
        ib0:
            addresses:
            - 192.168.207.26/24
            mtu: 9000
---

Ryan Harper (raharper) wrote :

Thanks for testing that out. We can currently match on name, driver or mac. Let's file a separate bug for getting netplan.io support for IB interfaces, we can sort out whether netplan.io/systemd-networkd need fixes from what cloud-init generates.

Darren Birkett (darren-birkett) wrote :

Hi Ryan,

Is this patch OK to merge then?

I've separately opened https://bugs.launchpad.net/cloud-init/+bug/1848471 to add netplan support once netplan is able to deal with long-form HWADDR.

Darren Birkett (darren-birkett) wrote :

I've also added this bug report to netplan to get long-form HWADDR support included.

https://bugs.launchpad.net/netplan/+bug/1848474

Ryan Harper (raharper) wrote :

Thanks. Ill point ci at this branch and if we're good, then we can land. If it fails, I'll pass along the error.

FAILED: Continuous integration, rev:fd252e6dd234dbf980f1a56e0c1465d1f22bd724
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1216/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1216//rebuild

review: Needs Fixing (continuous-integration)
Dan Watkins (daniel-thewatkins) wrote :

There are pycodestyle errors:

cloudinit/net/sysconfig.py:333:80: E501 line too long (96 > 79 characters)
tests/unittests/test_net.py:1480:80: E501 line too long (90 > 79 characters)

Let us know when you've fixed those up (bear in mind you can run these tests locally with `tox -e pycodestyle`!) and we'll re-run CI.

Darren Birkett (darren-birkett) wrote :

Hi Dan,

These errors have now been fixed.

Thanks,
Darren

FAILED: Continuous integration, rev:4a94332920192b36e83d5ea71ddacc37f00c6a4e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1221/
Executed test runs:
    FAILED: Checkout

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1221//rebuild

review: Needs Fixing (continuous-integration)

FAILED: Continuous integration, rev:4a94332920192b36e83d5ea71ddacc37f00c6a4e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1222/
Executed test runs:
    FAILED: Checkout

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1222//rebuild

review: Needs Fixing (continuous-integration)

PASSED: Continuous integration, rev:4a94332920192b36e83d5ea71ddacc37f00c6a4e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1223/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1223//rebuild

review: Approve (continuous-integration)
Ryan Harper (raharper) wrote :

Alright, CI is happy!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
2index 530922b..a9a80c9 100644
3--- a/cloudinit/net/eni.py
4+++ b/cloudinit/net/eni.py
5@@ -94,7 +94,7 @@ def _iface_add_attrs(iface, index, ipv4_subnet_mtu):
6 ]
7
8 renames = {'mac_address': 'hwaddress'}
9- if iface['type'] not in ['bond', 'bridge', 'vlan']:
10+ if iface['type'] not in ['bond', 'bridge', 'infiniband', 'vlan']:
11 ignore_map.append('mac_address')
12
13 for key, value in iface.items():
14@@ -472,9 +472,10 @@ class Renderer(renderer.Renderer):
15 order = {
16 'loopback': 0,
17 'physical': 1,
18- 'bond': 2,
19- 'bridge': 3,
20- 'vlan': 4,
21+ 'infiniband': 2,
22+ 'bond': 3,
23+ 'bridge': 4,
24+ 'vlan': 5,
25 }
26
27 sections = []
28diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
29index 4e65676..e381596 100644
30--- a/cloudinit/net/sysconfig.py
31+++ b/cloudinit/net/sysconfig.py
32@@ -330,7 +330,8 @@ class Renderer(renderer.Renderer):
33 old_value = iface.get(old_key)
34 if old_value is not None:
35 # only set HWADDR on physical interfaces
36- if old_key == 'mac_address' and iface['type'] != 'physical':
37+ if (old_key == 'mac_address' and
38+ iface['type'] not in ['physical', 'infiniband']):
39 continue
40 iface_cfg[new_key] = old_value
41
42diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
43index f5a9cae..d220199 100644
44--- a/tests/unittests/test_net.py
45+++ b/tests/unittests/test_net.py
46@@ -1176,6 +1176,12 @@ iface eth4 inet manual
47 # control-manual eth5
48 iface eth5 inet dhcp
49
50+auto ib0
51+iface ib0 inet static
52+ address 192.168.200.7/24
53+ mtu 9000
54+ hwaddress a0:00:02:20:fe:80:00:00:00:00:00:00:ec:0d:9a:03:00:15:e2:c1
55+
56 auto bond0
57 iface bond0 inet6 dhcp
58 bond-mode active-backup
59@@ -1457,7 +1463,19 @@ pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
60 ONBOOT=no
61 STARTMODE=manual
62 TYPE=Ethernet
63- USERCTL=no""")
64+ USERCTL=no"""),
65+ 'ifcfg-ib0': textwrap.dedent("""\
66+ BOOTPROTO=none
67+ DEVICE=ib0
68+ HWADDR=a0:00:02:20:fe:80:00:00:00:00:00:00:ec:0d:9a:03:00:15:e2:c1
69+ IPADDR=192.168.200.7
70+ MTU=9000
71+ NETMASK=255.255.255.0
72+ NM_CONTROLLED=no
73+ ONBOOT=yes
74+ STARTMODE=auto
75+ TYPE=InfiniBand
76+ USERCTL=no"""),
77 },
78 'yaml': textwrap.dedent("""
79 version: 1
80@@ -1532,6 +1550,15 @@ pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
81 vlan_id: 200
82 subnets:
83 - type: dhcp4
84+ # An infiniband
85+ - type: infiniband
86+ name: ib0
87+ mac_address: >-
88+ a0:00:02:20:fe:80:00:00:00:00:00:00:ec:0d:9a:03:00:15:e2:c1
89+ subnets:
90+ - type: static
91+ address: 192.168.200.7/24
92+ mtu: 9000
93 # A bridge.
94 - type: bridge
95 name: br0

Subscribers

People subscribed via source and target branches