12:19 p.m.
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...
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
12:19 p.m.
New subject: [PATCH 1/5] Display MAC Address in guest interface tab
- 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(a)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
12:19 p.m.
New subject: [PATCH 2/5] Fix URI format of guest interfaces
Signed-off-by: Jose Ricardo Ziviani <joserz(a)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
12:19 p.m.
New subject: [PATCH 3/5] Implement backend code to edit MAC address of a guest
Signed-off-by: Jose Ricardo Ziviani <joserz(a)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
8:50 a.m.
New subject: [PATCH 3/5] Implement backend code to edit MAC address of a guest
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(a)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(-)
12:19 p.m.
New subject: [PATCH 4/5] Implement frontend code to edit MAC address of a guest
Signed-off-by: Jose Ricardo Ziviani <joserz(a)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
9:10 a.m.
New subject: [PATCH 4/5] Implement frontend code to edit MAC address of a guest
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(a)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(-)
12:19 p.m.
New subject: [PATCH 5/5] Update test cases to reflect MAC address update changes
Signed-off-by: Jose Ricardo Ziviani <joserz(a)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
3487
days inactive
3488
days old
7 comments
2 participants
participants (2)
-
Crístian Deives -
Jose Ricardo Ziviani