Bug 31729 - Enable automatic filesystem refresh in Plack
Summary: Enable automatic filesystem refresh in Plack
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Thomas Klausner
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-10 16:37 UTC by Mason James
Modified: 2024-03-15 15:14 UTC (History)
10 users (show)

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


Attachments
Bug 31729: Enable automatic filesystem refresh in Plack (1.17 KB, patch)
2022-10-10 16:41 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (2) (795 bytes, patch)
2022-10-10 16:46 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (1.20 KB, patch)
2022-10-11 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (2) (819 bytes, patch)
2022-10-11 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31729: Add missing brace (and tidy) (6.07 KB, patch)
2022-10-11 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (6.47 KB, patch)
2022-10-12 06:25 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (2) (833 bytes, patch)
2022-10-12 06:31 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729 - Enable automatic filesystem refresh in Plack (3) (807 bytes, patch)
2022-10-12 07:25 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: watch only perl files (1.17 KB, patch)
2022-10-17 23:04 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (3) (806 bytes, patch)
2022-10-17 23:05 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: add 2 sec loop to koha-watcher, to avoid reload flooding (3.55 KB, patch)
2022-10-18 06:46 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (7.48 KB, patch)
2023-03-23 10:26 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: fix quoting and set sh (941 bytes, patch)
2023-03-23 10:46 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: fix quoting and set sh (941 bytes, patch)
2023-03-23 10:58 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: move $tmpfile to /tmp dir, and add {} (806 bytes, patch)
2023-03-23 11:49 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: renamed log file to plack-watcher.log (1.10 KB, patch)
2023-03-23 13:48 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: renamed log file to plack-watcher.log (1.62 KB, patch)
2023-03-23 14:06 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (11.48 KB, patch)
2023-03-26 13:53 UTC, Mason James
Details | Diff | Splinter Review
Bug 31729: Enable automatic filesystem refresh in Plack (11.64 KB, patch)
2023-08-17 11:10 UTC, Pascal Uphaus
Details | Diff | Splinter Review
Bug 31729: enable plackup hot restart in ktd [Alternate] (5.63 KB, patch)
2023-08-18 08:23 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 31729: enable plackup hot restart in ktd [Alternate] (5.63 KB, patch)
2023-08-18 08:37 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 31729: [Alternate] enable plackup hot restart in ktd (5.68 KB, patch)
2023-08-21 01:29 UTC, David Cook
Details | Diff | Splinter Review
Bug 31729: [Alternate][follow-up] watch whole codebase (831 bytes, patch)
2023-08-21 01:29 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2022-10-10 16:37:33 UTC
this patch adds an automatic filesystem refresh in Plack, 

patch uses the following module...
 https://metacpan.org/pod/Plack::Middleware::Refresh

to test:

 1/ start ktd
 2/ add warn to about.pl file, loadpage, observe no output in log :(
 3/ apply patch to /etc/koha/plack.psgi
 4/ restart plack 
 5/ add warn to about.pl file, loadpage, observe output in log :)
Comment 1 Mason James 2022-10-10 16:41:16 UTC
Created attachment 141577 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

patch uses the following module...
 https://metacpan.org/pod/Plack::Middleware::Refresh

to test:

 1/ start ktd
 2/ add warn to about.pl file, load page, observe no output in log :(
 3/ apply patch to /etc/koha/plack.psgi
 4/ restart plack
 5/ add warn to about.pl file, load page, observe output in log :)
Comment 2 Mason James 2022-10-10 16:46:48 UTC
Created attachment 141578 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack (2)

updating mojo psgi file too, (not sure how to test patch)
Comment 3 Mason James 2022-10-10 19:55:18 UTC
hmm, i've spotted a problem here

switching back to ASSIGNED
Comment 4 David Cook 2022-10-10 22:30:59 UTC
(In reply to Mason James from comment #1)
> to test:
> 
>  1/ start ktd
>  2/ add warn to about.pl file, load page, observe no output in log :(
>  3/ apply patch to /etc/koha/plack.psgi
>  4/ restart plack
>  5/ add warn to about.pl file, load page, observe output in log :)

I don't think Plack::Middleware::Refresh would apply in this situation, since Module::Refresh scans the %INC for modified files. 

With this test plan, I think that it would only seem to work because the Starman worker replying to the request hasn't yet cached the compiled version of about.pl in its memory (in the Plack::App::CGIBin namespace). 

All the more reason for us to move our code out of .pl scripts and into .pm modules hehe.
Comment 5 Mason James 2022-10-11 00:50:01 UTC
(In reply to David Cook from comment #4)
> (In reply to Mason James from comment #1)
> > to test:
> > 
> >  1/ start ktd
> >  2/ add warn to about.pl file, load page, observe no output in log :(
> >  3/ apply patch to /etc/koha/plack.psgi
> >  4/ restart plack
> >  5/ add warn to about.pl file, load page, observe output in log :)
> 
> I don't think Plack::Middleware::Refresh would apply in this situation,
> since Module::Refresh scans the %INC for modified files. 

this seems to be a better solution for ktd

 https://gitlab.com/mjames/koha-reload-starman/-/blob/master/koha-reload-starman
Comment 6 David Cook 2022-10-11 02:30:09 UTC
(In reply to Mason James from comment #5)
> this seems to be a better solution for ktd
> 
>  https://gitlab.com/mjames/koha-reload-starman/-/blob/master/koha-reload-
> starman

Yeah I think inotify is probably the easiest way to do this. 

Personally, I probably wouldn't want to use this feature. I'm happy to make a batch of changes and then run either a "koha-plack --restart" or "koha-plack --reload". 

But it would be nice to have that hot deploy option - at least for ktd.
Comment 7 Martin Renvoize 2022-10-11 07:29:42 UTC
Created attachment 141596 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

patch uses the following module...
 https://metacpan.org/pod/Plack::Middleware::Refresh

to test:

 1/ start ktd
 2/ add warn to about.pl file, load page, observe no output in log :(
 3/ apply patch to /etc/koha/plack.psgi
 4/ restart plack
 5/ add warn to about.pl file, load page, observe output in log :)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-10-11 07:29:46 UTC
Created attachment 141597 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack (2)

updating mojo psgi file too, (not sure how to test patch)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-10-11 07:29:50 UTC
Created attachment 141598 [details] [review]
Bug 31729: Add missing brace (and tidy)

This adds a missing closing bracket and runs perltidy over the .psgi
scripts.
Comment 10 Martin Renvoize 2022-10-11 07:32:12 UTC
Oop, I tested first thing this morning without refreshing to see the comments.
Comment 11 Mason James 2022-10-12 06:25:42 UTC
Created attachment 141714 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

to test:

0/ apt install inotify-tools

1/ apply patch

2/ copy files, and set perms manually
 sudo cp debian/script/koha-plack  /usr/sbin
 sudo cp debian/script/koha-watcher to /usr/sbin
 sudo chmod 755 debian/script/koha-plack  /usr/sbin/koha-plack
 sudo chmod 755 debian/script/koha-watcher to /usr/sbin/koha-watcher

3/ koha-plack --start --watch kohadev

4/ pat a kitten 🐱

5/ modify file in watched dir
 echo `date` >  /usr/share/koha/xxx1
 echo `date` >  /usr/share/koha/xxx2

6/ see HUP/inotify messages in log file
 $ tail -f /var/log/koha/kohadev/plack-error.log
 /usr/share/koha/ MODIFY xxx1
 Sending children hup signal
 /usr/share/koha/ MODIFY xxx2
 Sending children hup signal

7/ observe koha-watch process
  ps -ef | grep koha-watcher | grep -v grep | wc -l
  1

8/ koha-plack --stop kohadev

9/ observe no koha-watch process
  ps -ef | grep koha-watcher | grep -v grep | wc -l
  0
Comment 12 Mason James 2022-10-12 06:31:35 UTC
Created attachment 141715 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack (2)

fix typo
Comment 13 Mason James 2022-10-12 07:25:47 UTC
Created attachment 141716 [details] [review]
Bug 31729 - Enable automatic filesystem refresh in Plack (3)

fix 'elif' typo
Comment 14 Mason James 2022-10-12 07:36:33 UTC
> 5/ modify file in watched dir
>  echo `date` >  /usr/share/koha/xxx1
>  echo `date` >  /usr/share/koha/xxx2


if testing in KTD, try...
  # echo `date` >  /kohadevbox/koha/xxx2
Comment 15 Jonathan Druart 2022-10-14 12:25:46 UTC
Shouldn't we watch for perl files only?

What about git checkout branch? Is it going to send 1 signal per file modified?
Comment 16 Mason James 2022-10-17 23:04:38 UTC
Created attachment 142043 [details] [review]
Bug 31729: watch only perl files
Comment 17 Mason James 2022-10-17 23:05:06 UTC
Created attachment 142044 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack (3)

fix 'elif' typo
Comment 18 Mason James 2022-10-18 06:46:26 UTC
Created attachment 142050 [details] [review]
Bug 31729: add 2 sec loop to koha-watcher, to avoid reload flooding
Comment 19 Mason James 2022-10-18 06:48:09 UTC
(In reply to Jonathan Druart from comment #15)
> Shouldn't we watch for perl files only?
> 
> What about git checkout branch? Is it going to send 1 signal per file
> modified?

hiya, i've added 2 patches to handle these 2 conditions
Comment 20 Olivier Hubert 2023-01-31 20:45:18 UTC
I tried out the latest test plan but cannot continue past Step 3. It seems the test plan is only appropriate for koha installations made with the debian package, is that correct?
Comment 21 Mason James 2023-03-23 10:26:36 UTC
Created attachment 148600 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

to test (on KTD)

- install inotify-tools
  $ sudo apt install inotify-tools

- apply patch
  $ git bz apply 31729

- manually install files and set perms
  $ cd /kohadevbox/koha
  $ sudo cp debian/scripts/koha-plack   /usr/sbin
  $ sudo cp debian/scripts/koha-watcher /usr/sbin
  $ sudo chmod 755 /usr/sbin/koha-plack
  $ sudo chmod 755 /usr/sbin/koha-watcher

- restart, with --watch arg
  $ sudo koha-plack --restart --watch kohadev

- observe koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  3

- modify some perl files in watched dir
  $ echo `date` >  /kohadevbox/koha/xxx.pl
  $ echo `date` >  /kohadevbox/koha/xxx.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-error.log
  CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/
  CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/
  Sending children hup signal

- stop koha-plack
  $ sudo koha-plack --stop kohadev

- observe no koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  0
Comment 22 Mason James 2023-03-23 10:31:26 UTC
(In reply to Olivier Hubert from comment #20)
> I tried out the latest test plan but cannot continue past Step 3. It seems
> the test plan is only appropriate for koha installations made with the
> debian package, is that correct?

hi Olivier
if you follow the updated patch and test plan - you should have success

many thanks :)
Comment 23 Mason James 2023-03-23 10:46:19 UTC
Created attachment 148602 [details] [review]
Bug 31729: fix quoting and set sh
Comment 24 Mason James 2023-03-23 10:58:08 UTC
Created attachment 148603 [details] [review]
Bug 31729: fix quoting and set sh
Comment 25 Mason James 2023-03-23 11:49:58 UTC
Created attachment 148606 [details] [review]
Bug 31729: move $tmpfile to /tmp dir, and add {}
Comment 26 Jonathan Druart 2023-03-23 13:07:42 UTC
Why are you logging in plack-error.log?
Comment 27 Mason James 2023-03-23 13:48:02 UTC
Created attachment 148612 [details] [review]
Bug 31729: renamed log file to plack-watcher.log

to test...

- modify some perl files in watched dir
  $ echo `date` >  /kohadevbox/koha/xxx.pl
  $ echo `date` >  /kohadevbox/koha/xxx.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log <<<<<<<<
  CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/
  CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/
  Sending children hup signal
Comment 28 Mason James 2023-03-23 14:06:31 UTC
Created attachment 148613 [details] [review]
Bug 31729: renamed log file to plack-watcher.log

to test...

- modify some perl files in watched dir
  $ echo `date` >  /kohadevbox/koha/xxx.pl
  $ echo `date` >  /kohadevbox/koha/xxx.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log  \
            /var/log/koha/kohadev/plack-error.log
  CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/
  CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/
  Sending children hup signal
Comment 29 Mason James 2023-03-23 14:16:26 UTC
(In reply to Jonathan Druart from comment #26)
> Why are you logging in plack-error.log?

oops! i've changed log file to plack-watcher.log
Comment 30 Mason James 2023-03-26 13:53:45 UTC
Created attachment 148720 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

to test (on KTD)

- install inotify-tools
  $ sudo apt install inotify-tools

- apply patch
  $ git bz apply 31729

- manually install files and set perms
  $ cd /kohadevbox/koha
  $ sudo cp debian/scripts/koha-plack   /usr/sbin
  $ sudo cp debian/scripts/koha-watcher /usr/sbin
  $ sudo chmod 755 /usr/sbin/koha-plack
  $ sudo chmod 755 /usr/sbin/koha-watcher

- restart, with --watch arg
  $ sudo koha-plack --restart --watch kohadev

- observe koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  3

- modify some perl files in watched dir
  $ echo `date` >  /kohadevbox/koha/xxx.pl
  $ echo `date` >  /kohadevbox/koha/xxx.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pl
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pm
  koha-plack reloaded

- add syntax error to file in watched dir
  $ echo `date` > /kohadevbox/koha/C4/Context.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm
  koha-plack failed! sleeping 2 secs
  koha-plack failed! sleeping 3 secs
  koha-plack failed! sleeping 4 secs
  ...
  koha-plack failed! sleeping 10 secs

- fix syntax error in file in watched dir
  $ git checkout /kohadevbox/koha/C4/Context.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm
  koha-plack reloaded ok

- stop koha-plack
  $ sudo koha-plack --stop kohadev

- observe no koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  0
Comment 31 Katrin Fischer 2023-06-11 16:19:14 UTC
This sounds like it would be great time saver!
Comment 32 Pascal Uphaus 2023-08-17 11:10:14 UTC
Created attachment 154542 [details] [review]
Bug 31729: Enable automatic filesystem refresh in Plack

to test (on KTD)

- install inotify-tools
  $ sudo apt install inotify-tools

- apply patch
  $ git bz apply 31729

- manually install files and set perms
  $ cd /kohadevbox/koha
  $ sudo cp debian/scripts/koha-plack   /usr/sbin
  $ sudo cp debian/scripts/koha-watcher /usr/sbin
  $ sudo chmod 755 /usr/sbin/koha-plack
  $ sudo chmod 755 /usr/sbin/koha-watcher

- restart, with --watch arg
  $ sudo koha-plack --restart --watch kohadev

- observe koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  3

- modify some perl files in watched dir
  $ echo `date` >  /kohadevbox/koha/xxx.pl
  $ echo `date` >  /kohadevbox/koha/xxx.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pl
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pm
  koha-plack reloaded

- add syntax error to file in watched dir
  $ echo `date` > /kohadevbox/koha/C4/Context.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm
  koha-plack failed! sleeping 2 secs
  koha-plack failed! sleeping 3 secs
  koha-plack failed! sleeping 4 secs
  ...
  koha-plack failed! sleeping 10 secs

- fix syntax error in file in watched dir
  $ git checkout /kohadevbox/koha/C4/Context.pm

- see HUP/inotify messages in log file
  $ tail -f /var/log/koha/kohadev/plack-watcher.log
  CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm
  koha-plack reloaded ok

- stop koha-plack
  $ sudo koha-plack --stop kohadev

- observe no koha-watch process
  $ ps -ef | grep koha-watcher | grep -v grep | wc -l
  0

Signed-off-by: Pascal Uphaus <pascal.uphaus@gwdg.de>
Signed-off-by: Thomas Klausner <domm@plix.at>
Signed-off-by: Lari Strand <lari.strand@koha-suomi.fi>
Comment 33 Thomas Klausner 2023-08-17 14:11:10 UTC
FYI, I have another approach nearly finished, where instead of creating a new daemon and using inotify we start the Plack app via plackup (instead of just usnig starman) and use the -R flag to reload when something changed.

This is the common way to do is in Plack apps, and does not need another service.

The patch will be ready tomorrow at the latest

(side note: Not sure if you want the patch in this ticket or in a new one?)
Comment 34 David Cook 2023-08-17 23:51:50 UTC
(In reply to Thomas Klausner from comment #33)
> FYI, I have another approach nearly finished, where instead of creating a
> new daemon and using inotify we start the Plack app via plackup (instead of
> just usnig starman) and use the -R flag to reload when something changed.
> 
> This is the common way to do is in Plack apps, and does not need another
> service.

I use plackup (without -R) for a different Plack app of mine, and I find it doesn't work very well. Maybe it's how I'm using it or some middleware I'm using, but it tends to be slow and seems to hang on some requests until I send another request. 

But it's worth a try.
 
> The patch will be ready tomorrow at the latest
> 
> (side note: Not sure if you want the patch in this ticket or in a new one?)

I think you could attach it here and just mark it as [Alternate]
Comment 35 David Cook 2023-08-17 23:53:45 UTC
Just reiterating that it would be great to have this be "optional", since I wouldn't want to use it. Definitely not in prod, and not even in koha-testing-docker.

I prefer to manually restart my Plack processes.
Comment 36 Mason James 2023-08-18 03:05:52 UTC
(In reply to Thomas Klausner from comment #33)
> FYI, I have another approach nearly finished, where instead of creating a
> new daemon and using inotify we start the Plack app via plackup (instead of
> just usnig starman) and use the -R flag to reload when something changed.
> 
> This is the common way to do is in Plack apps, and does not need another
> service.
> 
> The patch will be ready tomorrow at the latest
> 
> (side note: Not sure if you want the patch in this ticket or in a new one?)

hi Thomas, 

i think that plack needs to be restarted if template files (.inc and .tt) are modified

...if so, i dont think `plackup -R` with handle modifying template files?
Comment 37 David Cook 2023-08-18 03:36:35 UTC
(In reply to Mason James from comment #36)
> i think that plack needs to be restarted if template files (.inc and .tt)
> are modified

Generally it shouldn't need to be restarted if templates are updated. (Although off the top of my head I don't know why since the templates should be cached... 😬)
Comment 38 Thomas Klausner 2023-08-18 08:23:23 UTC
Created attachment 154601 [details] [review]
Bug 31729: enable plackup hot restart in ktd [Alternate]

This patch implements a more idiomatoic way for hot-reloading. Instead
of adding a new daemon/watcher+inotify or use a Middleware, we can use
`plackup -R /path/to/dir`, which is the standard Perl way to do hot
reloading for plack apps.

Instead of calling `/usr/bin/perl starman` directly, we use
`plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to
watch that directory for changes and hot-reload the plack app.

I've added a new option `koha-plack --hot-reload`. Only if this option
is set AND we're running inside koha-testing-docker (by checking
KOHA_INSTANCE = "kohadev") hot reloading is enabled.

Test plan:

Before applying the patch:

* Start koha-testing-docker and enter the container

* Stop plack: koha-plack --stop kohadev

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  You should see lines like
    Worker processes cleaned up
    Server closing!

* Start it again: koha-plack --start kohadev
  Output should be "Starting Plack daemon for kohadev:"
  And you end back at the interactive shell

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  it should look like:
    Starman::Server (type Net::Server::PreFork) starting!
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Stop it again: koha-plack --stop kohadev

Now apply the patch.

As in koha-testing-docker the koha-plack script (after applying the
patch) is NOT installed to /sbin you have to call the dev version.

* Start the server with hot-reloading:
  debian/scripts/koha-plack --start --hot-reload kohadev

* Output should be:
  Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates.

* You are now NOT back in the shell, as the plack app is NOT running
  daemonized

* So you need to open another ktd --shell

* Check the log:
  tail -n 4 /var/log/koha/kohadev/plack-error.log
    Starman::Server (type Net::Server::PreFork) starting! pid(27638)
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Update a code file
  touch Koha/Biblios.pm

* in the other shell (where plack is running) you should see:
  -- /kohadevbox/koha/Koha/Biblios.pm updated.
  Killing the existing server
  Successfully killed! Restarting the new server process.

* Yay!

* If you want to still stop/restart plack manually, you can hit CTRL-C
  in the shell were you started plack to stop the process.
Comment 39 Thomas Klausner 2023-08-18 08:37:07 UTC
Created attachment 154605 [details] [review]
Bug 31729: enable plackup hot restart in ktd [Alternate]

This patch implements a more idiomatoic way for hot-reloading. Instead
of adding a new daemon/watcher+inotify or use a Middleware, we can use
`plackup -R /path/to/dir`, which is the standard Perl way to do hot
reloading for plack apps.

Instead of calling `/usr/bin/perl starman` directly, we use
`plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to
watch that directory for changes and hot-reload the plack app.

I've added a new option `koha-plack --hot-reload`. Only if this option
is set AND we're running inside koha-testing-docker (by checking
KOHA_INSTANCE = "kohadev") hot reloading is enabled.

Test plan:

Before applying the patch:

* Start koha-testing-docker and enter the container

* Stop plack: koha-plack --stop kohadev

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  You should see lines like
    Worker processes cleaned up
    Server closing!

* Start it again: koha-plack --start kohadev
  Output should be "Starting Plack daemon for kohadev:"
  And you end back at the interactive shell

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  it should look like:
    Starman::Server (type Net::Server::PreFork) starting!
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Stop it again: koha-plack --stop kohadev

Now apply the patch.

As in koha-testing-docker the koha-plack script (after applying the
patch) is NOT installed to /sbin you have to call the dev version.

* Start the server with hot-reloading:
  debian/scripts/koha-plack --start --hot-reload kohadev

* Output should be:
  Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates.

* You are now NOT back in the shell, as the plack app is NOT running
  daemonized

* So you need to open another ktd --shell

* Check the log:
  tail -n 4 /var/log/koha/kohadev/plack-error.log
    Starman::Server (type Net::Server::PreFork) starting! pid(27638)
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Update a code file
  touch Koha/Biblios.pm

* in the other shell (where plack is running) you should see:
  -- /kohadevbox/koha/Koha/Biblios.pm updated.
  Killing the existing server
  Successfully killed! Restarting the new server process.

* Yay!

* If you want to still stop/restart plack manually, you can hit CTRL-C
  in the shell were you started plack to stop the process.

Sponsored-by: HKS3 - https://koha-support.eu
Comment 40 David Cook 2023-08-20 23:21:23 UTC
Ohhh, Thomas that sounds very interesting!

Plack::Loader::Restarter is still using  inotify under the hood, but it's much easier to implement and maintain this way.

I'm going to come back to this...
Comment 41 David Cook 2023-08-21 01:27:07 UTC
Comment on attachment 154605 [details] [review]
Bug 31729: enable plackup hot restart in ktd [Alternate]

Review of attachment 154605 [details] [review]:
-----------------------------------------------------------------

Overall, this is looking pretty good. I think I'd just make 1 tweak.

I applied the tweak locally and then I applied some patches using "git bz". I noticed all the regular files (and .git files) were updated and then just restarted the server process 1 time, which was good.

::: debian/scripts/koha-plack
@@ +112,4 @@
>      fi
>  
> +    if [ "$hot_reload" = "yes" ] && [ "$KOHA_INSTANCE" = "kohadev" ] && [ -d "/kohadevbox/koha/Koha" ]; then
> +        plack_hot_reload="-R /kohadevbox/koha/Koha";

I think this should be /kohadevbox/koha since /kohadevbox/koha/Koha is only a subset of the whole codebase
Comment 42 David Cook 2023-08-21 01:29:43 UTC
Created attachment 154676 [details] [review]
Bug 31729: [Alternate] enable plackup hot restart in ktd

This patch implements a more idiomatoic way for hot-reloading. Instead
of adding a new daemon/watcher+inotify or use a Middleware, we can use
`plackup -R /path/to/dir`, which is the standard Perl way to do hot
reloading for plack apps.

Instead of calling `/usr/bin/perl starman` directly, we use
`plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to
watch that directory for changes and hot-reload the plack app.

I've added a new option `koha-plack --hot-reload`. Only if this option
is set AND we're running inside koha-testing-docker (by checking
KOHA_INSTANCE = "kohadev") hot reloading is enabled.

Test plan:

Before applying the patch:

* Start koha-testing-docker and enter the container

* Stop plack: koha-plack --stop kohadev

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  You should see lines like
    Worker processes cleaned up
    Server closing!

* Start it again: koha-plack --start kohadev
  Output should be "Starting Plack daemon for kohadev:"
  And you end back at the interactive shell

* Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log
  it should look like:
    Starman::Server (type Net::Server::PreFork) starting!
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Stop it again: koha-plack --stop kohadev

Now apply the patch.

As in koha-testing-docker the koha-plack script (after applying the
patch) is NOT installed to /sbin you have to call the dev version.

* Start the server with hot-reloading:
  debian/scripts/koha-plack --start --hot-reload kohadev

* Output should be:
  Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates.

* You are now NOT back in the shell, as the plack app is NOT running
  daemonized

* So you need to open another ktd --shell

* Check the log:
  tail -n 4 /var/log/koha/kohadev/plack-error.log
    Starman::Server (type Net::Server::PreFork) starting! pid(27638)
    Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
    Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/

* Update a code file
  touch Koha/Biblios.pm

* in the other shell (where plack is running) you should see:
  -- /kohadevbox/koha/Koha/Biblios.pm updated.
  Killing the existing server
  Successfully killed! Restarting the new server process.

* Yay!

* If you want to still stop/restart plack manually, you can hit CTRL-C
  in the shell were you started plack to stop the process.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 43 David Cook 2023-08-21 01:29:46 UTC
Created attachment 154677 [details] [review]
Bug 31729: [Alternate][follow-up] watch whole codebase

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 44 David Cook 2023-08-21 01:31:14 UTC
Signing off on the "Alternate" patch.
Comment 45 Jonathan Druart 2023-08-30 12:14:17 UTC
I think we should:
1. test DEV_INSTALL instead of KOHA_INSTANCE, and watch KOHA_HOME instead of /kohadevbox/koha
2. Rename hot_reload with watch (what we use to rebuild JS: `yarn js:watch`)
3. There is a tab character l.457 >-----hot-reload)

Then, a question:
4. We are moving from starman to plackup. What do we lose/gain?
I know I was using plackup before the move to kohadevbox.

And, linking to a 2016 related discussion:
5. Found that from a random guy: https://lists.koha-community.org/pipermail/koha-devel/2016-September/043026.html
Comment 46 David Cook 2023-08-30 23:19:38 UTC
(In reply to Jonathan Druart from comment #45)
> I think we should:
> 1. test DEV_INSTALL instead of KOHA_INSTANCE, and watch KOHA_HOME instead of
> /kohadevbox/koha
> 2. Rename hot_reload with watch (what we use to rebuild JS: `yarn js:watch`)
> 3. There is a tab character l.457 >-----hot-reload)

Sounds good to me.

> Then, a question:
> 4. We are moving from starman to plackup. What do we lose/gain?
> I know I was using plackup before the move to kohadevbox.

Good point. We should double-check that --max-requests and --workers flow through as expected.

I was playing around with plackup and Starman on a different project the other day, and there can be subtle differences with options that don't show obvious warnings/errors. Worth taking another look at that.

> And, linking to a 2016 related discussion:
> 5. Found that from a random guy:
> https://lists.koha-community.org/pipermail/koha-devel/2016-September/043026.
> html

Also interesting.

Personally, I'm not even interested in an automatic filesystem refresh for Plack, so I'm not sure why I'm even still involved in this one. I think I originally just commented so that we didn't go down certain problematic paths 😅
Comment 47 Thomas Klausner 2023-08-31 09:37:21 UTC
Maybe it would be easiest to NOT change anything in the koha-plack shell script (which is mostly used for prod, where we don't want the hot reload anyway). But to provided either documentations for koha-testing-docker and/or another wrapper script so people who want hot reloading during dev can start ktd, stop the daemonized koha-plack that's running there per default, and then just start a non-daemonizes hot-reloadable plack for their dev session.

This way nothing will change for "normal" installations etc, but devs can still increase their hack/reload/try-time

Greetings,
domm
Comment 48 Thomas Klausner 2023-08-31 10:51:32 UTC
Here is a rather simple bash script that will set all the envs etc and start Koha via plackup with hot-reload. I did not provide this as a patch (yet), because I don't think this script should go into the Koha repo, as it's tailored specifically for koha-testing-docker.

But I'd still appreciate some feedback (and some hints on where to add this to ktd; or the OK to add it to Koha itself)

cat koha-plack-hotreload

set -e

instancename='kohadev'
instance_user="${instancename}-koha"
placksocket="/var/run/koha/${instancename}/plack.sock"
psgifile="/etc/koha/plack.psgi"

export DEV_INSTALL=1
export KOHA_HOME=/kohadevbox/koha
export KOHA_CONF="/etc/koha/sites/${instancename}/koha-conf.xml"
export PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/koha/installer

# run plackup with hot reload
/usr/bin/plackup -M FindBin -R /kohadevbox/koha --workers 2 --user=${instance_user} --group ${instance_user} -E deployment --socket ${placksocket} -s Starman ${psgifile}

Greetings,
domm
Comment 49 Victor Grousset/tuxayo 2023-11-03 16:22:58 UTC
plackup -R doesn't with --daemonize, could that be a problem?

https://github.com/miyagawa/Starman/issues/37
Comment 50 Olivier Hubert 2023-11-06 14:43:12 UTC
> plackup -R doesn't with --daemonize, could that be a problem?

It's certainly going to be a problem for us, as that is how we run Plack.
Comment 51 Thomas Klausner 2023-11-06 15:30:48 UTC
I think we should NOT apply any of these patches:

Hot reload is only needed during dev, and it's much easier to (manually) start koha-plack in a non-demaonized way with hot-reload in Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to start Koha in production.

This way we won't cause any problems with prod (where hot-reload should never be active anyway).

It's a tiny bit annoying that one first has to start ktd, enter the container, stop the daemonzied version, and then start the hot-reload plack, but as one only has to do this once a day (or once per hack-session..) it's acceptable IMO.

See Comment 48

So I suggest we reject these patches and instead add either docs or a small wrapper script to koha-testing-docker.
Comment 52 Katrin Fischer 2023-11-06 16:19:25 UTC
(In reply to Thomas Klausner from comment #51)
> I think we should NOT apply any of these patches:
> 
> Hot reload is only needed during dev, and it's much easier to (manually)
> start koha-plack in a non-demaonized way with hot-reload in
> Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to
> start Koha in production.

I am not sure if it's only needed during dev time - we have the issue of updating and installing plugins not taking effect without a Plack restart, which would be great to resolve.
Comment 53 Katrin Fischer 2023-11-06 16:25:55 UTC
(In reply to Katrin Fischer from comment #52)
> (In reply to Thomas Klausner from comment #51)
> > I think we should NOT apply any of these patches:
> > 
> > Hot reload is only needed during dev, and it's much easier to (manually)
> > start koha-plack in a non-demaonized way with hot-reload in
> > Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to
> > start Koha in production.
> 
> I am not sure if it's only needed during dev time - we have the issue of
> updating and installing plugins not taking effect without a Plack restart,
> which would be great to resolve.

I might have been on the wrong bug for that one... see bug 31074
Comment 54 Thomas Klausner 2023-11-09 14:29:07 UTC
Here's what I'm using to start a hot-reload server in koha-testing-docker:

ktd --shell

kohadev-koha@kohadevbox:koha$ koha-plack --stop kohadev
Stopping Plack daemon for kohadev:.

kohadev-koha@kohadevbox:koha$ DEV_INSTALL=1 KOHA_HOME=/kohadevbox/koha KOHA_CONF=/etc/koha/sites/kohadev/koha-conf.xml PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/koha/installer /usr/bin/plackup -M FindBin -R /kohadevbox/koha --workers 2 --user=kohadev-koha --group=kohadev-koha  -E deployment --socket /var/run/koha/kohadev/plack.sock -s Starman /etc/koha/plack.psgi

Watching /kohadevbox/koha /etc/koha/plack.psgi for file updates.
2023/11/09-09:15:15 Starman::Server (type Net::Server::PreFork) starting! pid(2344)
Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
Comment 55 Victor Grousset/tuxayo 2023-11-10 16:58:36 UTC
(In reply to Olivier Hubert from comment #50)
> > plackup -R doesn't with --daemonize, could that be a problem?
> 
> It's certainly going to be a problem for us, as that is how we run Plack.

That would have been with --hot-reload so not relevant for prod.
Comment 56 Victor Grousset/tuxayo 2023-11-10 17:04:44 UTC
(In reply to Thomas Klausner from comment #51)
> I think we should NOT apply any of these patches:
> 
> Hot reload is only needed during dev, and it's much easier to (manually)
> start koha-plack in a non-demaonized way with hot-reload in
> Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to
> start Koha in production.
> 
> [...]
> So I suggest we reject these patches and instead add either docs or a small
> wrapper script to koha-testing-docker.

Thanks for your manual solution.

It would still be great to have hot reload on by default on KTD so it's less error prone for everyone to test patches. (at least the vast majority using KTD)
The question is how is it the cleanest. Here in koha-plack. Or with changing the KTD scripts.

This ticket has a counter part in the KTD issue tracker: https://gitlab.com/koha-community/koha-testing-docker/-/issues/307
Comment 57 Thomas Klausner 2024-03-15 15:14:14 UTC
Finally created a pull-req for KTD to add my "fix" to the KTD docs:

https://gitlab.com/koha-community/koha-testing-docker/-/merge_requests/492

I still see no easy way to add this to KTD itself, because KTD is calling koha-plack, and we don't want to add hot-reload there.