network.c: allow notification forwarding

[carlospeon]

ChangeLog: network.c allow notification forwarding.

Allow notification forwarding checking notification metadata to avoid loops.

[carlospeon]

Tagged as WIP: checks do not build

[octo]

High level feedback: the problem we need to solve here is when two instances of collectd are configured to send notifications to each other. Because we can't currently tell whether we've seen a notification before, collectd would potentially bounce a notification back and forth indefinitely.

To get around that, we need to "remember" notifications somehow. We could, for example, keep a copy of the notifications of the last 60 seconds and discard notifications that are older than that. Or we keep the last 100 notifications, which would make it possible to implement a "give me the last n notifications" API. Or a combination of the two.

[carlospeon]

Sorry, probably more context is needed for this PR.

The goal of this PR is to allow notifications forwarding, replicating the send behavior of "values": a notification will be sent when:

• Forwarding is "true"
• Forwarding is "false" but the notification is local, that is, do not have metadata "network:received" set to "true".

In complex network configurations with collectd forwarders, this PR makes "notifications" arrive destination as values do.

In the case of two instances of collectd configured to send notifications (and values) to each other, "forwarding" should be "false" and only local notifications (and values) would be send to the other instance.

Regarding build checks, it seems that is needed to include plugin.c in network_test.c (where all metadata functions for notifications are implemented) but this do not worked for me.

[eero-t]

All checks fail due to network test changes:

 In file included from src/network_test.c:25:
src/daemon/plugin.c: In function 'set_thread_name':
src/daemon/plugin.c:622:16: error: implicit declaration of function 'pthread_setname_np'; did you mean 'pthread_setcanceltype'? [-Werror=implicit-function-declaration]
  622 |   int status = pthread_setname_np(tid, n);
      |                ^~~~~~~~~~~~~~~~~~
      |                pthread_setcanceltype

[carlospeon]

Finally I manage to run checks. Removing WIP tag.

[octo]

Hi @carlospeon, thanks again for your work!

In the case of two instances of collectd configured to send notifications (and values) to each other, "forwarding" should be "false" and only local notifications (and values) would be send to the other instance.

I know several people who have configured their collectd servers this way, and I'm sure that's just the tip of the iceberg. These setups would be massively disrupted with the PR in its current form.

In order to forward notifications, we need a way to deduplicate them. E.g. add notifications to a list of notifications of the last 300 seconds (with some upper bound) and dismiss received notifications if they are in that list.

In the interest of avoiding miscommunication, let me be super direct: Duplicate detection is not optional. Without it, I won't merge this PR.

[carlospeon]

Hello @octo, thank you for your review!

I know several people who have configured their collectd servers this way, and I'm sure that's just the tip of the iceberg. These setups would be massively disrupted with the PR in its current form.

In my experience if you have a configuration with a loop, values are resent continuously and collectd crashs, so there should not be such setups working properly. I checked the code and although there is a de-duplication detection for values:

collectd/src/daemon/utils_cache.c

Line 350 in 4cc3426

if (ce->last_time >= vl->time) {

the return value of uc_update is not checked:

collectd/src/daemon/plugin.c

Line 2162 in 4cc3426

uc_update(ds, vl);

and duplicated values are dispatched to the write plugins. Or maybe I'm missing something.

We are running this code for a year without issues, our rate of notifications is something above a thousand per day and with a proper configuration we don't have any specific issue regarding loops.

In the interest of avoiding miscommunication, let me be super direct: Duplicate detection is not optional. Without it, I won't merge this PR.

I appreciate that, fair enough. This solution -maybe suboptimal- "works for me" and I just want to share it with the community. In this particular moment I'm not ready to code and test a more complex/complete solution.