Re: [Kimchi-devel] [PATCH v2] [WoK 00/10] '/config/plugins' API implementation

On 01/25/2017 07:17 PM, Daniel Henrique Barboza wrote: > > > On 01/25/2017 05:39 PM, Aline Manera wrote: >> >> >> On 01/25/2017 05:08 PM, Daniel Henrique Barboza wrote: >>> >>> >>> On 01/25/2017 03:37 PM, Aline Manera wrote: >>>> Hi Daniel, >>>> >>>> In overall, the patch set looks good but that will not be enough >>>> to close the feature request. >>>> >>>> You still need to add support to plugin dependencies. Kimchi and >>>> Ginger depends on Ginger Base and when enable/disabling one, the >>>> other should be enable/disable as well. >>>> We have already talked about a solution to do that and IMO it is >>>> simple to be implemented. Any special reason to do not do that >>>> right now? >>> >>> I am not sure if it's simple to be implemented but I'll look into >>> it and, if feasible, >>> add it in v3. >>> >>>> >>>> Also, you should investigate how to enable/disable a plugin on >>>> cherrypy configuration on the fly, without the need to restart Wok >>>> server. >>>> Cherrypy handles the API configuration in the form of a dict, >>>> maybe only removing the plugin entry is enough. Have you checked >>>> it? Any other solution/suggestion? >>> >>> I haven't found a way for cherrypy to reload a configuration on >>> the fly >>> without issuing server reload. As far as I've investigated it is >>> not possible to >>> retrieve the loaded modules after issuing cherrypy.tree.mount(). >>> >> >> The right place to look at is: cherrypy.tree.apps. It will be a dict >> of plugin URI and plugin Root instance. >> >> Something like: >> >> {/plugins/kimchi: <KimchiRoot instance>, /plugins/ginger: >> <GingerRoot instance>, ...} >> >> I have not tested it before, but shouldn't we edit this to remove >> the entry we want and add when needed? > > Not sure, I can give a try. But all this discussion raises a > question. Back in the RFC, we agreed on a > solution that consists on editing the plug-ins config file and then > reloads WoK. We're now discussing a > way to make cherrypy enable/disable them on the fly. Is this now a > part of this same work? > My initial understanding was that the /config/reload API + > /config/plugins API closes the feature > request. > > There's also UI challenges with this 'on the fly' reload. The whole > idea of doing 'on the fly' is > to avoid a full UI reload + relogin when enabling/disabling plug-in. > Say userA is creating a new > VM snapshot in Kimchi. adminB disables gingerbase. Let's ignore the > fact that it can take 3 seconds > for the UI to know about it (the Notification interval). In this > case, Kimchi was also disabled, so the > current operation userA was about to do is now invalid. A full UI > reload is now required to > remove both plug-ins from the UI. This is the trivial scenario. > > The "bad" scenario is, instead of disabling Gingerbase, adminB > disables Ginger. Should we disrupt > userA work with a full refresh? Kimchi wasn't affected by it. Should > we wait until he does > a refresh window to remove Ginger? > > The worst scenario: userA is using Gingers390x, Gingerbase is > disabled. Unless Gingers390x > UI is aware of its full dependency tree, it won't be aware that > disabling Gingerbase cascades > into disabling Ginger, so a full refresh is required because > Gingers390x was disabled too. > > Note that all these scenarios can be solved by forcing a UI refresh > every time but, in this case, > the only difference between the 'on the fly' reload versus a reboot > reload is that on the second > you'll need to login again. > > > To sum up, I am not against investigating the cherrypy on the fly > reload at this moment, but > I advise against pushing it together with this work, without UI > changes. The 'reboot reload' > we proposed in the RFC does not share these problems because when the > browser loses > connectivity it goes back to the login screen. > Yeap! Maybe I was not clear in my first comment. What you need to do to get this patch accepted upstream is only the plugin dependencies stuff. IMO we should have an **investigation** on what is needed to get the plugin enabling/disabling on the fly to know the next steps. For example, the UI development needs to wait that decision to avoid waste of time and code. All the scenarios you pointed above are still valid for plugin enabling/disabling through configuration file. And the need to restart the Wok server is more intrusive as it may cause issues in the ongoing tasks the plugin is performing. For example, some leftovers may be on system due that and cause issues on future actions. IMO it is better to an user get a message "API is no longer available" then 'break' the system in some way due a web server restart when some tasks was ongoing. Think about clone/create/migrate virtual machines, create debug reports, create/restore backups - which a mess it may cause in the system on a server restart. The plugin management on the fly will not interrupt any on going tasks - they only will not be shown on UI anymore but the background task will continue to work. And only future requests will be blocked - avoiding leftover on system. I personally think the on the fly configuration makes better the user experience and avoid issues. Also to have the plugin enabling/disabling on the fly working properly we will need to have the websockets communication to inform all open browser sessions to reload the UI. So, as I said, we should only have in mind what will be the next steps on that area. > > Daniel > > >> >>> Any help is appreciated. >>> >>> >>>> >>>> Regards, >>>> Aline Manera >>>> >>>> On 01/20/2017 03:32 PM, dhbarboza82(a)gmail.com wrote: >>>>> From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; >>>>> >>>>> v2: >>>>> - added User Log capabilities on /config/plugins/enable|disable >>>>> actions >>>>> - added 'enable=' as a valid entry in the parsing of the conf file >>>>> >>>>> This patch set implements the '/config/plugins' API. The idea >>>>> of this API is to replace the current '/plugins' API while >>>>> adding a new attribute called 'enabled' in its return value, >>>>> representing the current state of the plug-in. It will also >>>>> return all the plug-ins, regardless of whether it is loaded >>>>> or not. >>>>> >>>>> New actions /config/plugin/*name*/enable and >>>>> /config/plugin/*name*/disable were implemented to enable/disable >>>>> the plug-in *name* by editing the plug-in .conf file. >>>>> >>>>> >>>>> This is the API in action: >>>>> >>>>> [danielhb@arthas wok_all_plugins]$ curl -k -u danielhb -H >>>>> "Content-Type: application/json" -H "Accept: application/json" -X >>>>> GET 'https://localhost:8001/config/plugins'Enter host password >>>>> for user 'danielhb': >>>>> [ >>>>> { >>>>> "enabled":true, >>>>> "name":"gingerbase" >>>>> }, >>>>> { >>>>> "enabled":true, >>>>> "name":"kimchi" >>>>> }, >>>>> { >>>>> "enabled":true, >>>>> "name":"ginger" >>>>> } >>>>> ][danielhb@arthas wok_all_plugins]$ >>>>> >>>>> >>>>> [danielhb@arthas wok_all_plugins]$ curl -k -u danielhb -H >>>>> "Content-Type: application/json" -H "Accept: application/json" -X >>>>> POST 'https://localhost:8001/config/plugins/ginger/disable' -d'{}' >>>>> Enter host password for user 'danielhb': >>>>> { >>>>> "enabled":false, >>>>> "name":"ginger" >>>>> }[danielhb@arthas wok_all_plugins]$ >>>>> >>>>> >>>>> [danielhb@arthas wok_all_plugins]$ curl -k -u danielhb -H >>>>> "Content-Type: applicationjson" -H "Accept: application/json" -X >>>>> POST 'https://localhost:8001/config/plugins/ginger/enable' -d'{}' >>>>> Enter host password for user 'danielhb': >>>>> { >>>>> "enabled":true, >>>>> "name":"ginger" >>>>> }[danielhb@arthas wok_all_plugins]$ >>>>> >>>>> >>>>> >>>>> MIME-Version: 1.0 >>>>> Content-Type: text/plain; charset=UTF-8 >>>>> Content-Transfer-Encoding: 8bit >>>>> >>>>> >>>>> >>>>> Daniel Henrique Barboza (10): >>>>> /config/plugins API: docs changes >>>>> /config/plugins API: utils changes >>>>> /config/plugins: changes in config model >>>>> /config/plugins API: model changes >>>>> /config/plugins API: test changes >>>>> Removing old /plugins API >>>>> /config/plugins: changing existing UI calls >>>>> /config/plugins: exclude 'sample' plug-in when not in test_mode >>>>> Removing configobj, use file operations to edit conf files >>>>> /config/plugins API: user log and parser improvements >>>>> >>>>> docs/API/config.md | 31 ++++++++++++++++++++ >>>>> docs/API/plugins.md | 13 --------- >>>>> src/wok/control/config.py | 33 +++++++++++++++++++-- >>>>> src/wok/control/plugins.py | 29 ------------------- >>>>> src/wok/i18n.py | 4 +++ >>>>> src/wok/model/plugins.py | 32 +++++++++++++++------ >>>>> src/wok/server.py | 7 ++++- >>>>> src/wok/utils.py | 65 >>>>> ++++++++++++++++++++++++++++++++++++----- >>>>> tests/test_api.py | 47 +++++++++++++++++++++++++++++- >>>>> tests/test_utils.py | 72 >>>>> +++++++++++++++++++++++++++++++++++++++++++++- >>>>> ui/js/src/wok.api.js | 4 +-- >>>>> ui/js/src/wok.logos.js | 11 ++++--- >>>>> ui/js/src/wok.main.js | 12 +++++--- >>>>> 13 files changed, 288 insertions(+), 72 deletions(-) >>>>> delete mode 100644 docs/API/plugins.md >>>>> delete mode 100644 src/wok/control/plugins.py >>>>> >>>> >>>> _______________________________________________ >>>> Kimchi-devel mailing list >>>> Kimchi-devel(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>>> >>> >>> _______________________________________________ >>> Kimchi-devel mailing list >>> Kimchi-devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >>> >> > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel