Arvados

d3781de388530d06379974601247a6c044eee92e in branch 8028-crunch-dispatch-local

Implementation notes:

• main func with command line args for polling and crunch-run command

• Using ticker, poll for queued containers and invoke run goroutine for each

• (Upon discussing with Tom), any errors are logged but the program continues to run, without exiting. This is because, once we start one or more run procs, we need to wait for them complete. The program does end if there are any errors in args processing etc.

• (Upon discussing with Tom), after the crunch-run command completes, update the container state to "Complete" if it is still in "Running" state

• Log the events listed as applicable

Testing notes:

• Added containers fixtures, one in queued state and one in completed state (this one is not used in testing)

• Test the main method with args parsing

• Using the arvadostest.ServerStub implementation, verify the run goroutine functionality by verifying the contents of the log file

The "items available" is the total number of items that match the filter in the database. Because of paging, the number of items actually returned can be smaller. So you need to use len(containers.Items) here.

    for i := 0; i < containers.ItemsAvailable; i++ {

The dispatcher should probably record which containers UUIDs it has started and make sure they only are started once.

cmd := exec.Command(crunchRunCommand, "--job", uuid)

Crunch-run takes the first command line argument as the container UUID, so you can remove the "--job" from the runner command.

Crunch-run is responsible for moving the container from the "Queued" to "Running" state, so you can remove that part.

Please use cmd.Process.Signal(os.Interrupt) to terminate the process, so that crunch-run can shut down gracefully.

Instead of containers.ItemsAvailable, use len(containers.Items)

Done

The dispatcher should probably record which containers UUIDs it has started and make sure they only are started once.

Already doing this. However, noticed from testing log that I need to increase the poll-time in test slightly to ensure second run does not happen while the first one is still dispatching.

you can remove the "--job" from the runner command.

Done

Crunch-run is responsible for moving the container from the "Queued" to "Running" state, so you can remove that part

For the time being, I am leaving this alone. We will remove / update this when locking api is available and we have better state management in place.

Please use cmd.Process.Signal(os.Interrupt) to terminate the process, so that crunch-run can shut down gracefully.

Done

Please add a graceful shutdown:

If the dispatcher receives SIGINT or SIGTERM:

1. stop the runQueuedContainers loop with the doneProcessing channel
2. send Interrupt to any running processes
3. wait for all the processes to finish.
4. only then, terminate

Update Test_doMain() and testWithServerStub() to use the "wait for processes to finish" capability instead of time.Sleep().

Added signal handling to dispatcher.

Thanks for turning this around.

I suggest refactoring the implementation a litle bit:

1. The signal handler just sends "true" on the doneProcessing channel
2. Make wg sync.WaitGroup a global variable
3. The run() function is responsible for calling wg.Add() and defer wg.Done()
4. the for{} loop in runQueuedContainers should break, not return when doneProcessing is ready
5. after the for{} loop in runQueuedContainers breaks, that's when it should send the interrupt to each entry in runningCmds
6. then runQueuedContainers should call wg.Wait()

Also, there should be a sync.Mutex protecting runningCmds map everywhere it is accessed.

updated ci.curoverse.com, diff was 1e48d4f..7f05c28