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

[LIBCLOUD-820] added check if libvirt uri is local #788

Closed

Conversation

@Katana-Steel
Copy link
Contributor

@Katana-Steel Katana-Steel commented May 16, 2016

LIBCLOUD-820 added check if libvirt uri is local

Description

without this listing nodes on a remote libvirtd server will fail to
lookup the IPs in the arp cache.

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
without this listing nodes on a remote KVM server will fail to
lookup the IPs in the arp cache
@Katana-Steel
Copy link
Contributor Author

@Katana-Steel Katana-Steel commented May 19, 2016

a thing to note running arp on the systems I have access to (CentOS 6+7) requires root access.
maybe iproute2 should be a consideration to get IP from MAC address?

@Kami
Copy link
Member

@Kami Kami commented May 25, 2016

@Katana-Steel Thanks!

The change looks good to me.

As far as root requirement goes - I would prefer to stick to a tool which is available by default on most Linux distributions. Having said that, I would be fine with trying to use iproute2 first and falling back to arp if iproute2 is not available.

In case arp isn't accessable by the user (as is the case for me
on debian testing) the use ip neighbor
arp_table = {}
try:
cmd = ['arp', '-an']
child = subprocess.Popen(cmd, stdout=subprocess.PIPE,

This comment has been minimized.

@Kami

Kami May 28, 2016
Member

Minor improvement which would also make it easier to test separately would be to refactor this into two methods (e.g. _get_arp_table_using_arp_cmd and _get_arp_table_using_ip_cmd or something along those lines).

Anyways, not a blocker.

This comment has been minimized.

@Katana-Steel

Katana-Steel May 29, 2016
Author Contributor

Hi Tomaz,

I had a version calling a different function for parsing the arp table of
'ip neighbor' but it looked like code duplication to me. So I rolled it
into the existing one.

I'd be happy to refactor it out into 2 functions. And update the tests when
they get in.

thanks
Rene
On May 28, 2016 10:25 AM, "Tomaz Muraus" notifications@github.com wrote:

In libcloud/compute/drivers/libvirt_driver.py
#788 (comment):

     mac_addresses = self._get_mac_addresses_for_domain(domain=domain)
  •    cmd = ['arp', '-an']
    
  •    child = subprocess.Popen(cmd, stdout=subprocess.PIPE,
    
  •                             stderr=subprocess.PIPE)
    
  •    stdout, _ = child.communicate()
    
  •    arp_table = self._parse_arp_table(arp_output=stdout)
    
  •    arp_table = {}
    
  •    try:
    
  •        cmd = ['arp', '-an']
    
  •        child = subprocess.Popen(cmd, stdout=subprocess.PIPE,
    

Minor improvement which would also make it easier to test separately would
be to refactor this into two methods (e.g. _get_arp_table_using_arp_cmd
and _get_arp_table_using_ip_cmd or something along those lines).

Anyways, not a blocker.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apache/libcloud/pull/788/files/786a95ddbac28e832794957b84008e7275fb99d8#r64991609,
or mute the thread
https://github.com/notifications/unsubscribe/ADECW3-30sCa2IwJ9o13kJ93tx6FcGSLks5qGHqJgaJpZM4Ifqrg
.

@asfgit asfgit closed this in 9975bca May 28, 2016
asfgit pushed a commit that referenced this pull request May 28, 2016
In case arp isn't accessable by the user (as is the case for me
on debian testing) the use ip neighbor

Closes #788

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
asfgit pushed a commit that referenced this pull request May 28, 2016
Closes #788

Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
@Kami
Copy link
Member

@Kami Kami commented May 28, 2016

Thanks, I went ahead and merged changes into trunk.

I will add some tests in a separate commit (I was the one who originally added the driver without any tests, yeah, bad me!) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.