{"action":"created","comment":{"url":"https://api.github.com/repos/coreos/fleet/pulls/comments/77678587","id":77678587,"diff_hunk":"@@ -1067,78 +1072,87 @@ func assertUnitState(name string, js job.JobState, out io.Writer) (ret bool) {\n // tryWaitForSystemdActiveState tries to wait for systemd units to reach an\n // active state, making use of the systemd's DBUS interface. First it takes one\n // or more units as input, and ensures that every unit in the []units must be\n-// in the active state. If yes, return 0. Otherwise return 1.\n+// in the active state. If yes, return nil. Otherwise return error.\n //\n // NOTE: To avoid unnecessary conflicts with unit tests in start_test.go, we\n-// should return 0 for the case of sd_dbus.New() returning error. In that case,\n+// should return nil for the case of sd_dbus.New() returning error. In that case,\n // it just means that the unit test environment based on Travis is not capable\n // of setting up a DBUS connection of systemd, which would actually work fine\n // for production systems or functional tests.\n-func tryWaitForSystemdActiveState(units []string) (ret int) {\n-\tconn, err := sd_dbus.New()\n-\tif err != nil {\n-\t\tstderr(\"Failed to get systemd-dbus conn: %v\", err)\n-\t\treturn 0\n-\t}\n-\tif conn == nil {\n-\t\tstderr(\"Got a nil connection\")\n-\t\treturn 0\n+func tryWaitForSystemdActiveState(units []string, maxAttempts int) (err error) {\n+\tif maxAttempts <= -1 {\n+\t\tfor _, name := range units {\n+\t\t\tstdout(\"Triggered unit %s start\", name)\n+\t\t}\n+\t\treturn nil\n \t}\n-\tdefer conn.Close()\n \n-\t// Get systemd state and check the state is active & loaded.\n-\t// If any unit in []units cannot be found, then return an error.\n-\t// That should be the only case of returning 1.\n \tfor _, uName := range units {\n-\t\tfound, err := waitForSystemdActiveState(conn, uName)\n-\t\tif !found || err != nil {\n+\t\terr := waitForSystemdActiveState(uName)\n+\t\tif err != nil {\n \t\t\tstderr(\"cannot find an active unit: %v\", err)\n-\t\t\treturn 1\n+\t\t\treturn err\n \t\t}\n \t}\n \n-\treturn 0\n+\treturn nil\n }\n \n // waitForSystemdActiveState tries to assert that the given unit becomes\n // active, retrying periodically until the unit gets active. It will retry\n-// up to maxAttempts.\n-func waitForSystemdActiveState(conn *sd_dbus.Conn, unitName string) (found bool, err error) {\n-\treturn func() (found bool, errWait error) {\n+// up to 15 seconds.\n+func waitForSystemdActiveState(unitName string) (err error) {\n+\treturn func() error {\n \t\ttimeout := 15 * time.Second\n \t\talarm := time.After(timeout)\n \t\tticker := time.Tick(250 * time.Millisecond)\n \t\tfor {\n \t\t\tselect {\n \t\t\tcase <-alarm:\n-\t\t\t\treturn false, fmt.Errorf(\"Failed to reach expected state within %v.\", timeout)\n+\t\t\t\treturn fmt.Errorf(\"Failed to reach expected state within %v.\", timeout)\n \t\t\tcase <-ticker:\n-\t\t\t\tif found, errA := assertSystemdActiveState(conn, unitName); found && errA == nil {\n-\t\t\t\t\treturn found, nil\n+\t\t\t\tif errA := assertSystemdActiveState(unitName); errA == nil {\n+\t\t\t\t\treturn nil\n \t\t\t\t}\n \t\t\t}\n \t\t}\n \t}()\n }\n \n // assertSystemdActiveState determines if a given systemd unit is actually\n-// in the active state, making use of systemd's DBUS connection.\n-func assertSystemdActiveState(conn *sd_dbus.Conn, unitName string) (found bool, err error) {\n-\tlistUnits, err := conn.ListUnits()\n+// in the active state, making use of cAPI\n+func assertSystemdActiveState(unitName string) (err error) {\n+\tuState, err := getSingleUnitState(unitName)\n \tif err != nil {\n-\t\treturn false, fmt.Errorf(\"Failed to list systemd unit: %v\", err)\n+\t\treturn err\n \t}\n \n-\tfor _, u := range listUnits {\n-\t\tif strings.Compare(u.Name, unitName) != 0 {\n-\t\t\tcontinue\n-\t\t}\n-\t\tif u.ActiveState != \"active\" || u.LoadState != \"loaded\" {\n-\t\t\treturn false, fmt.Errorf(\"Failed to find an active unit %s\", unitName)\n-\t\t}\n+\t// Get systemd state and check the state is active & loaded.\n+\tif uState.ActiveState != \"active\" || uState.LoadState != \"loaded\" {\n+\t\treturn fmt.Errorf(\"Failed to find an active unit %s\", unitName)\n+\t}\n+\n+\treturn nil\n+}\n+\n+// getSingleUnitState returns a single uState of type suState, which consists\n+// of necessary systemd states, only for a given unit name.\n+func getSingleUnitState(unitName string) (suState, error) {","path":"fleetctl/fleetctl.go","position":158,"original_position":155,"commit_id":"6a64a4e3b7d3d988747cf74a765657cc75ec7df8","original_commit_id":"88a0a30207caf0dfaa08ec18a7c2242a7f066278","user":{"login":"jonboulle","id":2601015,"avatar_url":"https://avatars.githubusercontent.com/u/2601015?v=3","gravatar_id":"","url":"https://api.github.com/users/jonboulle","html_url":"https://github.com/jonboulle","followers_url":"https://api.github.com/users/jonboulle/followers","following_url":"https://api.github.com/users/jonboulle/following{/other_user}","gists_url":"https://api.github.com/users/jonboulle/gists{/gist_id}","starred_url":"https://api.github.com/users/jonboulle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonboulle/subscriptions","organizations_url":"https://api.github.com/users/jonboulle/orgs","repos_url":"https://api.github.com/users/jonboulle/repos","events_url":"https://api.github.com/users/jonboulle/events{/privacy}","received_events_url":"https://api.github.com/users/jonboulle/received_events","type":"User","site_admin":false},"body":"I'm a bit concerned about the efficiency of this - `UnitStates()` is a relatively expensive call, and we're only concerned about a single item. It's actually a little worse because we're potentially doing this in multiple goroutines simultaneously, per [`waitForUnitStates`](https://github.com/endocode/fleet/blob/88a0a30207caf0dfaa08ec18a7c2242a7f066278/fleetctl/fleetctl.go#L994).\r\n\r\nTwo suggestions:\r\n1) introduce a `UnitState.Get` call (similar to how, for units themselves, we have both `Unit.Get` and `Unit.List`)\r\nor,\r\n2) use a single `UnitState.List` call and share the results across all goroutines (in waitForUnitStates)\r\n\r\nthoughts?","created_at":"2016-09-06T17:16:40Z","updated_at":"2016-09-06T17:16:40Z","html_url":"https://github.com/coreos/fleet/pull/1667#discussion_r77678587","pull_request_url":"https://api.github.com/repos/coreos/fleet/pulls/1667","_links":{"self":{"href":"https://api.github.com/repos/coreos/fleet/pulls/comments/77678587"},"html":{"href":"https://github.com/coreos/fleet/pull/1667#discussion_r77678587"},"pull_request":{"href":"https://api.github.com/repos/coreos/fleet/pulls/1667"}}},"pull_request":{"url":"https://api.github.com/repos/coreos/fleet/pulls/1667","id":81987823,"html_url":"https://github.com/coreos/fleet/pull/1667","diff_url":"https://github.com/coreos/fleet/pull/1667.diff","patch_url":"https://github.com/coreos/fleet/pull/1667.patch","issue_url":"https://api.github.com/repos/coreos/fleet/issues/1667","number":1667,"state":"open","locked":false,"title":"fleetctl: check systemd active state via client API","user":{"login":"dongsupark","id":10096906,"avatar_url":"https://avatars.githubusercontent.com/u/10096906?v=3","gravatar_id":"","url":"https://api.github.com/users/dongsupark","html_url":"https://github.com/dongsupark","followers_url":"https://api.github.com/users/dongsupark/followers","following_url":"https://api.github.com/users/dongsupark/following{/other_user}","gists_url":"https://api.github.com/users/dongsupark/gists{/gist_id}","starred_url":"https://api.github.com/users/dongsupark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dongsupark/subscriptions","organizations_url":"https://api.github.com/users/dongsupark/orgs","repos_url":"https://api.github.com/users/dongsupark/repos","events_url":"https://api.github.com/users/dongsupark/events{/privacy}","received_events_url":"https://api.github.com/users/dongsupark/received_events","type":"User","site_admin":false},"body":"It's not appropriate to check systemd active states with DBUS connection, as that will return correct states only on the same machine. So we should also introduce a check using `cAPI.UnitStates()`, to check states on other machines.","created_at":"2016-08-19T14:14:38Z","updated_at":"2016-09-06T17:16:40Z","closed_at":null,"merged_at":null,"merge_commit_sha":"67be513fa464d19bd76bca52cf555abaf3b3d9dd","assignee":{"login":"dongsupark","id":10096906,"avatar_url":"https://avatars.githubusercontent.com/u/10096906?v=3","gravatar_id":"","url":"https://api.github.com/users/dongsupark","html_url":"https://github.com/dongsupark","followers_url":"https://api.github.com/users/dongsupark/followers","following_url":"https://api.github.com/users/dongsupark/following{/other_user}","gists_url":"https://api.github.com/users/dongsupark/gists{/gist_id}","starred_url":"https://api.github.com/users/dongsupark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dongsupark/subscriptions","organizations_url":"https://api.github.com/users/dongsupark/orgs","repos_url":"https://api.github.com/users/dongsupark/repos","events_url":"https://api.github.com/users/dongsupark/events{/privacy}","received_events_url":"https://api.github.com/users/dongsupark/received_events","type":"User","site_admin":false},"assignees":[{"login":"dongsupark","id":10096906,"avatar_url":"https://avatars.githubusercontent.com/u/10096906?v=3","gravatar_id":"","url":"https://api.github.com/users/dongsupark","html_url":"https://github.com/dongsupark","followers_url":"https://api.github.com/users/dongsupark/followers","following_url":"https://api.github.com/users/dongsupark/following{/other_user}","gists_url":"https://api.github.com/users/dongsupark/gists{/gist_id}","starred_url":"https://api.github.com/users/dongsupark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dongsupark/subscriptions","organizations_url":"https://api.github.com/users/dongsupark/orgs","repos_url":"https://api.github.com/users/dongsupark/repos","events_url":"https://api.github.com/users/dongsupark/events{/privacy}","received_events_url":"https://api.github.com/users/dongsupark/received_events","type":"User","site_admin":false}],"milestone":null,"commits_url":"https://api.github.com/repos/coreos/fleet/pulls/1667/commits","review_comments_url":"https://api.github.com/repos/coreos/fleet/pulls/1667/comments","review_comment_url":"https://api.github.com/repos/coreos/fleet/pulls/comments{/number}","comments_url":"https://api.github.com/repos/coreos/fleet/issues/1667/comments","statuses_url":"https://api.github.com/repos/coreos/fleet/statuses/6a64a4e3b7d3d988747cf74a765657cc75ec7df8","head":{"label":"endocode:dongsu/fleetctl-capi-systemd-states","ref":"dongsu/fleetctl-capi-systemd-states","sha":"6a64a4e3b7d3d988747cf74a765657cc75ec7df8","user":{"login":"endocode","id":4068543,"avatar_url":"https://avatars.githubusercontent.com/u/4068543?v=3","gravatar_id":"","url":"https://api.github.com/users/endocode","html_url":"https://github.com/endocode","followers_url":"https://api.github.com/users/endocode/followers","following_url":"https://api.github.com/users/endocode/following{/other_user}","gists_url":"https://api.github.com/users/endocode/gists{/gist_id}","starred_url":"https://api.github.com/users/endocode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/endocode/subscriptions","organizations_url":"https://api.github.com/users/endocode/orgs","repos_url":"https://api.github.com/users/endocode/repos","events_url":"https://api.github.com/users/endocode/events{/privacy}","received_events_url":"https://api.github.com/users/endocode/received_events","type":"Organization","site_admin":false},"repo":{"id":34001513,"name":"fleet","full_name":"endocode/fleet","owner":{"login":"endocode","id":4068543,"avatar_url":"https://avatars.githubusercontent.com/u/4068543?v=3","gravatar_id":"","url":"https://api.github.com/users/endocode","html_url":"https://github.com/endocode","followers_url":"https://api.github.com/users/endocode/followers","following_url":"https://api.github.com/users/endocode/following{/other_user}","gists_url":"https://api.github.com/users/endocode/gists{/gist_id}","starred_url":"https://api.github.com/users/endocode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/endocode/subscriptions","organizations_url":"https://api.github.com/users/endocode/orgs","repos_url":"https://api.github.com/users/endocode/repos","events_url":"https://api.github.com/users/endocode/events{/privacy}","received_events_url":"https://api.github.com/users/endocode/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/endocode/fleet","description":"A Distributed init System","fork":true,"url":"https://api.github.com/repos/endocode/fleet","forks_url":"https://api.github.com/repos/endocode/fleet/forks","keys_url":"https://api.github.com/repos/endocode/fleet/keys{/key_id}","collaborators_url":"https://api.github.com/repos/endocode/fleet/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/endocode/fleet/teams","hooks_url":"https://api.github.com/repos/endocode/fleet/hooks","issue_events_url":"https://api.github.com/repos/endocode/fleet/issues/events{/number}","events_url":"https://api.github.com/repos/endocode/fleet/events","assignees_url":"https://api.github.com/repos/endocode/fleet/assignees{/user}","branches_url":"https://api.github.com/repos/endocode/fleet/branches{/branch}","tags_url":"https://api.github.com/repos/endocode/fleet/tags","blobs_url":"https://api.github.com/repos/endocode/fleet/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/endocode/fleet/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/endocode/fleet/git/refs{/sha}","trees_url":"https://api.github.com/repos/endocode/fleet/git/trees{/sha}","statuses_url":"https://api.github.com/repos/endocode/fleet/statuses/{sha}","languages_url":"https://api.github.com/repos/endocode/fleet/languages","stargazers_url":"https://api.github.com/repos/endocode/fleet/stargazers","contributors_url":"https://api.github.com/repos/endocode/fleet/contributors","subscribers_url":"https://api.github.com/repos/endocode/fleet/subscribers","subscription_url":"https://api.github.com/repos/endocode/fleet/subscription","commits_url":"https://api.github.com/repos/endocode/fleet/commits{/sha}","git_commits_url":"https://api.github.com/repos/endocode/fleet/git/commits{/sha}","comments_url":"https://api.github.com/repos/endocode/fleet/comments{/number}","issue_comment_url":"https://api.github.com/repos/endocode/fleet/issues/comments{/number}","contents_url":"https://api.github.com/repos/endocode/fleet/contents/{+path}","compare_url":"https://api.github.com/repos/endocode/fleet/compare/{base}...{head}","merges_url":"https://api.github.com/repos/endocode/fleet/merges","archive_url":"https://api.github.com/repos/endocode/fleet/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/endocode/fleet/downloads","issues_url":"https://api.github.com/repos/endocode/fleet/issues{/number}","pulls_url":"https://api.github.com/repos/endocode/fleet/pulls{/number}","milestones_url":"https://api.github.com/repos/endocode/fleet/milestones{/number}","notifications_url":"https://api.github.com/repos/endocode/fleet/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/endocode/fleet/labels{/name}","releases_url":"https://api.github.com/repos/endocode/fleet/releases{/id}","deployments_url":"https://api.github.com/repos/endocode/fleet/deployments","created_at":"2015-04-15T15:16:33Z","updated_at":"2016-01-27T08:29:06Z","pushed_at":"2016-09-06T13:43:57Z","git_url":"git://github.com/endocode/fleet.git","ssh_url":"git@github.com:endocode/fleet.git","clone_url":"https://github.com/endocode/fleet.git","svn_url":"https://github.com/endocode/fleet","homepage":"","size":12521,"stargazers_count":0,"watchers_count":0,"language":"Go","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":1,"mirror_url":null,"open_issues_count":0,"forks":1,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"coreos:master","ref":"master","sha":"8c9401b0ce35b210d68dbd5d63ce61311ce4c483","user":{"login":"coreos","id":3730757,"avatar_url":"https://avatars.githubusercontent.com/u/3730757?v=3","gravatar_id":"","url":"https://api.github.com/users/coreos","html_url":"https://github.com/coreos","followers_url":"https://api.github.com/users/coreos/followers","following_url":"https://api.github.com/users/coreos/following{/other_user}","gists_url":"https://api.github.com/users/coreos/gists{/gist_id}","starred_url":"https://api.github.com/users/coreos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coreos/subscriptions","organizations_url":"https://api.github.com/users/coreos/orgs","repos_url":"https://api.github.com/users/coreos/repos","events_url":"https://api.github.com/users/coreos/events{/privacy}","received_events_url":"https://api.github.com/users/coreos/received_events","type":"Organization","site_admin":false},"repo":{"id":13512328,"name":"fleet","full_name":"coreos/fleet","owner":{"login":"coreos","id":3730757,"avatar_url":"https://avatars.githubusercontent.com/u/3730757?v=3","gravatar_id":"","url":"https://api.github.com/users/coreos","html_url":"https://github.com/coreos","followers_url":"https://api.github.com/users/coreos/followers","following_url":"https://api.github.com/users/coreos/following{/other_user}","gists_url":"https://api.github.com/users/coreos/gists{/gist_id}","starred_url":"https://api.github.com/users/coreos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coreos/subscriptions","organizations_url":"https://api.github.com/users/coreos/orgs","repos_url":"https://api.github.com/users/coreos/repos","events_url":"https://api.github.com/users/coreos/events{/privacy}","received_events_url":"https://api.github.com/users/coreos/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/coreos/fleet","description":"A Distributed init System","fork":false,"url":"https://api.github.com/repos/coreos/fleet","forks_url":"https://api.github.com/repos/coreos/fleet/forks","keys_url":"https://api.github.com/repos/coreos/fleet/keys{/key_id}","collaborators_url":"https://api.github.com/repos/coreos/fleet/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/coreos/fleet/teams","hooks_url":"https://api.github.com/repos/coreos/fleet/hooks","issue_events_url":"https://api.github.com/repos/coreos/fleet/issues/events{/number}","events_url":"https://api.github.com/repos/coreos/fleet/events","assignees_url":"https://api.github.com/repos/coreos/fleet/assignees{/user}","branches_url":"https://api.github.com/repos/coreos/fleet/branches{/branch}","tags_url":"https://api.github.com/repos/coreos/fleet/tags","blobs_url":"https://api.github.com/repos/coreos/fleet/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/coreos/fleet/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/coreos/fleet/git/refs{/sha}","trees_url":"https://api.github.com/repos/coreos/fleet/git/trees{/sha}","statuses_url":"https://api.github.com/repos/coreos/fleet/statuses/{sha}","languages_url":"https://api.github.com/repos/coreos/fleet/languages","stargazers_url":"https://api.github.com/repos/coreos/fleet/stargazers","contributors_url":"https://api.github.com/repos/coreos/fleet/contributors","subscribers_url":"https://api.github.com/repos/coreos/fleet/subscribers","subscription_url":"https://api.github.com/repos/coreos/fleet/subscription","commits_url":"https://api.github.com/repos/coreos/fleet/commits{/sha}","git_commits_url":"https://api.github.com/repos/coreos/fleet/git/commits{/sha}","comments_url":"https://api.github.com/repos/coreos/fleet/comments{/number}","issue_comment_url":"https://api.github.com/repos/coreos/fleet/issues/comments{/number}","contents_url":"https://api.github.com/repos/coreos/fleet/contents/{+path}","compare_url":"https://api.github.com/repos/coreos/fleet/compare/{base}...{head}","merges_url":"https://api.github.com/repos/coreos/fleet/merges","archive_url":"https://api.github.com/repos/coreos/fleet/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/coreos/fleet/downloads","issues_url":"https://api.github.com/repos/coreos/fleet/issues{/number}","pulls_url":"https://api.github.com/repos/coreos/fleet/pulls{/number}","milestones_url":"https://api.github.com/repos/coreos/fleet/milestones{/number}","notifications_url":"https://api.github.com/repos/coreos/fleet/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/coreos/fleet/labels{/name}","releases_url":"https://api.github.com/repos/coreos/fleet/releases{/id}","deployments_url":"https://api.github.com/repos/coreos/fleet/deployments","created_at":"2013-10-11T23:56:38Z","updated_at":"2016-09-06T14:21:52Z","pushed_at":"2016-09-06T13:43:58Z","git_url":"git://github.com/coreos/fleet.git","ssh_url":"git@github.com:coreos/fleet.git","clone_url":"https://github.com/coreos/fleet.git","svn_url":"https://github.com/coreos/fleet","homepage":"","size":14350,"stargazers_count":2285,"watchers_count":2285,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":287,"mirror_url":null,"open_issues_count":120,"forks":287,"open_issues":120,"watchers":2285,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/coreos/fleet/pulls/1667"},"html":{"href":"https://github.com/coreos/fleet/pull/1667"},"issue":{"href":"https://api.github.com/repos/coreos/fleet/issues/1667"},"comments":{"href":"https://api.github.com/repos/coreos/fleet/issues/1667/comments"},"review_comments":{"href":"https://api.github.com/repos/coreos/fleet/pulls/1667/comments"},"review_comment":{"href":"https://api.github.com/repos/coreos/fleet/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/coreos/fleet/pulls/1667/commits"},"statuses":{"href":"https://api.github.com/repos/coreos/fleet/statuses/6a64a4e3b7d3d988747cf74a765657cc75ec7df8"}}}}
{"actor":{"display_login":"jonboulle"}}