Merge "Change pxe dhcp options name to codes." into stable/pike

11 files changed, 85 insertions, 51 deletions

diff --git a/ironic/common/dhcp_factory.py b/ironic/common/dhcp_factory.py
index aaff635df..fa294cb46 100644
--- a/ironic/common/dhcp_factory.py
+++ b/ironic/common/dhcp_factory.py
@@ -72,12 +72,10 @@ class DHCPFactory(object):
 
                           ::
 
-                           [{'opt_name': 'bootfile-name',
+                           [{'opt_name': '67',
                              'opt_value': 'pxelinux.0'},
-                            {'opt_name': 'server-ip-address',
-                             'opt_value': '123.123.123.456'},
-                            {'opt_name': 'tftp-server',
-                             'opt_value': '123.123.123.123'}]
+                            {'opt_name': '66',
+                             'opt_value': '123.123.123.456'}]
 
         :param ports: A dict with keys 'ports' and 'portgroups' and
             dicts as values. Each dict has key/value pairs of the form
diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py
index d224a576d..d82018c29 100644
--- a/ironic/common/neutron.py
+++ b/ironic/common/neutron.py
@@ -18,6 +18,7 @@ from oslo_utils import uuidutils
 from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import keystone
+from ironic.common.pxe_utils import DHCP_CLIENT_ID
 from ironic.conf import CONF
 
 LOG = log.getLogger(__name__)
@@ -228,7 +229,8 @@ def add_ports_to_network(task, network_uuid, security_groups=None):
         body['port']['binding:profile'] = binding_profile
         client_id = ironic_port.extra.get('client-id')
         if client_id:
-            client_id_opt = {'opt_name': 'client-id', 'opt_value': client_id}
+            client_id_opt = {'opt_name': DHCP_CLIENT_ID,
+                             'opt_value': client_id}
             extra_dhcp_opts = body['port'].get('extra_dhcp_opts', [])
             extra_dhcp_opts.append(client_id_opt)
             body['port']['extra_dhcp_opts'] = extra_dhcp_opts
diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py
index 0ed9a21ff..806a00cf4 100644
--- a/ironic/common/pxe_utils.py
+++ b/ironic/common/pxe_utils.py
@@ -33,6 +33,13 @@ LOG = logging.getLogger(__name__)
 
 PXE_CFG_DIR_NAME = 'pxelinux.cfg'
 
+DHCP_CLIENT_ID = '61'  # rfc2132
+DHCP_TFTP_SERVER_NAME = '66'  # rfc2132
+DHCP_BOOTFILE_NAME = '67'  # rfc2132
+DHCP_TFTP_SERVER_ADDRESS = '150'  # rfc5859
+DHCP_IPXE_ENCAP_OPTS = '175'  # Tentatively Assigned
+DHCP_TFTP_PATH_PREFIX = '210'  # rfc5071
+
 
 def get_root_dir():
     """Returns the directory where the config files and images will live."""
@@ -317,30 +324,47 @@ def dhcp_options_for_instance(task):
         if dhcp_provider_name == 'neutron':
             # Neutron use dnsmasq as default DHCP agent, add extra config
             # to neutron "dhcp-match=set:ipxe,175" and use below option
-            dhcp_opts.append({'opt_name': 'tag:!ipxe,bootfile-name',
+            dhcp_opts.append({'opt_name': "tag:!ipxe,%s" % DHCP_BOOTFILE_NAME,
                               'opt_value': boot_file})
-            dhcp_opts.append({'opt_name': 'tag:ipxe,bootfile-name',
+            dhcp_opts.append({'opt_name': "tag:ipxe,%s" % DHCP_BOOTFILE_NAME,
                               'opt_value': ipxe_script_url})
         else:
             # !175 == non-iPXE.
             # http://ipxe.org/howto/dhcpd#ipxe-specific_options
-            dhcp_opts.append({'opt_name': '!175,bootfile-name',
+            dhcp_opts.append({'opt_name': "!%s,%s" % (DHCP_IPXE_ENCAP_OPTS,
+                              DHCP_BOOTFILE_NAME),
                               'opt_value': boot_file})
-            dhcp_opts.append({'opt_name': 'bootfile-name',
+            dhcp_opts.append({'opt_name': DHCP_BOOTFILE_NAME,
                               'opt_value': ipxe_script_url})
     else:
-        dhcp_opts.append({'opt_name': 'bootfile-name',
+        dhcp_opts.append({'opt_name': DHCP_BOOTFILE_NAME,
                           'opt_value': boot_file})
         # 210 == tftp server path-prefix or tftp root, will be used to find
         # pxelinux.cfg directory. The pxelinux.0 loader infers this information
         # from it's own path, but Petitboot needs it to be specified by this
         # option since it doesn't use pxelinux.0 loader.
-        dhcp_opts.append({'opt_name': '210',
+        dhcp_opts.append({'opt_name': DHCP_TFTP_PATH_PREFIX,
                           'opt_value': get_tftp_path_prefix()})
 
-    dhcp_opts.append({'opt_name': 'server-ip-address',
+    dhcp_opts.append({'opt_name': DHCP_TFTP_SERVER_NAME,
+                      'opt_value': CONF.pxe.tftp_server})
+    dhcp_opts.append({'opt_name': DHCP_TFTP_SERVER_ADDRESS,
                       'opt_value': CONF.pxe.tftp_server})
-    dhcp_opts.append({'opt_name': 'tftp-server',
+
+    # NOTE(vsaienko) set this option specially for dnsmasq case as it always
+    # sets `siaddr` field which is treated by pxe clients as TFTP server
+    # see page 9 https://tools.ietf.org/html/rfc2131.
+    # If `server-ip-address` is not provided dnsmasq sets `siaddr` to dnsmasq's
+    # IP which breaks PXE booting as TFTP server is configured on ironic
+    # conductor host.
+    # http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/dhcp-common.c;h=eae9ae3567fe16eb979a484976c270396322efea;hb=a3303e196e5d304ec955c4d63afb923ade66c6e8#l572 # noqa
+    # There is an informational RFC which describes how options related to
+    # tftp 150,66 and siaddr should be used https://tools.ietf.org/html/rfc5859
+    # All dhcp servers we've tried: contrail/dnsmasq/isc just silently ignore
+    # unknown options but potentially it may blow up with others.
+    # Related bug was opened on Neutron side:
+    # https://bugs.launchpad.net/neutron/+bug/1723354
+    dhcp_opts.append({'opt_name': 'server-ip-address',
                       'opt_value': CONF.pxe.tftp_server})
 
     # Append the IP version for all the configuration options
diff --git a/ironic/dhcp/base.py b/ironic/dhcp/base.py
index 94f61fd09..6e694be01 100644
--- a/ironic/dhcp/base.py
+++ b/ironic/dhcp/base.py
@@ -36,12 +36,10 @@ class BaseDHCP(object):
 
                              ::
 
-                              [{'opt_name': 'bootfile-name',
+                              [{'opt_name': '67',
                                 'opt_value': 'pxelinux.0'},
-                               {'opt_name': 'server-ip-address',
-                                'opt_value': '123.123.123.456'},
-                               {'opt_name': 'tftp-server',
-                                'opt_value': '123.123.123.123'}]
+                               {'opt_name': '66',
+                                'opt_value': '123.123.123.456'}]
         :param token: An optional authentication token.
 
         :raises: FailedToUpdateDHCPOptOnPort
@@ -56,12 +54,10 @@ class BaseDHCP(object):
 
                         ::
 
-                         [{'opt_name': 'bootfile-name',
+                         [{'opt_name': '67',
                            'opt_value': 'pxelinux.0'},
-                          {'opt_name': 'server-ip-address',
-                           'opt_value': '123.123.123.456'},
-                          {'opt_name': 'tftp-server',
-                           'opt_value': '123.123.123.123'}]
+                          {'opt_name': '66',
+                           'opt_value': '123.123.123.456'}]
 
         :param vifs: A dict with keys 'ports' and 'portgroups' and
             dicts as values. Each dict has key/value pairs of the form
diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py
index a474c501d..b885c1851 100644
--- a/ironic/dhcp/neutron.py
+++ b/ironic/dhcp/neutron.py
@@ -47,12 +47,10 @@ class NeutronDHCPApi(base.BaseDHCP):
 
                              ::
 
-                              [{'opt_name': 'bootfile-name',
+                              [{'opt_name': '67',
                                 'opt_value': 'pxelinux.0'},
-                               {'opt_name': 'server-ip-address',
-                                'opt_value': '123.123.123.456'},
-                               {'opt_name': 'tftp-server',
-                                'opt_value': '123.123.123.123'}]
+                               {'opt_name': '66',
+                                'opt_value': '123.123.123.456'}]
         :param token: optional auth token.
 
         :raises: FailedToUpdateDHCPOptOnPort
@@ -72,12 +70,10 @@ class NeutronDHCPApi(base.BaseDHCP):
 
                         ::
 
-                         [{'opt_name': 'bootfile-name',
+                         [{'opt_name': '67',
                            'opt_value': 'pxelinux.0'},
-                          {'opt_name': 'server-ip-address',
-                           'opt_value': '123.123.123.456'},
-                          {'opt_name': 'tftp-server',
-                           'opt_value': '123.123.123.123'}]
+                          {'opt_name': '66',
+                           'opt_value': '123.123.123.456'}]
         :param vifs: a dict of Neutron port/portgroup dicts
                      to update DHCP options on. The port/portgroup dict
                      key should be Ironic port UUIDs, and the values
diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py
index db4df5838..2ce888e28 100644
--- a/ironic/drivers/modules/network/common.py
+++ b/ironic/drivers/modules/network/common.py
@@ -24,6 +24,7 @@ from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import network
 from ironic.common import neutron
+from ironic.common.pxe_utils import DHCP_CLIENT_ID
 from ironic.common import states
 from ironic.common import utils
 from ironic import objects
@@ -245,7 +246,8 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None):
         local_link_info.append(port_like_obj.local_link_connection)
         client_id = port_like_obj.extra.get('client-id')
         if client_id:
-            client_id_opt = ({'opt_name': 'client-id', 'opt_value': client_id})
+            client_id_opt = ({'opt_name': DHCP_CLIENT_ID,
+                              'opt_value': client_id})
 
     # NOTE(sambetts) Only update required binding: attributes,
     # because other port attributes may have been set by the user or
@@ -415,7 +417,7 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin):
                 # from the neutron port
                 if vif:
                     api = dhcp_factory.DHCPFactory()
-                    client_id_opt = {'opt_name': 'client-id',
+                    client_id_opt = {'opt_name': DHCP_CLIENT_ID,
                                      'opt_value': updated_client_id}
 
                     api.provider.update_port_dhcp_opts(
diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py
index c7b9e201d..181c9299e 100644
--- a/ironic/tests/unit/common/test_neutron.py
+++ b/ironic/tests/unit/common/test_neutron.py
@@ -170,7 +170,7 @@ class TestNeutronNetworkActions(db_base.DbTestCase):
 
         if is_client_id:
             expected_body['port']['extra_dhcp_opts'] = (
-                [{'opt_name': 'client-id', 'opt_value': self._CLIENT_ID}])
+                [{'opt_name': '61', 'opt_value': self._CLIENT_ID}])
         # Ensure we can create ports
         self.client_mock.create_port.return_value = {
             'port': self.neutron_port}
diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py
index 80c9fd4ef..b3423e17c 100644
--- a/ironic/tests/unit/common/test_pxe_utils.py
+++ b/ironic/tests/unit/common/test_pxe_utils.py
@@ -621,18 +621,21 @@ class TestPXEUtils(db_base.DbTestCase):
         self.config(tftp_server='192.0.2.1', group='pxe')
         self.config(pxe_bootfile_name='fake-bootfile', group='pxe')
         self.config(tftp_root='/tftp-path/', group='pxe')
-        expected_info = [{'opt_name': 'bootfile-name',
+        expected_info = [{'opt_name': '67',
                           'opt_value': 'fake-bootfile',
                           'ip_version': ip_version},
                          {'opt_name': '210',
                           'opt_value': '/tftp-path/',
                           'ip_version': ip_version},
-                         {'opt_name': 'server-ip-address',
+                         {'opt_name': '66',
                           'opt_value': '192.0.2.1',
                           'ip_version': ip_version},
-                         {'opt_name': 'tftp-server',
+                         {'opt_name': '150',
                           'opt_value': '192.0.2.1',
                           'ip_version': ip_version},
+                         {'opt_name': 'server-ip-address',
+                          'opt_value': '192.0.2.1',
+                          'ip_version': ip_version}
                          ]
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertEqual(expected_info,
@@ -689,17 +692,20 @@ class TestPXEUtils(db_base.DbTestCase):
 
         self.config(dhcp_provider='isc', group='dhcp')
         expected_boot_script_url = 'http://192.0.3.2:1234/boot.ipxe'
-        expected_info = [{'opt_name': '!175,bootfile-name',
+        expected_info = [{'opt_name': '!175,67',
                           'opt_value': boot_file,
                           'ip_version': 4},
-                         {'opt_name': 'server-ip-address',
+                         {'opt_name': '66',
                           'opt_value': '192.0.2.1',
                           'ip_version': 4},
-                         {'opt_name': 'tftp-server',
+                         {'opt_name': '150',
                           'opt_value': '192.0.2.1',
                           'ip_version': 4},
-                         {'opt_name': 'bootfile-name',
+                         {'opt_name': '67',
                           'opt_value': expected_boot_script_url,
+                          'ip_version': 4},
+                         {'opt_name': 'server-ip-address',
+                          'opt_value': '192.0.2.1',
                           'ip_version': 4}]
 
         self.assertItemsEqual(expected_info,
@@ -707,17 +713,20 @@ class TestPXEUtils(db_base.DbTestCase):
 
         self.config(dhcp_provider='neutron', group='dhcp')
         expected_boot_script_url = 'http://192.0.3.2:1234/boot.ipxe'
-        expected_info = [{'opt_name': 'tag:!ipxe,bootfile-name',
+        expected_info = [{'opt_name': 'tag:!ipxe,67',
                           'opt_value': boot_file,
                           'ip_version': 4},
-                         {'opt_name': 'server-ip-address',
+                         {'opt_name': '66',
                           'opt_value': '192.0.2.1',
                           'ip_version': 4},
-                         {'opt_name': 'tftp-server',
+                         {'opt_name': '150',
                           'opt_value': '192.0.2.1',
                           'ip_version': 4},
-                         {'opt_name': 'tag:ipxe,bootfile-name',
+                         {'opt_name': 'tag:ipxe,67',
                           'opt_value': expected_boot_script_url,
+                          'ip_version': 4},
+                         {'opt_name': 'server-ip-address',
+                          'opt_value': '192.0.2.1',
                           'ip_version': 4}]
 
         self.assertItemsEqual(expected_info,
diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py
index af6d2451c..ab3ca59f2 100644
--- a/ironic/tests/unit/drivers/modules/network/test_common.py
+++ b/ironic/tests/unit/drivers/modules/network/test_common.py
@@ -1008,7 +1008,7 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
     @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts')
     def test_port_changed_client_id(self, dhcp_update_mock):
         expected_extra = {'vif_port_id': 'fake-id', 'client-id': 'fake2'}
-        expected_dhcp_opts = [{'opt_name': 'client-id', 'opt_value': 'fake2'}]
+        expected_dhcp_opts = [{'opt_name': '61', 'opt_value': 'fake2'}]
         self.port.extra = expected_extra
         with task_manager.acquire(self.context, self.node.id) as task:
             self.interface.port_changed(task, self.port)
diff --git a/ironic/tests/unit/drivers/modules/network/test_neutron.py b/ironic/tests/unit/drivers/modules/network/test_neutron.py
index 3708125cc..65c413b0e 100644
--- a/ironic/tests/unit/drivers/modules/network/test_neutron.py
+++ b/ironic/tests/unit/drivers/modules/network/test_neutron.py
@@ -328,9 +328,9 @@ class NeutronInterfaceTestCase(db_base.DbTestCase):
         }
         if is_client_id:
             port1_body['port']['extra_dhcp_opts'] = (
-                [{'opt_name': 'client-id', 'opt_value': client_ids[0]}])
+                [{'opt_name': '61', 'opt_value': client_ids[0]}])
             port2_body['port']['extra_dhcp_opts'] = (
-                [{'opt_name': 'client-id', 'opt_value': client_ids[1]}])
+                [{'opt_name': '61', 'opt_value': client_ids[1]}])
         with task_manager.acquire(self.context, self.node.id) as task:
             self.interface.configure_tenant_networks(task)
             client_mock.assert_called_once_with()
diff --git a/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml b/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml
new file mode 100644
index 000000000..167d83463
--- /dev/null
+++ b/releasenotes/notes/use-dhcp-option-numbers-8b0b0efae912ff5f.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Uses standard DHCP option codes instead of dnsmasq-specific option names,
+    because different backends use different option names. This fixes the
+    `compatibility issues with neutron's DHCP backends
+    <https://bugs.launchpad.net/ironic/+bug/1717236>`.