On 03/12/2014 04:34 AM, Royce Lv wrote:

On 2014年03月12日 12:00, Aline Manera wrote:

...

From: Aline Manera <alinefm@br.ibm.com>

Add a new parameter to network resource to identify the network is in use by a template or VM, and do not allow user disable/delete it in this case. The 'default' network is always 'in use' as Kimchi uses it when creating a new template.

Also update API and test cases to reflect this change

Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- docs/API.md | 2 ++ src/kimchi/control/networks.py | 1 + src/kimchi/i18n.py | 4 ++-- src/kimchi/mockmodel.py | 24 ++++++++++++++++++++++++ src/kimchi/model/networks.py | 31 +++++++++++++++++++++++++++---- tests/test_model.py | 3 +-- tests/test_rest.py | 6 +++--- 7 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/docs/API.md b/docs/API.md index b3b6c49..3fca87a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -469,6 +469,8 @@ A interface represents available interface on host. * interface: The name of a network interface on the host. For bridge network, the interface can be a bridge or nic/bonding device. For isolated or NAT network, the interface is ignored. + * in_use: True if network is in use by a template or virtual machine; + False, otherwise.

### Resource: Network

diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index dd7e08d..95e8523 100644 --- a/src/kimchi/control/networks.py +++ b/src/kimchi/control/networks.py @@ -39,6 +39,7 @@ class Network(Resource): def data(self): return {'name': self.ident, 'vms': self.info['vms'], + 'in_use': self.info['in_use'], 'autostart': self.info['autostart'], 'connection': self.info['connection'], 'interface': self.info['interface'], diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..0af90a0 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -180,8 +180,8 @@ messages = { "KCHNET0014E": _("Network interface must be a string"), "KCHNET0015E": _("Network VLAN ID must be an integer between 1 and 4094"), "KCHNET0016E": _("Specify name and type to create a Network"), - "KCHNET0017E": _("Unable to delete network %(name)s. There are still some VMs linked to this network."), - "KCHNET0018E": _("Unable to deactivate network %(name)s. There are some VMs running linked to this network."), + "KCHNET0017E": _("Unable to delete network %(name)s. There are some virtual machines and/or templates linked to this network."), + "KCHNET0018E": _("Unable to deactivate network %(name)s. There are some virtual machines and/or templates linked to this network."),

"KCHDR0001E": _("Debug report %(name)s does not exist"), "KCHDR0002E": _("Debug report tool not found in system"), diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index b842b6c..b79aead 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -594,18 +594,42 @@ class MockModel(object): vms.append(name) return vms

+ def _network_used_by_template(self, network): + for name, tmpl in self._mock_templates.iteritems(): + if network in tmpl.info['networks']: + return True + return False + + def _is_network_in_use(self, name): + # The network "default" is used for Kimchi proposal and should not be + # deactivate or deleted. Otherwise, we will allow user create + # inconsistent templates from scratch + if name == 'default': + return True + + vms = self._get_vms_attach_to_a_network(name) + return bool(vms) or self._network_used_by_template(name) + def network_lookup(self, name): network = self._get_network(name) network.info['vms'] = self._get_vms_attach_to_a_network(name) + network.info['in_use'] = self._is_network_in_use(name) + return network.info

def network_activate(self, name): self._get_network(name).info['state'] = 'active'

def network_deactivate(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0018E", {'name': name}) + self._get_network(name).info['state'] = 'inactive'

def network_delete(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0017E", {'name': name}) + # firstly, we should check the network actually exists network = self._get_network(name) del self._mock_networks[network.name] diff --git a/src/kimchi/model/networks.py b/src/kimchi/model/networks.py index 0d6ccec..cfea6ba 100644 --- a/src/kimchi/model/networks.py +++ b/src/kimchi/model/networks.py @@ -153,6 +153,7 @@ class NetworksModel(object): class NetworkModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore']

def lookup(self, name): network = self.get_network(self.conn.get(), name) @@ -183,9 +184,29 @@ class NetworkModel(object): 'subnet': subnet, 'dhcp': dhcp, 'vms': self._get_vms_attach_to_a_network(name), + 'in_use': self._is_network_in_use(name), 'autostart': network.autostart() == 1, 'state': network.isActive() and "active" or "inactive"}

+ def _is_network_in_use(self, name): + # The network "default" is used for Kimchi proposal and should not be + # deactivate or deleted. Otherwise, we will allow user create + # inconsistent templates from scratch + if name == 'default': + return True + + vms = self._get_vms_attach_to_a_network(name) + return bool(vms) or self._network_used_by_template(name) + + def _network_used_by_template(self, network): + with self.objstore as session: + templates = session.get_list('template') + for tmpl in templates: + tmpl_net = session.get('template', tmpl)['networks'] + if network in tmpl_net: + return True + return False + def _get_vms_attach_to_a_network(self, network, filter="all"): DOM_STATE_MAP = {'nostate': 0, 'running': 1, 'blocked': 2, 'paused': 3, 'shutdown': 4, 'shutoff': 5, @@ -210,17 +231,19 @@ class NetworkModel(object): network.create()

def deactivate(self, name): - network = self.get_network(self.conn.get(), name) - if self._get_vms_attach_to_a_network(name, "running"): + if self._is_network_in_use(name): raise InvalidOperation("KCHNET0018E", {'name': name}) + + network = self.get_network(self.conn.get(), name) network.destroy()

def delete(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0017E", {'name': name}) + network = self.get_network(self.conn.get(), name) if network.isActive(): raise InvalidOperation("KCHNET0005E", {'name': name}) - if self._get_vms_attach_to_a_network(name): - raise InvalidOperation("KCHNET0017E", {'name': name})

self._remove_vlan_tagged_bridge(network) network.undefine() diff --git a/tests/test_model.py b/tests/test_model.py index a3d8eff..db0f5f5 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -486,12 +486,11 @@ class ModelTests(unittest.TestCase): inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

- inst.network_delete(net_name) + self.assertRaises(InvalidOperation, inst.network_delete, net_name) Though we need to check network delete fails because it is referred by template. We still need to clear env (network) after this test finished by disconnecting this template with network.

Good point. I forgot it. I will update on V2.

...

shutil.rmtree(path)

info = inst.template_lookup('test') self.assertEquals(info['invalid']['cdrom'], [iso]) - self.assertEquals(info['invalid']['networks'], [net_name])

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_clone(self): diff --git a/tests/test_rest.py b/tests/test_rest.py index 65d50e0..4a1b6eb 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1109,8 +1109,9 @@ class RestTests(unittest.TestCase):

shutil.rmtree(path) # Delete the network - resp = request(host, port, '/networks/test-network', '{}', 'DELETE') - self.assertEquals(204, resp.status) + resp = json.loads(request(host, port, '/networks/test-network', + '{}', 'DELETE').read()) + self.assertIn("KCHNET0017E", resp["reason"])

# Delete the storagepool resp = request(host, port, '/storagepools/test-storagepool', @@ -1120,7 +1121,6 @@ class RestTests(unittest.TestCase): # Verify the template res = json.loads(self.request('/templates/test').read()) self.assertEquals(res['invalid']['cdrom'], [iso]) - self.assertEquals(res['invalid']['networks'], ['test-network']) self.assertEquals(res['invalid']['storagepools'], ['test-storagepool'])

# Delete the template

On 03/12/2014 06:10 AM, Shu Ming wrote:

于 2014/3/12 12:00, Aline Manera 写道:

...

From: Aline Manera <alinefm@br.ibm.com>

Add a new parameter to network resource to identify the network is in use by a template or VM, and do not allow user disable/delete it in this case. The 'default' network is always 'in use' as Kimchi uses it when creating a new template.

Also update API and test cases to reflect this change

Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- docs/API.md | 2 ++ src/kimchi/control/networks.py | 1 + src/kimchi/i18n.py | 4 ++-- src/kimchi/mockmodel.py | 24 ++++++++++++++++++++++++ src/kimchi/model/networks.py | 31 +++++++++++++++++++++++++++---- tests/test_model.py | 3 +-- tests/test_rest.py | 6 +++--- 7 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/docs/API.md b/docs/API.md index b3b6c49..3fca87a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -469,6 +469,8 @@ A interface represents available interface on host. * interface: The name of a network interface on the host. For bridge network, the interface can be a bridge or nic/bonding device. For isolated or NAT network, the interface is ignored. + * in_use: True if network is in use by a template or virtual machine; + False, otherwise.

### Resource: Network

diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index dd7e08d..95e8523 100644 --- a/src/kimchi/control/networks.py +++ b/src/kimchi/control/networks.py @@ -39,6 +39,7 @@ class Network(Resource): def data(self): return {'name': self.ident, 'vms': self.info['vms'], + 'in_use': self.info['in_use'], 'autostart': self.info['autostart'], 'connection': self.info['connection'], 'interface': self.info['interface'], diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..0af90a0 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -180,8 +180,8 @@ messages = { "KCHNET0014E": _("Network interface must be a string"), "KCHNET0015E": _("Network VLAN ID must be an integer between 1 and 4094"), "KCHNET0016E": _("Specify name and type to create a Network"), - "KCHNET0017E": _("Unable to delete network %(name)s. There are still some VMs linked to this network."), - "KCHNET0018E": _("Unable to deactivate network %(name)s. There are some VMs running linked to this network."), + "KCHNET0017E": _("Unable to delete network %(name)s. There are some virtual machines and/or templates linked to this network."), + "KCHNET0018E": _("Unable to deactivate network %(name)s. There are some virtual machines and/or templates linked to this network."),

"KCHDR0001E": _("Debug report %(name)s does not exist"), "KCHDR0002E": _("Debug report tool not found in system"), diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index b842b6c..b79aead 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -594,18 +594,42 @@ class MockModel(object): vms.append(name) return vms

+ def _network_used_by_template(self, network): + for name, tmpl in self._mock_templates.iteritems(): + if network in tmpl.info['networks']: + return True + return False Nits. For a uniform name style as the other bool function, "_is_network_usedby_template()".

ACK.

...

+ + def _is_network_in_use(self, name): + # The network "default" is used for Kimchi proposal and should not be + # deactivate or deleted. Otherwise, we will allow user create + # inconsistent templates from scratch + if name == 'default': + return True + + vms = self._get_vms_attach_to_a_network(name) + return bool(vms) or self._network_used_by_template(name) + def network_lookup(self, name): network = self._get_network(name) network.info['vms'] = self._get_vms_attach_to_a_network(name) + network.info['in_use'] = self._is_network_in_use(name) + return network.info

def network_activate(self, name): self._get_network(name).info['state'] = 'active'

def network_deactivate(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0018E", {'name': name}) + self._get_network(name).info['state'] = 'inactive'

def network_delete(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0017E", {'name': name}) + # firstly, we should check the network actually exists network = self._get_network(name) del self._mock_networks[network.name] diff --git a/src/kimchi/model/networks.py b/src/kimchi/model/networks.py index 0d6ccec..cfea6ba 100644 --- a/src/kimchi/model/networks.py +++ b/src/kimchi/model/networks.py @@ -153,6 +153,7 @@ class NetworksModel(object): class NetworkModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore']

def lookup(self, name): network = self.get_network(self.conn.get(), name) @@ -183,9 +184,29 @@ class NetworkModel(object): 'subnet': subnet, 'dhcp': dhcp, 'vms': self._get_vms_attach_to_a_network(name), + 'in_use': self._is_network_in_use(name), 'autostart': network.autostart() == 1, 'state': network.isActive() and "active" or "inactive"}

+ def _is_network_in_use(self, name): + # The network "default" is used for Kimchi proposal and should not be + # deactivate or deleted. Otherwise, we will allow user create + # inconsistent templates from scratch + if name == 'default': + return True + + vms = self._get_vms_attach_to_a_network(name) + return bool(vms) or self._network_used_by_template(name) + + def _network_used_by_template(self, network): + with self.objstore as session: + templates = session.get_list('template') + for tmpl in templates: + tmpl_net = session.get('template', tmpl)['networks'] + if network in tmpl_net: + return True + return False + def _get_vms_attach_to_a_network(self, network, filter="all"): DOM_STATE_MAP = {'nostate': 0, 'running': 1, 'blocked': 2, 'paused': 3, 'shutdown': 4, 'shutoff': 5, @@ -210,17 +231,19 @@ class NetworkModel(object): network.create()

def deactivate(self, name): - network = self.get_network(self.conn.get(), name) - if self._get_vms_attach_to_a_network(name, "running"): + if self._is_network_in_use(name): raise InvalidOperation("KCHNET0018E", {'name': name}) + + network = self.get_network(self.conn.get(), name) network.destroy()

def delete(self, name): + if self._is_network_in_use(name): + raise InvalidOperation("KCHNET0017E", {'name': name}) + network = self.get_network(self.conn.get(), name) if network.isActive(): raise InvalidOperation("KCHNET0005E", {'name': name}) - if self._get_vms_attach_to_a_network(name): - raise InvalidOperation("KCHNET0017E", {'name': name})

self._remove_vlan_tagged_bridge(network) network.undefine() diff --git a/tests/test_model.py b/tests/test_model.py index a3d8eff..db0f5f5 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -486,12 +486,11 @@ class ModelTests(unittest.TestCase): inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')

- inst.network_delete(net_name) + self.assertRaises(InvalidOperation, inst.network_delete, net_name) shutil.rmtree(path)

info = inst.template_lookup('test') self.assertEquals(info['invalid']['cdrom'], [iso]) - self.assertEquals(info['invalid']['networks'], [net_name])

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_template_clone(self): diff --git a/tests/test_rest.py b/tests/test_rest.py index 65d50e0..4a1b6eb 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1109,8 +1109,9 @@ class RestTests(unittest.TestCase):

shutil.rmtree(path) # Delete the network - resp = request(host, port, '/networks/test-network', '{}', 'DELETE') - self.assertEquals(204, resp.status) + resp = json.loads(request(host, port, '/networks/test-network', + '{}', 'DELETE').read()) + self.assertIn("KCHNET0017E", resp["reason"])

# Delete the storagepool resp = request(host, port, '/storagepools/test-storagepool', @@ -1120,7 +1121,6 @@ class RestTests(unittest.TestCase): # Verify the template res = json.loads(self.request('/templates/test').read()) self.assertEquals(res['invalid']['cdrom'], [iso]) - self.assertEquals(res['invalid']['networks'], ['test-network']) self.assertEquals(res['invalid']['storagepools'], ['test-storagepool'])

# Delete the template