fleetctl: check systemd active state via client API #1667

dongsupark · 2016-08-19T14:14:38Z

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.

jonboulle · 2016-08-23T11:38:23Z

I don't think it's ever really sane to fall back to D-Bus, is it?

dongsupark · 2016-08-23T12:18:06Z

@jonboulle Good question.
In the beginning I also tried to get it working only via cAPI. But it didn't work as expected. For some units it's reported to be not active, which made functional tests fail. So I had to fall back to checking via DBUS too, to get it working. I'd also like to know the reason.

jonboulle · 2016-08-30T12:51:34Z

@dongsupark could you link to an example of test failures in that case?

dongsupark · 2016-08-30T13:50:30Z

@jonboulle
Reverted fallback, printed out more debug messages:
https://gist.github.com/dongsupark/c444e10ad843090cc45750b85db51d7e

dongsupark · 2016-08-31T13:16:24Z

Finally I fixed a bug, and force-pushed.

It turns out, checking states via client API has been already working correctly, and "fleetctl list-units" has actually shown correct output. The only broken thing was the way of using loop around waitForSystemdActiveState(), where it didn't actually repeatedly try to retrieve states. So I fixed the logic to make it try to fetch the states repeatedly up to 15 seconds. The fallback routine to checking DBUS connection was removed.

Please have a look. Thanks.

jonboulle · 2016-09-06T12:41:52Z

fleetctl/fleetctl.go

@@ -1067,78 +1072,87 @@ func assertUnitState(name string, js job.JobState, out io.Writer) (ret bool) {
 // tryWaitForSystemdActiveState tries to wait for systemd units to reach an
 // active state, making use of the systemd's DBUS interface. First it takes one


This docstring needs updating now

@jonboulle Done. Thanks for spotting it.

jonboulle · 2016-09-06T17:16:40Z

fleetctl/fleetctl.go

+
+// getSingleUnitState returns a single uState of type suState, which consists
+// of necessary systemd states, only for a given unit name.
+func getSingleUnitState(unitName string) (suState, error) {


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.

Two suggestions:

introduce a UnitState.Get call (similar to how, for units themselves, we have both Unit.Get and Unit.List)
or,

use a single UnitState.List call and share the results across all goroutines (in waitForUnitStates)

thoughts?

@jonboulle
First of all, getSingleUnitState() is being called only by waitForSystemdActiveState(), not by waitForUnitStates(). So it's not true that getSingleUnitState() is called from multiple goroutines, because waitForSystemdActiveState() simply iterates over units in a sequential way.

On the other hand, your concern is still valid, because it's basically not efficient to call cAPI.UnitStates() for each unit. I think I'm going to choose the 2nd option, which is calling only a single UnitState.List to share its results for all units. Let me see.

Thanks for pointing it out! 👍

@dongsupark ah! good point, sorry about that. But now you've pointed that out - wouldn't it make more sense to unify the behaviour of the two so that they're both parallelised? :-)

@jonboulle
I pushed a new commit to call cAPI.UnitStates() only once. Please have a look.

I actually tried to also introduce cAPI.UnitState(), to allow this code to fetch a single unit status. However, that's definitely a lot of work all over the place in fleet. (schema, etcdRegistry, rpcRegistry, etc) So I'll do that in the future, creating another PR.

As for using goroutines also for waitForSystemdActiveState(), I'll try to do that.

@jonboulle Force-pushed the commit, after changing the logic from sequential iteration to goroutines.

jonboulle · 2016-09-16T15:17:33Z

fleetctl/fleetctl.go

+func checkSystemdActiveState(apiStates []*schema.UnitState, name string, maxAttempts int, wg *sync.WaitGroup, errchan chan error) {
+	defer wg.Done()
+
+	if maxAttempts < 1 {
 		for {


Not particularly fond of the duplication of these loop bodies - can we instead do something more clever with the loop conditional to collapse them into a single for?

@jonboulle Good point. I just split out the common part into assertFetchSystemdActiveState(), and let checkSystemdActiveState() go into a single loop. Such duplication was just a result of copy-and-paste.

jonboulle · 2016-09-16T15:21:23Z

Looks pretty good overall

To be as idiomatic as possible, do so: * make tryWaitForSystemdActiveState() return error instead of int * pass getBlockAttempts(cCmd) to tryWaitForSystemdActiveState() to handle such cases of --no-block or --block-attempts. * make assertSystemdActiveState(), waitForSystemdActiveState() return just error, not with 'found', as it's unnecessary. * remove unnecessary checking against conn == nil. * compare strings with '!=' instead of strings.Compare() != 0

Now that tryWaitForSystemdActiveState() returns error instead of int, tryWaitForUnitStates() should also return error instead of int, to be as idiomatic as possible.

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 introduce a check using cAPI.UnitStates(), to check states on other machines.

dongsupark · 2016-09-19T12:24:54Z

@jonboulle Now I suppose everything was addressed. I'll merge it soon.

As it's basically not efficient to call cAPI.UnitStates() for each unit, let's call it only once outside goroutines for processing units, i.e. calling in waitForSystemdActiveState(), not in getSingleUnitState(). To do that, we need to share the result apiStates across all units. Also make use of goroutines in waitForSystemdActiveState(), instead of sequential iteration over each unit. Note that we still need to keep calling UnitStates() when assertSystemdActiveState() failed, because in the next attempt the old apiState is not necessarily going to be valid. Ideally in that case, we should be able to fetch the state only for a single unit. However, we cannot do that for now, because cAPI.UnitState() is not available. In the future we would need to implement cAPI.UnitState() and all dependendent parts all over the tree in fleet, e.g. schema, etcdRegistry, rpcRegistry, etc. to replace UnitStates() in this place with the new method UnitState(). In practice, calling UnitStates() here is not as badly inefficient as it looks, because it will be anyway rarely called only when the assertion failed.

dongsupark added kind/enhancement area/api component/fleetctl labels Aug 19, 2016

dongsupark self-assigned this Aug 19, 2016

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch 3 times, most recently from 839d97f to 0cb4369 Compare August 19, 2016 15:22

dongsupark force-pushed the master branch 7 times, most recently from 3b60e93 to 875d938 Compare August 23, 2016 11:00

dongsupark force-pushed the master branch 2 times, most recently from bdc94e8 to fa5aa3a Compare August 30, 2016 12:26

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from ffecd1e to ee5585d Compare August 31, 2016 13:05

dongsupark changed the title ~~fleetctl: check systemd active state also with client API~~ fleetctl: check systemd active state via client API Aug 31, 2016

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch 2 times, most recently from 4712b14 to 88a0a30 Compare August 31, 2016 14:03

jonboulle reviewed Sep 6, 2016
View reviewed changes

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from 88a0a30 to 6a64a4e Compare September 6, 2016 13:43

jonboulle reviewed Sep 6, 2016
View reviewed changes

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch 2 times, most recently from d7da0fc to d919ee8 Compare September 7, 2016 15:26

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from d919ee8 to 32a88e2 Compare September 8, 2016 07:59

This was referenced Sep 8, 2016

Introduce cAPI.UnitState() for a single unit #1675

Closed

api: support cAPI.UnitState() for a single unit #1679

Merged

jonboulle reviewed Sep 16, 2016

View reviewed changes

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from 32a88e2 to 7ce7460 Compare September 19, 2016 11:50

Dongsu Park added 3 commits September 19, 2016 13:50

fleetctl: make tryWaitForUnitStates return error instead of int

57b583f

Now that tryWaitForSystemdActiveState() returns error instead of int, tryWaitForUnitStates() should also return error instead of int, to be as idiomatic as possible.

fleetctl: check systemd active state via cAPI.UnitStates()

cc2837f

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 introduce a check using cAPI.UnitStates(), to check states on other machines.

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from 7ce7460 to 413f6e3 Compare September 19, 2016 12:19

dongsupark added the reviewed/lgtm label Sep 19, 2016

dongsupark force-pushed the dongsu/fleetctl-capi-systemd-states branch from 413f6e3 to 988fd84 Compare September 19, 2016 13:38

dongsupark merged commit 49420c5 into coreos:master Sep 19, 2016

dongsupark deleted the dongsu/fleetctl-capi-systemd-states branch September 19, 2016 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fleetctl: check systemd active state via client API #1667

fleetctl: check systemd active state via client API #1667

dongsupark commented Aug 19, 2016 •

edited

Loading

jonboulle commented Aug 23, 2016

dongsupark commented Aug 23, 2016

jonboulle commented Aug 30, 2016

dongsupark commented Aug 30, 2016

dongsupark commented Aug 31, 2016

jonboulle Sep 6, 2016

dongsupark Sep 6, 2016

jonboulle Sep 6, 2016

dongsupark Sep 7, 2016

jonboulle Sep 7, 2016

dongsupark Sep 7, 2016

dongsupark Sep 7, 2016

jonboulle Sep 16, 2016

dongsupark Sep 19, 2016

jonboulle commented Sep 16, 2016

dongsupark commented Sep 19, 2016

		@@ -1067,78 +1072,87 @@ func assertUnitState(name string, js job.JobState, out io.Writer) (ret bool) {
		// tryWaitForSystemdActiveState tries to wait for systemd units to reach an
		// active state, making use of the systemd's DBUS interface. First it takes one

fleetctl: check systemd active state via client API #1667

fleetctl: check systemd active state via client API #1667

Conversation

dongsupark commented Aug 19, 2016 • edited Loading

jonboulle commented Aug 23, 2016

dongsupark commented Aug 23, 2016

jonboulle commented Aug 30, 2016

dongsupark commented Aug 30, 2016

dongsupark commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonboulle commented Sep 16, 2016

dongsupark commented Sep 19, 2016

dongsupark commented Aug 19, 2016 •

edited

Loading