Arvados

Can you expand on this? What does "tracked" mean in this context? What is being tracked and why?

"tracked" here refers to the container records being managed by crunch-dispatch-slurm and their corresponding on slurm jobs. It queries the API server to get the latest status of "tracked" containers here:

https://dev.arvados.org/projects/arvados/repository/revisions/master/entry/sdk/go/dispatch/dispatch.go#L60

However, I'm not entirely sure why it is querying for specific containers rather than querying by some more general filter.

Peter Amstutz wrote:

However, I'm not entirely sure why it is querying for specific containers rather than querying by some more general filter.

Because it needs to know whether any of the containers it's currently tracking have changed so they no longer meet the criteria for tracking, i.e., wouldn't match such a filter.

Using POST should work. Another approach would be to check the "tracked" list in batches, and only the subset that doesn't already get returned by one of the filter-based queries.

        d.checkForUpdates([][]interface{}{
            {"uuid", "in", tracked}})
        d.checkForUpdates([][]interface{}{
            {"locked_by_uuid", "=", d.auth.UUID},
            {"uuid", "not in", tracked}})
        d.checkForUpdates([][]interface{}{
            {"state", "=", Queued},
            {"priority", ">", "0"},
            {"uuid", "not in", tracked}})

1. filter by locked_by_uuid == d.auth.UUID
2. filter by uuid in {tracked minus the UUIDs returned in the previous query}, either via POST or in batches of 10?
3. filter by state = Queued, ignoring tracked UUIDs instead of asking api to filter them out

If we do something like note-6 then we only expect each container to appear in that query string one time, right? So "batches of 10" would only come up in cases like "lots of containers finish at once."

I think it is better to get the current state and behave accordingly. If the container is finished, we'll do the right thing and the relevant UUID will never be in another of these queries. If not, there's some kind of error which we should not conveniently avoid seeing.

For review:

12446-dispatcher-query @ 32713b4b3c2e1685b79acb24059a5b817cf6cbfc

Batching bugfix LGTM.

• I don't think we should do this when checkForUpdates() hits "Error getting list of containers". As it stands, a transient network error would easily cause dispatch to close all trackers, which seems wrong: even if we resume tracking anyway, this would produce a bunch of logging noise.
• Apart from the transient network/API error case, this is an unexpected situation, so we should log a warning.

Putting the "seen" flag in the tracker itself seems weird -- it causes a lot of mutexing, and it's not usable by anyone other than the loop in Run() anyway because timing. How about returning a list of seen uuids from checkForUpdates, and keeping all the "missing" logic in one place, something like

Run() {
  todo := map[string]*runTracker
  for uuid, rt := range d.trackers {
    todo[uuid] = rt
  }
  for _, uuid := d.checkForUpdates(...) { delete(todo, uuid) }
  ...
  for uuid, t := range todo {
    // log something
    t.close()
  }
}

Tom Clegg wrote:

Batching bugfix LGTM.

The "seen" stuff looks like it's fixing a different condition, where a dispatcher is tracking a container but even fetching that container by UUID doesn't find it. This seems like it avoids a memory/goroutine leak in this unexpected situation, which is good, but

• I don't think we should do this when checkForUpdates() hits "Error getting list of containers". As it stands, a transient network error would easily cause dispatch to close all trackers, which seems wrong: even if we resume tracking anyway, this would produce a bunch of logging noise.

Good point. Fixed.

• Apart from the transient network/API error case, this is an unexpected situation, so we should log a warning.

Agree. Done.

Putting the "seen" flag in the tracker itself seems weird -- it causes a lot of mutexing, and it's not usable by anyone other than the loop in Run() anyway because timing. How about returning a list of seen uuids from checkForUpdates, and keeping all the "missing" logic in one place, something like

I went with a slightly different approach of passing in the "todo" map and removing each UUID as it is seen in checkListForUpdates(). This is mainly because checkForUpdates would otherwise have to build a list of uuids, return it, and then Run() would have to iterate over it in several places, which overall seems like a lot of boilerplate to get to a single delete() operation.

12446-dispatcher-query @ 1b411c1882a37ae8f88c0b770994dd257cf3dee4

https://ci.curoverse.com/job/developer-run-tests/484/

LGTM, thanks