Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TRIGGER] Fix ZeroTier Online/Offline Trigger #2804

Closed
zalmanlew opened this issue May 6, 2022 · 12 comments · Fixed by #2843
Closed

[TRIGGER] Fix ZeroTier Online/Offline Trigger #2804

zalmanlew opened this issue May 6, 2022 · 12 comments · Fixed by #2843
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed trigger / source New trigger / source request

Comments

@zalmanlew
Copy link
Contributor

The current ZeroTier trigger for node online/offline is using the unique dedupe parameter.

The issue with this is that it checks for unique values across the entire object. If another value like clock or ipAssignments change it will trigger the event again.

For these two triggers, it needs to be updated to only check if the online (boolean) state has changed - and ignore all the other keys.

@zalmanlew zalmanlew added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed trigger / source New trigger / source request labels May 6, 2022
@dannyroosevelt
Copy link
Collaborator

@zalmanlew thanks for submitting! The developer who did the original dev work on the other ZeroTier components is out for a bit, but we'll get this added to the queue.

@ctrlaltdylan
Copy link
Contributor

Just to add more context on the bug.

The problem is that a dedupe strategy is being used for this source, but the built in dedupe strategies aren't quite suited for this type of problem.

I recommend using the $.service.db instead to keep track of each ZeroTier node and it's current status instead.

For example:

const currentNodeStatus = this.service.db.get(node.id);

if(node.status != currentNodeStatus) {
   // emit event
   // update db as well:
   this.service.db.set(currentNodeStatus);
}

Please note, this is pseudocode, but the idea is that we emit events only on node state changes.

@luancazarine luancazarine self-assigned this May 10, 2022
@luancazarine
Copy link
Collaborator

I did several tests and I couldn't replicate this behavior.
the $emit id is based on nodeId and lastOnline(timestamp) so it is not duplicating

@zalmanlew
Copy link
Contributor Author

zalmanlew commented May 10, 2022

@luancazarine the lastOnline timestamp can (and will) be constantly updating even though the online state hasn't changed from its previous state.

This means after some period of time it will emit a new event just from the lastOnline being updated, even if it didn't change from online: true to online:false (or vice-versa).

For the purpose of the online/offline trigger, it should only monitor the state of the online boolean for changes.

@luancazarine
Copy link
Collaborator

luancazarine commented May 10, 2022

@zalmanlew
Do you know how long it takes to happen?
I have my node offline for more than 30 minutes and lastOnline remains the same

@zalmanlew
Copy link
Contributor Author

@luancazarine that's because you have your node offline, I guess his is primarily an issue in reverse (as long as your node is online, the lastOnline will constantly be updated with the ping time).

If it's going from offline > online then lastOnline wouldn't update, however other fields can be updated (manually) and this shouldn't fire the online event. Only the change of the online state should emit that event.

@luancazarine
Copy link
Collaborator

@zalmanlew
ok, i understood what is happening.
whenever you update a node's information it is "restarted", so as soon as it comes online again, lastOnline updates.

should the trigger ignore this?
because if the trigger catches at the exact moment the node is "restarted" by some update, you will receive this event too

@zalmanlew
Copy link
Contributor Author

@luancazarine I don't think this should trigger the node online/offline trigger. The only thing that should trigger the event is the state changing of the online key specifically (the other keys are irrelevant for these triggers).

Example 1 - Online

  • Previous state: online: false
  • Current state: online: true
  • Result: this should trigger the node-online event

Example 2 - Offline

  • Previous state: online: true
  • Current state: online: false
  • Result: this should trigger the node-offline event

Example 3 - Ignore

  • Previous state: online: true
  • Current state: online: true
  • Result: this should not trigger any event as the online status hasn't changed.

The easiest way to test this is to leave a node online and running, it will currently keep firing online events, whereas it should only be emitting the online event if the previous state was offline (same goes for offline in vice-versa).

@michelle0927
Copy link
Collaborator

@luancazarine Can you comment with a link to the PR? Thanks!

@luancazarine
Copy link
Collaborator

@luancazarine Can you comment with a link to the PR? Thanks!

Sorry, I moved the wrong task

@luancazarine
Copy link
Collaborator

@zalmanlew
There is a problem with only validating by status and saving to compare. if the trigger gets offline, and the node switches to online and then to offline again before the next trigger event, it will receive offline again and you will not receive the event

@zalmanlew
Copy link
Contributor Author

@luancazarine while that is true, I think that's why it depends how frequently you poll the data.

It's worth noting that ZeroTier considers the machine online if lastOnline is in the last 3 minutes.
We can include this information in the description of the source.

@luancazarine luancazarine linked a pull request May 11, 2022 that will close this issue
@DilanAthukorala DilanAthukorala self-assigned this May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed trigger / source New trigger / source request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants