On November 16th, 2015, 3:19 p.m. UTC, Nils Philippsen wrote:

src/rolekit/server/io/rolesettings.py (Diff revision 2)
29	from rolekit.util import validate_name, get_target_unit_name	30	from rolekit.util import validate_name

This bogus import comes from review #226. Please remove it in that commit.

Done.

On November 16th, 2015, 3:19 p.m. UTC, Nils Philippsen wrote:

src/rolekit/server/io/systemd.py (Diff revision 2)
		63	def monitor_unit_state(unit, ):
		64	"""
		65	Monitor for changes in the ActiveState property of a unit
		66	:param unit: The systemd unit to monitor
		67	:param settings: A dictionary of settings with a key named "state" to update
		68	:return: None (Throws exceptions on errors)
		69	"""

That's pretty minimalistic. Are you sure you don't want to put this in a later commit that adds a little code? ;)

This was leftover from a first implementation that I ultimately scrapped but forgot to remove. New version of the patch corrects this.

On November 16th, 2015, 3:19 p.m. UTC, Nils Philippsen wrote:

src/rolekit/server/rolebase.py (Diff revision 2)
		130	elif self._settings["state"] == SYSTEMD:
		131	# We need to get the current state from systemd
		132	self._settings["state"] = target_unit_state(self.target_unit)

I'm not sure I'm too happy with overloading the state this way. Just from looking at the code, I think it should work but I still think it would be cleaner if we kept the state as SYSTEMD so it's explicit that one needs to consult systemd (whether by listening to signals or polling) to find out the state of the target. AIUI, this state gets saved to the JSON file in /etc/rolekit/roles, and I'd like it better if none of "must consult systemd" states are ever persisted there.

OK, so it turns out that there was actually only one case where it would get written out to disk: in the deferred role processing. I've modified that code so that it will always explicitly set the state to NASCENT before writing out the deferred settings JSON. I then modified rolesettings.py to only set the SYSTEMD state if something other than NASCENT or ERROR is present in the settings data.
I've examined the rest of the code and with the exception of deferred processing, settings.write() is never called anywhere before change_state(), so SYSTEMD should never appear in the file.

- Stephen

On November 12th, 2015, 9:21 p.m. UTC, Stephen Gallagher wrote:

Get role state from systemd