ethmonitor: fix to be able to monitor interface without IP

[oalbrigt]

No description provided.

[dmuhamedagic]

This seems to change the semantics of the test ("ip -f inet" vs "ip -f link"). Isn't it better to test the "inet" family?

[oalbrigt]

That wont work when monitoring devices without an IP.

[dmuhamedagic]

Do we want to test the device without an IP at all? Could there be a NIC running (as in cluster resource) and in some way provide service?

[oalbrigt]

Yeah. For Infiniband with pkeys.

[dmuhamedagic]

OK. I'm still bothered by the semantic change though (link vs inet). It is conceivable that "ip link" can return success but "ip inet" not necessarily. Whether that is a real world scenario I cannot say.

[lge]

On Tue, Apr 25, 2017 at 12:50:01AM -0700, Dejan Muhamedagic wrote: OK. I'm still bothered by the semantic change though (link vs inet). It is conceivable that "ip link" can return success but "ip inet" not necessarily. Whether that is a real world scenario I cannot say.

"ip -o -f link addr show | cut -d ' ' -f2" certainly will return a superset of "ip -o -f inet addr show | ...", but that is exactly the whole point of the cange, iiuc. I'd even suggest to drop that "ip | grep | cut | sort | grep | sed " call completely. case $NIC in */*) ... not a proper nic name *:*) ... not a proper nic name ipsec*) ... not a proper nic name *) test -d "/sys/class/net/$NIC" || { .. does not exist ... .. but if not validate-all, ignore anyways, .. because it may be a temporarily non-existing .. bond ... ... BTW, what strange logic is that, check only on validate, which is never called by pacemaker anyways? And only check for "*:*" if it exists? and filter out ipsec*, but then ignore it? Just wondering ... Cheers, Lars

[bubble75]

It is conceivable that "ip link" can return success but "ip inet" not necessarily. Whether that is a real world scenario I cannot say.

Bridge members, bond slaves, vlans carrier...

[dmuhamedagic]

On Tue, Apr 25, 2017 at 01:20:13AM -0700, Lars Ellenberg wrote: On Tue, Apr 25, 2017 at 12:50:01AM -0700, Dejan Muhamedagic wrote: > OK. I'm still bothered by the semantic change though (link vs > inet). It is conceivable that "ip link" can return success > but "ip inet" not necessarily. Whether that is a real world > scenario I cannot say. "ip -o -f link addr show | cut -d ' ' -f2" certainly will return a superset of "ip -o -f inet addr show | ...", but that is exactly the whole point of the cange, iiuc.

Yes, obviously it is. I'm just weary of semantic changes.

BTW, what strange logic is that, check only on validate, which is never called by pacemaker anyways?

The version I have always invokes ip_validate. Or did you mean something else?

[lge]

On Tue, Apr 25, 2017 at 03:34:27AM -0700, Dejan Muhamedagic wrote: On Tue, Apr 25, 2017 at 01:20:13AM -0700, Lars Ellenberg wrote: > On Tue, Apr 25, 2017 at 12:50:01AM -0700, Dejan Muhamedagic wrote: > > OK. I'm still bothered by the semantic change though (link vs > > inet). It is conceivable that "ip link" can return success > > but "ip inet" not necessarily. Whether that is a real world > > scenario I cannot say. > > "ip -o -f link addr show | cut -d ' ' -f2" certainly will return a superset of > "ip -o -f inet addr show | ...", > but that is exactly the whole point of the cange, iiuc. Yes, obviously it is. I'm just weary of semantic changes. > BTW, what strange logic is that, check only on validate, > which is never called by pacemaker anyways? The version I have always invokes ip_validate. Or did you mean something else?

https://github.com/oalbrigt/resource-agents/blob/8ec7bd4cfcbdbff10c1c5717eae91d8c41037cda/heartbeat/ethmonitor#L236 The change is to is_interface(), which is used only in if_init(), if is_interface ; then *:*) --> exit OCF_ERR_GENERIC otherwise: ok. else (apparently not an "interface" we currently know about, or explicitly filtered out by grep -v ipsec) $__OCF_ACTION == validate-all --> exit ERR_CONFIGURED all other actions (start,stop,monitor): ah well, log a warning, but otherwise ignore. fi

[dmuhamedagic]

On Tue, Apr 25, 2017 at 05:35:41AM -0700, Lars Ellenberg wrote: On Tue, Apr 25, 2017 at 03:34:27AM -0700, Dejan Muhamedagic wrote: > On Tue, Apr 25, 2017 at 01:20:13AM -0700, Lars Ellenberg wrote: > > On Tue, Apr 25, 2017 at 12:50:01AM -0700, Dejan Muhamedagic wrote: > > > OK. I'm still bothered by the semantic change though (link vs > > > inet). It is conceivable that "ip link" can return success > > > but "ip inet" not necessarily. Whether that is a real world > > > scenario I cannot say. > > > > "ip -o -f link addr show | cut -d ' ' -f2" certainly will return a superset of > > "ip -o -f inet addr show | ...", > > but that is exactly the whole point of the cange, iiuc. > > Yes, obviously it is. I'm just weary of semantic changes. > > > BTW, what strange logic is that, check only on validate, > > which is never called by pacemaker anyways? > > The version I have always invokes ip_validate. Or did you mean > something else? https://github.com/oalbrigt/resource-agents/blob/8ec7bd4cfcbdbff10c1c5717eae91d8c41037cda/heartbeat/ethmonitor#L236 The change is to is_interface(), which is used only in if_init(), if is_interface ; then *:*) --> exit OCF_ERR_GENERIC otherwise: ok. else (apparently not an "interface" we currently know about, or explicitly filtered out by grep -v ipsec) $__OCF_ACTION == validate-all --> exit ERR_CONFIGURED all other actions (start,stop,monitor): ah well, log a warning, but otherwise ignore. fi

OK, see now what you mean, strange thing. Renders this part effectively useless.

[oalbrigt]

if_validate is run before all actions except meta-data and usage/help.