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

coreos / fleet

fleet ties together systemd and etcd into a distributed init system

Apache License 2.0

2.42k stars 302 forks source link

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

Closed dongsupark closed 7 years ago

dongsupark commented 8 years ago

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()

Fixes https://github.com/coreos/fleet/issues/1675

dongsupark commented 8 years ago

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 https://github.com/coreos/fleet/pull/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 https://github.com/coreos/fleet/pull/1673 and https://github.com/coreos/fleet/pull/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 commented 8 years ago

Added necesssary unit tests too.

jonboulle commented 8 years ago

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

dongsupark commented 8 years ago

@jonboulle Yes. I think we should first have a look into https://github.com/coreos/fleet/pull/1673 and https://github.com/coreos/fleet/pull/1667 to merge those PRs.

dongsupark commented 8 years ago

Rebased on top of master, which already includes https://github.com/coreos/fleet/pull/1667. So this PR depends only on https://github.com/coreos/fleet/pull/1673.

dongsupark commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

dongsupark commented 7 years ago

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.

dongsupark commented 7 years ago

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.