Skip to content

Commit 7fea072

Browse files
underscorephilunderscorephil
committed
Merge pull request softlayer#684 from camporter/vs_upgrade_improvements
Check at least one upgrade option is provided.
2 parents e639012 + be5a95f commit 7fea072

6 files changed

Lines changed: 107 additions & 38 deletions

File tree

‎SoftLayer/CLI/virt/upgrade.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,14 @@ def cli(env, identifier, cpu, private, memory, network):
2727

2828
vsi = SoftLayer.VSManager(env.client)
2929

30+
if not any([cpu, memory, network]):
31+
raise exceptions.ArgumentError(
32+
"Must provide [--cpu], [--memory], or [--network] to upgrade")
33+
34+
if private and not cpu:
35+
raise exceptions.ArgumentError(
36+
"Must specify [--cpu] when using [--private]")
37+
3038
vs_id = helpers.resolve_id(vsi.resolve_ids, identifier, 'VS')
3139
if not (env.skip_confirmations or formatting.confirm(
3240
"This action will incur charges on your account. "

‎SoftLayer/fixtures/SoftLayer_Product_Package.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,18 @@
228228
'description': 'Computing Instance',
229229
'itemCategory': {'categoryCode': 'Computing Instance'},
230230
'prices': [{'id': 1144,
231+
'locationGroupId': None,
232+
'categories': [{'id': 80,
233+
'name': 'Computing Instance',
234+
'categoryCode': 'guest_core'}]}],
235+
},
236+
{
237+
'id': 112233,
238+
'capacity': '55',
239+
'description': 'Computing Instance',
240+
'itemCategory': {'categoryCode': 'Computing Instance'},
241+
'prices': [{'id': 332211,
242+
'locationGroupId': 1,
231243
'categories': [{'id': 80,
232244
'name': 'Computing Instance',
233245
'categoryCode': 'guest_core'}]}],

‎SoftLayer/managers/vs.py‎

Lines changed: 45 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import socket
1111
import time
1212

13+
from SoftLayer import exceptions
1314
from SoftLayer.managers import ordering
1415
from SoftLayer import utils
1516
# pylint: disable=no-self-use
@@ -745,22 +746,22 @@ def upgrade(self, instance_id, cpus=None, memory=None,
745746
"""
746747
package_items = self._get_package_items()
747748
prices = []
748-
if cpus:
749-
prices.append({
750-
'id': self._get_item_id_for_upgrade(package_items,
751-
'cpus',
752-
cpus,
753-
public)})
754-
if memory:
755-
prices.append({
756-
'id': self._get_item_id_for_upgrade(package_items,
757-
'memory',
758-
memory)})
759-
if nic_speed:
760-
prices.append({
761-
'id': self._get_item_id_for_upgrade(package_items,
762-
'nic_speed',
763-
nic_speed)})
749+
750+
for option, value in {'cpus': cpus,
751+
'memory': memory,
752+
'nic_speed': nic_speed}.items():
753+
if not value:
754+
continue
755+
price_id = self._get_price_id_for_upgrade(package_items,
756+
option,
757+
value,
758+
public)
759+
if not price_id:
760+
# Every option provided is expected to have a price
761+
raise exceptions.SoftLayerError(
762+
"Unable to find %s option with value %s" % (option, value))
763+
764+
prices.append({'id': price_id})
764765

765766
maintenance_window = datetime.datetime.now(utils.UTC())
766767
order = {
@@ -783,7 +784,7 @@ def _get_package_items(self):
783784
mask = [
784785
'description',
785786
'capacity',
786-
'prices[id,categories[name,id,categoryCode]]'
787+
'prices[id,locationGroupId,categories[name,id,categoryCode]]'
787788
]
788789
mask = "mask[%s]" % ','.join(mask)
789790

@@ -793,9 +794,9 @@ def _get_package_items(self):
793794

794795
return package_service.getItems(id=package_id, mask=mask)
795796

796-
def _get_item_id_for_upgrade(self, package_items, option, value,
797-
public=True):
798-
"""Find the item ids for the parameters you want to upgrade to.
797+
def _get_price_id_for_upgrade(self, package_items, option, value,
798+
public=True):
799+
"""Find the price id for the option and value to upgrade.
799800
800801
:param list package_items: Contains all the items related to an VS
801802
: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,
807808
'cpus': 'guest_core',
808809
'nic_speed': 'port_speed'
809810
}
810-
811+
category_code = option_category[option]
811812
for item in package_items:
812-
categories = item['prices'][0]['categories']
813-
for category in categories:
814-
if not (category['categoryCode'] == option_category[option] and
815-
str(item['capacity']) == str(value)):
813+
is_private = str(item['description']).startswith('Private')
814+
for price in item['prices']:
815+
if 'locationGroupId' in price and price['locationGroupId']:
816+
# Skip location based prices
817+
continue
818+
819+
if 'categories' not in price:
816820
continue
817-
if option == 'cpus':
818-
if public and ('Private' not in item['description']):
819-
return item['prices'][0]['id']
820-
elif not public and ('Private' in item['description']):
821-
return item['prices'][0]['id']
822-
elif option == 'nic_speed':
823-
if 'Public' in item['description']:
824-
return item['prices'][0]['id']
825-
else:
826-
return item['prices'][0]['id']
821+
822+
categories = price['categories']
823+
for category in categories:
824+
if not (category['categoryCode'] == category_code
825+
and str(item['capacity']) == str(value)):
826+
continue
827+
if option == 'cpus':
828+
if public and not is_private:
829+
return price['id']
830+
elif not public and is_private:
831+
return price['id']
832+
elif option == 'nic_speed':
833+
if 'Public' in item['description']:
834+
return price['id']
835+
else:
836+
return price['id']

‎tests/CLI/modules/vs_tests.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,3 +320,35 @@ def test_dns_sync_misc_exception(self, confirm_mock):
320320
result = self.run_command(['vs', 'dns-sync', '-a', '100'])
321321
self.assertEqual(result.exit_code, 2)
322322
self.assertIsInstance(result.exception, exceptions.CLIAbort)
323+
324+
def test_upgrade_no_options(self, ):
325+
result = self.run_command(['vs', 'upgrade', '100'])
326+
self.assertEqual(result.exit_code, 2)
327+
self.assertIsInstance(result.exception, exceptions.ArgumentError)
328+
329+
def test_upgrade_private_no_cpu(self):
330+
result = self.run_command(['vs', 'upgrade', '100', '--private',
331+
'--memory=1024'])
332+
self.assertEqual(result.exit_code, 2)
333+
self.assertIsInstance(result.exception, exceptions.ArgumentError)
334+
335+
@mock.patch('SoftLayer.CLI.formatting.confirm')
336+
def test_upgrade_aborted(self, confirm_mock):
337+
confirm_mock.return_value = False
338+
result = self.run_command(['vs', 'upgrade', '100', '--cpu=1'])
339+
self.assertEqual(result.exit_code, 2)
340+
self.assertIsInstance(result.exception, exceptions.CLIAbort)
341+
342+
@mock.patch('SoftLayer.CLI.formatting.confirm')
343+
def test_upgrade(self, confirm_mock):
344+
confirm_mock.return_value = True
345+
result = self.run_command(['vs', 'upgrade', '100', '--cpu=4',
346+
'--memory=2048', '--network=1000'])
347+
self.assertEqual(result.exit_code, 0)
348+
self.assert_called_with('SoftLayer_Product_Order', 'placeOrder')
349+
call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0]
350+
order_container = call.args[0]
351+
self.assertIn({'id': 1144}, order_container['prices'])
352+
self.assertIn({'id': 1133}, order_container['prices'])
353+
self.assertIn({'id': 1122}, order_container['prices'])
354+
self.assertEqual(order_container['virtualGuests'], [{'id': 100}])

‎tests/managers/loadbal_tests.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ def set_up(self):
1919
def test_get_lb_pkgs(self):
2020
result = self.lb_mgr.get_lb_pkgs()
2121

22-
self.assertEqual(len(result), 12)
22+
self.assertEqual(len(result), 13)
2323
_filter = {
2424
'items': {
2525
'description': {

‎tests/managers/vs_tests.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import mock
88

99
import SoftLayer
10+
from SoftLayer import exceptions
1011
from SoftLayer import fixtures
1112
from SoftLayer import testing
1213

@@ -672,10 +673,16 @@ def test_upgrade_full(self):
672673
self.assert_called_with('SoftLayer_Product_Order', 'placeOrder')
673674
call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0]
674675
order_container = call.args[0]
675-
self.assertEqual(order_container['prices'],
676-
[{'id': 1144}, {'id': 1133}, {'id': 1122}])
676+
self.assertIn({'id': 1144}, order_container['prices'])
677+
self.assertIn({'id': 1133}, order_container['prices'])
678+
self.assertIn({'id': 1122}, order_container['prices'])
677679
self.assertEqual(order_container['virtualGuests'], [{'id': 1}])
678680

681+
def test_upgrade_skips_location_based_prices(self):
682+
# Test that no prices that have locationGroupId set are used
683+
self.assertRaises(exceptions.SoftLayerError,
684+
self.vs.upgrade, 1, cpus=55, memory=2, public=True)
685+
679686
def test_get_item_id_for_upgrade(self):
680687
item_id = 0
681688
package_items = self.client['Product_Package'].getItems(id=46)

0 commit comments

Comments
 (0)