Differential D11463
Allow build implementations to extend their taskmaster lease Authored by hach-que on Jan 22 2015, 12:11 AM. Tags None Referenced Files
Subscribers
Details
Fixes T7006. Currently if a Harbormaster build target runs for longer than 24 hours, it will be executed multiple times. This is because the taskmaster lease expires on the target worker task. This provides build step implementations a mechanism to extend the task lease by a number of seconds; whileever build steps implementations are progressing they can increment the task lease time, while still ensuring that if the taskmaster encounters a fatal error the task will be picked up elsewhere. Modified the lease time on Harbormaster workers down to 30 seconds, and then ran a remote command 'sleep 60'. Saw the lease expiry time stay above 24 seconds under /daemon/ as the build occurred.
Diff Detail
Event TimelineComment Actions Have you looked to see if it's practical to have this happen automatically as part of the sleep() / stillWorking() stuff? I'm actually surprised long-running commands don't get detected as hung and killed right now, so maybe that's buggy. The expectation is that daemons must call sleep() (or stillWorking()) periodically to send a signal to the overseer indicating that they have not hung. If the overseer does not receive such a signal before a deadline, it assumes the daemon is hung and forcibly restarts it. It actually looks like the overseer allows the daemons to hang for 86400 seconds (24 hours) before force-restarting them right now, so maybe we are seeing that. Extending the lease by 6 seconds at a time also seems odd to me. This will cause us to do a relatively large number of fairly unnecessary writes, and it doesn't seem implausible to me that something might hang for 6 seconds sooner or later and incorrectly cause the lease to be freed. I'd expect the rule to be more like "check if we've used up 80% of the lease time; if we have, double the lease time". Comment Actions
It's quite possible that is happening; it would explain why we didn't seem to run out of taskmasters. I'll hook this up to sleep() / stillWorking() with the alternate logic to prevent lots of writes.
Comment Actions Some possible cleanups, but I think this is basically reasonable. Feel free to skip this feedback if it all seems like nitpicky junk.
Comment Actions I need to also update FeedPublisherWorker which constructs other workers to deal with the new worker constructor. Comment Actions FeedPublisherWorker manually constructs workers for Doorkeeper; make sure they pass the task in the constructor. Comment Actions Also ran grep to try and spot anywhere else that's manually constructing workers, but all the other places with worker names in strings seem to be uses of scheduleTask. Comment Actions This hasn't been re-reviewed since the last upstream feedback, so I'm abandoning it (and potentially just keeping it patched locally). | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||