Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a method gen_vm_mac() to generate valid mac for QEMU/KVM virtual machines #78

Closed
wants to merge 1 commit into from

Conversation

sghai
Copy link
Contributor

@sghai sghai commented Sep 17, 2015

For discovery feature, I need a valid mac that I can use for VM provisioning. However the current gen_mac() doesn't generate a valid mac for VM's on QEMU/KVM. The mac address must start with sequence: 54:52:00 otherwise VM creation fails with error:

ERROR XML error: expected unicast mac address, found multicast '63:8e:b3:53:77:b1'

So I added a new method to generate vm mac. gen_vm_mac(). We can update existing one too if that's the best option ?

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

Yeah. I think we should add a flag to the existing method and avoid adding a new one. Also, can I get tests? 😊

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

How about

@@ -574,10 +574,12 @@ def gen_ipaddr(ip3=False, ipv6=False, prefix=()):
     return _make_unicode(ipaddr)


-def gen_mac(delimiter=":"):
+def gen_mac(delimiter=":", vm=False):
     """Generates a random MAC address.

     :param str delimeter: Valid MAC delimeter (e.g ':', '-').
+    :param bool vm: If the MAC address is for a VM (eg. QEMU/KVM),
+    then it should start with '54:52:00'.
     :returns: A random MAC address.
     :rtype: str

@@ -589,9 +591,15 @@ def gen_mac(delimiter=":"):
     chars = ['a', 'b', 'c', 'd', 'e', 'f',
              '0', '1', '2', '3', '4', '5', '6', '7', '8', '9']

+    # If we need a MAC for a VM, then only generate the last 3 values.
+    mac_range = 3 if vm else 6
+
     mac = delimiter.join(
         chars[random.randrange(0, len(chars), 1)]+chars[random.randrange(
-            0, len(chars), 1)] for x in range(6))
+            0, len(chars), 1)] for x in range(mac_range))
+
+    if vm:
+        mac = u'54:52:00:{0}'.format(mac)

     return _make_unicode(mac)

@sthirugn
Copy link

@omaciel your suggestion looks great, but is 54:52:00 a universal standard? if not, I dont think it is a good idea to hard code that value here.

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

I honestly don't know... I could change the method to perhaps take an optional start and end parameter which would allow one to pass a string containing what the final MAC should have... will await on @sghai

@Ichimonji10
Copy link
Contributor

We could do something like:

def gen_mac(delimiter=':', type=None):

Where type could have a value of None, or it could be a keyword such as "multicast" or "unicast". If type is None, then a completely random MAC address is generated. Otherwise, the specified type of MAC address is generated. We can flesh out the valid values for the type argument over time.

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

Reading this...

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

@sghai do you think that the issue here is that you need a unicast MAC address but we're generating values that include multicast as well? If so, would it be a matter of allowing the user to optionally select unicast or multicast when calling the method?

See this for an example.

@omaciel
Copy link
Owner

omaciel commented Sep 17, 2015

@sghai together with @elyezer the following PR has been submitted. I would like to ask you to review it and perhaps test creating a MAC with multicast=False for your specific test.

@sghai
Copy link
Contributor Author

sghai commented Sep 18, 2015

Closing this. Referenced PR looks good and already merged.

@sghai sghai closed this Sep 18, 2015
@sghai sghai deleted the genmac branch September 18, 2015 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants