Re: [Kimchi-devel] [PATCH] Addition of custom mac address field for interfaces v4

Thinking this as a previous work for netboot, I think we should move the mac customization to PUT method as let the POST as it is. Usually I want to have a few ifaces in my VM and updating them accordingly when needed. So the flow would be: curl -u admin:linux99 -H "Content-Type: application/json" -H "Accept: application/json" http://localhost:8010/vms/ubunu14.04/ifaces/52:54:00:90:d2:3f -X GET { "mac":"52:54:00:90:d2:3f", "type":"network", "model":"virtio", "network":"default" } and: curl -u admin:linux99 -H "Content-Type: application/json" -H "Accept: application/json" http://localhost:8010/vms/ubunu14.04/ifaces/52:54:00:90:d2:3f - X PUT -d' { "mac": <new-mac> }' To do that you just need to move code around to the update() function. I mean, that change will use all the code you have already done. On 10/02/2014 04:47 PM, bbaude(a)redhat.com wrote: > From: Brent Baude <bbaude(a)redhat.com&gt; > > Included review feedback > --- > docs/API.md | 1 + > src/kimchi/API.json | 6 ++++++ > src/kimchi/control/vms.py | 2 +- > src/kimchi/exception.py | 1 - > src/kimchi/i18n.py | 4 ++++ > src/kimchi/model/vmifaces.py | 38 +++++++++++++++++++++++++++++-------- > tests/test_model.py | 12 ++++++++++++ > ui/css/theme-default/guest-edit.css | 4 ++-- > ui/js/src/kimchi.guest_edit_main.js | 18 ++++++++++++------ > ui/pages/guest-edit.html.tmpl | 11 ++++++++--- > 10 files changed, 76 insertions(+), 21 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index b679ce7..4f92eaa 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -221,6 +221,7 @@ Represents all network interfaces attached to a > Virtual Machine. > interface type is network. > * type: The type of VM network interface that libvirt supports. > Now kimchi just supports 'network' type. > + * custommac: A custom MAC address for the interface > > ### Sub-Resource: Virtual Machine Network Interface > > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index b8604d2..6cea55d 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -334,6 +334,12 @@ > "type": "string", > "pattern": > "^ne2k_pci|i82551|i82557b|i82559er|rtl8139|e1000|pcnet|virtio$", > "error": "KCHVMIF0006E" > + }, > + "custommac": { > + "description": "optional custom mac address for the network > interface", > + "type": "string", > + "pattern": "(^(?i)([0-9A-F]{2}[:-]){5}([0-9A-F]{2})$)?", > + "error": "KCHVMIF0009E" > } > } > }, > diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py > index 88d8a81..627f1cc 100644 > --- a/src/kimchi/control/vms.py > +++ b/src/kimchi/control/vms.py > @@ -36,7 +36,7 @@ class VM(Resource): > super(VM, self).__init__(model, ident) > self.role_key = 'guests' > self.update_params = ["name", "users", "groups", "cpus", "memory", > - "graphics"] > + "graphics", "custommac"] As it is related to the /vms/<name>/ifaces you need to update the self.update_params in model/vm/ifaces.py > self.screenshot = VMScreenShot(model, ident) > self.uri_fmt = '/vms/%s' > for ident, node in sub_nodes.items(): > diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py > index 039152a..7ab1878 100644 > --- a/src/kimchi/exception.py > +++ b/src/kimchi/exception.py > @@ -46,7 +46,6 @@ class KimchiException(Exception): > msg = self._get_translation() > else: > msg = _messages.get(code, code) > - > msg = unicode(msg, 'utf-8') % args > pattern = "%s: %s" % (code, msg) > Exception.__init__(self, pattern) > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 9e66c68..4473a99 100644 > --- a/src/kimchi/i18n.py > +++ b/src/kimchi/i18n.py > @@ -105,6 +105,10 @@ messages = { > "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"), > + "KCHVMIF0009E": _("Invalid MAC address format. Expected a 12 > character hexadecimal value separated by colons(:)"), > + "KCHVMIF0010E": _("The custom MAC address %(custommac)s is already > in use on this guest"), > + "KCHVMIF0011E": _("Libvirt requires a unicast MAC address which > %(value)s is not."), > + > > "KCHTMPL0001E": _("Template %(name)s already exists"), > "KCHTMPL0002E": _("Template %(name)s does not exist"), > diff --git a/src/kimchi/model/vmifaces.py b/src/kimchi/model/vmifaces.py > index 66b3827..ef67d34 100644 > --- a/src/kimchi/model/vmifaces.py > +++ b/src/kimchi/model/vmifaces.py > @@ -18,6 +18,7 @@ > # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA > > import random > +import re > > import libvirt > from lxml import etree, objectify > @@ -45,6 +46,20 @@ class VMIfacesModel(object): > random.randint(0x00, 0xff)] > return ':'.join(map(lambda x: "%02x" % x, mac)) > > + def checkduplicatemacs(vm, custommac): > + macs = self.get_list(vm) > + if custommac in macs: > + raise InvalidParameter("KCHVMIF0010E", {'custommac': mac}) > + > + def validMac(mymac): > + pattern = r'^(?i)([0-9A-F]{2}[:-]){5}([0-9A-F]{2})$' > + if bool(re.match(pattern, mac)) is not True: > + raise InvalidParameter("KCHVMIF0009E", {'custommac': mac}) The pattern verification is done the jsonchema, ie, when the app gets this point the data was already validated so you can only use it > + else: > + # Checking for multicast > + if (int(mymac.split(":")[0], 16) & 0x1) == 1: > + raise InvalidParameter("KCHVMIF0011E", {'custommac': mac}) From the message statement I have: + "KCHVMIF0011E": _("Libvirt requires a unicast MAC address which %(value)s is not."), Which means you need to pass a value for "value" instead of "custommac" > + > conn = self.conn.get() > networks = conn.listNetworks() + conn.listDefinedNetworks() > > @@ -56,14 +71,22 @@ class VMIfacesModel(object): > if DOM_STATE_MAP[dom.info()[0]] != "shutoff": > raise InvalidOperation("KCHVMIF0003E") > > - macs = (iface.mac.get('address') > - for iface in self.get_vmifaces(vm, self.conn)) > - > - mac = randomMAC() > - while True: > - if mac not in macs: > - break > + if 'custommac' not in params: > + macs = (iface.mac.get('address') > + for iface in self.get_vmifaces(vm, self.conn)) > mac = randomMAC() > + while True: > + if mac not in macs: > + break > + mac = randomMAC() > + > + else: > + if len(params['custommac']) == 0: > + mac = randomMAC() If the user is updating a mac value, he/she needs to pass a valid input otherwise we should raise an error instead of updating the value automatically. > + else: > + mac = params["custommac"] > + validMac(mac) > + checkduplicatemacs(vm, mac) > > children = [E.mac(address=mac)] > ("network" in params.keys() and > @@ -75,7 +98,6 @@ class VMIfacesModel(object): > xml = etree.tostring(E.interface(*children, **attrib)) > > dom.attachDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) > - > return mac > > @staticmethod > diff --git a/tests/test_model.py b/tests/test_model.py > index ceedc6f..6d67398 100644 > --- a/tests/test_model.py > +++ b/tests/test_model.py > @@ -219,6 +219,18 @@ class ModelTests(unittest.TestCase): > self.assertEquals("default", iface['network']) > self.assertEquals("e1000", iface["model"]) > > + # custom MAC address test > + iface_args = {"type": "network", > + "network": "test-network", > + "model": "virtio", > + "custommac": "52:54:00:17:cd:e7"} > + mac = inst.vmifaces_create('kimchi-ifaces', iface_args) > + rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac) > + iface = inst.vmiface_lookup('kimchi-ifaces', mac) > + self.assertEquals("52:54:00:17:cd:e7", iface['mac']) > + self.assertRaises(InvalidParameter, inst.vmifaces_create, > + 'kimchi-ifaces', iface_args)

> + > @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') > def test_vm_disk(self): > disk_path = '/tmp/existent2.iso' > diff --git a/ui/css/theme-default/guest-edit.css > b/ui/css/theme-default/guest-edit.css > index 74c2237..f0e1bb0 100644 > --- a/ui/css/theme-default/guest-edit.css > +++ b/ui/css/theme-default/guest-edit.css > @@ -112,7 +112,7 @@ > #form-guest-edit-storage .cell, > .guest-edit-interface .cell { > display: inline-block; > - width: 250px; > + width: 150px; > } > > #form-guest-edit-storage .cell.dev { > @@ -130,7 +130,7 @@ > } > > .guest-edit-interface .body select { > - width: 180px; > + width: 150px; > padding: 0px; > } > > diff --git a/ui/js/src/kimchi.guest_edit_main.js > b/ui/js/src/kimchi.guest_edit_main.js > index c281289..7baf296 100644 > --- a/ui/js/src/kimchi.guest_edit_main.js > +++ b/ui/js/src/kimchi.guest_edit_main.js > @@ -176,7 +176,10 @@ kimchi.guest_edit_main = function() { > var toggleEdit = function(item, on){ > $("label", item).toggleClass("hide", on); > $("select", item).toggleClass("hide", !on); > - $(".action-area", item).toggleClass("hide"); > + $("input", item).toggleClass("hide", !on); > + $(".action-area#editmode", item).toggleClass("hide", !on); > + $(".action-area#viewmode", item).toggleClass("hide", on); > + > }; > var addItem = function(data) { > var itemNode = > $.parseHTML(kimchi.substitute($('#interface-tmpl').html(),data)); > @@ -215,20 +218,23 @@ kimchi.guest_edit_main = function() { > var item = $(this).parent().parent(); > var interface = { > network: $("select", item).val(), > - type: "network" > + type: "network", > + custommac: $("input",item).val().toLowerCase() > }; > - var postUpdate = function(){ > + var postUpdate = function(mymac){ > $("label", item).text(interface.network); > - toggleEdit(item, false); > + $("label#custommac", item).text(mymac); > + toggleEdit(item.prop, false); > }; > if(item.prop("id")==""){ > kimchi.createGuestInterface(kimchi.selectedGuest, interface, > function(data){ > item.prop("id", data.mac); > - postUpdate(); > + $("label#custommac", item).text(data.mac) > + postUpdate(data.mac); > }); > }else{ > kimchi.updateGuestInterface(kimchi.selectedGuest, item.prop("id"), > interface, function(){ > - postUpdate(); > + postUpdate(data.mac); > }); > } > }); > diff --git a/ui/pages/guest-edit.html.tmpl > b/ui/pages/guest-edit.html.tmpl > index 0b7dad3..0aa9f3b 100644 > --- a/ui/pages/guest-edit.html.tmpl > +++ b/ui/pages/guest-edit.html.tmpl > @@ -108,7 +108,8 @@ > <div class="header"> > <span class="cell">$_("Network")</span> > <span class="cell">$_("Type")</span> > - <button class="add action-area"></button> > + <span class="cell">$_("MAC")</span> > + <button id="interfaceadd" class="add action-area"></button> > </div> > <div class="body"></div> > </form> > @@ -192,10 +193,14 @@ > <span class="cell"> > <span>{type}</span> > </span> > - <span class="action-area {editMode}"> > + <span class="cell"> > + <label class="{viewMode}" id="custommac">{mac}</label> > + <input type="text" id="insertmac" class="{editMode}"/> > + </span> > + <span class="action-area {editMode}" id="editmode"> > <button class="save"></button><button class="cancel"></button> > </span> > - <span class="action-area {viewMode}"> > + <span class="action-area {viewMode}" id="viewmode"> > <button class="edit"></button><button class="delete"></button> > </span> > <div> From UI perspective, we can display the MAC on list view and enable the "Edit" icon. When selecting "Edit" we turn the label for MAC writable, when "save" we get the value and do the request. Does that make sense? _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel