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

Thu Oct 9 08:53:32 UTC 2014

I think we are almost there, just 1 comment along with Christy and Aline's.

On 2014年10月04日 03:45, Aline Manera wrote:
>
> 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 at redhat.com wrote:
>> From: Brent Baude <bbaude at redhat.com>
>>
>> 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)
Sorry for not replying last time, I mean:
+ "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."),

You identify 3 types of errors, we'd better check this 3 types of wrong 
address will be denied by kimchi.
Hope I'm not requiring too much:)
>> +
>> @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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>