Merge ~brtknr/cloud-init:infiniband-patch into cloud-init:master

[Bharat Kunwar]

Add support for Infiniband network interfaces (IPoIB).

OpenStack ironic references Infiniband interfaces via a 6 byte 'MAC
address' formed from bytes 13-15 and 18-20 of interface's hardware
address. This address is used as the ethernet_mac_address of Infiniband
links in network_data.json in configdrives generated by OpenStack nova.
We can use this address to map links in network_data.json to their
corresponding interface names.

When generating interface configuration files, we need to use the
interface's full hardware address as the HWADDR, rather than the 6 byte
MAC address provided by network_data.json.

This change allows IB interfaces to be referenced in this dual mode - by
MAC address and hardware address, depending on the context.

Support TYPE=InfiniBand for sysconfig configuration of IB interfaces.

[Scott Moser (smoser)]

Hi,
Thank you for contributing to cloud-init.

To contribute, you must sign the Canonical Contributor License Agreement (CLA) [1].

If you have already signed it as an individual, your Launchpad user will be listed in the contributor-agreement-canonical launchpad group [2]. Unfortunately there is no easy way to check if an organization or company you are doing work for has signed. If you are unsure or have questions, email <email address hidden> or ping smoser in #cloud-init channel via freenode.

For information on how to sign, please see the HACKING document [3].

Thanks again, and please feel free to reach out with any questions.

[Mark Goddard (mgoddard)]

LGTM.

review: Approve

[Bharat Kunwar (brtknr)]

I’ve just signed it. Although, I put in <email address hidden> <mailto:<email address hidden>> instead of Scott Moser as Canonical Project Manager. Sorry.

> On 17 Aug 2018, at 15:24, Scott Moser <email address hidden> wrote:
>
> Hi,
> Thank you for contributing to cloud-init.
>
> To contribute, you must sign the Canonical Contributor License Agreement (CLA) [1].
>
> If you have already signed it as an individual, your Launchpad user will be listed in the contributor-agreement-canonical launchpad group [2]. Unfortunately there is no easy way to check if an organization or company you are doing work for has signed. If you are unsure or have questions, email <email address hidden> or ping smoser in #cloud-init channel via freenode.
>
> For information on how to sign, please see the HACKING document [3].
>
> Thanks again, and please feel free to reach out with any questions.
> --
> https://code.launchpad.net/~brtknr/cloud-init/+git/cloud-init/+merge/353182
> You are the owner of ~brtknr/cloud-init:infiniband-patch.

[Scott Moser (smoser)]

good enough on the CLA. thanks.

On Fri, Aug 17, 2018 at 10:36 AM Bharat Kunwar <email address hidden> wrote:

> I’ve just signed it. Although, I put in <email address hidden> <mailto:
> <email address hidden>> instead of Scott Moser as Canonical Project
> Manager. Sorry.
>
> > On 17 Aug 2018, at 15:24, Scott Moser <email address hidden> wrote:
> >
> > Hi,
> > Thank you for contributing to cloud-init.
> >
> > To contribute, you must sign the Canonical Contributor License Agreement
> (CLA) [1].
> >
> > If you have already signed it as an individual, your Launchpad user will
> be listed in the contributor-agreement-canonical launchpad group [2].
> Unfortunately there is no easy way to check if an organization or company
> you are doing work for has signed. If you are unsure or have questions,
> email <email address hidden> or ping smoser in #cloud-init channel via
> freenode.
> >
> > For information on how to sign, please see the HACKING document [3].
> >
> > Thanks again, and please feel free to reach out with any questions.
> > --
> >
> https://code.launchpad.net/~brtknr/cloud-init/+git/cloud-init/+merge/353182
> > You are the owner of ~brtknr/cloud-init:infiniband-patch.
>
>
> --
> https://code.launchpad.net/~brtknr/cloud-init/+git/cloud-init/+merge/353182
> You are subscribed to branch cloud-init:master.
>

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6a3880f2b1922f04d8c3bc7a589ee0a9810a3f76
https://jenkins.ubuntu.com/server/job/cloud-init-ci/245/
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/245/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

This only adds rendering for sysconfig, right?
We'd definitely like to have support for ENI rendering.
We'd need upstream netplan feature for netplan rendering, but I'd like to see a warning emitted should that path get hit.

review: Needs Information

[Scott Moser (smoser)]

@Mark,

Could you respond to the above?

[Mark Goddard (mgoddard)]

Hi Scott, sorry for not replying sooner. I spoke to Bharat about this yesterday, and adding ENI support is on his todo list. It seems that ENI config should be the same as for ethernet, but we'll need to make a small additional change to ensure that we handle the new infiniband interface type. I've not used netplan before, but we can look at adding a warning.

[Bharat Kunwar (brtknr)]

@smoser, any progress on this patch? To summarise our discussion from IRC, I tested this on Ubuntu to see if ENI would be rendered and it appears that it does since openstack helper obtains the fixed ipv4 address from ConfigDrive which ENI already knows how to handle.

[Scott Moser (smoser)]

2018-09-26 11:40:18 @smoser it is IPoIB right?
2018-09-26 11:40:31 @smoser sorry for being infiniband unaware.
2018-09-26 11:47:08 brtknr smoser: hmm i dont see any response. where exactly?
2018-09-26 11:48:10 @smoser brtknr: bah. sorry
2018-09-26 11:48:19 @smoser i didn't hit 'save'
2018-09-26 11:48:26 @smoser it configuers IPoIB right ?
2018-09-26 11:48:36 brtknr smoser: yes
2018-09-26 11:48:38 @smoser and in some cases... that could be problematic ?
2018-09-26 11:48:47 brtknr smoser: which cases?
2018-09-26 11:49:06 brtknr smoser: it only configures IPoIB if ConfigDrive is enabled
2018-09-26 11:49:19 brtknr smoser: as a data source
2018-09-26 11:49:25 @smoser i was under theimpression that enabling IPoIB might make be incompatible with other infiniband "modes"
2018-09-26 11:50:05 @smoser (for your info, it is config drive or openstack metadata i'm pretty sure at this point, both would work with your change)
2018-09-26 11:50:33 @smoser so what i'm thinking is possibly problematic
2018-09-26 11:51:04 @smoser a.) user had infiniband hardware on openstack cloud. they manually configured some infiniband function after deplyoing an instance.
2018-09-26 11:51:29 @smoser b.) cloud-init upgraade, new instances with new cloud-init would then cause their existing process to fail
2018-09-26 11:53:33 @smoser is that possible ?
2018-09-26 11:54:09 @smoser I'm *not* suggesting that makes the MP unacceptable. I'm just wanting to clarify if that is possible.
2018-09-26 11:54:33 brtknr smoser: users need to enable these kernel modules: ib_ipoib
2018-09-26 11:54:35 brtknr mlx5_ib
2018-09-26 11:54:42 brtknr in order for this to work
2018-09-26 11:55:06 brtknr therefore, it wouldnt automatically interfere with existing setup
2018-09-26 11:55:49 brtknr e.g. openstack cloud with infiniband hardware would need to have these modules enabled by default in the respective cloud images
2018-09-26 11:56:26 brtknr mgoddard also says it should only have an entry in the metadata/configdrive if there is a port for the IB interface, and the port has an IP address
2018-09-26 11:57:38 brtknr as far as im aware, ib_ipoib and mlx5_ib are not enabled by default on neither Centos nor Ubuntu

[Scott Moser (smoser)]

review: Approve

[Server Team CI bot (server-team-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/71/
Executed test runs:
    FAILED: Checkout

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Autolanding.
Unapproved changes made after approval.
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/72/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 5e87bca..f83d368 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -569,6 +569,20 @@ def get_interface_mac(ifname):
     return read_sys_net_safe(ifname, path)
 
 
+def get_ib_interface_hwaddr(ifname, ethernet_format):
+    """Returns the string value of an Infiniband interface's hardware
+    address. If ethernet_format is True, an Ethernet MAC-style 6 byte
+    representation of the address will be returned.
+    """
+    # Type 32 is Infiniband.
+    if read_sys_net_safe(ifname, 'type') == '32':
+        mac = get_interface_mac(ifname)
+        if mac and ethernet_format:
+            # Use bytes 13-15 and 18-20 of the hardware address.
+            mac = mac[36:-14] + mac[51:]
+        return mac
+
+
 def get_interfaces_by_mac():
     """Build a dictionary of tuples {mac: name}.
 
@@ -580,6 +594,15 @@ def get_interfaces_by_mac():
                 "duplicate mac found! both '%s' and '%s' have mac '%s'" %
                 (name, ret[mac], mac))
         ret[mac] = name
+        # Try to get an Infiniband hardware address (in 6 byte Ethernet format)
+        # for the interface.
+        ib_mac = get_ib_interface_hwaddr(name, True)
+        if ib_mac:
+            if ib_mac in ret:
+                raise RuntimeError(
+                    "duplicate mac found! both '%s' and '%s' have mac '%s'" %
+                    (name, ret[ib_mac], ib_mac))
+            ret[ib_mac] = name
     return ret
 
 
@@ -607,6 +630,21 @@ def get_interfaces():
     return ret
 
 
+def get_ib_hwaddrs_by_interface():
+    """Build a dictionary mapping Infiniband interface names to their hardware
+    address."""
+    ret = {}
+    for name, _, _, _ in get_interfaces():
+        ib_mac = get_ib_interface_hwaddr(name, False)
+        if ib_mac:
+            if ib_mac in ret:
+                raise RuntimeError(
+                    "duplicate mac found! both '%s' and '%s' have mac '%s'" %
+                    (name, ret[ib_mac], ib_mac))
+            ret[name] = ib_mac
+    return ret
+
+
 class EphemeralIPv4Network(object):
     """Context manager which sets up temporary static network configuration.
 
diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
index 72c803e..f76e508 100644
--- a/cloudinit/net/network_state.py
+++ b/cloudinit/net/network_state.py
@@ -483,6 +483,10 @@ class NetworkStateInterpreter(object):
 
         interfaces.update({iface['name']: iface})
 
+    @ensure_command_keys(['name'])
+    def handle_infiniband(self, command):
+        self.handle_physical(command)
+
     @ensure_command_keys(['address'])
     def handle_nameserver(self, command):
         dns = self._network_state.get('dns')
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 66e970e..9c16d3a 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -174,6 +174,7 @@ class NetInterface(ConfigMap):
         'ethernet': 'Ethernet',
         'bond': 'Bond',
         'bridge': 'Bridge',
+        'infiniband': 'InfiniBand',
     }
 
     def __init__(self, iface_name, base_sysconf_dir, templates,
@@ -569,6 +570,18 @@ class Renderer(renderer.Renderer):
             cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
 
     @classmethod
+    def _render_ib_interfaces(cls, network_state, iface_contents):
+        ib_filter = renderer.filter_by_type('infiniband')
+        for iface in network_state.iter_interfaces(ib_filter):
+            iface_name = iface['name']
+            iface_cfg = iface_contents[iface_name]
+            iface_cfg.kind = 'infiniband'
+            iface_subnets = iface.get("subnets", [])
+            route_cfg = iface_cfg.routes
+            cls._render_subnets(iface_cfg, iface_subnets)
+            cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
+
+    @classmethod
     def _render_sysconfig(cls, base_sysconf_dir, network_state,
                           templates=None):
         '''Given state, return /etc/sysconfig files + contents'''
@@ -586,6 +599,7 @@ class Renderer(renderer.Renderer):
         cls._render_bond_interfaces(network_state, iface_contents)
         cls._render_vlan_interfaces(network_state, iface_contents)
         cls._render_bridge_interfaces(network_state, iface_contents)
+        cls._render_ib_interfaces(network_state, iface_contents)
         contents = {}
         for iface_name, iface_cfg in iface_contents.items():
             if iface_cfg or iface_cfg.children:
diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py
index 76a6e31..9c29cea 100644
--- a/cloudinit/sources/helpers/openstack.py
+++ b/cloudinit/sources/helpers/openstack.py
@@ -675,6 +675,17 @@ def convert_net_json(network_json=None, known_macs=None):
             else:
                 cfg[key] = fmt % link_id_info[target]['name']
 
+    # Infiniband interfaces may be referenced in network_data.json by a 6 byte
+    # Ethernet MAC-style address, and we use that address to look up the
+    # interface name above. Now ensure that the hardware address is set to the
+    # full 20 byte address.
+    ib_known_hwaddrs = net.get_ib_hwaddrs_by_interface()
+    if ib_known_hwaddrs:
+        for cfg in config:
+            if cfg['name'] in ib_known_hwaddrs:
+                cfg['mac_address'] = ib_known_hwaddrs[cfg['name']]
+                cfg['type'] = 'infiniband'
+
     for service in services:
         cfg = service
         cfg.update({'type': 'nameserver'})
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index f3165da..5d9c7d9 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -3260,11 +3260,15 @@ class TestGetInterfacesByMac(CiTestCase):
     def _se_interface_has_own_mac(self, name):
         return name in self.data['own_macs']
 
+    def _se_get_ib_interface_hwaddr(self, name, ethernet_format):
+        ib_hwaddr = self.data.get('ib_hwaddr', {})
+        return ib_hwaddr.get(name, {}).get(ethernet_format)
+
     def _mock_setup(self):
         self.data = copy.deepcopy(self._data)
         self.data['devices'] = set(list(self.data['macs'].keys()))
         mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
-                 'interface_has_own_mac', 'is_vlan')
+                 'interface_has_own_mac', 'is_vlan', 'get_ib_interface_hwaddr')
         self.mocks = {}
         for n in mocks:
             m = mock.patch('cloudinit.net.' + n,
@@ -3338,6 +3342,20 @@ class TestGetInterfacesByMac(CiTestCase):
         ret = net.get_interfaces_by_mac()
         self.assertEqual('lo', ret[empty_mac])
 
+    def test_ib(self):
+        ib_addr = '80:00:00:28:fe:80:00:00:00:00:00:00:00:11:22:03:00:33:44:56'
+        ib_addr_eth_format = '00:11:22:33:44:56'
+        self._mock_setup()
+        self.data['devices'] = ['enp0s1', 'ib0']
+        self.data['own_macs'].append('ib0')
+        self.data['macs']['ib0'] = ib_addr
+        self.data['ib_hwaddr'] = {'ib0': {True: ib_addr_eth_format,
+                                          False: ib_addr}}
+        result = net.get_interfaces_by_mac()
+        expected = {'aa:aa:aa:aa:aa:01': 'enp0s1',
+                    ib_addr_eth_format: 'ib0', ib_addr: 'ib0'}
+        self.assertEqual(expected, result)
+
 
 class TestInterfacesSorting(CiTestCase):
 
@@ -3352,6 +3370,67 @@ class TestInterfacesSorting(CiTestCase):
             ['enp0s3', 'enp0s8', 'enp0s13', 'enp1s2', 'enp2s0', 'enp2s3'])
 
 
+class TestGetIBHwaddrsByInterface(CiTestCase):
+
+    _ib_addr = '80:00:00:28:fe:80:00:00:00:00:00:00:00:11:22:03:00:33:44:56'
+    _ib_addr_eth_format = '00:11:22:33:44:56'
+    _data = {'devices': ['enp0s1', 'enp0s2', 'bond1', 'bridge1',
+                         'bridge1-nic', 'tun0', 'ib0'],
+             'bonds': ['bond1'],
+             'bridges': ['bridge1'],
+             'own_macs': ['enp0s1', 'enp0s2', 'bridge1-nic', 'bridge1', 'ib0'],
+             'macs': {'enp0s1': 'aa:aa:aa:aa:aa:01',
+                      'enp0s2': 'aa:aa:aa:aa:aa:02',
+                      'bond1': 'aa:aa:aa:aa:aa:01',
+                      'bridge1': 'aa:aa:aa:aa:aa:03',
+                      'bridge1-nic': 'aa:aa:aa:aa:aa:03',
+                      'tun0': None,
+                      'ib0': _ib_addr},
+             'ib_hwaddr': {'ib0': {True: _ib_addr_eth_format,
+                                   False: _ib_addr}}}
+    data = {}
+
+    def _mock_setup(self):
+        self.data = copy.deepcopy(self._data)
+        mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
+                 'interface_has_own_mac', 'get_ib_interface_hwaddr')
+        self.mocks = {}
+        for n in mocks:
+            m = mock.patch('cloudinit.net.' + n,
+                           side_effect=getattr(self, '_se_' + n))
+            self.addCleanup(m.stop)
+            self.mocks[n] = m.start()
+
+    def _se_get_devicelist(self):
+        return self.data['devices']
+
+    def _se_get_interface_mac(self, name):
+        return self.data['macs'][name]
+
+    def _se_is_bridge(self, name):
+        return name in self.data['bridges']
+
+    def _se_interface_has_own_mac(self, name):
+        return name in self.data['own_macs']
+
+    def _se_get_ib_interface_hwaddr(self, name, ethernet_format):
+        ib_hwaddr = self.data.get('ib_hwaddr', {})
+        return ib_hwaddr.get(name, {}).get(ethernet_format)
+
+    def test_ethernet(self):
+        self._mock_setup()
+        self.data['devices'].remove('ib0')
+        result = net.get_ib_hwaddrs_by_interface()
+        expected = {}
+        self.assertEqual(expected, result)
+
+    def test_ib(self):
+        self._mock_setup()
+        result = net.get_ib_hwaddrs_by_interface()
+        expected = {'ib0': self._ib_addr}
+        self.assertEqual(expected, result)
+
+
 def _gzip_data(data):
     with io.BytesIO() as iobuf:
         gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)