Standardize node_server config data
Closed this issue · 35 comments
Since we currently can not set parameters in the polyglot web interface, we need a standard way for node_servers to deal with configuration data like username and password of the device it is controlling.
I needed the same for my camera server, so I have a get_cam_config method in:
https://github.com/jimboca/ISYHelperPolyglot/blob/master/node_servers/camera/camera.py
which creates/reads a config.yaml in the sandbox.
So I would like to propose creating a method in NodeServer or SimpleNodeServer to grab this data. Hopefully it can be written in such a way so when data is added to the web interface, the same method can be used.
I have not completely thought through how this will work, but wanted to put it out here for discussion.
Hi Jim,
Aren't these stored in ISY?
With kind regards,
Michel
@mkohanim How do we store strings in the ISY? I didn't think that was possible (yet)?
Hi Jim,
The question is whether or not configuration information should be stored/configured in Polyglot. We had decided that we would store those in ISY rather than Polyglot. So, in what cases do we want to let the user actually configure something on Polyglot?
With kind regards,
Michel
@mkohanim If storing strings on the ISY is something that can happen in the near future, then we can forget about it and just wait. In the meantime, a couple of us had to implement a way to handle this and of course we did it differently, so was just looking for a common way to handle it for now.
Regarding whether or not configuration information should be stored/configured in Polyglot: It would be ideal if all configuration information COULD be stored in the ISY, but alas that's not currently possible - and it's likely that it won't be possible for all node servers no matter what UDI or Polyglot implements.
I think that everyone would prefer that nodes exposed by Polyglot act just like common Insteon nodes or Z-wave nodes, in that configuration of the node should happen with something like an "options" menu on the ISY, or the "params" menu for z-wave devices on the ISY. Nobody wants to go to a separate place (physically or with a browser ) to do that. However, reality is troublesome -- for example, there are number of Insteon settings that cannot be set by the ISY (requiring the user to physically access the device and push buttons and watch LEDs in some magical/mystical sequence to turn on/off some feature). My point is that there will certainly be similar cases where a node server will require some 'fiddling' locally to make it work. We can anticipate as many of the needs as we can brainstorm, but no matter what we do, we'll miss something, and the end result will be that somebody somewhere will be forced to implement some sort of local configuration anyway. So IMO we might as well have some pre-defined mechanism to do so, so that users at least have a common place to go to do this, when required.
For the moment, the issue is that one cannot provide, for example, a username and password via an ISY node definition -- which is why, for example, the Kodi node server as currently implemented only works with Kodi servers that have all security features disabled.
I completely agree with everything @mjwesterhof said.
I think maybe there is some confusion regarding the GUI for adding/changing options vs where they are stored. It was never the intention that they be stored on the ISY, but instead would be made accessible between the ISY and the node server through REST APIs.
I agree that for now, having a separate common interface may be useful, but ultimately the best place for a user to manage these options is the ISY GUI (when its fully supported). In general, using the ISY GUI should be the first choice, but I certainly do see the case for a custom configuration UI in the node server i.e. a complex configuration with a lot of real time validation and/or options depending on other options etc.. In other words, something that really couldn't be done using the generic option support in ISY (or even generic support in the node server for that matter).
@cjudi Thanks for the explanation, I think this is making sense now.
So each time a user clicks on the device in the ISY GUI it will query the node server to get these values? Or will it work like the "Options" button on devices like a Insteon Motion sensor or Z-Wave device?
Do you have a timeline on when that will be available? But, like you say, in the meantime we should create a standard way to handle this so all node_server have a common way.
It would work like the options button in Insteon/Z-Wave. I don't have a timeline for it yet, so I think creating a standard interface in the node server would be a good way to go for now. Hopefully, it will just be a simple matter to switch it over using the new REST apis when we get the support added to the ISY.
That being said, should we just do a json config file like the configuration.json for polyglot? If it is in the node_server folder then read it in and allow the nodeserver access to it a la the 'manifest' or something similar?
I was thinking it should be yaml since that is a more human writable format then json?
Should we convert the polyglot config to the same format then?
The polyglot config (configuration.json) may need to be refactored (it mixes polyglot data with node-server-specific data, as well as mixing dynamic and static values). But before we get there, we'll need to add some support in Node and SimpleNodeServer to help separate dynamic from static values - so I don't think we're ready for a complete switch-over of the configuration.json file just yet. So my vote would be yes for YAML, not yet for converting configuration.json, and let's not use the existing manifest mechanism since that would make it a dreadful mess to sort out the data from the two configuration files (because we need to ensure that when the node server updates its config every long_poll seconds it only updates the configuration.json stuff and doesn't mix in the other configuration file's info).
Explain that one to me a little further on the Polyglot configuration.json. Are you wanting to split node_server configuration off into another config file? So we would have (eventually):
configuration.yaml
node_server.yaml
And then the individual node_server in its node_server folder:
config.yaml
Are you also saying we split off the dynamic(I assume you mean drivers) from the static as well?
As two of the major contributors to this project are saying yes to YAML, and I personally don't care, let's go with that for the individual node_servers as of now. Jimbo, do you want to own that or throw it back in the pool to let someone grab?
We will continue this discussion and work on getting the actual polyglot config lined out here as well.
The "config.yaml" file in each node server's unique folder makes sense to me - this is a human-modifiable file, intended to provide configuration information to a given node server because we cannot currently provide that via the ISY's limited UI.
Regarding the existing configuration.json file - for now, let's just completely ignore it as a side topic to be explored separate from the "config.yaml" file issue. But let me explain the issues I've encountered with that file so we all know why I'm suggesting we break it up.
At present the configuration.json file contains the persistent state of all the node servers, plus the ISY configuration information entered by the user into the Polyglot admin web UI. As we know, we had troubles originally with that file getting truncated in certain error conditions. It's actually worse than we thought, although by now I think I have most of the issues that could result in truncation or lost data addressed. Nevertheless, it just feels "fragile" to me - anything going wrong could result in the loss of all the node server data in addition to the Polyglot server's configuration. I keep a regular backup copy handy because it's happened so often to me!
Another issue is that the state maintained by the configuration.json file can result in constant updates, and in some cases those updates are completely unnecessary. In a nutshell, the intent of the manifest/driver interaction in the Node object is that when a node server is started, it sets its driver values to the last known value (e.g. Hue bulb "SE Living RM" is set to "80%" at startup). This is quite reasonable for things that do not get polled quickly and easily, and reasonable for things that do not constantly change. Consider the "heartbeat" node server I'm developing -- I'm planning to expose counters from the host (the value of time.time(), perhaps load average, etc). For these values, it simply makes no sense to save the last known value -- it's trivial to fetch the current value when the node server starts, and since the value changes on each poll(), the configuration.json file will end up being updated (i.e. rewritten) completely each long_poll() interval - with data that will not be used (because it's already stale by the time it's written, and it's absolutely trivial to fetch new values).
Finally, in building and messing with several test and diagnostic node servers, I've noticed that my configuration.json file has accumulated a lot of trash - manifest data for values no longer used in the current version of the node server, as well as entire sections for node servers that are long dead and gone. There's no good way to clear this junk/clutter out. Now, as long as everything is working, the extra stuff is just that - extra stuff - and it doesn't matter. The concern is for the future, and for debugging -- who knows what will happen if we don't have a good way to clean out all the crud.
The logical conclusion I come to is that splitting out the polyglot master config data from each of the node server's config data will minimize the re-writing of a single file, and limit the risk of corruption to a single node server instead of bringing down all of polyglot. It will also facilitate simple instructions to reset a node server entirely (stop the node server, delete the configuration file for that node server, restart and re-configure it from scratch).
As for the needless saving of driver values - that's just an additional setting in the driver list that will permit one to mark a given driver as "do not save", which will signal to the SimpleNodeServer that it should simply write a zero or null for that driver's value into the manifest and leave it to the node server's author to fetch the value at initialization time.
I'll be happy to take on the configuration.json file restructuring at a later date -- it's working right now (I think I found the last case that was erasing Hue bulb nodes some weeks ago). If it turns out that others are observing corruption or inexplicable loss of nodes in their node server after restarts, we can escalate that issue separately.
Great write up. Thank you for the clarification. I concur on all points.
Let's split that off into a separate Issue. I'm going to be in and out for business the next few weeks, so if you'd like to take that new one, that would be great. Or just open it and leave it for whomever.
That all sounds good and thanks for the detailed information since I didn't understand before either. I have been out of town unexpectedly, then on vacation for 2 weeks so I will not be able to work on this. So if someone else can take it, feel free or I can hopefully work on it when I get back because I need it for my camera server at some point.
Ok... I assume we are all good with pyyaml then? I have some time tonight and I have 5 nodeservers that I'll have to update as well, so I'll give it a start. I don't know YAML at all though, so I'm sure it'll need tweaked by someone that knows the intricacies later.
Awesome, and yes pyaml is good as long as you catch any errors since it doesn't always give clear messages about formatting issues.
Understood. Have a good vacation. My daughter has friends over this evening, so I'm going to try to hide in my office! 👍
This is added and working. Please test and verify. Documentation here: http://ud-polyglot.readthedocs.io/en/development/nsexample.html#custom-node-server-configuration-file
@Einstein42 - tested for compatability (i.e. no config files at all) - works as expected. I would, however, like to move the log messages about no config file existing to the main polyglot log file (using smsg() instead of the LOGGER interface), and add the node server name to the message.
The thinking here is that all messages generated by code that's part of polyglot should use smsg(), thus by default ending up in the main polyglot log. Messages generated by each node server implementation should use the LOGGER mechanism to log to their own private log file. This should help in preliminary triage of an issue -- if the message is in the main polyglot log, then it was generated by Polyglot; otherwise it was generated by the specific node server. (And, of course, a specific node server implementation can override the smsg() methods to redirect them all to the LOGGER interface if desired).
There is no smsg in Polygloconnector. Would you prefer I create one or use poly.send_error()?
I added a smsg method into PolyglotConnector and converted all logger calls to that and mirrored your **INFO/**ERROR formatting.
Let me know if all is good, and I will merge this as 0.0.4 to unstable-rc for validation.
Nice. Looks good, I'm fine with merging to unstable-rc.
We will close this as resolved.
@Einstein42 I realized I never converted to using this, my camera-polyglot is still my custom code. I was doing it for my new polyglot and I see that as the doc states http://ud-polyglot.readthedocs.io/en/development/nsexample.html?highlight=configfile#custom-node-server-configuration-file
it's stored at the root level, same location as server.json. Was there a reason it was not stored in the 'sandbox'?
The sandbox is only created upon first run. Therefore if you had the config file for a nodeserver, shouldn't it be in the codebase? That way it can be deployed normally. That was my thought at least.
Yes, that makes sense, but the way I handled that was to create a default config in the sandbox for the user to edit then restart.
I hear you. That's a fine way to tackle it as well. Shall we change it?
I'd like to change it, but I don't want to force a new release just for this so others can use my new polyglot. So, I'm torn...
I don't actually use that config file for any of mine, I suppose we can make it backwards compatible as well. Look in both places, save to the sandbox.
Sorry, missed your last comment. I can look at making that change so it's backwards compatible.
Now that I am using this for my harmony polyglot, I've determined it's better to keep it where it is :) The reason is that before starting the node server the user will need to create the basic config, then run command which pulls the full harmony config, which then adds more info to the config.yaml and builds the custom profile based on the devices and activities.