From f528dba3968d69b5dd97345757561ec614f9ae51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20Szczepa=C5=84ski?= Date: Wed, 20 Jun 2018 11:14:55 +0200 Subject: [PATCH] VCenter Port management refactor Use Criteria to fetch DVPorts Add method for retrieving DVS by UUID Change VCenterPort constructor to consume whole device object Change set_vlan_id signature to consume whole VCenterPort object Change get_vlan_id signature to consume only VCenterPort object Change restore_vlan_id signature to consume only VCenterPort object Extract fetching port from dvs to a function Extract creating inherited port config spec to a function Remove unused get_ip_pool methods from VCenterAPIClient Catch exceptions for vrouter-uuid annotation reading Remove NoneType check from create_and_read_instance_ip Partial-Bug: #1777404 Change-Id: I271ff574509fbbb070acb0ddaa0a35f5a76ea5ce (cherry picked from commit 7f3b6588ace16c7ad24412239c14852e5a4b9afc) --- cvm/clients.py | 71 ++++++++++++++++++------------------------ cvm/models.py | 15 ++++----- cvm/services.py | 22 +++++++------ tests/test_clients.py | 47 ++++++++++++++++++++++------ tests/test_events.py | 12 +++++-- tests/test_models.py | 6 ++-- tests/test_services.py | 34 +++++++++++++++++--- 7 files changed, 129 insertions(+), 78 deletions(-) diff --git a/cvm/clients.py b/cvm/clients.py index ac34908..08e4b50 100644 --- a/cvm/clients.py +++ b/cvm/clients.py @@ -42,13 +42,16 @@ def make_filter_spec(obj, filters): return filter_spec -def make_dv_port_spec(dv_port, vlan_id): +def make_dv_port_spec(dv_port, vlan_id=None): dv_port_config_spec = vim.dvs.DistributedVirtualPort.ConfigSpec() dv_port_config_spec.key = dv_port.key dv_port_config_spec.operation = 'edit' dv_port_config_spec.configVersion = dv_port.config.configVersion vlan_spec = vim.dvs.VmwareDistributedVirtualSwitch.VlanIdSpec() - vlan_spec.vlanId = vlan_id + if vlan_id: + vlan_spec.vlanId = vlan_id + else: + vlan_spec.inherited = True dv_port_setting = vim.dvs.VmwareDistributedVirtualSwitch.VmwarePortConfigPolicy() dv_port_setting.vlan = vlan_spec dv_port_config_spec.setting = dv_port_setting @@ -63,6 +66,15 @@ def make_pg_config_vlan_override(portgroup): return pg_config_spec +def fetch_port_from_dvs(dvs, port_key): + criteria = vim.dvs.PortCriteria() + criteria.portKey = port_key + try: + return next(port for port in dvs.FetchDVPorts(criteria)) + except StopIteration: + return None + + class VSphereAPIClient(object): def __init__(self): self._si = None @@ -166,50 +178,32 @@ def get_dpg_by_name(self, name): return dpg return None - def get_ip_pool_for_dpg(self, dpg): - return self._get_ip_pool_by_id(dpg.summary.ipPoolId) - - def _get_ip_pool_by_id(self, pool_id): - for ip_pool in self._si.content.ipPoolManager.QueryIpPools(self._datacenter): - if ip_pool.id == pool_id: - return ip_pool - return None - - def set_vlan_id(self, dvs_name, key, vlan_id): - dvs = self._get_object([vim.dvs.VmwareDistributedVirtualSwitch], dvs_name) - try: - dv_port = next(port for port in dvs.FetchDVPorts() if port.key == key) - except StopIteration: + def set_vlan_id(self, vcenter_port): + dvs = self._get_dvs_by_uuid(vcenter_port.dvs_uuid) + dv_port = fetch_port_from_dvs(dvs, vcenter_port.port_key) + if not dv_port: return - dv_port_spec = make_dv_port_spec(dv_port, vlan_id) - logger.info('Setting VLAN ID of port %s to %d', key, vlan_id) + dv_port_spec = make_dv_port_spec(dv_port, vcenter_port.vlan_id) + logger.info('Setting VLAN ID of port %s to %d', vcenter_port.port_key, vcenter_port.vlan_id) dvs.ReconfigureDVPort_Task(port=[dv_port_spec]) - def get_vlan_id(self, vmi_model): - dvs = self._get_object([vim.dvs.VmwareDistributedVirtualSwitch], vmi_model.vn_model.dvs_name) - try: - # TODO: Use criteria to fetch single port - dv_port = next(port for port in dvs.FetchDVPorts() if port.key == vmi_model.vcenter_port.port_key) - except StopIteration: - return None + def get_vlan_id(self, vcenter_port): + dvs = self._get_dvs_by_uuid(vcenter_port.dvs_uuid) + dv_port = fetch_port_from_dvs(dvs, vcenter_port.port_key) if not dv_port.config.setting.vlan.inherited: return dv_port.config.setting.vlan.vlanId return None - def restore_vlan_id(self, dvs_name, key): - dvs = self._get_object([vim.dvs.VmwareDistributedVirtualSwitch], dvs_name) - dv_port = [port for port in dvs.FetchDVPorts() if port.key == key][0] - dv_port_config_spec = vim.dvs.DistributedVirtualPort.ConfigSpec() - dv_port_config_spec.key = dv_port.key - dv_port_config_spec.operation = 'edit' - dv_port_config_spec.configVersion = dv_port.config.configVersion - vlan_spec = vim.dvs.VmwareDistributedVirtualSwitch.VlanIdSpec() - vlan_spec.inherited = True - dv_port_setting = vim.dvs.VmwareDistributedVirtualSwitch.VmwarePortConfigPolicy() - dv_port_setting.vlan = vlan_spec - dv_port_config_spec.setting = dv_port_setting + def restore_vlan_id(self, vcenter_port): + dvs = self._get_dvs_by_uuid(vcenter_port.dvs_uuid) + dv_port = fetch_port_from_dvs(dvs, vcenter_port.port_key) + dv_port_config_spec = make_dv_port_spec(dv_port) dvs.ReconfigureDVPort_Task(port=[dv_port_config_spec]) + def _get_dvs_by_uuid(self, uuid): + dvs_manager = self._si.content.dvSwitchManager + return dvs_manager.QueryDvsByUuid(uuid) + @staticmethod def enable_vlan_override(portgroup): pg_config_spec = make_pg_config_vlan_override(portgroup) @@ -366,11 +360,8 @@ def _create_ipam(self): return ipam def create_and_read_instance_ip(self, instance_ip): - if not instance_ip: - return None try: return self._read_instance_ip(instance_ip.uuid) - # TODO: Refactor this except NoIdError: self.vnc_lib.instance_ip_create(instance_ip) logger.debug("Created instanceIP: %s", instance_ip.name) diff --git a/cvm/models.py b/cvm/models.py index 2fdaaa3..c011a62 100644 --- a/cvm/models.py +++ b/cvm/models.py @@ -97,14 +97,10 @@ def rename(self, name): def update_ports(self): self.ports = self._read_ports() - # port = next(port for port in self.ports if port.mac_address == mac_address) - # port.portgroup_key = portgroup_key def _read_ports(self): try: - return [VCenterPort(mac_address=device.macAddress, - port_key=device.backing.port.portKey, - portgroup_key=device.backing.port.portgroupKey) + return [VCenterPort(device) for device in self.vmware_vm.config.hardware.device if isinstance(device.backing, vim.vm.device.VirtualEthernetCard.DistributedVirtualPortBackingInfo)] except AttributeError: @@ -307,8 +303,9 @@ def free(self, vlan_id): class VCenterPort(object): - def __init__(self, mac_address, port_key, portgroup_key): - self.mac_address = mac_address - self.port_key = port_key - self.portgroup_key = portgroup_key + def __init__(self, device): + self.mac_address = device.macAddress + self.port_key = device.backing.port.portKey + self.portgroup_key = device.backing.port.portgroupKey + self.dvs_uuid = device.backing.port.switchUuid self.vlan_id = None diff --git a/cvm/services.py b/cvm/services.py index 3a86f5e..568b95b 100644 --- a/cvm/services.py +++ b/cvm/services.py @@ -26,9 +26,14 @@ def _can_delete_from_vnc(self, vnc_obj): existing_obj = self._vnc_api_client.read_vm(vnc_obj.uuid) if vnc_obj.get_type() == 'virtual-machine-interface': existing_obj = self._vnc_api_client.read_vmi(vnc_obj.uuid) - existing_obj_vrouter_uuid = next(pair.value - for pair in existing_obj.get_annotations().key_value_pair - if pair.key == 'vrouter-uuid') + try: + existing_obj_vrouter_uuid = next(pair.value + for pair in existing_obj.get_annotations().key_value_pair + if pair.key == 'vrouter-uuid') + except (AttributeError, StopIteration): + logger.error('Cannot read vrouter-uuid annotation for %s %s.', vnc_obj.get_type(), vnc_obj.name) + return False + if existing_obj_vrouter_uuid == self._vrouter_uuid: return True logger.error('%s %s is managed by vRouter %s and cannot be deleted from VNC.', @@ -160,8 +165,7 @@ def update_vmis_vn(self, vmware_vm, mac_address): vmi_model.security_group = self._default_security_group if vmi_model.vn_model != vn_model: with self._vcenter_api_client: - self._vcenter_api_client.restore_vlan_id( - vmi_model.vn_model.dvs_name, vmi_model.vcenter_port.port_key) + self._vcenter_api_client.restore_vlan_id(vmi_model.vcenter_port) vmi_model.clear_vlan_id() vmi_model.vn_model = vn_model vmi_model.vcenter_port = vcenter_port @@ -170,11 +174,10 @@ def update_vmis_vn(self, vmware_vm, mac_address): def _create_or_update(self, vmi_model): with self._vcenter_api_client: - current_vlan_id = self._vcenter_api_client.get_vlan_id(vmi_model) + current_vlan_id = self._vcenter_api_client.get_vlan_id(vmi_model.vcenter_port) vmi_model.acquire_vlan_id(current_vlan_id) if not current_vlan_id: - self._vcenter_api_client.set_vlan_id( - vmi_model.vn_model.dvs_name, vmi_model.vcenter_port.port_key, vmi_model.vcenter_port.vlan_id) + self._vcenter_api_client.set_vlan_id(vmi_model.vcenter_port) self._vnc_api_client.update_or_create_vmi(vmi_model.to_vnc()) vmi_model.construct_instance_ip() if vmi_model.vnc_instance_ip: @@ -219,8 +222,7 @@ def _delete_from_vnc(self, vmi_model): def _restore_vlan_id(self, vmi_model): with self._vcenter_api_client: - self._vcenter_api_client.restore_vlan_id( - vmi_model.vn_model.dvs_name, vmi_model.vcenter_port.port_key) + self._vcenter_api_client.restore_vlan_id(vmi_model.vcenter_port) vmi_model.clear_vlan_id() def _delete_vrouter_port(self, vmi_model): diff --git a/tests/test_clients.py b/tests/test_clients.py index 3a15ef6..8aee218 100644 --- a/tests/test_clients.py +++ b/tests/test_clients.py @@ -36,13 +36,14 @@ def setUp(self): def test_set_vlan_id(self): dv_port = Mock(key='8') dv_port.config.configVersion = '1' - dvs = Mock(name='DSwitch') + dvs = Mock() dvs.FetchDVPorts.return_value = [dv_port] - with patch('cvm.clients.VSphereAPIClient._get_object') as get_obj_mock: - get_obj_mock.return_value = dvs + vcenter_port = Mock(port_key='8', vlan_id=10) + with patch('cvm.clients.VCenterAPIClient._get_dvs_by_uuid') as get_dvs_mock: + get_dvs_mock.return_value = dvs with patch('cvm.clients.SmartConnectNoSSL'): with self.vcenter_client: - self.vcenter_client.set_vlan_id(dvs_name=dvs.name, key='8', vlan_id=10) + self.vcenter_client.set_vlan_id(vcenter_port) dvs.ReconfigureDVPort_Task.assert_called_once() spec = dvs.ReconfigureDVPort_Task.call_args[1].get('port', [None])[0] @@ -71,17 +72,35 @@ def test_get_vlan_id(self): dv_port.config.setting.vlan.inherited = False dvs = Mock(name='DSwitch') dvs.FetchDVPorts.return_value = [dv_port] - vmi_model = Mock() - vmi_model.vcenter_port.port_key = '20' + vcenter_port = Mock(port_key='20') - with patch('cvm.clients.VSphereAPIClient._get_object') as get_obj_mock: - get_obj_mock.return_value = dvs + with patch('cvm.clients.VCenterAPIClient._get_dvs_by_uuid') as get_dvs_mock: + get_dvs_mock.return_value = dvs with patch('cvm.clients.SmartConnectNoSSL'): with self.vcenter_client: - result = self.vcenter_client.get_vlan_id(vmi_model) + result = self.vcenter_client.get_vlan_id(vcenter_port) self.assertEqual(10, result) + def test_restore_vlan_id(self): + dv_port = Mock(key='8') + dv_port.config.configVersion = '1' + dvs = Mock() + dvs.FetchDVPorts.return_value = [dv_port] + vcenter_port = Mock(port_key='8', vlan_id=10) + with patch('cvm.clients.VCenterAPIClient._get_dvs_by_uuid') as get_dvs_mock: + get_dvs_mock.return_value = dvs + with patch('cvm.clients.SmartConnectNoSSL'): + with self.vcenter_client: + self.vcenter_client.restore_vlan_id(vcenter_port) + + dvs.ReconfigureDVPort_Task.assert_called_once() + spec = dvs.ReconfigureDVPort_Task.call_args[1].get('port', [None])[0] + self.assertIsNotNone(spec) + self.assertEqual('8', spec.key) + self.assertEqual('1', spec.configVersion) + self.assertTrue(spec.setting.vlan.inherited) + class TestFunctions(TestCase): def test_make_dv_port_spec(self): @@ -91,6 +110,16 @@ def test_make_dv_port_spec(self): self.assertEqual('8', spec.key) self.assertEqual('edit', spec.operation) self.assertEqual(10, spec.setting.vlan.vlanId) + self.assertFalse(spec.setting.vlan.inherited) + self.assertEqual('1', spec.configVersion) + + def test_make_port_spec_restore(self): + dv_port = Mock(key='8') + dv_port.config.configVersion = '1' + spec = make_dv_port_spec(dv_port) + self.assertEqual('8', spec.key) + self.assertEqual('edit', spec.operation) + self.assertTrue(spec.setting.vlan.inherited) self.assertEqual('1', spec.configVersion) diff --git a/tests/test_events.py b/tests/test_events.py index a8f35de..7c0ed2d 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -257,7 +257,9 @@ def test_vm_created(vcenter_api_client, vn_model_1, vm_created_update, vrouter_api_client.add_port.assert_called_once_with(vmi_model) # Check if VLAN ID has been set using VLAN Override - vcenter_api_client.set_vlan_id.assert_called_once_with(vmi_model.vn_model.dvs_name, '10', 2) + vcenter_port = vcenter_api_client.set_vlan_id.call_args[0][0] + assert vcenter_port.port_key == '10' + assert vcenter_port.vlan_id == 2 # Check inner VMI model state assert_vmi_model_state( @@ -329,7 +331,9 @@ def test_vm_renamed(vcenter_api_client, vn_model_1, vm_created_update, assert vrouter_api_client.add_port.call_count == 2 # Check if VLAN ID has been set using VLAN Override - vcenter_api_client.set_vlan_id.assert_called_once_with(vmi_model.vn_model.dvs_name, '10', 2) + vcenter_port = vcenter_api_client.set_vlan_id.call_args[0][0] + assert vcenter_port.port_key == '10' + assert vcenter_port.vlan_id == 2 # Check inner VMI model state assert_vmi_model_state( @@ -415,7 +419,9 @@ def test_vm_reconfigured(vcenter_api_client, vn_model_1, vn_model_2, vm_created_ # Check if VLAN ID has been set using VLAN Override assert vcenter_api_client.set_vlan_id.call_count == 2 - assert vcenter_api_client.set_vlan_id.call_args[0] == (vmi_model.vn_model.dvs_name, '11', 4) + vcenter_port = vcenter_api_client.set_vlan_id.call_args[0][0] + assert vcenter_port.port_key == '11' + assert vcenter_port.vlan_id == 4 # Check inner VMI model state assert_vmi_model_state( diff --git a/tests/test_models.py b/tests/test_models.py index 861a085..f1d11e1 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -162,7 +162,7 @@ def setUp(self): vnc_vn = Mock(uuid='d376b6b4-943d-4599-862f-d852fd6ba425') vnc_vn.name = 'VM Network' self.vn_model = VirtualNetworkModel(vmware_vn, vnc_vn, VlanIdPool(0, 100)) - self.vcenter_port = VCenterPort('c8:5b:76:53:0f:f5', None, '123') + self.vcenter_port = VCenterPort(device) def test_to_vnc(self): vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, self.vcenter_port) @@ -269,7 +269,9 @@ def test_vmi_vlan_id_aquisition(self): self.ports.append(create_port_mock(1)) vn_model = VirtualNetworkModel(self.dpg, None, VlanIdPool(0, 100)) vm_model = VirtualMachineModel(*create_vmware_vm_mock([self.dpg])) - vcenter_port = VCenterPort('', 'dvportgroup-20', '') + device = Mock() + device.backing.port.portgroupKey = 'dvportgroup-20' + vcenter_port = VCenterPort(device) vmi_model = VirtualMachineInterfaceModel(vm_model, vn_model, vcenter_port) vmi_model.acquire_vlan_id(None) diff --git a/tests/test_services.py b/tests/test_services.py index 506492e..757329f 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -207,15 +207,18 @@ def test_update_existing_vmi(self): self.database.save(self.vm_model) second_vn_model = create_vn_model(name='DPortGroup', key='dportgroup-51') self.database.save(second_vn_model) + device = Mock(macAddress='c8:5b:76:53:0f:f5') + device.backing.port.portgroupKey = 'dportgroup-50' vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, - VCenterPort('c8:5b:76:53:0f:f5', '', 'dportgroup-50')) + VCenterPort(device)) vmi_model.parent = vnc_api.Project() vmi_model.security_group = vnc_api.SecurityGroup() vnc_instance_ip = Mock() vnc_instance_ip.uuid = 'uuid' vmi_model.vnc_instance_ip = vnc_instance_ip self.database.save(vmi_model) - self.vm_model.ports[0] = VCenterPort('c8:5b:76:53:0f:f5', '', 'dportgroup-51') + device.backing.port.portgroupKey = 'dportgroup-51' + self.vm_model.ports[0] = VCenterPort(device) self.vmi_service.update_vmis_for_vm_model(self.vm_model) @@ -229,8 +232,9 @@ def test_update_existing_vmi(self): def test_removes_unused_vmis(self): """ VMIs are deleted when the VM is no longer connected to corresponding DPG. """ + device = Mock(macAddress='c8:5b:76:53:0f:f5') vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, - VCenterPort('c8:5b:76:53:0f:f5', '', '')) + VCenterPort(device)) vmi_model.vnc_instance_ip = Mock() self.database.save(vmi_model) @@ -276,7 +280,8 @@ def test_sync_deletes_unused_vmis(self): self.vnc_client.delete_vmi.assert_called_once() def test_remove_vmis_for_vm_model(self): - vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, VCenterPort('mac_addr', '', '')) + device = Mock(macAddress='mac_addr') + vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, VCenterPort(device)) vmi_model.vnc_instance_ip = Mock() self.database.save(vmi_model) self.database.save(self.vm_model) @@ -300,7 +305,8 @@ def test_remove_vmis_no_vm_model(self): self.vnc_client.delete_vmi.assert_not_called() def test_rename_vmis(self): - vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, VCenterPort('mac_addr', '', '')) + vmi_model = VirtualMachineInterfaceModel(self.vm_model, self.vn_model, + VCenterPort(Mock(macAddress='mac_addr'))) vmi_model.parent = vnc_api.Project() vmi_model.security_group = vnc_api.SecurityGroup() self.database.save(vmi_model) @@ -516,6 +522,24 @@ def test_vnc_vmi_false(self): self.assertFalse(result) + def test_no_annotations(self): + vnc_vm = vnc_api.VirtualMachine('VM', vnc_api.Project()) + self.vnc_api_client.read_vm.return_value = vnc_vm + + result = self.vm_service._can_delete_from_vnc(vnc_vm) + + self.assertFalse(result) + + def test_no_vrouter_uuid(self): + vnc_vm = vnc_api.VirtualMachine('VM', vnc_api.Project()) + vnc_vm.set_annotations(KeyValuePairs( + [KeyValuePair('key', 'value')])) + self.vnc_api_client.read_vm.return_value = vnc_vm + + result = self.vm_service._can_delete_from_vnc(vnc_vm) + + self.assertFalse(result) + def construct_vrouter_response(): return {'author': '/usr/bin/contrail-vrouter-agent',