Dan Kenigsberg has posted comments on this change. Change subject: hooks:checkips: add checkips hook ......................................................................
Patch Set 4: Code-Review-1 (14 comments) thanks a lot; looking forward for your fixes! https://gerrit.ovirt.org/#/c/54102/4/vdsm_hooks/checkips/README File vdsm_hooks/checkips/README: Line 2: ===================== Line 3: This hook check connectivity from host to given addresses Line 4: Line 5: This hook is useful in cases where you need to check connectivity from host to Line 6: specific vlan, before you start vm on it. this is not specific to vm networks; it applies just as well to a migration or display network. Please make it explicit that the custom property is to be set on the network. Line 7: Line 8: Line 9: Syntax: Line 10: checkips=ipv4,ipv6 Line 6: specific vlan, before you start vm on it. Line 7: Line 8: Line 9: Syntax: Line 10: checkips=ipv4,ipv6 it would be cooler to allow fqdn of ping target, not only ip address. this would become possible if we have two custom properties (checkipv4 and checkipv6) handled by the hook, so it does not need to check the family of the address. Line 11: Line 12: Where: Line 13: ipv4 is ip address in ipv4 format Line 14: ipv6 is ip address in ipv6 format https://gerrit.ovirt.org/#/c/54102/4/vdsm_hooks/checkips/after_get_stats.py File vdsm_hooks/checkips/after_get_stats.py: Line 19: # Refer to the README and COPYING files for full details of the license Line 20: # Line 21: import os Line 22: import sys Line 23: import time please split public from project-local import. sort each group alphabetically. Line 24: import hooking Line 25: import traceback Line 26: import checkips_utils Line 27: from vdsm import constants Line 30: CHECK_IPS is this constant needed? Line 46: TEST_DIR should be local to the test() function. use mkdtemp() to avoid name clashes. Line 50: file_dir the name is too vague. how about stats_dir? Line 105: 'check_ipv4': 'up', Line 106: 'check_ipv6': 'up', Line 107: 'test_network': 'down' Line 108: } Line 109: for interface, state in expected_states.iteritems(): please remember python 3 Line 110: test_msg = 'pass' Line 111: if network_stats['network'][interface]['state'] != state: Line 112: test_msg = 'fail' Line 113: print( https://gerrit.ovirt.org/#/c/54102/4/vdsm_hooks/checkips/checkips File vdsm_hooks/checkips/checkips: Line 22: import six Line 23: import time Line 24: import syslog Line 25: import socket Line 26: import hooking same comment about import order. Line 27: import threading Line 28: import checkips_utils Line 29: from vdsm import constants Line 30: import vdsm.netconfpersistence as persist_net Line 57: params we usually call this net_attrs Line 69: Line 70: Line 71: def main(): Line 72: try: Line 73: while True: it's more precise to take the time before and after _ping_address, and sleep only (PING_SAMPLE_TIME - elapsed_time). this way a single stalled pinger would not delay your next interval. Line 74: _ping_addresses() Line 75: time.sleep(PING_SAMPLE_TIME) Line 76: except Exception as e: Line 77: syslog.syslog("Service checkips failed to start: %s" % e) https://gerrit.ovirt.org/#/c/54102/4/vdsm_hooks/checkips/checkips.service.in File vdsm_hooks/checkips/checkips.service.in: Line 1: [Unit] Line 2: Description=Service that ping given ips each 60 seconds naming issue: make sure that this service has the vdsm- prefix in its name. Line 3: Line 4: [Service] Line 5: Type=simple Line 6: LimitCORE=infinity https://gerrit.ovirt.org/#/c/54102/4/vdsm_hooks/checkips/checkips_utils.py File vdsm_hooks/checkips/checkips_utils.py: Line 25: Line 26: def touch(net, file_dir): Line 27: file_path = os.path.join(file_dir, net) Line 28: with open(file_path, 'a'): Line 29: os.utime(file_path, None) I stand corrected about open() not updating mtime. Line 30: Line 31: Line 32: def get_address(params): Line 33: if 'custom' in params and CHECK_IPS in params['custom']: Line 32: params net_attrs Line 30: Line 31: Line 32: def get_address(params): Line 33: if 'custom' in params and CHECK_IPS in params['custom']: Line 34: return params['custom'][CHECK_IPS] be explicit about this returning None if no address is found. -- To view, visit https://gerrit.ovirt.org/54102 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53cec37310f0f1844d6fe244419fd8c10e9b7ebb Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Artyom Lukianov <aluki...@redhat.com> Gerrit-Reviewer: Artyom Lukianov <aluki...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
