Switch from subprocess to ironicclient

As one large improvement to register-nodes, stop using subprocess to
fork off the ironic CLI, and instead use python-ironicclient and its
Python API to register nodes.

Change-Id: I2fd51873b892026c21045e6b912ef35b0be66fc5

os_cloud_config/nodes.py

@@ -13,8 +13,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import subprocess
 
+from ironicclient import client as ironicclient
+
 
 def _get_id_line(lines, id_description, position=3):
     for line in lines.split('\n'):
@@ -35,7 +38,7 @@ def _check_output(command):
         return output
 
 
-def register_nova_bm_node(service_host, node):
+def register_nova_bm_node(service_host, node, client=None):
     if not service_host:
         raise ValueError("Nova-baremetal requires a service host.")
     out = _check_output(["nova", "baremetal-node-create",
@@ -49,45 +52,51 @@ def register_nova_bm_node(service_host, node):
         subprocess.check_call(["nova", "baremetal-interface-add", bm_id, mac])
 
 
-def register_ironic_node(service_host, node):
+def register_ironic_node(service_host, node, client=None):
-    out = _check_output(["ironic", "node-create", "-d", node["pm_type"]])
+    properties = {"cpus": node["cpu"],
-    node_id = _get_id_line(out, "uuid")
+                  "memory_mb": node["memory"],
-    node_properties = ["properties/cpus=%s" % node["cpu"],
+                  "local_gb": node["disk"],
-                       "properties/memory_mb=%s" % node["memory"],
+                  "cpu_arch": node["arch"]}
-                       "properties/local_gb=%s" % node["disk"],
-                       "properties/cpu_arch=%s" % node["arch"]]
     if "ipmi" in node["pm_type"]:
-        pm_password = node["pm_password"]
+        driver_info = {"ipmi_address": node["pm_addr"],
-        ipmi_properties = ["driver_info/ipmi_address=%s" % node["pm_addr"],
+                       "ipmi_username": node["pm_user"],
-                           "driver_info/ipmi_username=%s" % node["pm_user"],
+                       "ipmi_password": node["pm_password"]}
-                           "driver_info/ipmi_password=%s" % pm_password]
-        node_properties.extend(ipmi_properties)
     elif node["pm_type"] == "pxe_ssh":
-        ssh = ("driver_info/ssh_key_filename=/mnt/state/var/lib/ironic/"
+        ssh_key_filename = "/mnt/state/var/lib/ironic/virtual-power-key"
-               "virtual-power-key")
+        driver_info = {"ssh_address": node["pm_addr"],
-        ssh_properties = ["driver_info/ssh_address=%s" % node["pm_addr"],
+                       "ssh_username": node["pm_user"],
-                          "driver_info/ssh_username=%s" % node["pm_user"],
+                       "ssh_key_filename": ssh_key_filename,
-                          ssh, "driver_info/ssh_virt_type=virsh"]
+                       "ssh_virt_type": "virsh"}
-        node_properties.extend(ssh_properties)
     else:
         raise Exception("Unknown pm_type: %s" % node["pm_type"])
 
-    subprocess.check_call(["ironic", "node-update", node_id, "add"]
+    ironic_node = client.node.create(driver=node["pm_type"],
-                          + node_properties)
+                                     driver_info=driver_info,
-    # Ironic should do this directly, see bug 1315225.
+                                     properties=properties)
-    subprocess.check_call(["ironic", "node-set-power-state", node_id, "off"])
+
     for mac in node["mac"]:
-        subprocess.check_call(["ironic", "port-create", "-a", mac, "-n",
+        client.port.create(address=mac, node_uuid=ironic_node.uuid)
-                              node_id])
+    # Ironic should do this directly, see bug 1315225.
+    client.node.set_power_state(ironic_node.uuid, 'off')
 
 
-def register_all_nodes(service_host, nodes_list):
+def _get_ironic_client():
+    kwargs = {'os_username': os.environ['OS_USERNAME'],
+              'os_password': os.environ['OS_PASSWORD'],
+              'os_auth_url': os.environ['OS_AUTH_URL'],
+              'os_tenant_name': os.environ['OS_TENANT_NAME']}
+    return ironicclient.get_client(1, **kwargs)
+
+
+def register_all_nodes(service_host, nodes_list, client=None):
     if using_ironic():
+        if client is None:
+            client = _get_ironic_client()
         register_func = register_ironic_node
     else:
         register_func = register_nova_bm_node
     for node in nodes_list:
-        register_func(service_host, node)
+        register_func(service_host, node, client=client)
 
 
 # TODO(StevenK): Perhaps this should spin over the first node until it is

os_cloud_config/tests/test_nodes.py

@@ -41,38 +41,44 @@ class NodesTest(base.TestCase):
              "2048", "30", "aaa"])
         check_mock.has_calls([nova_bm_call, nova_bm_call])
 
-    @mock.patch('os_cloud_config.nodes._check_output')
+    @mock.patch('os.environ')
+    @mock.patch('ironicclient.client.get_client')
+    def test_get_ironic_client(self, client_mock, environ):
+        nodes._get_ironic_client()
+        client_mock.assert_called_once_with(
+            1, os_username=environ["OS_USERNAME"],
+            os_password=environ["OS_PASSWORD"],
+            os_auth_url=environ["OS_AUTH_URL"],
+            os_tenant_name=environ["OS_TENANT_NAME"])
+
     @mock.patch('os_cloud_config.nodes.using_ironic', return_value=True)
-    @mock.patch('os_cloud_config.nodes._get_id_line', return_value="1")
+    def test_register_all_nodes_ironic(self, using_ironic):
-    @mock.patch('subprocess.check_call')
-    def test_register_all_nodes_ironic(self, check_call, id_mock, ironic_mock,
-                                       check_output_mock):
         node_list = [self._get_node(), self._get_node()]
         node_list[1]["pm_type"] = "ipmi"
-        nodes.register_all_nodes('servicehost', node_list)
+        node_properties = {"cpus": "1",
-        pxe_node_create = mock.call(
+                           "memory_mb": "2048",
-            ["ironic", "node-create", "-d", "pxe_ssh"])
+                           "local_gb": "30",
-        ipmi_node_create = mock.call(
+                           "cpu_arch": "amd64"}
-            ["ironic", "node-create", "-d", "ipmi"])
+        ironic = mock.MagicMock()
-        check_output_mock.has_calls([pxe_node_create, ipmi_node_create])
+        nodes.register_all_nodes('servicehost', node_list, client=ironic)
         ssh_key = "/mnt/state/var/lib/ironic/virtual-power-key"
-        common_update_args = [
+        pxe_node_driver_info = {"ssh_address": "foo.bar",
-            "ironic", "node-update", "1", "add", "properties/cpus=1",
+                                "ssh_username": "test",
-            "properties/memory_mb=2048", "properties/local_gb=30",
+                                "ssh_key_filename": ssh_key,
-            "properties/cpu_arch=amd64"]
+                                "ssh_virt_type": "virsh"}
-        pxe_node_update_call = mock.call(
+        ipmi_node_driver_info = {"ipmi_address": "foo.bar",
-            common_update_args + ["driver_info/ssh_address=foo.bar",
+                                 "ipmi_username": "test",
-                                  "driver_info/ssh_username=test",
+                                 "ipmi_password": "random"}
-                                  "driver_info/ssh_key_filename=%s" % ssh_key,
+        pxe_node = mock.call(driver="pxe_ssh",
-                                  "driver_info/ssh_virt_type=virsh"])
+                             driver_info=pxe_node_driver_info,
-        ipmi_node_update_call = mock.call(
+                             properties=node_properties)
-            common_update_args + ["driver_info/ipmi_address=foo.bar",
+        port_call = mock.call(node_uuid=ironic.node.create.return_value.uuid,
-                                  "driver_info/ipmi_username=test",
+                              address='aaa')
-                                  "driver_info/ipmi_password=random"])
+        power_off_call = mock.call(ironic.node.create.return_value.uuid, 'off')
-        power_off_call = mock.call(
+        ipmi_node = mock.call(driver="ipmi",
-            ["ironic", "node-set-power-state", "1", "off"])
+                              driver_info=ipmi_node_driver_info,
-        port_update_call = mock.call(
+                              properties=node_properties)
-            ["ironic", "port-create", "-a", "aaa", "-n", "1"])
+        ironic.node.create.assert_has_calls([pxe_node, ipmi_node])
-        check_call.assert_has_calls(
+        ironic.port.create.assert_has_calls([port_call, port_call])
-            [pxe_node_update_call, power_off_call, port_update_call,
+        ironic.node.set_power_state.assert_has_calls(
-             ipmi_node_update_call, power_off_call, port_update_call])
+            [power_off_call, power_off_call])

requirements.txt

@@ -2,6 +2,7 @@ pbr>=0.6,<1.0
 
 argparse
 Babel>=1.3
+python-ironicclient
 python-keystoneclient>=0.6.0
 oslo.config>=1.2.0
 simplejson>=2.0.9