I performed a different experiment, this time we had 6 inactive units per host with a total of 18 inactive units in the whole cluster. We used our benchmarking tool called fleemmer (as done above) and deployed 900 units, which were stopped/destroy after a fixed timeout.

CPU usage WITHOUT unit state cache:

CPU usage using our implementation of an UNIT STATE CACHE:

CONCLUSION: There is a slight increment in the CPU usage due to the inactive units approx. 7-8% CPU usage increment. However, the CPU usage always increases in a higher percentage when scheduling units how shown in the spikes.

@hectorj2f thank you very much for the detailed information, we will read it again carefully and try to push it forward.

For the moment I'm just a bit curious, you say "We decided to build a cache to reduce the amount of calls from systemd to dbus. Fleet periodically performs operations to gather the state of the units in the cluster, so for each inactive unit located in a host, a read operation is triggered to obtain the state from dbus. Therefore, you could have an impression of how this operation could use more resources than the desired." and later you also say that it can be solved by updating go-dbus, for me that's the best solution. Your patches seem clean and they don't touch lot of code. However I would first ask if we can support notification from systemd ? subscribe() then get UnitNew() and UnitRemoved() signals https://www.freedesktop.org/wiki/Software/systemd/dbus/ systemd supports notification and it should not trigger any load at all... is this currently not possible ?

Thank you!

@tixxdz Subscriptions are already used in the function GetUnitStates() of systemd/manager.go. In the code, fleet defines subscribers for any instance to get its status update, e.g. when running async operations. However those subscribers are just for loops that use systemd-dbus to get the current state (look at SubscribeUnitsCustom function in go-dbus). This is done to be aware of any state change.

The dbus Subscribe() function you mentioned, it reacts against any systemd dbus event. I believe it is not what we would want. We want to define subscribers for specific fleet units, (similar to what subscriptor set offers) but in a smart way. The subscribers should be asynchronous to report whenever a state changes instead of done via for loops querying all the states and filtering the units.

Indeed, my long-term proposal was to refactor go-dbus to report state updates only when the state changes and not by constantly querying its state.

For a little history: this was previously purely event-driven - similar to how we previously used etcd - but as with etcd, we could end up missing events and have no way to sanely recover, so we ended up moving this to the reconciliation model as well.

@jonboulle Thanks for the information :). Then it makes more sense to me cause it made it better regarding data consistency and failure recovery. Unfortunately we also found some situations on which fleet had a different state than the one in systemd, but that is another issue.

@jonboulle Do you think the current solution could have similar problems ? or do you refer to the implementation of dbus watchers ?

This PR aims to reduce the workload caused by this periodic unit state calls for all the subscribed units. @tixxdz the state of the subscribed units is collected via dbus ListUnits calls. Moreover for all those units that don't appear in the ListUnits call, an explicit getProperties dbus call is triggered for each unit.

Do you see any blocker or potential issue coming out when using this caching approach @jonboulle ?

Hi @hectorj2f
In the mean time could you please force push your branch: giantswarm:unit_state_cache ? so the pull request is updated which will trigger the functional tests.

Thank you!

@tixxdz Done. However there is an error that seems to be unrelated :/ .
https://travis-ci.org/coreos/fleet/jobs/117506419

ping @tixxdz

@hectorj2f yes thank you for the update! the failing is due to travis-ci. Hopefully we will give it more time next week as it is in the 0.13 priority.

Thanks!

@tixxdz test are passing, what's next ?

@hectorj2f to be honest, I think the main hurdle is that we are still pretty new to this, and it's hard for any of us to judge whether such a change is indeed a good idea... (While @jonboulle doesn't really have the time to think about this too much I guess.)

I haven't really looked into this myself; one thing that comes to mind though is that AIUI the cache gets out of sync if a unit state changes because of "external" factors, not at a direct request of fleet?... How does that deal with crashing units for example, or unit dependencies fleet is not aware of?

BTW, this is a bit tangential, but there is one bit in your original message I do not agree with: "Obviously we declined this solution as it required the modification of third party software used by fleet." To quote a certain Lennart Poettering: "You own the entire stack." This is free software -- you never have to take limitations in underlying infrastructure as granted, and work around them. Fix problems at the source.

@hectorj2f hi sorry for delay, we had to handle some other issues, at the same time I'm reviewing this issue #478 since it relates the one exposed here, just trying to get the whole context from your comments and @jonboulle ones ;-)

@tixxdz Thanks

How does that deal with crashing units for example, or unit dependencies fleet is not aware of?

@antrik Well, that has a very simple answer. This cache won't interfere as the state of the systemd units remains with the same logic than before. Please have a look on line 242 in systemd/manager.go. This cache aims to reduce the workload of the inactive units.

@tixxdz PRs seem to fail. Please, ping me when it is fixed to push it again. Thx

@hectorj2f just an update, we are making the functional tests run when state cache is set and not, will merge the functional test change first, then later this PR. I may add some minor comments.

Thank you!

So more notes:

1. Did you investigate if you put the transition of states inside say
   systemdUnitManager.cache_hashes { hashes map[string]unit.Hash ; state string => {active or inactive }

So we have a new cache_hashes with two fields where we unload the unit we don't remove it but we put the state into the inactive state, the other code should check if the state is always active and the addition should check if the state is inactive. This will allow us to use the same cache but in the other way I'm not sure if it will really scale ? cause with the current implementation we just forget about the old cache https://github.com/coreos/fleet/pull/1418/files#diff-561bbb797eb2f941c1384271efe28c0bR273 which will be collected and the registerUnitOperation cache is also updated where it's appropriate, this of course saves memory but at the end we may have three hashes running at the same time, may be we can achieve the same with two or one hash which will survive the calls; one used inside the functions as a temporary hash to update the final one ?

2. Would it be possible to have some benchmark comparison with more inactive units, a higher number ? Just to make sure before merging.

3. It would be nice if we put the details of the logic inside functions, say instead of:
   if m.enableUnitStateCache { delete(m.unitStateCache.registerUnitOperation, name) }

func (m *systemdUnitManager) cacheUpdateActiveUnits(blabla....) { if !m.enableUnitStateCache { return } .... continue ....}

It will allows us to hide the logic and maybe improve the cache later ?

So 1) needs further thinking, do you think it makes sense ? I need more time to came up with something that can work, otherwise just use what's already here and improve it later.

And for 2) and 3) it would be awesome to update the PR based on these! and just matter of time before we merge this ;-)

Thanks for your patience.

Also, squashing the commits where it's possible would be nice. btw we merged more functional tests for unit states these past days, I guess that's enough or do you think that there are corner cases that can be added to assert the functionality ?

@tixxdz I like the idea but I need to think about the corner cases.

Regarding 2) and 3), they are feasible and understandable :).

@hectorj2f for 1) if it's too much, then please don't bother. Without it, the feature will be an option disabled by default, later if it can be done then it could be enabled by default for every one. At the same time if you use or depend on this PR #1556 would be nice to test it, confirm.

Thank you!

@hectorj2f I'm not quite sure, but I suppose this improvement should be done in low-level side. I've started to implement systemd-dbus optimizations withing this PR: #1564

@kayrus My concern are mainly the inactive - dead units. I'll test your changes at the end of this week, and let you to know if there are any improvements. I am currently quite busy with other stuff :-/.

@hectorj2f I've reworked my patch a bit and created systemd pull request. If you wish to test systemd v225 which is currectly used by stable CoreOS, please use this patch: endocode/systemd@1fdb12b

I'll try to implement new d-bus method which will optimize getUnitState soon.

After talking with @kayrus, I think this PR might get obsolete once #1564 is merged. I'd like to test it once that is done.

Let's consider this on hold in favour of #1564