Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC plugin management #2771

Merged
merged 11 commits into from Jul 28, 2019

Conversation

@darosior
Copy link
Collaborator

commented Jun 24, 2019

This PR adds a new RPC command : reloadplugins, which will initialize and configure each plugin from the default directory (~/.lightning/plugins/) -or from another passed as parameter- that were not already loaded at lightningd startup.

This PR has mutated to now add a plugin RPC command : #2771 (comment).

This permits to manage plugins without having to restart lightningd.

@darosior darosior force-pushed the darosior:plugin_reload branch 4 times, most recently from 4ca10ce to bf41958 Jun 24, 2019

@darosior darosior requested a review from cdecker as a code owner Jun 24, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Can we have a top-level "plugin" command, with "stop", "start", "adddir"? Then we can stop and start them individually or all?

And list of course...

@ZmnSCPxj
Copy link
Collaborator

left a comment

Design-wise, I would suggest adding a new function plugin_reload_all that correctly performs the reload of plugins, which would be async, and create a new plugin_control.c file that contains the RPC commands.

You also need to consider what happens to ongoing plugin-based commands. Would not it be better if we enter a "shutting down" mode where existing plugin-provided commands are allowed to complete, new plugin-provided commands are delayed until after reload, and once current plugin commands have completed, only then kill and reload plugins?

Consider that we may eventually move existing commands like withdraw to a plugin (using txprepare and txbroadcast). There is always a race condition where client is executing a withdraw and then another client executes a reload_plugin, it would be a regression in behavior if before withdraw is a plugin this is not a problem but if withdraw is moved to a plugin then it becomes problematic if we reload_plugin while a withdraw command is ongoing.

Even fundchannel can eventually be moved to a plugin (it can eventually be implemented with fundchannel_start, txprepare, fundchannel_complete, and txbroadcast).

lightningd/plugin.c Show resolved Hide resolved
lightningd/plugin.c Outdated Show resolved Hide resolved
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

Can we have a top-level "plugin" command, with "stop", "start", "adddir"? Then we can stop and start them individually or all?

And list of course...

Yes, I'm not sure but I think I can do that.
@rustyrussell by the way, do you know why calling another function from lightningd/plugin.c in lightningd.c when having configured with --enable-dev causes a link error ? Though we can call plugins_init we cannot call plugins_add_default_dir ..

@ZmnSCPxj thank you for the review

Design-wise, I would suggest adding a new function plugin_reload_all that correctly performs the reload of plugins, which would be async, and create a new plugin_control.c file that contains the RPC commands.

I agree for the seperated file. Do you recommend this command in addition to the one Rusty proposed ?

You also need to consider what happens to ongoing plugin-based commands. Would not it be better if we enter a "shutting down" mode where existing plugin-provided commands are allowed to complete, new plugin-provided commands are delayed until after reload, and once current plugin commands have completed, only then kill and reload plugins?

I may add a check, even if it seems weird to me that someone would reload_plugins if a withdraw command is ongoing.
EDIT: The command name was misleading, actually it does not reload already started plugins : it just checks for new plugins present and load them.

@darosior darosior force-pushed the darosior:plugin_reload branch from bf41958 to 0a5776c Jun 26, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2019

According to the above comments by @rustyrussell and @ZmnSCPxj :

  • it is now a plugin command which takes a command and an optional parameter. The command returns in any case a list active_plugins of plugins loaded after the command execution.
    • The start command adds a plugin given a path as parameter
    • The stop command stops (kill and unload) a plugin given a plugin name as parameter
    • The adddir command adds a plugin directory given a directory path as parameter
    • The reload command adds all non-already-loaded plugins from the default plugins directory
    • The list command just outputs the active plugins
  • it has been moved to a new file : plugin_control.c

@darosior darosior force-pushed the darosior:plugin_reload branch from 0a5776c to 7b9ee72 Jun 26, 2019

doc/lightning-reloadplugins.7.txt Outdated Show resolved Hide resolved
doc/lightning-reloadplugins.7.txt Outdated Show resolved Hide resolved

@darosior darosior force-pushed the darosior:plugin_reload branch 2 times, most recently from 136a98d to c602cde Jun 26, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2019

Rebased to move plugins_add_directory() back from lightningd.c's main() to plugins_init() in plugin.c in order to make the --enable-developer build pass.

Since the load from the default plugin directory is now done each time we call a plugin command there is no more need for a dedicated reload command, so I removed it along with the above movement.

@darosior darosior force-pushed the darosior:plugin_reload branch 3 times, most recently from e8402f5 to 726d6b1 Jun 26, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

The travis failure is due to lightningd crashing if the plugin command is used just after having started the daemon.
I tried (the above force pushs) to increase the time.sleep(n) in tests to make Travis tests pass (they pass locally) but I need to find the root cause of this crash in order to make a condition in the command so that plugins_init() is not called too early.
I've been investigating today and did not find it : the backtrace gets back to the plugins_init()'s io_loop to wait for manifests, but it appears that the loop is broken very early and that it's not the cause of the crash. Could anyone explain me why we can't make another io_loop just after startup without waiting about 5 seconds ? Knowing that passed that delay, we can call the plugin command as many times in a row as we want ? Is disabling the command for 5s after startup (just to avoid the crash for the user) something that should be avoided ?

Backtrace to avoid pull and compilation

The backtrace after having quiet the warning notifications (plugin_notify()) which were part of the backtrace (memleak due to crash) without being the root cause.

2019-06-27T20:52:39.201Z lightningd(23821): --------------------------------------------------
2019-06-27T20:52:39.201Z lightningd(23821): Server started with public key 023bafa606486c3066f71d98f385dcfb24742191589922bcbea49373c21c4e7457, alias PEEVEDSOURCE-623-g726d6b1-modded (color #023baf) and lightningd v0.7.0-623-g726d6b1-modded
Already in transaction from lightningd/jsonrpc.c:653
2019-06-27T20:52:40.251Z lightningd(23821): Already in transaction from lightningd/jsonrpc.c:653
lightningd: FATAL SIGNAL 6 (version v0.7.0-623-g726d6b1-modded)
0x55af55bb0aa8 send_backtrace
	common/daemon.c:40
0x55af55bb0b4e crashdump
	common/daemon.c:53
0x7fa88ffdc83f ???
	???:0
0x7fa88ffdc7bb ???
	???:0
0x7fa88ffc7534 ???
	???:0
0x55af55b8be73 fatal
	lightningd/log.c:622
0x55af55bdd0ae db_begin_transaction_
	wallet/db.c:637
0x55af55ba990e sd_msg_read
	lightningd/subd.c:409
0x55af55bf64c9 next_plan
	ccan/ccan/io/io.c:59
0x55af55bf7046 do_plan
	ccan/ccan/io/io.c:407
0x55af55bf7084 io_ready
	ccan/ccan/io/io.c:417
0x55af55bf90d9 io_loop
	ccan/ccan/io/poll.c:445
0x55af55ba778d plugins_init
	lightningd/plugin.c:969
0x55af55ba8042 json_plugin_control
	lightningd/plugin_control.c:65
0x55af55b8729b parse_request
	lightningd/jsonrpc.c:654
0x55af55b876d5 read_json
	lightningd/jsonrpc.c:752
0x55af55bf64c9 next_plan
	ccan/ccan/io/io.c:59
0x55af55bf7046 do_plan
	ccan/ccan/io/io.c:407
0x55af55bf7084 io_ready
	ccan/ccan/io/io.c:417
0x55af55bf90d9 io_loop
	ccan/ccan/io/poll.c:445
0x55af55b83b0d io_loop_with_timers
	lightningd/io_loop_with_timers.c:24
0x55af55b89d43 main
	lightningd/lightningd.c:827
0x7fa88ffc909a ???
	???:0
0x55af55b720d9 ???
	???:0
0xffffffffffffffff ???
	???:0
2019-06-27T20:52:40.258Z lightningd(23821): FATAL SIGNAL 6 (version v0.7.0-623-g726d6b1-modded)
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: common/daemon.c:45 (send_backtrace) 0x55af55bb0afe
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: common/daemon.c:53 (crashdump) 0x55af55bb0b4e
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0x7fa88ffdc83f
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0x7fa88ffdc7bb
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0x7fa88ffc7534
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/log.c:622 (fatal) 0x55af55b8be73
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: wallet/db.c:637 (db_begin_transaction_) 0x55af55bdd0ae
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/subd.c:409 (sd_msg_read) 0x55af55ba990e
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:59 (next_plan) 0x55af55bf64c9
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:407 (do_plan) 0x55af55bf7046
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:417 (io_ready) 0x55af55bf7084
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/poll.c:445 (io_loop) 0x55af55bf90d9
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/plugin.c:969 (plugins_init) 0x55af55ba778d
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/plugin_control.c:65 (json_plugin_control) 0x55af55ba8042
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/jsonrpc.c:654 (parse_request) 0x55af55b8729b
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/jsonrpc.c:752 (read_json) 0x55af55b876d5
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:59 (next_plan) 0x55af55bf64c9
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:407 (do_plan) 0x55af55bf7046
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/io.c:417 (io_ready) 0x55af55bf7084
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: ccan/ccan/io/poll.c:445 (io_loop) 0x55af55bf90d9
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/io_loop_with_timers.c:24 (io_loop_with_timers) 0x55af55b83b0d
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: lightningd/lightningd.c:827 (main) 0x55af55b89d43
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0x7fa88ffc909a
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0x55af55b720d9
2019-06-27T20:52:40.258Z lightningd(23821): backtrace: (null):0 ((null)) 0xffffffffffffffff
Log dumped in crash.log.20190627205240
Aborted

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

The root reason is that every command is already inside a sqlite3 db transaction. We do everything inside db transactions, only exiting when returning to the mainloop (e.g. async operations). What's happening is that you are trying to run a mainloop within a mainloop, so we try to make a transaction inside a transaction, which isn't supported (at least by our own code; sqlite3 might work, our code might not).

Like I suggested, you should split plugins_init. Initiating the startup of new plugins should be a separate function, which plugins_init calls, e.g. plugins_start_plugins.

In particular, you should not do an io_loop within an io_loop.

So plugins_start_plugins would be an async interface which calls the callback only when pending_manifests drops to 0. Both plugins_init and json_plugin_control should call into it. And json_plugin_control MUST NOT call plugins_init.

For plugins_init, it provides a callback which executes io_break. plugins_init will also include its own io_loop.

For json_plugin_control, it provides a callback which provides the actual result object of the plugin command. In particular it would not have its own io_loop.

@darosior darosior force-pushed the darosior:plugin_reload branch 2 times, most recently from a9a834a to 3ec8de2 Jun 29, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 29, 2019

@ZmnSCPxj thank you the support : I had to modify plugin.c some more but it now works well.

I added the reload control back since json_plugin_control does not call plugin_init anymore. I also added a unregister_hook control that might be useful.

@darosior darosior force-pushed the darosior:plugin_reload branch 5 times, most recently from 9135b53 to bd14d0b Jun 29, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2019

@ZmnSCPxj do the changes seem good to you ?

lightningd/plugin_control.c Show resolved Hide resolved
lightningd/plugin_control.h Outdated Show resolved Hide resolved
doc/lightning-plugin.7.txt Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

The only really important thing is the new arguments for pylightning which I need to find a fix for.

Would rpc.plugin.start() etc be more pythony? ie. make plugin like a command namespace... somehow?

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 25, 2019

The only really important thing is the new arguments for pylightning which I need to find a fix for.

Would rpc.plugin.start() etc be more pythony? ie. make plugin like a command namespace... somehow?

I think this was about dynamic json fields unpacking to function arguments which breaks plugins made with Pylightning if we add a new field in JSONRPC params (in init for example :-))

@darosior darosior force-pushed the darosior:plugin_reload branch from 9ce4216 to e92fb85 Jul 25, 2019

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 25, 2019

Rebased on top of master and resolve CHANGELOG conflicts. Added new brag lines for init and getmanifest new fields.

@darosior darosior force-pushed the darosior:plugin_reload branch from e92fb85 to 0b8e5dc Jul 25, 2019

@@ -12,6 +12,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- build: now requires `python3-mako` to be installed, i.e. `sudo apt-get install python3-mako`
- plugins: a new notification type `invoice_payment` (sent when an invoice is paid) has been added
- rpc: a new rpc command is added, `plugin`. It allows one to manage plugins without restarting `lightningd`.
- plugins: a new boolean field is added to the `init`'s `configuration`, `startup`. It allows a plugin to know if it has been started on `lightningd` startup.
- plugins: a new boolean field can be added to a plugin manifest, `dynamic`. It allows a plugin to tell if it can be started or stopped "on-the-fly".

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 28, 2019

Contributor

I'll probably collapse these three lines into "plugins: dynamic plugin support", which is the better summary for this level IMHO.

(I do a CHANGELOG.md cleanup sweep as part of the release process, so I won't hold up the merge for this!)

@rustyrussell
Copy link
Contributor

left a comment

Ack 0b8e5dc

darosior added some commits Jun 29, 2019

lightningd/plugin_hook: make it possible to unregister a hook
This adds 'plugin_unregister_hook' and 'plugin_unregister_hook_all'
functions to unregister a given hook a plugin registered, or all hooks a
plugin registered for. Since hooks can only be registered once, it's
useful in the case a new plugin is added which would be prefered for
hook registration over an already loaded plugin.
lightningd/plugin_control: add a 'plugin' command
This adds a new pair of files : lightningd/plugin_control, along with a new RPC
command : 'plugin'. This command can be used to manage plugins without restarting lightningd:

lightning-cli plugin start helloworld.py
lightning-cli plugin stop helloworld.py
lightningd/plugin: Add a 'configured' member to the plugin struct, sp…
…lit 'plugins_init'

This adds a 'configured' boolean member to the plugin struct so that we can add plugins to ld->plugins' list and differenciate fresh plugins.
This also adds 'plugins_start' so that new plugins can be started without calling 'plugins_init' and running an io loop
lightningd/plugin: Add a 'dynamic' field to getmanifest and a 'startu…
…p' field to init

This lets a plugin specify whether it can be restarted, and to know if it is started at lightningd startup

@rustyrussell rustyrussell force-pushed the darosior:plugin_reload branch from 0b8e5dc to a298c50 Jul 28, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

Trivial rebase to resolve CHANGELOG.md conflict.

Ack a298c50

@rustyrussell rustyrussell merged commit 1e2f379 into ElementsProject:master Jul 28, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@darosior darosior deleted the darosior:plugin_reload branch Jul 28, 2019

@SimonVrouwe

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

Can someone add the plugin label to this threat?

@ZmnSCPxj ZmnSCPxj added the plugin label Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.