Bug 18250 - koha-common should start after memcached
Summary: koha-common should start after memcached
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords:
: 20820 (view as bug list)
Depends on: 18342
Blocks: 17311
  Show dependency treegraph
 
Reported: 2017-03-13 12:53 UTC by Marcel de Rooy
Modified: 2018-07-15 18:56 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18250: Update LSB block of koha-common init script (1.34 KB, patch)
2017-03-13 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18250: Force startup order in postinst script (1.03 KB, patch)
2017-03-13 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18250: Update LSB block of koha-common init script (1.37 KB, patch)
2018-05-28 14:09 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 18250: Force startup order in postinst script (1.05 KB, patch)
2018-05-28 14:10 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 18250: Update LSB block of koha-common init script (1.42 KB, patch)
2018-07-11 01:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18250: Force startup order in postinst script (1.10 KB, patch)
2018-07-11 01:50 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-03-13 12:53:28 UTC

    
Comment 1 Marcel de Rooy 2017-03-13 13:03:57 UTC
Created attachment 61034 [details] [review]
Bug 18250: Update LSB block of koha-common init script

Currently koha-common may start before memcached. We should prevent that.
If we add memcached to the LSB section of the koha-common init script,
Debian's insserv will know about this requirement.

Note: This patch is only a step in resolving the issue.

Test plan:
[1] Copy the updated koha-common script to /etc/init.d
[2] Look for S..koha-common and S..memcached in /etc/rcX.d (say X=5)
[3] Run sudo update-rc.d koha-common disable
[4] Run sudo update-rc.d koha-common enable
[5] Look again for S..koha-common in /etc/rcX.d (say X=5).
    The number for koha-common should now be higher than for memcached.
Comment 2 Mirko Tietgen 2017-03-13 13:08:04 UTC
Memcached is optional for Koha. What happens with this patch if it is not installed?
Comment 3 Marcel de Rooy 2017-03-13 13:17:57 UTC
(In reply to Mirko Tietgen from comment #2)
> Memcached is optional for Koha. What happens with this patch if it is not
> installed?

True. We need to take care of that too.
Comment 4 Marcel de Rooy 2017-03-13 13:18:18 UTC
You will see this warning when disabling:
insserv: warning: current start runlevel(s) (empty) of script `koha-common' overrides LSB defaults (2 3 4 5).
insserv: warning: current stop runlevel(s) (0 1 2 3 4 5 6) of script `koha-common' overrides LSB defaults (0 1 6).
Comment 5 Marcel de Rooy 2017-03-13 13:18:42 UTC
Created attachment 61035 [details] [review]
Bug 18250: Force startup order in postinst script

This is a workaround meant to correct upgrading installs.
We force the order by disable/enable.

Test plan:
[1] When upgrading an existing install, check that the startup order
    after the upgrade has been corrected in /etc/rcX.d (say X=5).
Comment 6 Marcel de Rooy 2017-03-13 13:19:00 UTC
We should check if this change is enough for a new install. When we are installing koha-common with this new LSB block, you would expect a better startup order in /etc/rc.d.

When we are upgrading, we could try to force the order as is done in the second patch. Must still be tested. It is a kind of workaround.
Comment 7 Marcel de Rooy 2017-03-13 13:24:53 UTC
(In reply to Marcel de Rooy from comment #3)
> (In reply to Mirko Tietgen from comment #2)
> > Memcached is optional for Koha. What happens with this patch if it is not
> > installed?
> 
> True. We need to take care of that too.

Maybe an easy way out here is the $all-approach in insserv? (Start after anything else..) Or removing the memcached from init.d with sed when there is no memcached. Only problem there: what happens if someone installs memcached later?
Comment 8 Marcel de Rooy 2017-03-13 13:29:04 UTC
Tested a notexisting service with disable/enable:

insserv: Service notexistingservice has to be enabled to start service koha-common
insserv: exiting now!
Comment 9 Marcel de Rooy 2017-03-13 13:44:04 UTC
Or should we do in postinst:

if memcached_is_installed
    if memcached_not_yet_in_lsb_header
        add to init.d/koha-common
        disable, enable
    fi
fi

That way we would only do the reshuffle once..
Comment 10 Marcel de Rooy 2017-03-13 13:45:58 UTC
So far my proposals; this report now first needs some feedback.
Comment 11 Mirko Tietgen 2017-03-13 14:08:16 UTC
To avoid workarounds, we could just make memcached mandatory. It is widely available in Debian versions. Feels like a cleaner approach to me. Any reasons not to do it?
Comment 12 Mason James 2017-03-28 07:04:55 UTC
(In reply to Mirko Tietgen from comment #11)
> To avoid workarounds, we could just make memcached mandatory. It is widely
> available in Debian versions. Feels like a cleaner approach to me. Any
> reasons not to do it?

Mirko, sounds like a great idea to me

I created a bug for this (BZ-18342)
Comment 13 Marcel de Rooy 2018-05-07 11:23:18 UTC
Bug 18342 got pushed in the meantime. Any further thoughts about this approach? Changing status to enter the arena ;)
Comment 14 Mirko Tietgen 2018-05-15 12:26:19 UTC
I'll have a look at this again.
Comment 15 Mirko Tietgen 2018-05-25 17:52:24 UTC
Testplan does not work apply to systemd. However the patch still works, systemd generates a target file

/run/systemd/generator.late/multi-user.target.wants/koha-common.service

Without patches

># Automatically generated by systemd-sysv-generator
>
>[Unit]
>Documentation=man:systemd-sysv-generator(8)
>SourcePath=/etc/init.d/koha-common
>Description=LSB: Start required services for each Koha instance
>Before=multi-user.target
>Before=multi-user.target
>Before=multi-user.target
>Before=graphical.target
>After=remote-fs.target

[…]

With the patches applied I get

># Automatically generated by systemd-sysv-generator
>
>[Unit]
>Documentation=man:systemd-sysv-generator(8)
>SourcePath=/etc/init.d/koha-common
>Description=LSB: Start required services for each Koha instance
>Before=multi-user.target
>Before=multi-user.target
>Before=multi-user.target
>Before=graphical.target
>After=remote-fs.target
>After=memcached.service

[…]

which means the patches do what they are supposed to do.

I don't know if we have an official policy regarding SysVinit vs. systemd. As far as I can see we rely on the backwards compatibility provided by systemd in other places too.

I'll send a signoff and consider to bundle it together with bug 17311 for a 18.05.00-2 release.
Comment 16 Mirko Tietgen 2018-05-25 17:52:56 UTC
*** Bug 20820 has been marked as a duplicate of this bug. ***
Comment 17 Mirko Tietgen 2018-05-28 14:09:04 UTC
Created attachment 75587 [details] [review]
Bug 18250: Update LSB block of koha-common init script

Currently koha-common may start before memcached. We should prevent that.
If we add memcached to the LSB section of the koha-common init script,
Debian's insserv will know about this requirement.

Note: This patch is only a step in resolving the issue.

Test plan:
[1] Copy the updated koha-common script to /etc/init.d
[2] Look for S..koha-common and S..memcached in /etc/rcX.d (say X=5)
[3] Run sudo update-rc.d koha-common disable
[4] Run sudo update-rc.d koha-common enable
[5] Look again for S..koha-common in /etc/rcX.d (say X=5).
    The number for koha-common should now be higher than for memcached.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 18 Mirko Tietgen 2018-05-28 14:10:22 UTC
Created attachment 75589 [details] [review]
Bug 18250: Force startup order in postinst script

This is a workaround meant to correct upgrading installs.
We force the order by disable/enable.

Test plan:
[1] When upgrading an existing install, check that the startup order
    after the upgrade has been corrected in /etc/rcX.d (say X=5).

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 19 Chris Cormack 2018-07-11 01:50:31 UTC
Created attachment 76855 [details] [review]
Bug 18250: Update LSB block of koha-common init script

Currently koha-common may start before memcached. We should prevent that.
If we add memcached to the LSB section of the koha-common init script,
Debian's insserv will know about this requirement.

Note: This patch is only a step in resolving the issue.

Test plan:
[1] Copy the updated koha-common script to /etc/init.d
[2] Look for S..koha-common and S..memcached in /etc/rcX.d (say X=5)
[3] Run sudo update-rc.d koha-common disable
[4] Run sudo update-rc.d koha-common enable
[5] Look again for S..koha-common in /etc/rcX.d (say X=5).
    The number for koha-common should now be higher than for memcached.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 20 Chris Cormack 2018-07-11 01:50:55 UTC
Created attachment 76856 [details] [review]
Bug 18250: Force startup order in postinst script

This is a workaround meant to correct upgrading installs.
We force the order by disable/enable.

Test plan:
[1] When upgrading an existing install, check that the startup order
    after the upgrade has been corrected in /etc/rcX.d (say X=5).

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 21 Nick Clemens 2018-07-13 11:24:26 UTC
Awesome work all!

Pushed to master for 18.11
Comment 22 Martin Renvoize 2018-07-15 18:56:17 UTC
Pushed to 18.05.x for 18.05.02