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

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>