Order specifications for hooks #4168

rustyrussell · 2020-10-29T00:42:46Z

This allows hooks to specify they must be before/after some other plugins. Either by basename or full pathname.

As our plugin ecosystem grows and gets more sophisticated, such conflicts will inevitably occur where multiple plugins want to ensure they are called in certain order.

We could implement a priority number system, but that's usually just window-dressing on what people really want (though, unlike this, would allow you to specify that you must be first, or last: we could add wildcards if we wanted to).

Getting this in now means that after a few releases, plugin authors will simply be able to assume its existence.

Fixes: #4005 (thanks @m-schmoock for pointing that out!)

It needs to use a non-clashing name for the internal var. Signed-off-by: Rusty Russell <[email protected]>

m-schmoock · 2020-10-29T11:07:47Z

This addresses #4005

niftynei

ACK 0c1d19b

niftynei · 2020-10-29T19:56:45Z

doc/PLUGINS.md

+"after" arrays of plugin names, which control what order they will be
+called in.  If a plugin name is unknown, it is ignored.
+
+if the plugins names are unknown, they are ignored, otherwise if the


this repeats the last line of the previous paragraph? i'd remove one.

niftynei · 2020-10-29T20:00:09Z

lightningd/plugin.c

+					       json_tok_full_len(t),
+					       json_tok_full(buffer, t));
+			name = json_strdup(tmpctx, buffer, nametok);
+			beforetok = json_get_member(buffer, t, "before");


it took me a second to fully grasp the implication of 'before' and 'after'. Renaming them to "run_before" and "run_after" might be more immediately obvious?

It's more obvious in context, than when reading in a patch, e.g.:

"hooks": [ {"name": "htlc_accepted", "before": ["someplugin.py"], "after": ["foo.py"]} ]

The next patch will use these to order the hooks. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: plugins: hooks can now specify that they must be called 'before' or 'after' other plugins.

Signed-off-by: Rusty Russell <[email protected]>

m-schmoock · 2020-10-30T10:33:39Z

concept ack

@rustyrussell as you wrote in 4005

3. Secondary sort by command line order, then config order.

It is also very helpful to give the user an option for plugin ordering (unless otherwise specified by the devs via before/after) by checking commandline and config order of plugins. The rationale behind is it the following: Before and after list are to be maintained by the plugin developers. Before that happens, a user typically has the problem first and needs to wait for the devs to to make the changes so his specific setup works. Since the devs will often not know all the plugin combinations out in the wild this can take a while or, in the worst case, never happens.

So I think we might also add the commandline/config order as 'last resort/default' if no before/after ordering has been specified for the plugins. For that we need a stable sorting algorithm and feed it with the initial order of plugins from commandline/config.

rustyrussell · 2020-10-31T22:59:29Z

The current implementation has this effect, in fact. Technically it's not quite a stable sort, as dependencies can pull in things out of order, but we could fix that (I doubt anyone will notice, though).

For example, assume plugins in alphabetical order (they're in directory-sort order usually BTW). A has to be before D, and B has to be before C.

In this case, algorithm will go A B D C. Easy to fix though.

rustyrussell · 2020-11-02T02:39:09Z

OK, fixed to make the default ordering equal to the specified one.

Also, removed a problem where we'd warn about missing dependencies since we evaluated them as we loaded. This was more invasive, but I think simplifies the code a little anyway.

m-schmoock · 2020-11-02T09:41:15Z

Thanks @rustyrussell

Super wired, I stress tested your changes a little, but it seems there is an issue with the default commandline/config order (unless otherwise by before/after specified).

How to reproduce

check out ece24e5
compile
apply a test change, that now should work (see below)
run the tests in parallel: py.test --count 50 -x -n 16 -v tests/test_plugin.py -k test_openchannel_hook_chaining

What happens

The flaky test can fail. When we look in the logs of l2 we see that the 'reject for a reason' hook is not called but 'reject by design', which should never happen if correct default order would be applied.

The patch for the test that uses the new feature

diff --git a/tests/test_plugin.py b/tests/test_plugin.py
index d2628cd25..9f35a4dc5 100644
--- a/tests/test_plugin.py
+++ b/tests/test_plugin.py
@@ -606,24 +606,15 @@ def test_openchannel_hook_chaining(node_factory, bitcoind):
     l1, l2 = node_factory.line_graph(2, fundchannel=False, opts=opts)
     l1.fundwallet(10**6)
 
-    hook_msg = "openchannel_hook rejects and says '"
     # 100005sat fundchannel should fail fatal() for l2
     # because hook_accepter.py rejects on that amount 'for a reason'
     with pytest.raises(RpcError, match=r'They sent error channel'):
         l1.rpc.fundchannel(l2.info['id'], 100005)
 
-    # Note: hook chain order is currently undefined, because hooks are registerd
-    #       as a result of the getmanifest call, which may take some random time.
-    #       We need to workaround that fact, so test can be stable
-    correct_order = l2.daemon.is_in_log(hook_msg + "reject for a reason")
-    if correct_order:
-        assert l2.daemon.wait_for_log(hook_msg + "reject for a reason")
-        # the other plugin must not be called
-        assert not l2.daemon.is_in_log("reject on principle")
-    else:
-        assert l2.daemon.wait_for_log(hook_msg + "reject on principle")
-        # the other plugin must not be called
-        assert not l2.daemon.is_in_log("reject for a reason")
+    hook_msg = "openchannel_hook rejects and says '"
+    assert l2.daemon.wait_for_log(hook_msg + "reject for a reason")
+    # the third plugin must now not be called anymore
+    assert not l2.daemon.is_in_log("reject on principle")
 
     # 100000sat is good for hook_accepter, so it should fail 'on principle'
     # at third hook openchannel_reject.py

rustyrussell · 2020-11-02T23:22:04Z

Argh, good catch: final patch actually fixes the problem, and for some reason I didn't push it!

rustyrussell · 2020-11-02T23:24:51Z

And I turned your patch into a commit, credited you and appended. Thanks!

m-schmoock · 2020-11-03T09:40:10Z

works like a charm

m-schmoock · 2020-11-03T09:41:19Z

@rustyrussell regarding IRC chat:

00:22 < rusty> m-schmoock: how do you feel about getting rid of all those single hooks? Or at least as many of them as possible...

Can you elaborate on this?

I was thinking that it would be sufficient to have the before and after lists on a per plugin level and not per hook. It would reduce (configuration) complexity. Having it on a per hook basis brings the option to have two different plugins with two (same) hooks each order their execution differently depending on what hook is called. This seems a bit overkill. Having plugin order rather than hook order would just prioritize one plugin over another, which will likely do the job.

cdecker

Some minor questions for my own understanding, and a tiny python bug, otherwise a very solid PR 👍

lightningd/plugin_hook.c

cdecker · 2020-11-05T10:52:32Z

contrib/pyln-client/pyln/client/plugin.py

+                 before: List[str] = [],
+                 after: List[str] = []) -> None:


This is dangerous: never use a mutable instance as the default argument. They'll be shared between invocations.

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

This should be before: List[str] = None and then check for None in the body.

Oh, nice catch, thanks!

cdecker · 2020-11-05T11:01:50Z

lightningd/plugin.c

+	/* If this was last one manifests were waiting for, handle deps */
+	if (p->plugin_state == AWAITING_GETMANIFEST_RESPONSE)
+		check_plugins_manifests(p->plugins);
+
+	/* If this was the last one init was waiting for, handle cmd replies */
+	if (p->plugin_state == AWAITING_INIT_RESPONSE)
+		check_plugins_initted(p->plugins);


This seems to be a weird place to perform these checks. We look at a random plugin being destroyed and if that was in a specific state we assume it must have been the last one in this state? We should kick this up to the set of plugins and perform the check there after removing the plugin to be destroyed. No need to replicate the last-one-turns-off-the-light test here :-)

No, we've already unlisted the plugin at this point. We could, and probably should simply call check_plugins_xxx() unconditionally here.

I wonder if we should have a plugin_set_state() which calls these internally. Less efficient, but clearer?

Similar to the payment_continue function that is used to continue in a state machine? I'm all for it ^^

cdecker · 2020-11-05T11:14:43Z

lightningd/plugin_hook.c

+	tal_resize(s, s_num + 1);
+	memmove(*s + pos+1, *s + pos, sizeof(**s) * (s_num - pos));
+	(*s)[pos] = n;


I don't quite follow why this is working: we're striking dependencies off of the graph node until we finally find that it has no more, then add it to the list. However, before this change we added it to the end of the list, which was always safe (no dependency could jump over us). After this change we suddenly use insertion sort on the index at a potentially earlier position in the list, meaning we could end up being inserted before one of our dependencies. What am I missing here?

You're right!

We should instead gather all the things we are going to add from one edge, and order those then add as a group. That means if B and C depend on A, we append A, then sort (B,C) and append those.

I will write a test which triggers this first...

lightningd/plugin_hook.c

rustyrussell · 2020-11-06T03:43:07Z

I ended up reworking and thus simplifying the algo. It's less efficient (O(N^2)) but nobody cares.

cdecker · 2020-11-06T14:16:23Z

contrib/pyln-client/pyln/client/plugin.py

+             before: List[str] = None,
+             after: List[str] = None) -> JsonDecoratorType:


This now becomes an Optional[List[str]]

Sorry for forgetting this in my first round of comments :-)

cdecker · 2020-11-06T14:21:57Z

contrib/pyln-client/pyln/client/plugin.py

+        if before:
+            method.before = before
+        if after:
+            method.after = after


We should always set these, otherwise we'd have to check with hasattr whether the hook has that field.

method.before = [] if before else before method.after = [] if after else after

Hmm, mypy didn't like that, so went for an open-coded variant.

cdecker · 2020-11-06T14:26:21Z

lightningd/plugin_hook.c

 struct hook_node {
+	bool finished;


What are the semantics of finished? It's only used in sorting so I'd assume it means that it is in its final position.

Included in sorted results. I'll add a comment

cdecker · 2020-11-06T14:28:06Z

lightningd/plugin_hook.c

+		if (graph[i].num_incoming != 0)
+			continue;
+		if (!best
+		    || best->hook->plugin->index > graph[i].hook->plugin->index)


We're never moving the plugin in the graph, so under no circumstance should this ever be true (a later iteration should always have a higher index). Since that's the case I think we can remove the index altogether, and break/return after finding a first best

That's making an assumption that graph is in index order. It (I'm fairly sure) starts that way, but can be perturbed by previous calls to plugin_hook_make_ordered. If plugin B says it has to be before A and after C, we'll go C B A. If B gets removed, we should return to A C.

Does anyone care? Probably not, but this way is clear, at least.

On further thought, if we define it as "minimum perturbation to achieve required ordering" which is a totally reasonable goal, we can indeed do this simplification.

... Nope. Hooks start in "who returned from getmanifest" order which is not the same as "order specified by config" order.

m-schmoock · 2020-11-07T11:32:53Z

Hm, I restarted a travis flake which I couldn't reproduce locally also not by running it 100 times and parallel: tests/test_plugin.py::test_plugin_command

It was complaining about unclean node shutdown, but it succeeded upon first restart/retry.

m-schmoock · 2020-11-07T11:34:21Z

@rustyrussell still (sorry for bugging):

regarding IRC chat:

00:22 < rusty> m-schmoock: how do you feel about getting rid of all those single hooks? Or at least as many of them as possible...

Can you elaborate on this?

I was thinking that it would be sufficient to have the before and after lists on a per plugin level and not per hook. It would reduce (configuration) complexity. Having it on a per hook basis brings the option to have two different plugins with two (same) hooks each order their execution differently depending on what hook is called. This seems a bit overkill. Having plugin order rather than hook order would just prioritize one plugin over another, which will likely do the job.

rustyrussell · 2020-11-09T04:27:20Z

@rustyrussell still (sorry for bugging):

regarding IRC chat:

00:22 < rusty> m-schmoock: how do you feel about getting rid of all those single hooks? Or at least as many of them as possible...

Can you elaborate on this?

We should make it so every hook can be registered by multiple plugins.

I was thinking that it would be sufficient to have the before and after lists on a per plugin level and not per hook. It would reduce (configuration) complexity. Having it on a per hook basis brings the option to have two different plugins with two (same) hooks each order their execution differently depending on what hook is called. This seems a bit overkill. Having plugin order rather than hook order would just prioritize one plugin over another, which will likely do the job.

I agree, at the moment. But hooks are the only place where plugins interact, so I'd rather see explicitly what they care about. If we ever get order conflicts, it's easier to see what they want when they have priorities on specific hooks.

rustyrussell · 2020-11-09T05:43:46Z

Trivial rebase to fold in fixups and add a comment on finished var.

And use that to add simple tests. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

Now both python and c libraries are updated, we can officially deprecate the old form. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: plugins: hooks should now be specified using objects, not raw names.

…rtup. We fail this, because we check dependencies as they come in. Signed-off-by: Rusty Russell <[email protected]>

This means we need to stop at this stage even in the runtime-loaded case. Signed-off-by: Rusty Russell <[email protected]>

…hange. Suggested-by: @mschmook Signed-off-by: Rusty Russell <[email protected]>

We previously registered hooks up in who-replies-to-getmanifest-first order, but then if any had dependencies it would scatter that order. This allows users to manually set dependencies developers have forgotten by specifying the plugins manually in their configuration or cmdline. This was an excellent consideration by @mschmook. Signed-off-by: Rusty Russell <[email protected]>

…istic.

I think this is what Travis is having an issue with, but it work fine locally. Signed-off-by: Rusty Russell <[email protected]>

m-schmoock · 2020-11-09T09:44:10Z

00:22 < rusty> m-schmoock: how do you feel about getting rid of all those single hooks? Or at least as many of them as possible...

Can you elaborate on this?

We should make it so every hook can be registered by multiple plugins.

absolutely yes, I missed the point that you were referring to 'unchained' hooks.

m-schmoock · 2020-11-09T09:53:55Z

doc/PLUGINS.md

@@ -678,14 +678,25 @@ declares that it'd like to be consulted on what to do next for certain
 events in the daemon. A hook can then decide how `lightningd` should
 react to the given event.

+When hooks are registered, they can optionally specify "before" and
+"after" arrays of plugin names, which control what order they will be


General question about 'plugin names': Is this same as for cli plugin start/stop commands, meaning either full path (useless for this feature) or basename? If so, we may note that the plugin name includes its extension, meaning full filename.

Yes, filename or full path name.

m-schmoock · 2020-11-09T16:59:28Z

ACK 7d41997

common: allow tal_arr_expand() to have an n argument.

11a0e38

It needs to use a non-clashing name for the internal var. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell requested a review from cdecker as a code owner October 29, 2020 00:42

rustyrussell force-pushed the guilt/order-for-hooks branch from 8809845 to 0c1d19b Compare October 29, 2020 06:25

niftynei approved these changes Oct 29, 2020

View reviewed changes

rustyrussell added the Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! label Oct 29, 2020

rustyrussell added 2 commits October 30, 2020 11:43

plugins: allow 'before' and 'after' arrays for hooks.

f86b66b

The next patch will use these to order the hooks. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: plugins: hooks can now specify that they must be called 'before' or 'after' other plugins.

lightningd: actually order the hooks.

b5cab17

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/order-for-hooks branch from 0c1d19b to 7dd4c39 Compare October 30, 2020 01:13

rustyrussell requested a review from niftynei November 2, 2020 02:39

niftynei added this to the v0.9.2 milestone Nov 2, 2020

cdecker reviewed Nov 5, 2020

View reviewed changes

cdecker reviewed Nov 6, 2020

View reviewed changes

rustyrussell force-pushed the guilt/order-for-hooks branch from 689c8aa to 7defce9 Compare November 9, 2020 04:25

rustyrussell force-pushed the guilt/order-for-hooks branch from 7defce9 to fe6b1e0 Compare November 9, 2020 05:43

rustyrussell added 2 commits November 9, 2020 19:50

pyln: add support for dependent hooks.

05fe058

And use that to add simple tests. Signed-off-by: Rusty Russell <[email protected]>

libplugin: add support for before and after deps on hooks.

c3afd29

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell and others added 7 commits November 9, 2020 19:50

plugins: deprecate old form of hooks.

3101bef

Now both python and c libraries are updated, we can officially deprecate the old form. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: plugins: hooks should now be specified using objects, not raw names.

pytest: test that we don't complain about missing dependencies at sta…

5124378

…rtup. We fail this, because we check dependencies as they come in. Signed-off-by: Rusty Russell <[email protected]>

plugins: check order once all plugins have returned from getmanifest.

2ca13b2

This means we need to stop at this stage even in the runtime-loaded case. Signed-off-by: Rusty Russell <[email protected]>

pytest: test that we maintain load order unless hook deps require a c…

77c27af

…hange. Suggested-by: @mschmook Signed-off-by: Rusty Russell <[email protected]>

pytest: simplify test_openchannel_hook_chaining now order is determin…

ed8f556

…istic.

pytest: make test_hook_dep tests more stable.

7d41997

I think this is what Travis is having an issue with, but it work fine locally. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the guilt/order-for-hooks branch from fe6b1e0 to 7d41997 Compare November 9, 2020 09:26

m-schmoock reviewed Nov 9, 2020

View reviewed changes

niftynei merged commit 4745e7e into ElementsProject:master Nov 9, 2020

		before: List[str] = None,
		after: List[str] = None) -> JsonDecoratorType:

Order specifications for hooks #4168

Order specifications for hooks #4168

Conversation

rustyrussell commented Oct 29, 2020 • edited Loading

m-schmoock commented Oct 29, 2020

niftynei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-schmoock commented Oct 30, 2020 • edited Loading

rustyrussell commented Oct 31, 2020

rustyrussell commented Nov 2, 2020

m-schmoock commented Nov 2, 2020

How to reproduce

What happens

The patch for the test that uses the new feature

rustyrussell commented Nov 2, 2020

rustyrussell commented Nov 2, 2020

m-schmoock commented Nov 3, 2020

m-schmoock commented Nov 3, 2020 • edited Loading

cdecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-schmoock commented Nov 7, 2020

m-schmoock commented Nov 7, 2020

rustyrussell commented Nov 9, 2020

rustyrussell commented Nov 9, 2020

m-schmoock commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-schmoock commented Nov 9, 2020

rustyrussell commented Oct 29, 2020 •

edited

Loading

m-schmoock commented Oct 30, 2020 •

edited

Loading

m-schmoock commented Nov 3, 2020 •

edited

Loading