node-neigh: Reduce arping related log msg's level

[brb]

It is not the end of the world if any arping related operation fails (e.g. frequent connections between nodes ensure the presence of relevant L2 entries in the neigh table). So, decrease the log level of the log msgs.

[aanm]

I'm fine reducing the log msg to level "Info" but if we are printing a "Info" message with the word "Fail" in it it might be confusing to users. If they are not that important maybe considering setting them as debug or add a new metric?

[brb]

If they are not that important maybe considering setting them as debug or add a new metric?

@aanm I still want to see what exactly failed, so keeping them as log msg in info lvl instead of adding a metric.

[aanm]

If they are not that important maybe considering setting them as debug or add a new metric?

@aanm I still want to see what exactly failed, so keeping them as log msg in info lvl instead of adding a metric.

@brb you, a developer, yes, but users will see this and find it confusing.

[brb]

@aanm Recently we had some big changes in the arp handling code. So I'd like to observe any possible discrepancies at least for a while. Later on, we can mute the messages, and expose errors via metrics.

[aanm]

@aanm Recently we had some big changes in the arp handling code. So I'd like to observe any possible discrepancies at least for a while. Later on, we can mute the messages, and expose errors via metrics.

Then let's prefix [DEBUG] in these messages

[brb]

Then let's prefix [DEBUG] in these messages

@aanm I've changed the logging subsystem to node-neigh-debug. PTAL.

[jrfastab]

LGTM, could change "Failed" -> "Unable" then those messages would read less severe, e.g. "Failed to remove neighbor entry" becomes "Unable to remove neighbor entry".

[jrfastab]

why would this happen and not be an error? Is it possible for the entry to be removed async to this operation so it does not exist or something?

[brb]

I was thinking about the case when a network operator removes an entry manually.

[jrfastab]

Beyond the scope of this patch, but I took look at the callers of insertNeighbor and shouldn't we return an error here and kick the retry logic? Otherwise we are waiting for the refresh logic to kick in from neighbor-table-refresh? I would expect a failed arp could be retried almost immediately and then backed off from there if it keeps failing.

[brb]

Beyond the scope of this patch, but I took look at the callers of insertNeighbor and shouldn't we return an error here and kick the retry logic

@jrfastab I've added a retry logic into both arping libraries we use. They will retry 3 times. If all fail, then it is up the periodic refresh to fix any outstanding issues.