[Kimchi-devel] [PATCH] Adding ability to pass an optional custom MAC address when users add an interface to a cold vm.
Royce Lv
lvroyce at linux.vnet.ibm.com
Wed Sep 17 07:36:13 UTC 2014
On 2014年09月16日 22:18, Brent Baude wrote:
> 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 think if you are going to integrate generation and handling (include
the unicast check) in a single function, I'm OK with it.
>
> 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 at redhat.com wrote:
>>> From: Brent Baude <bbaude at redhat.com>
>>>
>>> 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>
>
More information about the Kimchi-devel
mailing list