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

Add option to search for interface names in ganeti run-time to kvm_net #946

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions plugins/libvirt/kvm_net
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ In all other cases you need to use other munin plugins instead, e.g. "libvirt".
parsed environment variables:

* vmsuffix: part of vm name to be removed

* ganeti: True/False (default False), use ganeti run-time information to resolve
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a more descriptive name for the specific purpose (maybe there will be more ganeti-related features in the future).
How about: use_ganeti_nic_name_map?

interface names by pid

=head1 AUTHOR

Expand Down Expand Up @@ -136,9 +137,28 @@ def get_vm_network_interface_names(pid):
match = KVM_INTERFACE_NAME_REGEX.search(netdev_description)
if match:
result.add(match.groups()[0])
if os.getenv('ganeti'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a non-surprising parsing approach. E.g. here you would interprete ganeti=0 as True.
How about the following instead?

bool(distutils.util.strtobool(os.getenv("ganeti", "False")))

vm_name = _get_ganeti_vm_name(pid)
if vm_name:
for root, dirs, files in os.walk('/var/run/ganeti/kvm-hypervisor/nic/{0}'.format(vm_name)):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this part of the code (walking through the ganeti directories) into the _get_ganeti_vm_name function. This keeps the ganeti-related code in a single place.

for name in files:
with open(os.path.join(root, name)) as f:
result.add(f.read())
return result


def _get_ganeti_vm_name(pid):
# grep -rl /var/run/ganeti/kvm-hypervisor/pid $PID
vm_name = ''
for root, dirs, files in os.walk('/var/run/ganeti/kvm-hypervisor/pid'):
for name in files:
with open(os.path.join(root, name)) as f:
read_pid = f.read()
if int(read_pid) == int(pid):
vm_name = name
break
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, a quite pythonic approach would be to return name here and add return "" after the loop.
The break that you used instead, is usually harder to comprehend and requires more context.

return vm_name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow PEP8 and add two empty lines here.
python3 -m flake8 FILENAME is a good test for compliance.

def detect_kvm():
""" Check if kvm is installed """
kvm = Popen(["which", "kvm"], stdout=PIPE)
Expand All @@ -148,7 +168,6 @@ def detect_kvm():

def find_vm_names(pids):
"""Find and clean vm names from pids

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8 and the related PEP257 advocate the use of an empty line after the first (summary) line in a docstring. Thus you should not remove it here without a good reason.

@return a dictionnary of {pids : cleaned vm name}
"""
result = {}
Expand Down