Yaniv Kaul has posted comments on this change. Change subject: hooks:checkips: add checkips hook ......................................................................
Patch Set 11: (9 comments) Some minor comments added. https://gerrit.ovirt.org/#/c/54102/11/configure.ac File configure.ac: Line 400: vdsm/virt/Makefile Line 401: vdsm/virt/vmdevices/Makefile Line 402: vdsm_hooks/Makefile Line 403: vdsm_hooks/allocate_net/Makefile Line 404: vdsm_hooks/checkips/Makefile checkips should come after checkimages. I believe we sort alphabetically? Line 405: vdsm_hooks/checkimages/Makefile Line 406: vdsm_hooks/diskunmap/Makefile Line 407: vdsm_hooks/ethtool_options/Makefile Line 408: vdsm_hooks/extnet/Makefile https://gerrit.ovirt.org/#/c/54102/11/vdsm_hooks/Makefile.am File vdsm_hooks/Makefile.am: Line 34: # Additional hooks Line 35: if HOOKS Line 36: SUBDIRS += \ Line 37: allocate_net \ Line 38: checkips \ same as previously - should come after checkimages. Line 39: checkimages \ Line 40: diskunmap \ Line 41: extnet \ Line 42: extra_ipv4_addrs \ https://gerrit.ovirt.org/#/c/54102/11/vdsm_hooks/checkips/README File vdsm_hooks/checkips/README: Line 34: In the host setup network configuration window, choose edit assigned Line 35: logical network, select custom property checkipv4 or checkipv6 and Line 36: enter VLAN IP or FQDN that you want to check for Line 37: connectivity(examples 8.8.8.8,2001:4860:4860::8888). So if you defined Line 38: checipv4 and checipv6 custom properties and if the host fails to ping typo: checipv4 -> checkipv4 Line 39: both IP's, it will drop the network's state to down. Line 40: If the network is defined as "required", https://gerrit.ovirt.org/#/c/54102/11/vdsm_hooks/checkips/after_get_stats.py File vdsm_hooks/checkips/after_get_stats.py: Line 47: Line 48: CONNECTIVITY_TIMEOUT = 60 Line 49: Line 50: Line 51: def _is_network_accessible(net, stats_dir): the name of the function is not related to what it does? Line 52: file_path = os.path.join(stats_dir, net) Line 53: if os.path.exists(file_path): Line 54: return ( Line 55: time.time() - os.stat(file_path).st_mtime <= Line 124: print( Line 125: 'test %s: interface %s has state %s' % Line 126: (test_msg, interface, state) Line 127: ) Line 128: os.unlink(os.path.join(temp_dir, 'check_ipv4')) Perhaps worth putting all this in a finally (and the above in a try, so any exception would still remove the files and the directory? Line 129: os.unlink(os.path.join(temp_dir, 'check_ipv6')) Line 130: os.rmdir(temp_dir) Line 131: Line 132: https://gerrit.ovirt.org/#/c/54102/11/vdsm_hooks/checkips/checkipsd File vdsm_hooks/checkips/checkipsd: Line 1: #!/usr/bin/python Shouldn't this file have a .py extension? Line 2: # Line 3: # Copyright 2008-2016 Red Hat, Inc. Line 4: # Line 5: # This program is free software; you can redistribute it and/or modify Line 35: CHECKIPV6 = 'checkipv6' Line 36: VDSM_CHECKIPS = 'vdsm-checkips' Line 37: Line 38: Line 39: def get_ping_addresses(net_attrs): Isn't this function available in the utils? Line 40: ping_addresses = [] Line 41: if 'custom' in net_attrs: Line 42: for address_type in (CHECKIPV4, CHECKIPV6): Line 43: if address_type in net_attrs['custom']: Line 54: with open(file_path, 'a'): Line 55: os.utime(file_path, None) Line 56: Line 57: Line 58: def _ping_address(address, address_type, net): rename 'net' to 'interface' ? Line 59: ping_cmd = 'ping' if address_type == CHECKIPV4 else 'ping6' Line 60: command = [ Line 61: ping_cmd, Line 62: '-c', '1', Line 58: def _ping_address(address, address_type, net): Line 59: ping_cmd = 'ping' if address_type == CHECKIPV4 else 'ping6' Line 60: command = [ Line 61: ping_cmd, Line 62: '-c', '1', There were several comments that a single ping is not enough. For example, in hosted-engine setup - there was a request to try with more pings. I'd either have it as a variable, or by default use more than a single ping. Line 63: '-w', str(PING_TIMEOUT), Line 64: '-I', net, Line 65: address Line 66: ] -- 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: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Artyom Lukianov <[email protected]> Gerrit-Reviewer: Artyom Lukianov <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
