From 410941e435b09f94ef108096e0d6362085d0483a Mon Sep 17 00:00:00 2001 From: Brian Sidebotham Date: Wed, 22 Aug 2018 16:09:26 +0100 Subject: [PATCH 1/4] Modify the proxmox VM when using clone in the salt-cloud profile if settings are present --- salt/cloud/clouds/proxmox.py | 88 ++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/salt/cloud/clouds/proxmox.py b/salt/cloud/clouds/proxmox.py index cde89860826e..c33041e0da6e 100644 --- a/salt/cloud/clouds/proxmox.py +++ b/salt/cloud/clouds/proxmox.py @@ -491,6 +491,38 @@ def list_nodes_select(call=None): list_nodes_full(), __opts__['query.selection'], call, ) +def _stringlist_to_dictionary(input_string): + ''' + Convert a stringlist (comma separated settings) to a dictionary + + The result of the string setting1=value1,setting2=value2 will be a python dictionary: + + {'setting1':'value1','setting2':'value2'} + ''' + li = str(input_string).split(',') + ret = {} + for item in li: + pair = str(item).replace(' ','').split('=') + if len(pair) != 2: + log.warn("Cannot process stringlist item %s", item) + continue + + ret[pair[0]] = pair[1] + return ret + +def _dictionary_to_stringlist(input_dict): + ''' + Convert a dictionary to a stringlist (comma separated settings) + + The result of the dictionary {'setting1':'value1','setting2':'value2'} will be: + + setting1=value1,setting2=value2 + ''' + string_value="" + for s in input_dict: + string_value += "{0}={1},".format(s, input_dict[s]) + string_value = string_value[:-1] + return string_value def create(vm_): ''' @@ -575,6 +607,62 @@ def create(vm_): if not wait_for_created(data['upid'], timeout=300): return {'Error': 'Unable to create {0}, command timed out'.format(name)} + if 'clone' in vm_ and vm_['clone'] is True and vm_['technology'] == 'qemu': + # If we cloned a machine, see if we need to reconfigure any of the options such as net0, + # ide2, etc. This enables us to have a different cloud-init ISO mounted for each VM that's + # brought up + + # TODO: Support other settings here too as these are not the only ones that can be modified + # after a clone operation + log.info('Configuring cloned VM') + + # Modify the settings for the VM one at a time so we can see any problems with the values + # as quickly as possible + for setting_number in range(3): + setting = 'ide{0}'.format(setting_number) + if setting in vm_: + postParams = {} + postParams[setting] = vm_[setting] + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + + for setting_number in range(5): + setting = 'sata{0}'.format(setting_number) + if setting in vm_: + postParams = {} + postParams[setting] = vm_[setting] + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + + for setting_number in range(13): + setting = 'scsi{0}'.format(setting_number) + if setting in vm_: + postParams = {} + postParams[setting] = vm_[setting] + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + + # net strings are a list of comma seperated settings. We need to merge the settings so that + # the setting in the profile only changes the settings it touches and the other settings + # are left alone. An example of why this is necessary is because the MAC address is set + # in here and generally you don't want to alter or have to know the MAC address of the new + # instance, but you may want to set the VLAN bridge for example + for setting_number in range(20): + setting = 'net{0}'.format(setting_number) + if setting in vm_: + data = query('get', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid)) + + # Generate a dictionary of settings from the existing string + new_setting = {} + if setting in data: + new_setting.update(_stringlist_to_dictionary(data[setting])) + + # Merge the new settings (as a dictionary) into the existing dictionary to get the + # new merged settings + new_setting.update(_stringlist_to_dictionary(vm_[setting])) + + # Convert the dictionary back into a string list + postParams = { setting: _dictionary_to_stringlist(new_setting) } + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + + # VM has been created. Starting.. if not start(name, vmid, call='action'): log.error('Node %s (%s) failed to start!', name, vmid) From f010c57020b70bdcc14a2019bceb558fa19552fa Mon Sep 17 00:00:00 2001 From: Brian Sidebotham Date: Wed, 22 Aug 2018 16:35:57 +0100 Subject: [PATCH 2/4] Tidy up pep8 issues --- salt/cloud/clouds/proxmox.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/salt/cloud/clouds/proxmox.py b/salt/cloud/clouds/proxmox.py index c33041e0da6e..e2a66da32a18 100644 --- a/salt/cloud/clouds/proxmox.py +++ b/salt/cloud/clouds/proxmox.py @@ -491,6 +491,7 @@ def list_nodes_select(call=None): list_nodes_full(), __opts__['query.selection'], call, ) + def _stringlist_to_dictionary(input_string): ''' Convert a stringlist (comma separated settings) to a dictionary @@ -502,7 +503,7 @@ def _stringlist_to_dictionary(input_string): li = str(input_string).split(',') ret = {} for item in li: - pair = str(item).replace(' ','').split('=') + pair = str(item).replace(' ', '').split('=') if len(pair) != 2: log.warn("Cannot process stringlist item %s", item) continue @@ -510,6 +511,7 @@ def _stringlist_to_dictionary(input_string): ret[pair[0]] = pair[1] return ret + def _dictionary_to_stringlist(input_dict): ''' Convert a dictionary to a stringlist (comma separated settings) @@ -518,12 +520,13 @@ def _dictionary_to_stringlist(input_dict): setting1=value1,setting2=value2 ''' - string_value="" + string_value = "" for s in input_dict: string_value += "{0}={1},".format(s, input_dict[s]) string_value = string_value[:-1] return string_value + def create(vm_): ''' Create a single VM from a data dict @@ -659,10 +662,9 @@ def create(vm_): new_setting.update(_stringlist_to_dictionary(vm_[setting])) # Convert the dictionary back into a string list - postParams = { setting: _dictionary_to_stringlist(new_setting) } + postParams = {setting: _dictionary_to_stringlist(new_setting)} query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) - # VM has been created. Starting.. if not start(name, vmid, call='action'): log.error('Node %s (%s) failed to start!', name, vmid) From f0d5f513c4297d1cd5c41f07a80b2a99810a20fc Mon Sep 17 00:00:00 2001 From: Tyler Johnson Date: Mon, 6 Jan 2020 17:41:18 -0700 Subject: [PATCH 3/4] Wrote unit test for proxmox clone reconfigure - Refactored proxmox clone reconfigure - Refactored helper functions --- salt/cloud/clouds/proxmox.py | 116 ++++++++++-------------- tests/unit/cloud/clouds/test_proxmox.py | 98 ++++++++++++++++++++ 2 files changed, 145 insertions(+), 69 deletions(-) create mode 100644 tests/unit/cloud/clouds/test_proxmox.py diff --git a/salt/cloud/clouds/proxmox.py b/salt/cloud/clouds/proxmox.py index a50ebe535da5..aa6298e1ff4a 100644 --- a/salt/cloud/clouds/proxmox.py +++ b/salt/cloud/clouds/proxmox.py @@ -500,16 +500,7 @@ def _stringlist_to_dictionary(input_string): {'setting1':'value1','setting2':'value2'} ''' - li = str(input_string).split(',') - ret = {} - for item in li: - pair = str(item).replace(' ', '').split('=') - if len(pair) != 2: - log.warn("Cannot process stringlist item %s", item) - continue - - ret[pair[0]] = pair[1] - return ret + return dict(item.strip().split("=") for item in input_string.split(",") if item) def _dictionary_to_stringlist(input_dict): @@ -520,11 +511,50 @@ def _dictionary_to_stringlist(input_dict): setting1=value1,setting2=value2 ''' - string_value = "" - for s in input_dict: - string_value += "{0}={1},".format(s, input_dict[s]) - string_value = string_value[:-1] - return string_value + return ','.join('{}={}'.format(k, v) for k, v in input_dict.items()) + + +def _reconfigure_clone(vm_, vmid): + ''' + If we cloned a machine, see if we need to reconfigure any of the options such as net0, + ide2, etc. This enables us to have a different cloud-init ISO mounted for each VM that's brought up + :param vm_: + :return: + ''' + if not vm_.get('technology') == 'qemu': + log.warning('Reconfiguring clones is only available under `qemu`') + return + + # TODO: Support other settings here too as these are not the only ones that can be modified after a clone operation + log.info('Configuring cloned VM') + + # Modify the settings for the VM one at a time so we can see any problems with the values + # as quickly as possible + for setting in vm_: + if re.match(r'^(ide|sata|scsi)(\d+)$', setting): + postParams = {setting: vm_[setting]} + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + + elif re.match(r'^net(\d+)$', setting): + # net strings are a list of comma seperated settings. We need to merge the settings so that + # the setting in the profile only changes the settings it touches and the other settings + # are left alone. An example of why this is necessary is because the MAC address is set + # in here and generally you don't want to alter or have to know the MAC address of the new + # instance, but you may want to set the VLAN bridge + data = query('get', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid)) + + # Generate a dictionary of settings from the existing string + new_setting = {} + if setting in data: + new_setting.update(_stringlist_to_dictionary(data[setting])) + + # Merge the new settings (as a dictionary) into the existing dictionary to get the + # new merged settings + new_setting.update(_stringlist_to_dictionary(vm_[setting])) + + # Convert the dictionary back into a string list + postParams = {setting: _dictionary_to_stringlist(new_setting)} + query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) def create(vm_): @@ -610,60 +640,8 @@ def create(vm_): if not wait_for_created(data['upid'], timeout=300): return {'Error': 'Unable to create {0}, command timed out'.format(name)} - if 'clone' in vm_ and vm_['clone'] is True and vm_['technology'] == 'qemu': - # If we cloned a machine, see if we need to reconfigure any of the options such as net0, - # ide2, etc. This enables us to have a different cloud-init ISO mounted for each VM that's - # brought up - - # TODO: Support other settings here too as these are not the only ones that can be modified - # after a clone operation - log.info('Configuring cloned VM') - - # Modify the settings for the VM one at a time so we can see any problems with the values - # as quickly as possible - for setting_number in range(3): - setting = 'ide{0}'.format(setting_number) - if setting in vm_: - postParams = {} - postParams[setting] = vm_[setting] - query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) - - for setting_number in range(5): - setting = 'sata{0}'.format(setting_number) - if setting in vm_: - postParams = {} - postParams[setting] = vm_[setting] - query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) - - for setting_number in range(13): - setting = 'scsi{0}'.format(setting_number) - if setting in vm_: - postParams = {} - postParams[setting] = vm_[setting] - query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) - - # net strings are a list of comma seperated settings. We need to merge the settings so that - # the setting in the profile only changes the settings it touches and the other settings - # are left alone. An example of why this is necessary is because the MAC address is set - # in here and generally you don't want to alter or have to know the MAC address of the new - # instance, but you may want to set the VLAN bridge for example - for setting_number in range(20): - setting = 'net{0}'.format(setting_number) - if setting in vm_: - data = query('get', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid)) - - # Generate a dictionary of settings from the existing string - new_setting = {} - if setting in data: - new_setting.update(_stringlist_to_dictionary(data[setting])) - - # Merge the new settings (as a dictionary) into the existing dictionary to get the - # new merged settings - new_setting.update(_stringlist_to_dictionary(vm_[setting])) - - # Convert the dictionary back into a string list - postParams = {setting: _dictionary_to_stringlist(new_setting)} - query('post', 'nodes/{0}/qemu/{1}/config'.format(vm_['host'], vmid), postParams) + if vm_.get('clone') is True: + _reconfigure_clone(vm_, vmid) # VM has been created. Starting.. if not start(name, vmid, call='action'): diff --git a/tests/unit/cloud/clouds/test_proxmox.py b/tests/unit/cloud/clouds/test_proxmox.py new file mode 100644 index 000000000000..cb593d8e3fa9 --- /dev/null +++ b/tests/unit/cloud/clouds/test_proxmox.py @@ -0,0 +1,98 @@ +# -*- coding: utf-8 -*- +''' + :codeauthor: Tyler Johnson +''' + +# Import Salt Libs +from __future__ import absolute_import, print_function, unicode_literals + +# Import Salt Testing Libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.unit import TestCase +from tests.support.mock import ( + MagicMock, + patch, +) + +# Import Salt Libs +from salt.cloud.clouds import proxmox + + +class ProxmoxTest(TestCase, LoaderModuleMockMixin): + def setup_loader_modules(self): + return { + proxmox: { + '__utils__': { + 'cloud.fire_event': MagicMock(), + 'cloud.bootstrap': MagicMock() + }, + '__opts__': { + 'sock_dir': True, + 'transport': True, + 'providers': {'my_proxmox': {}}, + 'profiles': {'my_proxmox': {}} + }, + '__active_provider_name__': 'my_proxmox:proxmox' + } + } + + def setUp(self): + self.vm_ = { + 'profile': 'my_proxmox', + 'name': 'vm4', + 'driver': 'proxmox', + 'technology': 'qemu', + 'host': '127.0.0.1', + 'clone': True, + 'ide0': 'data', + 'sata0': 'data', + 'scsi0': 'data', + 'net0': 'a=b,c=d', + } + + def tearDown(self): + del self.vm_ + + def test__stringlist_to_dictionary(self): + result = proxmox._stringlist_to_dictionary('') + self.assertEqual(result, {}) + + result = proxmox._stringlist_to_dictionary('foo=bar, ignored_space=bar,internal space=bar') + self.assertEqual(result, {'foo': 'bar', 'ignored_space': 'bar', 'internal space': 'bar'}) + + # Negative cases + self.assertRaises(ValueError, proxmox._stringlist_to_dictionary, 'foo=bar,foo') + self.assertRaises(ValueError, proxmox._stringlist_to_dictionary, 'foo=bar,totally=invalid=assignment') + + def test__dictionary_to_stringlist(self): + result = proxmox._dictionary_to_stringlist({}) + self.assertEqual(result, '') + + result = proxmox._dictionary_to_stringlist({'a': 'a'}) + self.assertEqual(result, 'a=a') + + result = proxmox._dictionary_to_stringlist({'a': 'a', 'b': 'b'}) + self.assertEqual(result, 'a=a,b=b') + + # Negative cases + result = proxmox._dictionary_to_stringlist({1: {}, None: True}) + self.assertEqual(result, '1={},None=True') + + def test__reconfigure_clone(self): + # The return_value is for the net reconfigure assertions, it is irrelevant for the rest + with patch.object(proxmox, 'query', return_value={'net0': 'c=overwritten,g=h'}) as query: + # Test a vm that lacks the required attributes + proxmox._reconfigure_clone({}, 0) + query.assert_not_called() + + # Test a fully mocked vm + proxmox._reconfigure_clone(self.vm_, 0) + + # net reconfigure + query.assert_any_call('get', 'nodes/127.0.0.1/qemu/0/config') + query.assert_any_call('post', 'nodes/127.0.0.1/qemu/0/config', {'net0': 'a=b,c=d,g=h'}) + + # hdd reconfigure + query.assert_any_call('post', 'nodes/127.0.0.1/qemu/0/config', {'ide0': 'data'}) + query.assert_any_call('post', 'nodes/127.0.0.1/qemu/0/config', {'sata0': 'data'}) + query.assert_any_call('post', 'nodes/127.0.0.1/qemu/0/config', {'scsi0': 'data'}) From 1319d4fb105cdf917efe315a56253f00dfd989af Mon Sep 17 00:00:00 2001 From: Tyler Johnson Date: Mon, 6 Jan 2020 22:48:07 -0700 Subject: [PATCH 4/4] Sort dict keys for consistent return --- salt/cloud/clouds/proxmox.py | 2 +- tests/unit/cloud/clouds/test_proxmox.py | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/salt/cloud/clouds/proxmox.py b/salt/cloud/clouds/proxmox.py index aa6298e1ff4a..d41f3c4d2a72 100644 --- a/salt/cloud/clouds/proxmox.py +++ b/salt/cloud/clouds/proxmox.py @@ -511,7 +511,7 @@ def _dictionary_to_stringlist(input_dict): setting1=value1,setting2=value2 ''' - return ','.join('{}={}'.format(k, v) for k, v in input_dict.items()) + return ','.join('{}={}'.format(k, input_dict[k]) for k in sorted(input_dict.keys())) def _reconfigure_clone(vm_, vmid): diff --git a/tests/unit/cloud/clouds/test_proxmox.py b/tests/unit/cloud/clouds/test_proxmox.py index cb593d8e3fa9..cc6b22f12495 100644 --- a/tests/unit/cloud/clouds/test_proxmox.py +++ b/tests/unit/cloud/clouds/test_proxmox.py @@ -74,10 +74,6 @@ def test__dictionary_to_stringlist(self): result = proxmox._dictionary_to_stringlist({'a': 'a', 'b': 'b'}) self.assertEqual(result, 'a=a,b=b') - # Negative cases - result = proxmox._dictionary_to_stringlist({1: {}, None: True}) - self.assertEqual(result, '1={},None=True') - def test__reconfigure_clone(self): # The return_value is for the net reconfigure assertions, it is irrelevant for the rest with patch.object(proxmox, 'query', return_value={'net0': 'c=overwritten,g=h'}) as query: