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

dongsupark · 2016-09-09T15:14:58Z

api: support GET method for stateResource
schema: introduce Get method for UnitState
client: introduce UnitState() for API interface
protobuf: define GetUnitState() for registryClient and registryServer
registry: introduce UnitState() for etcdRegistry and RPCRegistry
fleetctl: replace cAPI.UnitStates() with .UnitState()
registry: add new unit tests for GetUnitState() and relevant ones
fleetctl: periodically check for systemd states using waitForState
fleetctl: make use of waitForState also in assertUnitState()

dongsupark · 2016-09-12T11:22:57Z

Fixed bugs.

Made checkSystemdActiveState() retrieve unit state after sleeps, to avoid occasional errors from assertion of active states.
Got rid of getSingleUnitState() to simplify the logic in checkSystemdActiveState().
Introduced methods inmemoryRegistry.UnitState() as well as rpcserver.GetUnitState(). Without these, grpc-enabled tests cannot work at all. It was my fault to have missed these methods. :-(
Let this PR depend also on registry/rpc: use simpleBalancer instead of ClientConn.State() #1673. This part is indeed subtle, because in theory updating vendors gRPC & protobuf would have nothing to do with cAPI.UnitState(). Due to dependency chain through protobuf, however, I cannot manage to make this PR work on the older version of gRPC & protobuf.

My suggestion is to merge first #1673 and #1667. After that, we could take some time to review this PR for cAPI.UnitState().

Anyway every functional test should now work as expected. :-)

dongsupark · 2016-09-13T07:45:15Z

Added necesssary unit tests too.

jonboulle · 2016-09-13T12:30:28Z

should I hold off until #1667 #1673 before taking a look?

dongsupark · 2016-09-13T12:36:00Z

@jonboulle Yes.
I think we should first have a look into #1673 and #1667 to merge those PRs.

dongsupark · 2016-09-19T15:10:54Z

Rebased on top of master, which already includes #1667. So this PR depends only on #1673.

dongsupark · 2016-10-12T11:29:33Z

I managed to get this PR decoupled from #1673. Now it's working even without updating gRPC altogether.

At the beginning it didn't work with old gRPC, because for some reason the new method GetUnitState() was introduced only to RegistryClient interface, but not to RegistryServer. That's why gRPC connections failed with messages like "unknown method GetUnitState". With GetUnitState() included in RegistryServer interface, it's working fine.

It's ready for review.

dongsupark · 2016-11-04T11:31:54Z

Although it contains many lines of codes, I think its logic is relatively straightforward.
I'm going to merge it in the next week.

jonboulle · 2016-11-07T16:16:48Z

fleetctl/fleetctl.go

-	apiStates, errU = cAPI.UnitStates()
-	if errU != nil {
-		return fmt.Errorf("Error retrieving list of units: %v", errU)
+	if _, err := cAPI.UnitState(name); err != nil {


I'm missing something, what is the point of the sleep + this call here?

@jonboulle Yeah, I'd also like to remove them, but without calling the sleep and again UnitState, several functional tests like TestUnitStart seem to fail occasionally. I didn't have a chance to find out the reason.
Let me think how I can avoid such errors with calling UnitState just once.

jonboulle · 2016-11-07T16:17:06Z

fleetctl/fleetctl.go

 	if err != nil {
-		return err
+		return fmt.Errorf("Error retrieving list of units: %v", err)


this needs an update

@jonboulle Will do.

jonboulle · 2016-11-07T16:22:55Z

A couple of questions about the fleetctl code, but overall this looks really great, thanks!

HTTP client should be able to retrieve a unit state via URL like http://*/state/unitName. Support GET for stateResource just like unitResource.

Define Get method for UnitState in the schema, and regenerate both json and go.

Introduce UnitState() for API interface as well as HTTPClient struct.

Protobuf must also its own method GetUnitState().

UnitState() needs to be defined for etcdRegistry, RPCRegistry, and *RegistryMux*. Also inmemoryRegistry() needs to have UnitState().

Now fleetctl should be able to retrieve a unit state, using cAPI.UnitState(). Also get rid of getSingleUnitState() to simplify the logic.

We should add new unit tests like TestUnitState() as well as TestInMemoryUnitState(), to cover the new cases like EtcdRegistry.UnitState() or inmemoryRegistry.UnitState().

dongsupark · 2016-11-08T13:00:47Z

Rebased, and pushed 2 more commits, "fleetctl: periodically check for systemd states using waitForState" and "fleetctl: make use of waitForState also in assertUnitState()".
I came up with an idea of making use of waitForState, just like that in functional tests. Now the ugly sleeps can be removed.

jonboulle · 2016-11-08T16:57:28Z

fleetctl/fleetctl.go

+// If it cannot get the expected states within the period, return error.
+func assertSystemdActiveState(unitName string) error {
+	var errbuf error
+	fetchSystemdActiveState := func() bool {


Why not just have this func() err and propagate the error instead of using this buffer variable?

@jonboulle Done.

jonboulle · 2016-11-09T13:47:21Z

fleetctl/fleetctl.go

+		case <-alarm:
+			return timeout, fmt.Errorf("Failed to fetch systemd active states within %v", timeout)
+		case <-ticker:
+			isChecked, err := stateCheckFunc()


Sorry to keep nitting on this, but I might not have explained clearly: err will always be nil when isChecked is true, right? which means that you can just remove the bool value and rely on the nil-ness of the error :-)

@jonboulle Yeah right. It's done.
The return type of waitForState was like that because of copy-and-paste from functional tests. So I'm now thinking, maybe it would make sense to convert the return type of waitForState also in functional tests: error instead of bool. TODO.

Although checkSystemdActiveState() doesn't have to depend on cAPI.UnitStates(), it's still impossible to remove additional sleep in case of error from assertSystemdActiveState(). That's actually a known issue. Sometimes it simply takes much time until fleetctl became able to get valid unit states. Adding additional sleeps or tuning the sleep time wouldn't be a good approach, as an optimal sleep interval could vary a lot case by case. To gracefully handle this case, let's do similar checking as done in functional tests. * Introduce a new helper waitForState(), just like util.WaitForState() from functional tests. * Squash assertFetchSystemdActiveState() into assertSystemdActiveState() to make it retry the assertion periodically up to defaultSleepTime. * Increase defaultSleepTime from 500 to 2000 msec. * Remove the additional sleep call as well as cAPI.UnitState() call.

waitForUnitState() has been making use of an additional sleep, in case assertUnitState() returned error. Now that a helper waitForState() is available for fleetctl, we don't need to sleep there. So remove the sleep calls, and instead make assertUnitState() a wrapper of waitForState() over fetchUnitState(). That way, assertUnitState() can periodically retry to get a unit, up to defaultSleepTime. This is also good for consistency in the entire code, as assertSystemdActiveState() already depends on waitForState().

dongsupark · 2016-11-10T09:56:34Z

I suppose everything was addressed. I'm merging it.
In case of any other issues, feel free to comment. Nitpicking is also welcome. :-) I would then create another PR to fix it.

dongsupark added the area/api label Sep 9, 2016

dongsupark self-assigned this Sep 9, 2016

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch 2 times, most recently from e1b436b to 0454b49 Compare September 12, 2016 11:19

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch 2 times, most recently from cfaf321 to fc18d4e Compare September 12, 2016 11:57

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from 0ba8e0f to 94f2b69 Compare September 13, 2016 08:02

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from 94f2b69 to 4caeaf6 Compare September 19, 2016 14:59

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch 2 times, most recently from 6d42b12 to e541e1a Compare September 20, 2016 07:37

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from e541e1a to f674d6f Compare October 12, 2016 11:01

dongsupark added the needs review label Oct 12, 2016

dongsupark changed the title ~~[WIP] api: support cAPI.UnitState() for a single unit~~ api: support cAPI.UnitState() for a single unit Oct 12, 2016

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch 2 times, most recently from 6d83f94 to 70510b3 Compare October 21, 2016 13:44

jonboulle reviewed Nov 7, 2016

View reviewed changes

Dongsu Park added 5 commits November 7, 2016 17:31

api: support GET method for stateResource

76c43f9

HTTP client should be able to retrieve a unit state via URL like http://*/state/unitName. Support GET for stateResource just like unitResource.

schema: introduce Get method for UnitState

826f9c5

Define Get method for UnitState in the schema, and regenerate both json and go.

client: introduce UnitState() for API interface

ead9372

Introduce UnitState() for API interface as well as HTTPClient struct.

protobuf: define GetUnitState() for registryClient

7afdc1d

Protobuf must also its own method GetUnitState().

registry: introduce UnitState() for etcdRegistry and RPCRegistry

43e23ef

UnitState() needs to be defined for etcdRegistry, RPCRegistry, and *RegistryMux*. Also inmemoryRegistry() needs to have UnitState().

Dongsu Park added 2 commits November 7, 2016 17:31

fleetctl: replace cAPI.UnitStates() with .UnitState()

2133db7

Now fleetctl should be able to retrieve a unit state, using cAPI.UnitState(). Also get rid of getSingleUnitState() to simplify the logic.

registry: add new unit tests for GetUnitState() and relevant ones

8aa001f

We should add new unit tests like TestUnitState() as well as TestInMemoryUnitState(), to cover the new cases like EtcdRegistry.UnitState() or inmemoryRegistry.UnitState().

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from 70510b3 to 08891bf Compare November 8, 2016 12:54

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from 08891bf to bb33268 Compare November 8, 2016 13:32

jonboulle reviewed Nov 8, 2016

View reviewed changes

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch from bb33268 to 2c014f0 Compare November 8, 2016 22:10

jonboulle reviewed Nov 9, 2016

View reviewed changes

dongsupark force-pushed the dongsu/fleetctl-capi-unitstate-single branch 2 times, most recently from b438a2c to 9137746 Compare November 9, 2016 14:12

dongsupark added reviewed/lgtm and removed needs review labels Nov 10, 2016

dongsupark merged commit 97a7adb into coreos:master Nov 10, 2016

dongsupark deleted the dongsu/fleetctl-capi-unitstate-single branch November 10, 2016 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dongsupark commented Sep 9, 2016 •

edited

Loading

dongsupark commented Sep 12, 2016 •

edited

Loading

dongsupark commented Sep 13, 2016

jonboulle commented Sep 13, 2016

dongsupark commented Sep 13, 2016

dongsupark commented Sep 19, 2016

dongsupark commented Oct 12, 2016 •

edited

Loading

dongsupark commented Nov 4, 2016

jonboulle Nov 7, 2016

dongsupark Nov 7, 2016

jonboulle Nov 7, 2016

dongsupark Nov 7, 2016

jonboulle commented Nov 7, 2016

dongsupark commented Nov 8, 2016 •

edited

Loading

jonboulle Nov 8, 2016

dongsupark Nov 8, 2016

jonboulle Nov 9, 2016

dongsupark Nov 9, 2016

dongsupark commented Nov 10, 2016

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

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

Conversation

dongsupark commented Sep 9, 2016 • edited Loading

dongsupark commented Sep 12, 2016 • edited Loading

dongsupark commented Sep 13, 2016

jonboulle commented Sep 13, 2016

dongsupark commented Sep 13, 2016

dongsupark commented Sep 19, 2016

dongsupark commented Oct 12, 2016 • edited Loading

dongsupark commented Nov 4, 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

jonboulle commented Nov 7, 2016

dongsupark commented Nov 8, 2016 • edited Loading

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

dongsupark commented Nov 10, 2016

dongsupark commented Sep 9, 2016 •

edited

Loading

dongsupark commented Sep 12, 2016 •

edited

Loading

dongsupark commented Oct 12, 2016 •

edited

Loading

dongsupark commented Nov 8, 2016 •

edited

Loading