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

Christy Perez christy at linux.vnet.ibm.com
Thu Oct 2 21:28:06 UTC 2014


A few comments below.

On 10/02/2014 02: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": {

Can we just call "custommac" "mac?" I mean...if you're passing it in,
it's implied that it's custom, right? Take it or leave that suggestion.

> +                    "description": "optional custom mac address for the network interface",
mac -> MAC
> +                    "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"]
>          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."),
That's a little awkward sounding. Since the only thing that triggers is
it a multicast MAC address, can you just change it to, "Libvirt does not
accept multicast MAC addresses." or "%(value)s is a multicast MAC
address, and will not be accepted by libvirt." Even better, since Kimchi
users might not know/care what libvirt is, just, "Multicast MAC
addresses are not allowed."
> +
> 
>      "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):
Convention is underscores between words.
> +            macs = self.get_list(vm)
> +            if custommac in macs:
> +                raise InvalidParameter("KCHVMIF0010E", {'custommac': mac})
> +
> +        def validMac(mymac):
Same comment here.
> +            pattern = r'^(?i)([0-9A-F]{2}[:-]){5}([0-9A-F]{2})$'
> +            if bool(re.match(pattern, mac)) is not True:
Shouldn't this be mymac?
> +                raise InvalidParameter("KCHVMIF0009E", {'custommac': mac})
Same.
> +            else:
> +                # Checking for multicast
> +                if (int(mymac.split(":")[0], 16) & 0x1) == 1:
> +                    raise InvalidParameter("KCHVMIF0011E", {'custommac': mac})
Same.
> +
>          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()
> +            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;
Did you test this smaller size with different languages? Most of the
time extra space is necessary.
>  }
> 
>  #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>
> 




More information about the Kimchi-devel mailing list