Close files on server create, add tests

The files opened for the --files and --user-data options were never
closed, potentially leaking memory in a long-running client. Close
them if they are file objects.

Add a couple of basic tests for server create.

Change-Id: I1658b0caa2d6af17308149cb52196ee28266ddf2

Author: Dean Troyer

openstackclient/compute/v2/server.py

@@ -17,6 +17,7 @@
 
 import argparse
 import getpass
+import io
 import logging
 import os
 import six
@@ -296,7 +297,7 @@ def take_action(self, parsed_args):
         for f in parsed_args.file:
             dst, src = f.split('=', 1)
             try:
-                files[dst] = open(src)
+                files[dst] = io.open(src, 'rb')
             except IOError as e:
                 raise exceptions.CommandError("Can't open '%s': %s" % (src, e))
 
@@ -313,7 +314,7 @@ def take_action(self, parsed_args):
         userdata = None
         if parsed_args.user_data:
             try:
-                userdata = open(parsed_args.user_data)
+                userdata = io.open(parsed_args.user_data)
             except IOError as e:
                 msg = "Can't open '%s': %s"
                 raise exceptions.CommandError(msg % (parsed_args.user_data, e))
@@ -368,7 +369,17 @@ def take_action(self, parsed_args):
 
         self.log.debug('boot_args: %s', boot_args)
         self.log.debug('boot_kwargs: %s', boot_kwargs)
-        server = compute_client.servers.create(*boot_args, **boot_kwargs)
+
+        # Wrap the call to catch exceptions in order to close files
+        try:
+            server = compute_client.servers.create(*boot_args, **boot_kwargs)
+        finally:
+            # Clean up open files - make sure they are not strings
+            for f in files:
+                if hasattr(f, 'close'):
+                    f.close()
+            if hasattr(userdata, 'close'):
+                userdata.close()
 
         if parsed_args.wait:
             if utils.wait_for_status(

openstackclient/tests/compute/v2/fakes.py

@@ -26,6 +26,7 @@
 SERVER = {
     'id': server_id,
     'name': server_name,
+    'metadata': {},
 }
 
 extension_name = 'Multinic'

openstackclient/tests/compute/v2/test_server.py

@@ -14,11 +14,13 @@
 #
 
 import copy
+import mock
 
 from openstackclient.compute.v2 import server
 from openstackclient.tests.compute.v2 import fakes as compute_fakes
 from openstackclient.tests import fakes
 from openstackclient.tests.image.v2 import fakes as image_fakes
+from openstackclient.tests import utils
 
 
 class TestServer(compute_fakes.TestComputev2):
@@ -30,6 +32,10 @@ def setUp(self):
         self.servers_mock = self.app.client_manager.compute.servers
         self.servers_mock.reset_mock()
 
+        # Get a shortcut to the ImageManager Mock
+        self.cimages_mock = self.app.client_manager.compute.images
+        self.cimages_mock.reset_mock()
+
         # Get a shortcut to the FlavorManager Mock
         self.flavors_mock = self.app.client_manager.compute.flavors
         self.flavors_mock.reset_mock()
@@ -39,6 +45,174 @@ def setUp(self):
         self.images_mock.reset_mock()
 
 
+class TestServerCreate(TestServer):
+
+    def setUp(self):
+        super(TestServerCreate, self).setUp()
+
+        self.servers_mock.create.return_value = fakes.FakeResource(
+            None,
+            copy.deepcopy(compute_fakes.SERVER),
+            loaded=True,
+        )
+        new_server = fakes.FakeResource(
+            None,
+            copy.deepcopy(compute_fakes.SERVER),
+            loaded=True,
+        )
+        new_server.__dict__['networks'] = {}
+        self.servers_mock.get.return_value = new_server
+
+        self.image = fakes.FakeResource(
+            None,
+            copy.deepcopy(image_fakes.IMAGE),
+            loaded=True,
+        )
+        self.cimages_mock.get.return_value = self.image
+
+        self.flavor = fakes.FakeResource(
+            None,
+            copy.deepcopy(compute_fakes.FLAVOR),
+            loaded=True,
+        )
+        self.flavors_mock.get.return_value = self.flavor
+
+        # Get the command object to test
+        self.cmd = server.CreateServer(self.app, None)
+
+    def test_server_create_no_options(self):
+        arglist = [
+            compute_fakes.server_id,
+        ]
+        verifylist = [
+            ('server_name', compute_fakes.server_id),
+        ]
+        try:
+            # Missing required args should bail here
+            self.check_parser(self.cmd, arglist, verifylist)
+        except utils.ParserException:
+            pass
+
+    def test_server_create_minimal(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            compute_fakes.server_id,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('config_drive', False),
+            ('server_name', compute_fakes.server_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping={},
+            nics=[],
+            scheduler_hints={},
+            config_drive=None,
+        )
+        # ServerManager.create(name, image, flavor, **kwargs)
+        self.servers_mock.create.assert_called_with(
+            compute_fakes.server_id,
+            self.image,
+            self.flavor,
+            **kwargs
+        )
+
+        collist = ('addresses', 'flavor', 'id', 'image', 'name', 'properties')
+        self.assertEqual(columns, collist)
+        datalist = (
+            '',
+            'Large ()',
+            compute_fakes.server_id,
+            'graven ()',
+            compute_fakes.server_name,
+            '',
+        )
+        self.assertEqual(data, datalist)
+
+    @mock.patch('openstackclient.compute.v2.server.io.open')
+    def test_server_create_userdata(self, mock_open):
+        mock_file = mock.MagicMock(name='File')
+        mock_open.return_value = mock_file
+        mock_open.read.return_value = '#!/bin/sh'
+
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            '--user-data', 'userdata.sh',
+            compute_fakes.server_id,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('user_data', 'userdata.sh'),
+            ('config_drive', False),
+            ('server_name', compute_fakes.server_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Ensure the userdata file is opened
+        mock_open.assert_called_with('userdata.sh')
+
+        # Ensure the userdata file is closed
+        mock_file.close.assert_called()
+
+        # Set expected values
+        kwargs = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=mock_file,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping={},
+            nics=[],
+            scheduler_hints={},
+            config_drive=None,
+        )
+        # ServerManager.create(name, image, flavor, **kwargs)
+        self.servers_mock.create.assert_called_with(
+            compute_fakes.server_id,
+            self.image,
+            self.flavor,
+            **kwargs
+        )
+
+        collist = ('addresses', 'flavor', 'id', 'image', 'name', 'properties')
+        self.assertEqual(columns, collist)
+        datalist = (
+            '',
+            'Large ()',
+            compute_fakes.server_id,
+            'graven ()',
+            compute_fakes.server_name,
+            '',
+        )
+        self.assertEqual(data, datalist)
+
+
 class TestServerDelete(TestServer):
 
     def setUp(self):

openstackclient/tests/utils.py

@@ -23,6 +23,10 @@
 from openstackclient.tests import fakes
 
 
+class ParserException(Exception):
+    pass
+
+
 class TestCase(testtools.TestCase):
     def setUp(self):
         testtools.TestCase.setUp(self)
@@ -84,7 +88,7 @@ def check_parser(self, cmd, args, verify_args):
         try:
             parsed_args = cmd_parser.parse_args(args)
         except SystemExit:
-            raise Exception("Argument parse failed")
+            raise ParserException("Argument parse failed")
         for av in verify_args:
             attr, value = av
             if attr: