Re: [Kimchi-devel] [PATCH] Adding ability to pass an optional custom MAC address when users add an interface to a cold vm.

Royce, I appreciate the response. I had paused on the duplicate MAC issue and decided to see if anyone cared during review. My first instinct was the same as yours, which was to error that out. I'll go ahead and work on that. As for your comment about moving the validMAC function, I did intentionally put it there as that kept all the MAC address generation and handling together. Was your comment a suggestion a requirement?

I also will look into your unicast/multicast comment. Seems appropriate. I am traveling this week so I will likely provide an update next week. Brent On Tue, 2014-09-16 at 11:51 +0800, Royce Lv wrote: > Thanks for the patch Brent! Some comments below: > > On 2014年09月15日 03:27, bbaude(a)redhat.com wrote: >> From: Brent Baude <bbaude(a)redhat.com&gt; >> >> Feature to add a custom mac address when creating network interfaces. >> >> --- >> docs/API.md | 1 + >> src/kimchi/API.json | 4 ++++ >> src/kimchi/i18n.py | 2 ++ >> src/kimchi/model/vmifaces.py | 25 +++++++++++++++++++++---- >> tests/test_model.py | 10 ++++++++++ >> ui/css/theme-default/guest-edit.css | 4 ++-- >> ui/js/src/kimchi.guest_edit_main.js | 18 ++++++++++++------ >> ui/pages/guest-edit.html.tmpl | 11 ++++++++--- >> 8 files changed, 60 insertions(+), 15 deletions(-) >> >> diff --git a/docs/API.md b/docs/API.md >> index c070fa7..02daa37 100644 >> --- a/docs/API.md >> +++ b/docs/API.md >> @@ -223,6 +223,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 1319531..a020e1e 100644 >> --- a/src/kimchi/API.json >> +++ b/src/kimchi/API.json >> @@ -334,6 +334,10 @@ >> "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" >> } >> } >> }, >> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py >> index 9e66c68..8d024f9 100644 >> --- a/src/kimchi/i18n.py >> +++ b/src/kimchi/i18n.py >> @@ -105,6 +105,8 @@ 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": _("The custom MAC address %(custommac)s is not properly formatted as 12 character hexidecimal value seperated by colons(:)"), >> + >> >> "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..2b3b31d 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,13 @@ class VMIfacesModel(object): >> random.randint(0x00, 0xff)] >> return ':'.join(map(lambda x: "%02x" % x, 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: >> + return False >> + else: >> + return True > It'll be better to put these helper function in network.py, besides, can > we put this in API.json, and let json validator to do this validation? >> + >> conn = self.conn.get() >> networks = conn.listNetworks() + conn.listDefinedNetworks() >> >> @@ -60,6 +68,14 @@ class VMIfacesModel(object): >> for iface in self.get_vmifaces(vm, self.conn)) > Below line needs to be removed. >> mac = randomMAC() > What about: > if 'custommac' not in params or len(params['custommac']) ==0: > mac = randomMAC() > else: > xxxx >> + if 'custommac' not in params: >> + mac = randomMAC() >> + else: >> + if len(params['custommac']) == 0: >> + mac = randomMAC() >> + else: >> + mac = params["custommac"] >> + >> while True: >> if mac not in macs: >> break > Here is a problem: if you assign a MAC conflict with current MAC, kimchi > will automatically change it to a generated one, I think it'll be better > to warn user that he assigned an occupied MAC, so that he can make change. >> @@ -73,10 +89,11 @@ class VMIfacesModel(object): >> attrib = {"type": params["type"]} >> >> xml = etree.tostring(E.interface(*children, **attrib)) >> - >> - dom.attachDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) >> - >> - return mac >> + if validMac(mac): >> + dom.attachDeviceFlags(xml, libvirt.VIR_DOMAIN_AFFECT_CURRENT) >> + return mac > When I assign a multicast address, libvirt will warn me with: > libvirtError: XML error: expected unicast mac address, found multicast > '23:12:12:12:12:12' > > Check randomMAC(): we guranteed it is a unicast address when generating, > we need to check if address passed is a valid unicast address. >> + else: >> + raise NotFoundError("KCHVMIF0009E", {'custommac': mac}) >> >> @staticmethod >> def get_vmifaces(vm, conn): >> diff --git a/tests/test_model.py b/tests/test_model.py >> index 5774c82..13b5269 100644 >> --- a/tests/test_model.py >> +++ b/tests/test_model.py >> @@ -219,6 +219,16 @@ 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) >> + iface = inst.vmiface_lookup('kimchi-ifaces', mac) >> + self.assertEquals("52:54:00:17:cd:e7", iface['mac']) >> + rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac) >> + >> @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 938dfd9..a7c822a 100644 >> --- a/ui/js/src/kimchi.guest_edit_main.js >> +++ b/ui/js/src/kimchi.guest_edit_main.js >> @@ -175,7 +175,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)); >> @@ -211,20 +214,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>