Skip to content

Conversation

kienanstewart
Copy link

Virtual machines started using ganeti have the NICs defined in the command line by hotplug IDs. The mapping of interfaces -> virtual machine is stored in the the ganeti run-time directory. Searching for ganeti interface mappings is disabled by default.

Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to improve this plugin!

Please take a look at my comments below.


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.

* 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?

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")))

if os.getenv('ganeti'):
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.

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.

vm_name = name
break
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.

@sumpfralle
Copy link
Collaborator

Ping?

@github-actions
Copy link

Stale pull request message

@sumpfralle
Copy link
Collaborator

Ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants