Bug 36736 - Add ability to load DBIx::Class Schema files found in plugins
Summary: Add ability to load DBIx::Class Schema files found in plugins
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: additional_work_needed
Depends on:
Blocks:
 
Reported: 2024-04-30 16:03 UTC by Kyle M Hall (khall)
Modified: 2024-08-27 00:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 36736: Load plugins at the start of background job processing (1.71 KB, patch)
2024-07-11 18:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36736: Load plugins at the start of background job processing (1.76 KB, patch)
2024-07-23 03:06 UTC, David Cook
Details | Diff | Splinter Review
Bug 36736: Load plugins at the start of background job processing (1.82 KB, patch)
2024-07-23 11:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36736: Tests [POC] (8.35 KB, patch)
2024-07-23 18:53 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36736: Tests [POC] (8.37 KB, patch)
2024-07-24 16:49 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36736: Load plugins at the start of background job processing (1.88 KB, patch)
2024-07-25 13:32 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2024-04-30 16:03:37 UTC
Right now, it is not possible to create custom Koha::Object(s) for a plugin without breaking things. For example we did this:

BEGIN {
    my $path = Module::Metadata->find_module_by_name(__PACKAGE__);
    $path =~ s!\.pm$!/lib!;
    unshift @INC, $path;

    require Koha::CurbsidePickupIssues;
    require Koha::CurbsidePickupPolicies;
    require Koha::CurbsidePickups;
    require Koha::Schema::Result::CurbsidePickup;
    require Koha::Schema::Result::CurbsidePickupIssue;
    require Koha::Schema::Result::CurbsidePickupPolicy;

    # register the additional schema classes
    Koha::Schema->register_class(CurbsidePickup => 'Koha::Schema::Result::CurbsidePickup');
    Koha::Schema->register_class(CurbsidePickupPolicy => 'Koha::Schema::Result::CurbsidePickupPolicy');
    Koha::Schema->register_class(CurbsidePickupIssue => 'Koha::Schema::Result::CurbsidePickupIssue');
    # ... and force a refresh of the database handle so that it includes
    # the new classes
    Koha::Database->schema({ new => 1 });
}

In the curbside pickups plugin before it was integrated. The problem we've found is that calling Koha::Database->schema({ new => 1 }); resets the database connection and breaks any transactions you were in when the plugin was loaded!

We should be able to search for and load any schema files found in plugin directories to make this all unnecessary.
Comment 1 Kyle M Hall (khall) 2024-04-30 16:05:30 UTC
Forgot to mention, we probably need to do that search and load in Koha::Schema
Comment 2 Nick Clemens (kidclamp) 2024-07-09 13:14:59 UTC
So, I played around here a bit - my first thought was that we could simply avoid the reconnect if the objects were already loaded, so I updated a plugin like so:

$schema = Koha::Database->schema({ new => 1 }) unless $schema->source_registrations->{KohaPluginComBywatersolutionsContractsContract};

With some warns I found that this was executed when plack was restarted, so the object are loaded in plack from the start, that's good. But I found that imports still failed and caused reconnect - that's bad.

What I eventually realized was that we are forking our background jobs - and these get a new DB connection, without the new objects. The dirty fix that worked was adding to the worker:
Koha::Plugins->get_enabled_plugins;

After spawning the process, but before beginning the job. And that fixes it.

I am not sure where we load the plugins when we start plack, but if we did the same thing when launching a background worker process - or built it in to the process method of BackgroundJob

To move the logic out of the plugin we could add a 'plugin_classes' method and have Koha::Schema check for if the objects are loaded - but I think maybe just loading plugins for the background tasks at spawn is a good start?
Comment 3 Kyle M Hall (khall) 2024-07-11 12:16:52 UTC
That sounds like a reasonable plan to me!
Comment 4 Nick Clemens (kidclamp) 2024-07-11 18:14:06 UTC
Created attachment 168861 [details] [review]
Bug 36736: Load plugins at the start of background job processing

This patch adds a call to get_enabled_plugins before processing background jobs to ensure
that all plugin hooks are loaded and cached

To test:
1 - Install a plugin that adds new objects e.g. the Contracts plugin
    https://github.com/bywatersolutions/fs-koha-plugin-contracts
    or the KOha Advent plugin:
    https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin
2 - Restart all
3 - Tail all your logs
4 - Stage and import a file containing items
5 - Note in the logs
    DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction
6 - Apply this patch
7 - Restart all
8 - Stage and import again
9 - Success!
Comment 5 David Cook 2024-07-15 00:20:26 UTC
Sounds interesting. Could you walk me/us through this a bit more?
Comment 6 Nick Clemens (kidclamp) 2024-07-22 12:05:30 UTC
(In reply to David Cook from comment #5)
> Sounds interesting. Could you walk me/us through this a bit more?

The idea is really to make plugin code a bit simpler, easier to read, more consistent with Koha core, and leave open a path for future integration. 


Adding new objects via the plugin lets plugins use KohaTables and other helper code as well.

Adding objects was featured in the Koha Advent plugin, but it seems the background jobs broke this trick 

This patch just ensures the plugins are loaded, and hence all functions and objects available, for our background processes
Comment 7 David Cook 2024-07-23 01:59:26 UTC
(In reply to Nick Clemens (kidclamp) from comment #6)
> Adding new objects via the plugin lets plugins use KohaTables and other
> helper code as well.

I certainly like the sound of that! 

> Adding objects was featured in the Koha Advent plugin, but it seems the
> background jobs broke this trick 
>
> This patch just ensures the plugins are loaded, and hence all functions and
> objects available, for our background processes

So I was hoping you'd be able to explain it easier than I'd be able to puzzle it out by trying it myself, but I think it's one of those things where a person just needs to try it out.

Trying the test plan, and I see the errors in /var/log/koha/kohadev/worker-output.log

I'm a bit confused though... I installed the plugin, restarted everything, and we're using a plugin that isn't used in the MARC import background job.

So I feel like we're not really addressing the root problem here.

Actually... looking at the log /var/log/koha/kohadev/worker-output.log again I see this:

Argument "{VERSION}" isn't numeric in int at /kohadevbox/koha/Koha/Plugins/Base.pm line 339.
Argument "{VERSION}" isn't numeric in int at /kohadevbox/koha/Koha/Plugins/Base.pm line 339.

I'm going to investigate that a bit further...
Comment 8 Nick Clemens (kidclamp) 2024-07-23 02:13:43 UTC
Ah, so, basically the background job looks for plugins with an 'after_biblio_action' hook. To find them we end up loading all the plugins.modules. This is what causes the begin block to fire again. If we call GetPlugins before this check, everything is cached and we don't reload the modules, so we don't trigger the new dbic
Comment 9 David Cook 2024-07-23 02:44:41 UTC
Okie dokie...

So Koha/BackgroundJob.pm looks up any plugins by querying the database for the method "background_tasks". Excellent. That means only plugins that have the background_tasks method get loaded by the background worker when it's getting set up (prior to forking).

448         my @plugins = Koha::Plugins->new()->GetPlugins( { method => 'background_tasks', } );

--

Unfortunately... during the work of Koha/BackgroundJob/MARCImportCommitBatch.pm, we call C4::Biblio::ModBiblioMarc, which runs the following code:

2914     Koha::Plugins->call(
2915         'before_biblio_action',
2916         { action => 'save', payload => { biblio_id => $biblionumber, record => $record } }
2917     );

The "call" function loads every installed plugin and then checks if the Perl object "can" do a particular method. 

That causes the problem that Kyle and Nick have noted. 

--

We could use the patch attached here, but I'm thinking that we might want to first fix the Koha::Plugins->call() so that it uses the database lookup for the method, caches it, and goes from there.

That should hopefully fix this problem *plus* improve the efficiency of Koha::Plugins->call().

I wonder if there's already a bug for that in fact...
Comment 10 David Cook 2024-07-23 02:47:36 UTC
(In reply to Nick Clemens (kidclamp) from comment #8)
> Ah, so, basically the background job looks for plugins with an
> 'after_biblio_action' hook. To find them we end up loading all the
> plugins.modules. This is what causes the begin block to fire again. If we
> call GetPlugins before this check, everything is cached and we don't reload
> the modules, so we don't trigger the new dbic

Something like that. I guess I haven't worked out the exact cause. Just that it's C4::Biblio::ModBiblioMarc loading all the modules, and that's triggering that BEGIN block on the Koha Advent plugin, which is doing that connection reset within the BatchMarcCommit transaction.
Comment 11 David Cook 2024-07-23 03:04:27 UTC
(In reply to David Cook from comment #9)
> We could use the patch attached here, but I'm thinking that we might want to
> first fix the Koha::Plugins->call() so that it uses the database lookup for
> the method, caches it, and goes from there.

It looks like I talked to Julian about this years ago on bug 29672. I thought there was a newer bug...

--

I was concerned about RAM usage here, but it looks like you've added the "Koha::Plugins->get_enabled_plugins();" after the background_jobs_worker.pl forks, so that's good. 

It would be more efficient to update Koha::Plugins->call(), but that would be more work. This is a practical solution... 

--
On a side note, for Koha::Plugin::FancyPlugin, does the Koha::Schema->register_class and Koha::Database->schema({ new => 1 }) have to be in the BEGIN block?

Could you wrap it with a check for a plack environmental variable?
Comment 12 David Cook 2024-07-23 03:06:39 UTC
Created attachment 169389 [details] [review]
Bug 36736: Load plugins at the start of background job processing

This patch adds a call to get_enabled_plugins before processing background jobs to ensure
that all plugin hooks are loaded and cached

To test:
1 - Install a plugin that adds new objects e.g. the Contracts plugin
    https://github.com/bywatersolutions/fs-koha-plugin-contracts
    or the KOha Advent plugin:
    https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin
2 - Restart all
3 - Tail all your logs
4 - Stage and import a file containing items
5 - Note in the logs
    DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction
6 - Apply this patch
7 - Restart all
8 - Stage and import again
9 - Success!

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 13 David Cook 2024-07-23 03:07:09 UTC
If someone else adds a sign off, I think I could convert this to a Passed QA.
Comment 14 Martin Renvoize (ashimema) 2024-07-23 11:57:00 UTC
Created attachment 169410 [details] [review]
Bug 36736: Load plugins at the start of background job processing

This patch adds a call to get_enabled_plugins before processing background jobs to ensure
that all plugin hooks are loaded and cached

To test:
1 - Install a plugin that adds new objects e.g. the Contracts plugin
    https://github.com/bywatersolutions/fs-koha-plugin-contracts
    or the Koha Advent plugin:
    https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin
2 - Restart all
3 - Tail all your logs
4 - Stage and import a file containing items
5 - Note in the logs
    DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction
6 - Apply this patch
7 - Restart all
8 - Stage and import again
9 - Success!

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Tomás Cohen Arazi (tcohen) 2024-07-23 14:33:14 UTC
I'm writing tests for this. Hold on.
Comment 16 Tomás Cohen Arazi (tcohen) 2024-07-23 18:53:12 UTC
Created attachment 169440 [details] [review]
Bug 36736: Tests [POC]
Comment 17 Tomás Cohen Arazi (tcohen) 2024-07-23 18:56:12 UTC
Hi, my work day is ending and I wanted to share what I've got so far. I haven't managed to reproduce the error so far.

I've added:
* A schema file
* Koha::Object-based classes for that schema
* Tweaked the Test plugin so it has the usual ->register call
* A plugin background job that (for now) just prints the schema it finds.

I expected it to not have the schemas defined, but they are.

I hope I missed something obvious and someone beats me on finishing the tests before I wake up :-D
Comment 18 David Cook 2024-07-24 00:03:09 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> Hi, my work day is ending and I wanted to share what I've got so far. I
> haven't managed to reproduce the error so far.
> 
> I've added:
> * A schema file
> * Koha::Object-based classes for that schema
> * Tweaked the Test plugin so it has the usual ->register call
> * A plugin background job that (for now) just prints the schema it finds.
> 
> I expected it to not have the schemas defined, but they are.
> 
> I hope I missed something obvious and someone beats me on finishing the
> tests before I wake up :-D

You won't be able to reproduce the problem in "t/db_dependent/Koha/Plugins/Plugins.t", because Koha::Plugin::Test is already loaded in the BEGIN block of this test file. We'll need to have a separate test file just for this test.

We'll want to add a Koha::Plugins->call() within a database transaction into Koha::BackgroundJob::TestBackgroundJob->process(), because that's what triggers the bug.

Koha::Plugins->call() loads all enabled plugins, which will then reset the connection, and since we're in a database transaction, then we'll get a lock issue I believe.
Comment 19 David Cook 2024-07-24 00:07:21 UTC
Let's see if I can really quickly bang this out...
Comment 20 David Cook 2024-07-24 01:50:25 UTC
(In reply to David Cook from comment #19)
> Let's see if I can really quickly bang this out...

Interrupted by too many meetings. Hoping to sit down and look at this later today.
Comment 21 David Cook 2024-07-24 05:22:31 UTC
I've had a go at writing the test, but I've actually made it so that even with the patch, the problem will still occur.

I think because we wrap all our tests with transactions it makes the database connection stuff problematic. 

But then again... I'm having all kinds of weird database issues in this test. 

Like I'm inserting data before the transactions... and I still can't get the following to work:

my @plugins = Koha::Plugins->new()->GetPlugins( { method => 'background_tasks', } );

But I've wasted way too much time on this already... I don't think I can work on this anymore.
Comment 22 David Cook 2024-07-24 05:29:36 UTC
Btw another thing I noticed with the current attached test... in order to utilize Nick's change, you need to use create the background job via Koha::BackgroundJob and call its process() method, which then will invoke the background job type listed in its "type" attribute.

Like Julian says on bug 29672, maybe we should be relying on a standardized bootstrap step instead. 

In any case, I think there's a lot of pain ahead with this in any case...
Comment 23 Tomás Cohen Arazi (tcohen) 2024-07-24 16:43:42 UTC
It seems I forgot to add my last attempt yesterday:

```
index 460b710a4e5..58ffc9aadac 100755
--- a/t/db_dependent/Koha/Plugins/Plugins.t
+++ b/t/db_dependent/Koha/Plugins/Plugins.t
@@ -322,9 +322,13 @@ subtest 'Koha::Plugin::Test' => sub {

         plan tests => 1;

-        use DDP;
-        my $job = Koha::BackgroundJob::TestBackgroundJob->new;
-        $job->process;
+        # Launch the sleep script
+        my $pid = fork();
+        if ( $pid == 0 ) {
+            use DDP;
+            my $job = Koha::BackgroundJob::TestBackgroundJob->new;
+            $job->process;
+        }
```

which didn't work either :-P
Comment 24 Tomás Cohen Arazi (tcohen) 2024-07-24 16:49:55 UTC
Created attachment 169508 [details] [review]
Bug 36736: Tests [POC]
Comment 25 Tomás Cohen Arazi (tcohen) 2024-07-24 16:50:59 UTC
So it now explodes with 

`Exception 'Koha::Exception' thrown 'test_background_job is not a valid job_type'`

meaning the plugins didn't get initialized?
Comment 26 David Cook 2024-07-24 23:29:29 UTC
(In reply to Tomás Cohen Arazi from comment #25)
> So it now explodes with 
> 
> `Exception 'Koha::Exception' thrown 'test_background_job is not a valid
> job_type'`
> 
> meaning the plugins didn't get initialized?

Yeah, that was the same error I ended up dealing with. I didn't have time to puzzle it out.

(In reply to David Cook from comment #21)
> But I've wasted way too much time on this already... I don't think I can
> work on this anymore.

Apologies if this sounded harsh. I was just feeling really frustrated after struggling for a long time with the unit test. I really can't rationalize spending more time on this one, when I've got a lot of other things that I know I can make progress instead of spinning my wheels :/.
Comment 27 David Cook 2024-07-24 23:31:01 UTC
Still think the change overall is interesting though! 

I do think that some of the issues with the unit tests does suggest that this current strategy might not be enough, and that maybe we should try to plan/design this more carefully...
Comment 28 Martin Renvoize (ashimema) 2024-07-25 11:34:01 UTC
I'm tempted to move the tests to a new bug.. whilst it would be lovely to cover it I'm keen to get the fix into core sooner rather than later.
Comment 29 Kyle M Hall (khall) 2024-07-25 11:35:47 UTC
(In reply to Martin Renvoize from comment #28)
> I'm tempted to move the tests to a new bug.. whilst it would be lovely to
> cover it I'm keen to get the fix into core sooner rather than later.

+1
Comment 30 Tomás Cohen Arazi (tcohen) 2024-07-25 13:32:57 UTC
Created attachment 169555 [details] [review]
Bug 36736: Load plugins at the start of background job processing

This patch adds a call to get_enabled_plugins before processing background jobs to ensure
that all plugin hooks are loaded and cached

To test:
1 - Install a plugin that adds new objects e.g. the Contracts plugin
    https://github.com/bywatersolutions/fs-koha-plugin-contracts
    or the Koha Advent plugin:
    https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin
2 - Restart all
3 - Tail all your logs
4 - Stage and import a file containing items
5 - Note in the logs
    DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction
6 - Apply this patch
7 - Restart all
8 - Stage and import again
9 - Success!

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi (tcohen) 2024-07-25 13:34:59 UTC
(In reply to Martin Renvoize from comment #28)
> I'm tempted to move the tests to a new bug.. whilst it would be lovely to
> cover it I'm keen to get the fix into core sooner rather than later.

+1
Comment 32 Katrin Fischer 2024-08-16 14:30:12 UTC
Is that really major or not more an enh?
Comment 33 Katrin Fischer 2024-08-16 14:44:36 UTC
Hm, the title of the bug and the content of the patch don't seem to match up. Maybe we should update the bug description?
Comment 34 Katrin Fischer 2024-08-16 15:37:03 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 35 David Cook 2024-08-26 01:59:44 UTC
I'm curious how this would interact with bug 36721, as I preload Koha::Schema there into the Starman master process.
Comment 36 Kyle M Hall (khall) 2024-08-26 11:11:19 UTC
(In reply to David Cook from comment #35)
> I'm curious how this would interact with bug 36721, as I preload
> Koha::Schema there into the Starman master process.

My instinct is that your patch won't interact with this one, as the only module you load from the Koha namespace is Koha::Schema which itself doesn't anything else in the Koha or C4 namespaces. I suppose that begs the question of "should we include Koha/C4 packages in Preload.pm" but that's more of a question to be answered on that bug :)
Comment 37 David Cook 2024-08-27 00:23:41 UTC
(In reply to Kyle M Hall from comment #36)
> (In reply to David Cook from comment #35)
> > I'm curious how this would interact with bug 36721, as I preload
> > Koha::Schema there into the Starman master process.
> 
> My instinct is that your patch won't interact with this one, as the only
> module you load from the Koha namespace is Koha::Schema which itself doesn't
> anything else in the Koha or C4 namespaces. I suppose that begs the question
> of "should we include Koha/C4 packages in Preload.pm" but that's more of a
> question to be answered on that bug :)

Yeah, I don't think it will conflict either. I figure your plugin just adds on top of the Koha::Schema::ResultSet classes, so it should be fine...