{"action":"created","issue":{"url":"https://api.github.com/repos/hashicorp/nomad/issues/5007","repository_url":"https://api.github.com/repos/hashicorp/nomad","labels_url":"https://api.github.com/repos/hashicorp/nomad/issues/5007/labels{/name}","comments_url":"https://api.github.com/repos/hashicorp/nomad/issues/5007/comments","events_url":"https://api.github.com/repos/hashicorp/nomad/issues/5007/events","html_url":"https://github.com/hashicorp/nomad/pull/5007","id":391150458,"node_id":"MDExOlB1bGxSZXF1ZXN0MjM4NzUyNzIy","number":5007,"title":"allocrunner: Async api for shutdown/destroy/update","user":{"login":"dantoml","id":1330683,"node_id":"MDQ6VXNlcjEzMzA2ODM=","avatar_url":"https://avatars2.githubusercontent.com/u/1330683?v=4","gravatar_id":"","url":"https://api.github.com/users/dantoml","html_url":"https://github.com/dantoml","followers_url":"https://api.github.com/users/dantoml/followers","following_url":"https://api.github.com/users/dantoml/following{/other_user}","gists_url":"https://api.github.com/users/dantoml/gists{/gist_id}","starred_url":"https://api.github.com/users/dantoml/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dantoml/subscriptions","organizations_url":"https://api.github.com/users/dantoml/orgs","repos_url":"https://api.github.com/users/dantoml/repos","events_url":"https://api.github.com/users/dantoml/events{/privacy}","received_events_url":"https://api.github.com/users/dantoml/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-12-14T15:16:21Z","updated_at":"2018-12-18T10:54:26Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/hashicorp/nomad/pulls/5007","html_url":"https://github.com/hashicorp/nomad/pull/5007","diff_url":"https://github.com/hashicorp/nomad/pull/5007.diff","patch_url":"https://github.com/hashicorp/nomad/pull/5007.patch"},"body":"This PR updates the interface to the AllocRunner to return as quickly as\r\npossible when updating, shutting down, and destroying a runner.\r\n\r\nThese changes have a few benefits:\r\n- It exposes a cleaner API to the client\r\n- We can now correctly shutdown a client in parallel in DevMode\r\n- We can potentially start parallising certain garbage collection actions more efficiently\r\n- Allocation Update throughput is _significantly_ higher, for example, when preempting allocations, we used to have to wait for each preempted allocation to shut down sequentially, now these can all happen in parallel."},"comment":{"url":"https://api.github.com/repos/hashicorp/nomad/issues/comments/448180358","html_url":"https://github.com/hashicorp/nomad/pull/5007#issuecomment-448180358","issue_url":"https://api.github.com/repos/hashicorp/nomad/issues/5007","id":448180358,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0ODE4MDM1OA==","user":{"login":"dantoml","id":1330683,"node_id":"MDQ6VXNlcjEzMzA2ODM=","avatar_url":"https://avatars2.githubusercontent.com/u/1330683?v=4","gravatar_id":"","url":"https://api.github.com/users/dantoml","html_url":"https://github.com/dantoml","followers_url":"https://api.github.com/users/dantoml/followers","following_url":"https://api.github.com/users/dantoml/following{/other_user}","gists_url":"https://api.github.com/users/dantoml/gists{/gist_id}","starred_url":"https://api.github.com/users/dantoml/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dantoml/subscriptions","organizations_url":"https://api.github.com/users/dantoml/orgs","repos_url":"https://api.github.com/users/dantoml/repos","events_url":"https://api.github.com/users/dantoml/events{/privacy}","received_events_url":"https://api.github.com/users/dantoml/received_events","type":"User","site_admin":false},"created_at":"2018-12-18T10:54:26Z","updated_at":"2018-12-18T10:54:26Z","author_association":"MEMBER","body":"@schmichael The biggest issue right now, imo, is the long running locks to the internal state of the alloc runner, that can cause fairly strange and unexpected behaviour. Some middle ground of basically this pr, but modified to block on the channel before returning would deal with that issue pretty effectively, while still giving a blocking api to the Client.\r\n\r\nWith regards to tests, there are only a handful that actually destroy an allocrunner, and so updating them is fairly minor:\r\n\r\n```\r\n[nomad(dani/f-allocrunner-async)] $ git grep -E ar(.|)\\.Destroy | grep -v deprecated\r\nclient/allocrunner/alloc_runner_test.go:        defer ar.Destroy()\r\nclient/allocrunner/alloc_runner_test.go:        defer ar.Destroy()\r\nclient/allocrunner/alloc_runner_test.go:        defer ar2.Destroy()\r\nclient/allocrunner/alloc_runner_test.go:        ar2.Destroy()\r\nclient/allocrunner/alloc_runner_test.go:        case <-ar2.DestroyCh():\r\nclient/client.go:                       ar.Destroy()\r\nclient/client.go:                       arGroup.AddCh(ar.DestroyCh())\r\nclient/client_test.go:          ar.Destroy()\r\nclient/client_test.go:          ar.Destroy()\r\nclient/gc.go:   ar.Destroy()\r\n```\r\n\r\nThe second point is one that mostly comes from @dadgar, which is wanting an AllocRunner to expose an API that returns as soon as possible to the client, with the consumer handling waiting. This comes with some nice benefits around assumptions we can make, and makes it harder to accidentally block a routine for longer than is necessary.\r\n\r\nI'm not really too sold on either path,  and can either finish updating tests for correctness (we're lucky rn in that a couple either use a waitUntil someOtherState happens, or it happens in a defer as a cleanup action) or do the jiggling we'd need to make the api blocking again (which would at least be consistent with `Run()`)."}}
{"actor":{"display_login":"dantoml"}}