From be5a95f5c214e7d45a96378c6fe71507df3898c7 Mon Sep 17 00:00:00 2001 From: Cameron Porter Date: Wed, 3 Feb 2016 00:46:50 -0600 Subject: [PATCH] Check at least one upgrade option is provided. Ensure that private option requires the cpu option. Filter out location based prices when upgrading. Add additional tests around guest upgrading. --- SoftLayer/CLI/virt/upgrade.py | 8 ++ .../fixtures/SoftLayer_Product_Package.py | 12 +++ SoftLayer/managers/vs.py | 80 +++++++++++-------- tests/CLI/modules/vs_tests.py | 32 ++++++++ tests/managers/loadbal_tests.py | 2 +- tests/managers/vs_tests.py | 11 ++- 6 files changed, 107 insertions(+), 38 deletions(-) diff --git a/SoftLayer/CLI/virt/upgrade.py b/SoftLayer/CLI/virt/upgrade.py index bd17059d0..419456f91 100644 --- a/SoftLayer/CLI/virt/upgrade.py +++ b/SoftLayer/CLI/virt/upgrade.py @@ -27,6 +27,14 @@ def cli(env, identifier, cpu, private, memory, network): vsi = SoftLayer.VSManager(env.client) + if not any([cpu, memory, network]): + raise exceptions.ArgumentError( + "Must provide [--cpu], [--memory], or [--network] to upgrade") + + if private and not cpu: + raise exceptions.ArgumentError( + "Must specify [--cpu] when using [--private]") + vs_id = helpers.resolve_id(vsi.resolve_ids, identifier, 'VS') if not (env.skip_confirmations or formatting.confirm( "This action will incur charges on your account. " diff --git a/SoftLayer/fixtures/SoftLayer_Product_Package.py b/SoftLayer/fixtures/SoftLayer_Product_Package.py index c4b7844ef..1a5fbed05 100644 --- a/SoftLayer/fixtures/SoftLayer_Product_Package.py +++ b/SoftLayer/fixtures/SoftLayer_Product_Package.py @@ -228,6 +228,18 @@ 'description': 'Computing Instance', 'itemCategory': {'categoryCode': 'Computing Instance'}, 'prices': [{'id': 1144, + 'locationGroupId': None, + 'categories': [{'id': 80, + 'name': 'Computing Instance', + 'categoryCode': 'guest_core'}]}], + }, + { + 'id': 112233, + 'capacity': '55', + 'description': 'Computing Instance', + 'itemCategory': {'categoryCode': 'Computing Instance'}, + 'prices': [{'id': 332211, + 'locationGroupId': 1, 'categories': [{'id': 80, 'name': 'Computing Instance', 'categoryCode': 'guest_core'}]}], diff --git a/SoftLayer/managers/vs.py b/SoftLayer/managers/vs.py index a78bdba44..320d7c4ae 100644 --- a/SoftLayer/managers/vs.py +++ b/SoftLayer/managers/vs.py @@ -10,6 +10,7 @@ import socket import time +from SoftLayer import exceptions from SoftLayer.managers import ordering from SoftLayer import utils # pylint: disable=no-self-use @@ -745,22 +746,22 @@ def upgrade(self, instance_id, cpus=None, memory=None, """ package_items = self._get_package_items() prices = [] - if cpus: - prices.append({ - 'id': self._get_item_id_for_upgrade(package_items, - 'cpus', - cpus, - public)}) - if memory: - prices.append({ - 'id': self._get_item_id_for_upgrade(package_items, - 'memory', - memory)}) - if nic_speed: - prices.append({ - 'id': self._get_item_id_for_upgrade(package_items, - 'nic_speed', - nic_speed)}) + + for option, value in {'cpus': cpus, + 'memory': memory, + 'nic_speed': nic_speed}.items(): + if not value: + continue + price_id = self._get_price_id_for_upgrade(package_items, + option, + value, + public) + if not price_id: + # Every option provided is expected to have a price + raise exceptions.SoftLayerError( + "Unable to find %s option with value %s" % (option, value)) + + prices.append({'id': price_id}) maintenance_window = datetime.datetime.now(utils.UTC()) order = { @@ -783,7 +784,7 @@ def _get_package_items(self): mask = [ 'description', 'capacity', - 'prices[id,categories[name,id,categoryCode]]' + 'prices[id,locationGroupId,categories[name,id,categoryCode]]' ] mask = "mask[%s]" % ','.join(mask) @@ -793,9 +794,9 @@ def _get_package_items(self): return package_service.getItems(id=package_id, mask=mask) - def _get_item_id_for_upgrade(self, package_items, option, value, - public=True): - """Find the item ids for the parameters you want to upgrade to. + def _get_price_id_for_upgrade(self, package_items, option, value, + public=True): + """Find the price id for the option and value to upgrade. :param list package_items: Contains all the items related to an VS :param string option: Describes type of parameter to be upgraded @@ -807,20 +808,29 @@ def _get_item_id_for_upgrade(self, package_items, option, value, 'cpus': 'guest_core', 'nic_speed': 'port_speed' } - + category_code = option_category[option] for item in package_items: - categories = item['prices'][0]['categories'] - for category in categories: - if not (category['categoryCode'] == option_category[option] and - str(item['capacity']) == str(value)): + is_private = str(item['description']).startswith('Private') + for price in item['prices']: + if 'locationGroupId' in price and price['locationGroupId']: + # Skip location based prices + continue + + if 'categories' not in price: continue - if option == 'cpus': - if public and ('Private' not in item['description']): - return item['prices'][0]['id'] - elif not public and ('Private' in item['description']): - return item['prices'][0]['id'] - elif option == 'nic_speed': - if 'Public' in item['description']: - return item['prices'][0]['id'] - else: - return item['prices'][0]['id'] + + categories = price['categories'] + for category in categories: + if not (category['categoryCode'] == category_code + and str(item['capacity']) == str(value)): + continue + if option == 'cpus': + if public and not is_private: + return price['id'] + elif not public and is_private: + return price['id'] + elif option == 'nic_speed': + if 'Public' in item['description']: + return price['id'] + else: + return price['id'] diff --git a/tests/CLI/modules/vs_tests.py b/tests/CLI/modules/vs_tests.py index 002b17905..f4f318416 100644 --- a/tests/CLI/modules/vs_tests.py +++ b/tests/CLI/modules/vs_tests.py @@ -320,3 +320,35 @@ def test_dns_sync_misc_exception(self, confirm_mock): result = self.run_command(['vs', 'dns-sync', '-a', '100']) self.assertEqual(result.exit_code, 2) self.assertIsInstance(result.exception, exceptions.CLIAbort) + + def test_upgrade_no_options(self, ): + result = self.run_command(['vs', 'upgrade', '100']) + self.assertEqual(result.exit_code, 2) + self.assertIsInstance(result.exception, exceptions.ArgumentError) + + def test_upgrade_private_no_cpu(self): + result = self.run_command(['vs', 'upgrade', '100', '--private', + '--memory=1024']) + self.assertEqual(result.exit_code, 2) + self.assertIsInstance(result.exception, exceptions.ArgumentError) + + @mock.patch('SoftLayer.CLI.formatting.confirm') + def test_upgrade_aborted(self, confirm_mock): + confirm_mock.return_value = False + result = self.run_command(['vs', 'upgrade', '100', '--cpu=1']) + self.assertEqual(result.exit_code, 2) + self.assertIsInstance(result.exception, exceptions.CLIAbort) + + @mock.patch('SoftLayer.CLI.formatting.confirm') + def test_upgrade(self, confirm_mock): + confirm_mock.return_value = True + result = self.run_command(['vs', 'upgrade', '100', '--cpu=4', + '--memory=2048', '--network=1000']) + self.assertEqual(result.exit_code, 0) + self.assert_called_with('SoftLayer_Product_Order', 'placeOrder') + call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0] + order_container = call.args[0] + self.assertIn({'id': 1144}, order_container['prices']) + self.assertIn({'id': 1133}, order_container['prices']) + self.assertIn({'id': 1122}, order_container['prices']) + self.assertEqual(order_container['virtualGuests'], [{'id': 100}]) diff --git a/tests/managers/loadbal_tests.py b/tests/managers/loadbal_tests.py index 960fd241b..f7fd63910 100644 --- a/tests/managers/loadbal_tests.py +++ b/tests/managers/loadbal_tests.py @@ -19,7 +19,7 @@ def set_up(self): def test_get_lb_pkgs(self): result = self.lb_mgr.get_lb_pkgs() - self.assertEqual(len(result), 12) + self.assertEqual(len(result), 13) _filter = { 'items': { 'description': { diff --git a/tests/managers/vs_tests.py b/tests/managers/vs_tests.py index 1ad40add8..a0cecb205 100644 --- a/tests/managers/vs_tests.py +++ b/tests/managers/vs_tests.py @@ -7,6 +7,7 @@ import mock import SoftLayer +from SoftLayer import exceptions from SoftLayer import fixtures from SoftLayer import testing @@ -672,10 +673,16 @@ def test_upgrade_full(self): self.assert_called_with('SoftLayer_Product_Order', 'placeOrder') call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0] order_container = call.args[0] - self.assertEqual(order_container['prices'], - [{'id': 1144}, {'id': 1133}, {'id': 1122}]) + self.assertIn({'id': 1144}, order_container['prices']) + self.assertIn({'id': 1133}, order_container['prices']) + self.assertIn({'id': 1122}, order_container['prices']) self.assertEqual(order_container['virtualGuests'], [{'id': 1}]) + def test_upgrade_skips_location_based_prices(self): + # Test that no prices that have locationGroupId set are used + self.assertRaises(exceptions.SoftLayerError, + self.vs.upgrade, 1, cpus=55, memory=2, public=True) + def test_get_item_id_for_upgrade(self): item_id = 0 package_items = self.client['Product_Package'].getItems(id=46)