[PATCH 0/5] View/Edit guest MAC address
This patchset implements a new feature in Kimchi, it allows users to view and/or edit a guest MAC address. I have a fork of kimchi in my github, so if you prefer to use their diff tool (syntax highlight) you can check this link out: https://github.com/kimchi-project/kimchi/compare/master...jrziviani:guest_ma... Jose Ricardo Ziviani (5): Display MAC Address in guest interface tab Fix URI format of guest interfaces Implement backend code to edit MAC address of a guest Implement frontend code to edit MAC address of a guest Update test cases to reflect MAC address update changes src/kimchi/API.json | 13 +++++++ src/kimchi/control/vm/ifaces.py | 2 +- src/kimchi/i18n.py | 4 +- src/kimchi/model/vmifaces.py | 60 +++++++++++++++++++++--------- tests/test_model.py | 17 +++++++-- tests/test_rest.py | 12 +++--- ui/css/theme-default/guest-edit.css | 2 +- ui/js/src/kimchi.guest_edit_main.js | 74 ++++++++++++++++++++++++++++++------- ui/pages/guest-edit.html.tmpl | 8 +++- 9 files changed, 149 insertions(+), 43 deletions(-) -- 1.9.1
- This commit adds a field in the guest interface tab to display the MAC address of a network card. Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- ui/css/theme-default/guest-edit.css | 2 +- ui/pages/guest-edit.html.tmpl | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ui/css/theme-default/guest-edit.css b/ui/css/theme-default/guest-edit.css index 6815ecf..d4b3166 100644 --- a/ui/css/theme-default/guest-edit.css +++ b/ui/css/theme-default/guest-edit.css @@ -115,7 +115,7 @@ #form-guest-edit-storage .cell, .guest-edit-interface .cell { display: inline-block; - width: 250px; + width: 200px; } .guest-edit-snapshot .cell { diff --git a/ui/pages/guest-edit.html.tmpl b/ui/pages/guest-edit.html.tmpl index 9db1e1c..7568fce 100644 --- a/ui/pages/guest-edit.html.tmpl +++ b/ui/pages/guest-edit.html.tmpl @@ -110,6 +110,7 @@ <div class="header"> <span class="cell">$_("Network")</span> <span class="cell">$_("Type")</span> + <span class="cell">$_("MAC Address")</span> <button class="add action-area"></button> </div> <div class="body"></div> @@ -232,6 +233,9 @@ <span class="cell"> <span>{type}</span> </span> + <span class="cell"> + <span>{mac}</span> + </span> <span class="action-area {editMode}"> <button class="save"></button><button class="cancel"></button> </span> -- 1.9.1
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/control/vm/ifaces.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/control/vm/ifaces.py b/src/kimchi/control/vm/ifaces.py index e31210e..8a60217 100644 --- a/src/kimchi/control/vm/ifaces.py +++ b/src/kimchi/control/vm/ifaces.py @@ -38,7 +38,7 @@ class VMIface(Resource): self.ident = ident self.info = {} self.model_args = [self.vm, self.ident] - self.uri_fmt = '/vms/%s/iface/%s' + self.uri_fmt = '/vms/%s/ifaces/%s' @property def data(self): -- 1.9.1
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/API.json | 13 ++++++++++ src/kimchi/i18n.py | 4 ++- src/kimchi/model/vmifaces.py | 60 +++++++++++++++++++++++++++++++------------- 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 474661c..71d7a72 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -382,12 +382,25 @@ "description": "the name of one available network", "minLength": 1, "type": "string", + "required": true, "error": "KCHVMIF0005E" }, + "type": { + "description": "The type of VM network interface that libvirt supports", + "type": "string", + "pattern": "^network$", + "required": true, + "error": "KCHVMIF0004E" + }, "model": { "description": "model of emulated network interface card", "enum": ["ne2k_pci", "i82551", "i82557b", "i82559er", "rtl8139", "e1000", "pcnet", "virtio", "spapr-vlan"], "error": "KCHVMIF0006E" + }, + "mac": { + "description": "Network Interface Card MAC address", + "type": "string", + "error": "KCHVMIF0005E" } }, "additionalProperties": false diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 18e84bc..f04f54e 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -126,7 +126,9 @@ messages = { "KCHVMIF0005E": _("Network name for virtual machine interface must be a string"), "KCHVMIF0006E": _("Invalid network model card specified for virtual machine interface"), "KCHVMIF0007E": _("Specify type and network to add a new virtual machine interface"), - "KCHVMIF0008E": _("Specify type and network to update a virtual machine interface"), + "KCHVMIF0008E": _("Specify MAC Address to update a virtual machine interface"), + "KCHVMIF0009E": _("MAC Address %(mac)s already exists in virtual machine %(name)s"), + "KCHVMIF0010E": _("Invalid MAC Address %(mac)s"), "KCHTMPL0001E": _("Template %(name)s already exists"), "KCHTMPL0003E": _("Network '%(network)s' specified for template %(template)s does not exist"), diff --git a/src/kimchi/model/vmifaces.py b/src/kimchi/model/vmifaces.py index 8bf6b3d..4c0a2a9 100644 --- a/src/kimchi/model/vmifaces.py +++ b/src/kimchi/model/vmifaces.py @@ -20,6 +20,7 @@ import random import libvirt +import re from lxml import etree, objectify from kimchi.exception import InvalidParameter, MissingParameter, NotFoundError @@ -27,6 +28,8 @@ from kimchi.model.config import CapabilitiesModel from kimchi.model.vms import DOM_STATE_MAP, VMModel from kimchi.xmlutils.interface import get_iface_xml +RE_MACADDR = '^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$' + class VMIfacesModel(object): def __init__(self, **kargs): @@ -57,10 +60,24 @@ class VMIfacesModel(object): macs = (iface.mac.get('address') for iface in self.get_vmifaces(vm, self.conn)) - while True: - params['mac'] = VMIfacesModel.random_mac() - if params['mac'] not in macs: - break + # user defined customized mac address + if 'mac' in params and params['mac']: + # make sure it is unique + if params['mac'] in macs: + raise InvalidParameter('KCHVMIF0009E', + {'name': vm, 'mac': params['mac']}) + + # invalid mac address + if not re.match(RE_MACADDR, params['mac']): + raise InvalidParameter('KCHVMIF0010E', + {'name': vm, 'mac': params['mac']}) + + # otherwise choose a random mac address + else: + while True: + params['mac'] = VMIfacesModel.random_mac() + if params['mac'] not in macs: + break dom = VMModel.get_vm(vm, self.conn) @@ -147,22 +164,31 @@ class VMIfaceModel(object): if iface is None: raise NotFoundError("KCHVMIF0001E", {'name': vm, 'iface': mac}) + # mac address is a required parameter + if 'mac' not in params: + raise MissingParameter('KCHVMIF0008E') + + # invalid mac address + if not re.match(RE_MACADDR, params['mac']): + raise InvalidParameter('KCHVMIF0010E', + {'name': vm, 'mac': params['mac']}) + + # new mac address must be unique + if self._get_vmiface(vm, params['mac']): + raise InvalidParameter('KCHVMIF0009E', + {'name': vm, 'mac': params['mac']}) + flags = 0 if dom.isPersistent(): flags |= libvirt.VIR_DOMAIN_AFFECT_CONFIG - if DOM_STATE_MAP[dom.info()[0]] != "shutoff": - flags |= libvirt.VIR_DOMAIN_AFFECT_LIVE - - if iface.attrib['type'] == 'network' and 'network' in params: - iface.source.attrib['network'] = params['network'] - xml = etree.tostring(iface) - - dom.updateDeviceFlags(xml, flags=flags) - if 'model' in params: - iface.model.attrib["type"] = params['model'] - xml = etree.tostring(iface) + # remove the current nic + xml = etree.tostring(iface) + dom.detachDeviceFlags(xml, flags=flags) - dom.updateDeviceFlags(xml, flags=flags) + # add the nic with the desired mac address + iface.mac.attrib['address'] = params['mac'] + xml = etree.tostring(iface) + dom.attachDeviceFlags(xml, flags=flags) - return mac + return [vm, params['mac']] -- 1.9.1
When I try to run the following command: UPDATE /vms/<vm>/ifaces/<iface> {"mac": "foo"} I get the error message: KCHVMIF0008E: Specify MAC Address to update a virtual machine interface Well, I guess I did. That error message is triggered by the REST definition in src/kimchi/API.json which states that "type" and "network" are required (*). As I'm not providing them, that error message is shown. However, the description doesn't match the scenario and it totally confuses the user. I just knew I had to enter "type" and "network" because I looked at API.json, but the error message didn't help at all. (*): This might be unrelated to this patch but I have to comment: why is "type": "network" required to update a network interface? why is "network": "<network>" required? I wish my UPDATE command above (which updates the MAC address only) would work, that is a valid request from a user's point of view. I don't want to change the network, I just want to change the MAC address. Also, I couldn't change the interface network as well. Why is the field "network" required? Why does it exist at all? I'm required to enter two useless parameters here :-( (**) You don't need to address my complaints above for this patchset, just make sure the [broken?] existing behavior still works. I'm more concerned here about your change to the error message which doesn't help the user now - and it was helpful before. On 11-05-2015 14:19, Jose Ricardo Ziviani wrote:
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- src/kimchi/API.json | 13 ++++++++++ src/kimchi/i18n.py | 4 ++- src/kimchi/model/vmifaces.py | 60 +++++++++++++++++++++++++++++++------------- 3 files changed, 59 insertions(+), 18 deletions(-)
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_edit_main.js | 74 ++++++++++++++++++++++++++++++------- ui/pages/guest-edit.html.tmpl | 6 ++- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/ui/js/src/kimchi.guest_edit_main.js b/ui/js/src/kimchi.guest_edit_main.js index 9c088aa..b1dca96 100644 --- a/ui/js/src/kimchi.guest_edit_main.js +++ b/ui/js/src/kimchi.guest_edit_main.js @@ -164,6 +164,7 @@ kimchi.guest_edit_main = function() { }).click(function(evt){ evt.preventDefault(); addItem({ + id: -1, mac: "", network: "", type: "network", @@ -171,12 +172,18 @@ kimchi.guest_edit_main = function() { editMode: "" }); }); - var toggleEdit = function(item, on){ - $("label", item).toggleClass("hide", on); - $("select", item).toggleClass("hide", !on); + var toggleEdit = function(item, on, itemId){ + $("#label-mac-" + itemId, item).toggleClass("hide", on); + $("#edit-mac-" + itemId, item).toggleClass("hide", !on); + $("#label-network-" + itemId, item).toggleClass("hide", false); + $("select", item).toggleClass("hide", true); $(".action-area", item).toggleClass("hide"); }; var addItem = function(data) { + if (data.id == -1) { + var item = $(this).parent().parent(); + data.id = $(".item", item.parent()).size(); + } var itemNode = $.parseHTML(kimchi.substitute($('#interface-tmpl').html(),data)); $(".body", "#form-guest-edit-interface").append(itemNode); $("select", itemNode).append(networkOptions); @@ -188,12 +195,12 @@ kimchi.guest_edit_main = function() { $("select", itemNode).val(data.network); } $(".edit", itemNode).button({ - disabled: true, + disabled: false, icons: { primary: "ui-icon-pencil" }, text: false }).click(function(evt){ evt.preventDefault(); - toggleEdit($(this).parent().parent(), true); + toggleEdit($(this).parent().parent(), true, data.id); }); $(".delete", itemNode).button({ icons: { primary: "ui-icon-trash" }, @@ -213,21 +220,29 @@ kimchi.guest_edit_main = function() { var item = $(this).parent().parent(); var interface = { network: $("select", item).val(), - type: "network" + type: "network", + mac: $(":text", item).val() }; - var postUpdate = function(){ - $("label", item).text(interface.network); - toggleEdit(item, false); + var postUpdate = function(mac){ + $("#label-network-" + data.id, item).text(interface.network); + $("#label-mac-" + data.id, item).text(mac); + toggleEdit(item, false, data.id); }; if(item.prop("id")==""){ kimchi.createGuestInterface(kimchi.selectedGuest, interface, function(data){ item.prop("id", data.mac); - postUpdate(); + postUpdate(data.mac); }); }else{ - kimchi.updateGuestInterface(kimchi.selectedGuest, item.prop("id"), interface, function(){ - postUpdate(); - }); + if (item.prop('id') == interface.mac) { + toggleEdit(item, false, data.id); + } else { + kimchi.updateGuestInterface(kimchi.selectedGuest, item.prop('id'), + interface, function(data){ + item.prop("id", data.mac); + postUpdate(data.mac); + }); + } } }); $(".cancel", itemNode).button({ @@ -236,7 +251,7 @@ kimchi.guest_edit_main = function() { }).click(function(evt){ evt.preventDefault(); var item = $(this).parent().parent(); - $("label", item).text()==="" ? item.remove() : toggleEdit(item, false); + $("label", item).text()==="" ? item.remove() : toggleEdit(item, false, data.id); }); }; var networkOptions = ""; @@ -249,10 +264,41 @@ kimchi.guest_edit_main = function() { for(var i=0;i<data.length;i++){ data[i].viewMode = ""; data[i].editMode = "hide"; + data[i].id = i; addItem(data[i]); } }); }); + $(".body").on("keyup", "input:text", function(event) { + var text = $(this).val().replace(/:/g, ''); + var newText = ''; + for (var i = 0; i < text.length; i++) { + if (i > 0 && i % 2 == 0) { + newText += ':'; + } + newText += text[i]; + } + $(this).val(newText); + }); + $(".body").on("keydown", "input:text", function(event) { + var keycode = event.keyCode || event.which; + if (keycode == 37 || // left + keycode == 39 || // right + keycode == 46 || // del + keycode == 8 || // backspace + keycode == 9 ) { // tab + return; + } + var character = String.fromCharCode(keycode); + if (character.match(/[^0-9a-fA-F]/)) { + event.preventDefault(); + return; + } + if ($(this).val().length == 17) { + event.preventDefault(); + return; + } + }); }; var setupPermission = function() { diff --git a/ui/pages/guest-edit.html.tmpl b/ui/pages/guest-edit.html.tmpl index 7568fce..4787735 100644 --- a/ui/pages/guest-edit.html.tmpl +++ b/ui/pages/guest-edit.html.tmpl @@ -227,14 +227,16 @@ <script id="interface-tmpl" type="text/html"> <div class="item" id="{mac}"> <span class="cell"> - <label class="{viewMode}">{network}</label> + <label id="label-network-{id}" class="{viewMode}">{network}</label> <select class="{editMode}"></select> </span> <span class="cell"> <span>{type}</span> </span> <span class="cell"> - <span>{mac}</span> + <label id="label-mac-{id}" class="{viewMode}">{mac}</label> + <input class="{editMode}" type="text" + id="edit-mac-{id}" name="{mac}" value="{mac}" /> </span> <span class="action-area {editMode}"> <button class="save"></button><button class="cancel"></button> -- 1.9.1
I don't like the JavaScript filter/autocomplete/validation/wizard feature here. In my opinion, it gets more in the user's way than helps them: - There's no indication that the characters ":" will be added automatically. - I can't type the character ":" (e.g. try typing "AA" and then ":"; the character ":" won't be displayed, but it should, because it is the expected character there; the user might keep pressing ":" a lot of times until they realize that the character will be appended when they start typing the rest of the MAC address). - I can't type most of the useful keyboard combinations I have available: Home, End, Ctrl+V, Ctrl+C, Shift+Ctrl+End, etc. This patch only allows a small set of keys to be pressed. There's no reason I shouldn't type them. - It's inconsistent with all the other form fields on Kimchi. A much better approach here would be to let the user type whatever they want and validate the input when the user clicks the floppy disk button. If something is wrong, a nice message like "you need to enter the MAC address in such and such format" can be displayed, just like Kimchi does in every form. On 11-05-2015 14:19, Jose Ricardo Ziviani wrote:
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_edit_main.js | 74 ++++++++++++++++++++++++++++++------- ui/pages/guest-edit.html.tmpl | 6 ++- 2 files changed, 64 insertions(+), 16 deletions(-)
Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> --- tests/test_model.py | 17 ++++++++++++++--- tests/test_rest.py | 12 +++++++----- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index 88c020e..cd1556c 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -368,12 +368,23 @@ class ModelTests(unittest.TestCase): self.assertEquals("test-network", iface['network']) # update vm interface + newMacAddr = '54:50:e3:44:8a:af' iface_args = {"network": "default", - "model": "e1000"} + "model": "e1000", + "mac": newMacAddr, + "type": "network"} inst.vmiface_update(vm_name, mac, iface_args) + iface = inst.vmiface_lookup(vm_name, newMacAddr) + self.assertEquals(newMacAddr, iface['mac']) + + # undo mac address change + iface_args = {"network": "default", + "model": "e1000", + "mac": mac, + "type": "network"} + inst.vmiface_update(vm_name, newMacAddr, iface_args) iface = inst.vmiface_lookup(vm_name, mac) - self.assertEquals("default", iface['network']) - self.assertEquals("e1000", iface["model"]) + self.assertEquals(mac, iface['mac']) @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_disk(self): diff --git a/tests/test_rest.py b/tests/test_rest.py index c2ebd88..7add3d4 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -725,13 +725,15 @@ class RestTests(unittest.TestCase): self.assertEquals('network', iface['type']) # update vm interface - req = json.dumps({"network": "default", "model": "virtio"}) + newMacAddr = '54:50:e3:44:8a:af' + req = json.dumps({"network": "default", "model": "virtio", + "type": "network", "mac": newMacAddr}) resp = self.request('/vms/test-vm/ifaces/%s' % iface['mac'], req, 'PUT') - self.assertEquals(200, resp.status) - update_iface = json.loads(resp.read()) - self.assertEquals(u'virtio', update_iface['model']) - self.assertEquals('default', update_iface['network']) + self.assertEquals(303, resp.status) + iface = json.loads(self.request('/vms/test-vm/ifaces/%s' % + newMacAddr).read()) + self.assertEquals(newMacAddr, iface['mac']) # detach network interface from vm resp = self.request('/vms/test-vm/ifaces/%s' % iface['mac'], -- 1.9.1
participants (2)
-
Crístian Deives -
Jose Ricardo Ziviani