add shutdown hook

Add shutdown hook to provide a controlled way of stopping. This should be provided in class Main.

What exactly should be done when the shutdown-hook is called? Especially looking at the different reasons for shutdown (system maintenance, corrupt disk, full disk, ...).

changed milestone to %CollecTor 1.0.2

added component::metrics/collector ctip milestone::CollecTor 1.0.2 owner::iwakeh priority::low resolution::fixed severity::normal status::closed type::enhancement labels

Trac:
Status: new to needs_information

Oh wow, that's actually a tough question. And I think the answer depends on how modules are started (cron vs. internal scheduler), how that scheduler is started (cron vs. service script) and stopped (kill vs. service script), and so on. For example, I could imagine using a shutdown hook to release a lock file, but it might be that we don't need a lock file anymore once we switch to a service script.

One thing about reasons for shutdown: I don't think we can do anything useful in case of a corrupt or full disk. We should probably focus on system maintenance here. And doing that would be great, because then our sysadmins could simply install updates and reboot hosts whenever they need to.

How about we postpone this discussion until those other changes are implemented? I think we can do some good things with shutdown hooks, so I don't suggest closing this issue. But I think I can contribute more to this discussion in a month or two.

Yes, let's postpone this ticket.

I lower the priority and set it to new. We can increase it again when the other changes are implemented.

Trac:
Status: needs_information to new
Priority: Medium to Low

Added to second release milestone.

Trac:
Milestone: N/A to CollecTor 1.1.0

With the scheduler functionality it makes sense to revisit this ticket.

Tasks for the shutdown-hook:

• start a regular scheduler shutdown, which lets all running tasks finish and exits.
• this is written to the logs in order to make the operator aware that a shutdown is running.

What else?

Trac:
Status: new to needs_information

The shutdown hook that allows currently running modules to finish properly. Shutdown hook log statements are also printed to the console (by logback configuration) in order to indicate the shutdown process.

Please review this branch.

(The coverage-check is failing with the current master; this is also corrected in the above branch.)

Trac:
Status: needs_information to needs_review

Looks great! Here are a few suggestions to make it even better:

• Would it be possible to catch a second kill signal that terminates the process immediately? The tor daemon does this by first giving 30 seconds to shut down gracefully and shutting down ungracefully upon receiving the second kill signal. Of course, there's always the option to kill -9, but that might seem like, erm, overkill.

• Should the log message mention the 20 minutes grace period, like: "Waiting up to 20 minutes for termination of running tasks ..."? If we implement the previous suggestion, how about: "Waiting up to 20 minutes for termination of running tasks; kill again to terminate immediately ..."?

• How's this change log entry?

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf1ea12..ad9da3f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,7 +1,9 @@
-# Changes in version 1.1.0 - 2016-08-31
+# Changes in version 1.1.0 - 2016-09-xx
 
  * Medium changes
    - Add support for Bifroest's bridge descriptor tarballs.
+   - Use a shutdown hook that gives currently running modules up to 20
+     minutes to finish properly, rather than killing them immediately.
 
  * Minor changes
    - Remove quotes around base URL in index.json.

Trac:
Status: needs_review to needs_revision

Addendum: It looks like ShutdownHook.java contains quite a few unused imports. I'll let Eclipse organize imports of that class and squash those changes into your commit before merging to master.

Thanks, for the suggestions!

I think there is no way to catch a second kill signal; the shutdown thread is only called once by the jvm.

The 20 min waiting time is pretty arbitrary and could only be fully used by relaydesc module anyway, afaict. I thought that it should accommodate a just started relaydesc download to finish properly to make sense.

Maybe this grace period should be made configurable to give operators better control?

Please review my branch with the additional log-stmt and w/o the redundant imports (Thanks for spotting these!); and another commit with a configurable grace period.

With the configurable grace period the change log needs to be adapted otherwise it's fine.

Trac:
Status: needs_revision to needs_review

Looks good! Last question before merging: Should collector.properties contain 20 as default value rather than 10? (Happy to change that, but figured I should ask first, because there might be a reason for this deviation.)

I changed the default, b/c I observed that a relaydesc module run doesn't take more than 10 min to finish.

Feel free to adapt the minutes to the number of minutes the longest running module (on the main CollecTor instance) would take to finish properly.

Ah, 10 is fine. Do you mind if I change the remaining 20s in Scheduler.java (variable gracePeriodMinutes) to 10, too?

Replying to karsten:

Ah, 10 is fine. Do you mind if I change the remaining 20s in Scheduler.java (variable gracePeriodMinutes) to 10, too?

That's fine. Better be consistent when setting the defaults.

Merged! Thanks for building this, this is really a step towards making CollecTor more reliable! Closing.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Added to appropriate milestone.

Trac:
Milestone: CollecTor 1.1.0 to CollecTor 1.0.2

closed

mentioned in issue #20079 (moved)