Artyom Lukianov has posted comments on this change. Change subject: hooks:checkips: add checkips hook ......................................................................
Patch Set 11: (9 comments) 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? Done 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. Done 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 Done 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? Done 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 Done 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? I removed it because it used as a daemon and I saw we use the same approach for other vdsm services: mom - /usr/sbin/momd vdsm - /usr/share/vdsm/daemonAdapter ... 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? yes, but I can not use it, because checkips does not python module and also I do not want to include it to PYTHONPATH so when I install package: checkipsd - placed under /usr/libexec/vdsm/hooks and checips_utils.py - under /usr/libexec/vdsm/hooks/after_get_stats/ and I will recieve import error 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' ? it does not really interface, it is rhevm network name, I can change it to network, if you prefer 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, I checked a little behaviour of ping command: 1) if address pingable it sends number of packets equal to parameter under -c 2) if address does not pingable it sends packet each second until it reaches timeout ping -c 1 -w 15 10.35.4.204 PING 10.35.4.204 (10.35.4.204) 56(84) bytes of data. --- 10.35.4.204 ping statistics --- 15 packets transmitted, 0 received, 100% packet loss, time 14000ms also I see from documentation: If ping does not receive any reply packets at all it will exit with code 1. If a packet count and deadline are both specified, and fewer than count packets are received by the time the deadline has arrived, it will also exit with code 1. On other error it exits with code 2. Otherwise it exits with code 0. This makes it possible to use the exit code to see if a host is alive or not. so if network does not reliable and we will set -c parameter too high it will drop host to non-operational state also when we do not want. I believe it better to ask Dan, what we prefer. 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
