Dan Kenigsberg has posted comments on this change. Change subject: hooks:checkips: add checkips hook ......................................................................
Patch Set 5: Code-Review-1 (8 comments) few more notes! https://gerrit.ovirt.org/#/c/54102/5/vdsm_hooks/checkips/README File vdsm_hooks/checkips/README: Line 24: orkCustomProperties='c this would not work, as the second command would override the first. please explain how are they added. Line 22: - Use the engine-config to append the proper custom property: Line 23: $ sudo engine-config -s UserDefinedNetworkCustomProperties='checkipv4=^[0-9\.a-fA-F]+$' Line 24: $ sudo engine-config -s UserDefinedNetworkCustomProperties='checkipv6=^[0-9\.a-fA-F\:]+$' Line 25: - Verify that the checkipv4 and checkipv6 custom properties was properly added: Line 26: $ sudo engine-config -g UserDefinedNetworkCustomProperties the hook should be installed on all hosts, and its accompanying service should be enabled and started systemctl vdsm-checkips enable systemctl vdsm-checkips start Line 27: Line 28: Usage: Line 29: In the host setup network configuration window, choose edit assigned Line 30: logical network, select custom property checkipv4 or checkipv6 and enter VLAN IP or FQDN Line 30: N IP or FQD short lines are cooler Line 31: 127.0.0.1,::1 now it is a confusing example. how about 8.8.8.8 for ipv4 and 2001:4860:4860::8888 for ipv6 if networks need global connectivity Line 28: Usage: Line 29: In the host setup network configuration window, choose edit assigned Line 30: logical network, select custom property checkipv4 or checkipv6 and enter VLAN IP or FQDN Line 31: that you want check for connectivity(example 127.0.0.1,::1). If IP does Line 32: not connective from host, it will drop host to non-operational state. the semantic is not clear here. do we need BOTH to stay alive, or having one of the two is good enough? (I prefer the latter) https://gerrit.ovirt.org/#/c/54102/5/vdsm_hooks/checkips/checkips-vdsm.service.in File vdsm_hooks/checkips/checkips-vdsm.service.in: Line 1: [Unit] Line 2: Description=Service that ping given ips each 60 seconds pardon, but I very much prefer vdsm-checkips name. we already have vdsm-network (though we do have supervdsm service that does not follow this naming style) Line 3: Line 4: [Service] Line 5: Type=simple Line 6: LimitCORE=infinity https://gerrit.ovirt.org/#/c/54102/5/vdsm_hooks/checkips/checkips_utils.py File vdsm_hooks/checkips/checkips_utils.py: Line 29: with open(file_path, 'a'): Line 30: os.utime(file_path, None) Line 31: Line 32: Line 33: def get_ping_attrs(net_attrs): "attrs" is a very generic term, when all we have is the ping address and address family. get_ping_addresses() that returns a list of (addr, familty) pairs would be explicit. Line 34: ping_attrs = {} Line 35: if 'custom' in net_attrs: Line 36: if CHECKIPV4 in net_attrs['custom']: Line 37: ping_attrs['command'] = 'ping' Line 32: Line 33: def get_ping_attrs(net_attrs): Line 34: ping_attrs = {} Line 35: if 'custom' in net_attrs: Line 36: if CHECKIPV4 in net_attrs['custom']: this means that a network can have only one of the two. that's kinda find, but must be expressed clearly in the README and commit message. I'd prefer being able to give both addresses per network; if one is available, we're still OK. Line 37: ping_attrs['command'] = 'ping' Line 38: ping_attrs['address'] = net_attrs['custom'][CHECKIPV4] Line 39: elif CHECKIPV6 in net_attrs['custom']: Line 40: ping_attrs['command'] = 'ping6' -- 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: 5 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
