Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#19018 closed enhancement (fixed)

run CollecTor modules without crontab

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.0.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: ctip
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

use a scheduled executor as in Onionoo cf. #14826.

There are six modules:

  • bridgedescs
  • bridgepools
  • exitlists
  • relaydescs
  • torperf
  • updateindex

There should be a configuration about when to start each module, if it is supposed to start.

Suggestion:
have an entry <module>.timing in the (still to be improved) configuration, which
has the minute of the hour as value and a negative value means off.

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by iwakeh

depends on #19021

The changes proposed in this ticket introduce several new configuration parameters. It depends on the completion of #19021, b/c the new config process will reduce the implementation effort necessary for adding new config parameters.

The property name in the description just is there to sketch the concept; the actual name will look different (i.e. according to the naming chosen in #19021)

comment:2 Changed 4 years ago by karsten

I tried to apply the suggested configuration scheme to match what we're currently doing with cron. But the simplest way I could come up with that does what we need requires two values per module:

  • offsetMinutes: offset in minutes since the epoch and
  • periodMinutes: period in minutes.

Using these two values we can compute initialDelay and period for ScheduledExecutorService.scheduleAtFixedRate() as follows (mostly untested):

long ONE_MINUTE = 60L * 1000L;
long initialDelay = periodMinutes * ONE_MINUTE
    - (System.currentTimeMillis() - offsetMinutes * ONE_MINUTE)
    % (periodMinutes * ONE_MINUTE);
long period = periodMinutes * ONE_MINUTE;

Here's how I would configure runs on collector.torproject.org using that scheme:

bridgedescs 9,60
bridgepools -1,-1
exitlists 2,60
relaydescs 5,30
torperf 1,360
updateindex 0,2

Explanation:

  • Bridge descriptors are copied over from the bridge authority at :07 every hour, so we should start the sanitizing soon after at :09 every hour and be ready when Onionoo and others fetch sanitized bridge descriptors at :15.
  • Bridge pool assignments are not sanitized anymore, so I picked -1,-1 as special value to disable the module, similar to your suggestion. (Note that we could also remove the entire code for sanitizing bridge pool assignments. If you currently don't have any branches based on master and if you'd like me to remove that code, I would go ahead and do that today. But regardless, we would need a way to disable a module, and -1,-1 should work okay for that.)
  • Exit lists should be fetched once per hour at :02.
  • Relay descs can be fetched at :05, but we should try hard not to miss an hour there. If the server happens to be down at :05 we should try again half an hour later at :35. We're currently not doing that, but I think it would be smart to do it.
  • Torperf results are only fetched every six hours to reduce load on the Torperf hosts.
  • Updating the index should happen every two minutes.

How does that sound?

comment:3 Changed 4 years ago by iwakeh

Thanks for the quick reply and the very useful background information!

Please go ahead and remove all obsolete code!

I will add another property for having a particular module activated or not.
That way the configuration is readable without explanation and the module could be taken offline without loosing the timing configuration.

In addition, a negative number would actually make sense for an offset in minutes from the epoch.
But, I'll only make negative numbers available, if I don't incur any implementation overhead for that.
Thus, most likely only positive values for minutes from the epoch and strictly positive numbers for the period settings.

Last edited 4 years ago by iwakeh (previous) (diff)

comment:4 Changed 4 years ago by iwakeh

Status: newaccepted

comment:5 Changed 4 years ago by iwakeh

Regarding code removal I forgot to mention this change #18922 which is based on the current master.

comment:6 Changed 4 years ago by karsten

After merging #18922 I just removed the obsolete bridge pool assignments sanitizing code.

comment:7 in reply to:  6 Changed 4 years ago by iwakeh

Status: acceptedassigned

Replying to karsten:

After merging #18922 I just removed the obsolete bridge pool assignments sanitizing code.

Thanks a lot!

comment:8 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.0.0

comment:9 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Please review the implementation here.
Consists of two commits; the first is here and just extends the properties.

I'm going to test this on my collector mirror.

comment:10 Changed 3 years ago by iwakeh

Added one commit that omits META-INF files from other jars when creating collector.jar.

comment:11 Changed 3 years ago by iwakeh

And, another commit replacing the (hopefully) last occurrence of a hard-coded path.

comment:12 Changed 3 years ago by iwakeh

Status: needs_reviewneeds_revision

Found something that needs investigation.

Setting to needs revision.

comment:13 Changed 3 years ago by iwakeh

Status: needs_revisionneeds_review

Please review this branch.

The code has been running fine over the weekend on my mirror.

A longer diff, therefor some explanations:

Fix NPE: apparently empty files named like 'archive/relay-descriptors/tor/tor-20??-??.tar.xz' are recognized as directories and when calling listFiles this results in return value null.
This is a bug in both the shell script and CollecTor. The latter is fixed now and the former ought to be replaced by a java module (see CollecTor's development page).

Have one 'run' method in CollecTorMain to unify Exception logging; behavior as before: if a module fails the scheduler will attempt running it again.

Corrected period calculation and enhanced logging (cf. here)

comment:14 Changed 3 years ago by karsten

That branch looks good, I don't have any major requests for changes. Please find my branch task-19018 with two commits, one with trivial whitespace fixes and one that fixes the properties template. Let me know if you're okay with those changes.

And would you mind if we avoid that merge commit by going back to the latest commit that is in master and your branch and cherry-picking subsequent commits? I recognize that your branch is correct, but I remember having trouble with merges in the past, so I'm trying to rebase whenever I can. And with only two developers this still seems doable. Happy to try this, and if the result is the same as your branch, except for slightly changed commit history, I'd push to master. Sound okay?

comment:15 in reply to:  14 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

Replying to karsten:

That branch looks good, I don't have any major requests for changes. Please find my branch task-19018 with two commits, one with trivial whitespace fixes and one that fixes the properties template. Let me know if you're okay with those changes.

Thanks for looking through quickly and correcting my whitespace quirks.
The added equal is fine in properties.

And would you mind if we avoid that merge commit by going back to the latest commit that is in master and your branch and cherry-picking subsequent commits? I recognize that your branch is correct, but I remember having trouble with merges in the past, so I'm trying to rebase whenever I can. And with only two developers this still seems doable. Happy to try this, and if the result is the same as your branch, except for slightly changed commit history, I'd push to master. Sound okay?

Yes, that's ok, as long as all changes end up in master.

comment:16 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Cherry-picked and pushed to master. Please double-check and re-open if I messed up something! Thanks a lot! Closing.

comment:17 Changed 3 years ago by iwakeh

All the right cherries picked. Thanks!

Note: See TracTickets for help on using tickets.