[deb/rpm] try restart on upgrade #20473

jsoref · 2018-07-04T22:12:52Z

If submitting code, have you included unit tests that cover the changes?
no
If submitting code, have you tested and built your code locally prior to submission with yarn test && yarn build?
no
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
yes

systemd has this naturally

elasticmachine · 2018-07-04T22:12:53Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

timroes · 2018-07-05T06:59:18Z

Jenkins, test this

/cc @elastic/kibana-operations

elasticmachine · 2018-07-05T08:11:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jsoref · 2018-07-05T14:09:07Z

I don't see how the build failure corresponds to my changes...

mistic · 2018-07-05T15:13:12Z

Jenkins, test this

mistic

Overall it looks fine to me, I just point 2 small things @jsoref 😃

mistic · 2018-07-05T15:22:29Z

src/dev/build/tasks/os_packages/package_scripts/postinst_upgrade.sh

@@ -0,0 +1,15 @@
+#!/bin/sh


I believe the name of this file could be changed to post_upgrade.sh in order to follow the same logic of the others 😃

mistic · 2018-07-05T15:23:38Z

src/dev/build/tasks/os_packages/service_templates/sysv/etc/init.d/kibana

@@ -151,9 +155,15 @@ case "$1" in
    fi
    exit $code
    ;;
+  condrestart|try-restart)


why the addition of two options condrestart and try-restart ? Isn't try-restart enough?

This was copy-paste from someone else's script. There are essentially two different verbs in common use for this action. Since I'm adding the thing, it seemed reasonable to support both for humans as opposed to just for this thing itself.

I don't have a strong opinion about this. My systems going forward are Debian derivatives with systemd...

https://www.cyberciti.biz/faq/linux-unix-sysvinit-services-restart-vs-reload-vs-condrestart/ for lack of a better reference

@jsoref I also found docs about this here. I'm fine with it!

jsoref · 2018-07-05T16:15:40Z

src/dev/build/tasks/os_packages/package_scripts/postinst_upgrade.sh

+set -e
+
+if command -v systemctl >/dev/null ; then
+    systemctl reload kibana.service


This apparently doesn't do what I wanted sigh.

I'll change it to systemctl daemon-reload

Good one here, it escaped to me! We want to reload the systemd manager configuration here and the correct is systemctl daemon-reload.

elasticmachine · 2018-07-05T16:25:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jbudz · 2018-07-05T16:27:54Z

Thanks jsoref! No worries about the build for now, it's unrelated.

rename file; properly reload daemon

mistic

@jsoref It's good for me now! Thanks for the work!
What do you think @jbudz ?

jbudz · 2018-07-09T20:45:47Z

I believe fpm does some magic with --after-upgrade by wrapping the postrm script with a few if statements to handle remove/upgrade. The post remove script already covers these cases, and so when it gets wrapped and called as a function it loses arguments($1) and doesn't know what to do.

root@kbdeb64:/vagrant# sudo dpkg --purge kibana
(Reading database ... 56185 files and directories currently installed.)
Removing kibana (7.0.0-alpha1-SNAPSHOT) ...
post remove script called with unknown argument `'
dpkg: error processing package kibana (--purge):
 subprocess installed post-removal script returned error exit status 1

cat /var/lib/dpkg/info/kibana.postrm

jbudz · 2018-07-09T20:47:19Z

Dropping the 6.3 label, normally it would go in as a bug fix but the amount the testing involved is significant for a patch release :)

jbudz · 2018-07-09T21:00:01Z

I'd have to think about it more but I'm wondering if we can add try-restart to https://github.com/elastic/kibana/blob/master/src/dev/build/tasks/os_packages/package_scripts/post_install.sh#L7.

In deb we can detect upgrade by checking configure for $2 which will have the version we're upgrading from.
In rpm we can detect upgrade by the argument 1 is initial installation, and the argument 2 is upgrade.

mistic · 2018-07-10T01:29:18Z

@jbudz it looks like fpm is already doing a thing close to what you suggest at least for deb (https://github.com/jordansissel/fpm/blob/master/templates/deb/postinst_upgrade.sh.erb#L49).

Also for rpm it looks like it is relying on post-install because it is under %post section (https://github.com/jordansissel/fpm/blob/master/templates/rpm.erb#L185) and the %post section maps to after_install (https://github.com/jordansissel/fpm/blob/master/templates/rpm.erb#L213).

Do you believe the problem you talk about in the previous comment related with "fpm does some magic with --after-upgrade by wrapping the postrm script with a few if statements to handle remove/upgrade" could still exists? Or do my findings solve the doubts?

jbudz · 2018-07-11T19:14:05Z

fpm is doing it but we need it for other reasons, so it's kinda getting in the way. i'd prefer that we kept full control to be as flexible as possible but i could be convinced.

I opened jsoref#2 to demo what I was going with above. @jsoref if you don't have time lmk and I can run with it in a separate PR - thanks for all your patience on this.

jbudz · 2018-07-11T19:19:58Z

one other note for anyone testing: this won't work for one release cycle. the post_remove script of the old package will stop kibana, and try-restart will check if kibana is running and it won't be. i'd recommend installing new builds twice or creating a fake newer version.

mistic · 2018-07-11T19:28:06Z

@jbudz I'm not against what you have on jsoref#2

For me is good to go once it get merged into this PR!

jbudz

LGTM

Example test cases:

rpm/deb need to be tested separately, there are conditions for both and the script orders are different

upgrade from old to this:

install 6.3
start kibana
install 6.4
- kibana should be stopped (edge case - old 6.3 post_remove script still exists)
remove kibana
- no errors

upgrade from this to future:

install 6.4
start kibana
install 6.4+
- kibana should be running
remove kibana
- no errors

init scripts:

install 6.4
/etc/init.d/kibana try-restart
- kibana is not running
/etc/init.d/kibana start
/etc/init.d/kibana try-restart
- kibana is running
/etc/init.d/kibana stop
- kibana is not running
systemctl start kibana.service
systemctl try-restart kibana.service
- kibana is running

Release notes:
Future versions of kibana's deb/rpm packages will restart on upgrade

jbudz · 2018-07-11T21:10:30Z

Thanks a bunch @jsoref we're going to get one more reviewer on this before merging.

tylersmalley · 2018-07-18T18:27:06Z

The more I think about this, the more I am of the opinion that we should touch the service at all. There are possible issues with file locks if we let the service continue to run - and a restart is not likely to work if there are plugins which need to be upgraded.

tylersmalley

@jsoref, I really appreciate this PR and the effort that went into it. I have discussed with some other folks within the company, and the consensus was for the install scripts to avoid interacting with the service. This allows the user to decide how they intend on handling the service. This is also the path which Logstash has taken.

jsoref · 2018-07-18T19:23:46Z

@tylersmalley: I don't really have an opinion on this thing. All I can say is that the current behavior (as described in #9863) is really awful. Someone complained about it, I looked, found a vaguely reasonable approach and offered it.

If you or someone else plan to fix this some other way, that's great. If you plan to leave it broken, that's not great. If you want me to do something else, I'm open to suggestions w/in reason (I'm not rewriting your entire codebase, but a <20 line delta against where you are today or any state my PR has been would be reasonable).

As a user of these services, the fact that upgrades don't actually work w/ a simple command is IMHO a bug, and I wonder if there's any intention of fixing that (I wouldn't expect that to be done for this issue / PR, but, I'm asking about a roadmap).

As a general rule, I'm in favor of consistency, so if logstash has decided to not touch the service but no one updated kibana to have the same behavior, then making kibana consistent would be a positive change in my book. -- That said, I can't figure out where the equivalent-ish code lives in logstash.

I'm not sure how well logstash's approach works -- most modern things delay-load/dynamically load modules as needed, which means that if an installer replaces files on disk and they haven't yet been loaded by a program, then when the program decides to load them, if it gets a version that's newer (and probably incompatible w/ its running instance), they tend to explode -- I'm very used to java applications having exactly this problem when the java runtime is upgraded and the java process isn't. -- We have tried experimenting w/ addon modules in logstash and they seem to explode when things aren't perfect (and they certainly seem like they're loading dynamically).

rashmivkulkarni · 2018-10-10T15:15:08Z

There have been lots of changes in master . I would recommend pulling master into your branches and re run the tests.

elasticmachine · 2018-10-10T16:17:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-12-27T01:03:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jsoref added 2 commits July 4, 2018 14:04

Add condrestart/try-restart to sysv init script

b2c854d

systemd has this naturally

On upgrade, only restart services instead of stopping them

434a4a9

elastic#9863

mistic added Team:Operations Team label for Operations Team review labels Jul 5, 2018

mistic requested changes Jul 5, 2018

View reviewed changes

mistic added v7.0.0 v6.4.0 v6.3.1 labels Jul 5, 2018

mistic requested a review from jbudz July 5, 2018 15:30

jsoref commented Jul 5, 2018

View reviewed changes

jinmu03 added v6.3.2 and removed v6.3.1 labels Jul 5, 2018

jsoref added 2 commits July 6, 2018 01:13

Changes for postupgrade

e131875

rename file; properly reload daemon

Update script for rename

6727298

mistic approved these changes Jul 9, 2018

View reviewed changes

jbudz removed the v6.3.2 label Jul 9, 2018

jbudz added 2 commits July 11, 2018 10:18

updates

81be23b

fn fixes

b425cb1

jbudz approved these changes Jul 11, 2018

View reviewed changes

jbudz changed the title ~~Try restart~~ [deb/rpm] try restart on upgrade Jul 16, 2018

jbudz requested a review from tylersmalley July 16, 2018 16:11

tylersmalley suggested changes Jul 18, 2018

View reviewed changes

LeeDr added v6.5.0 and removed v6.5.0 v6.4.0 labels Jul 25, 2018

spalger added v6.6.0 and removed v6.6.0 v6.5.0 labels Oct 25, 2018

jsoref force-pushed the try-restart branch from 549921d to b425cb1 Compare December 27, 2018 00:59

rayafratkina added v6.7.0 and removed v6.6.0 labels Jan 29, 2019

snide added v7.0.1 and removed v7.0.0 labels Apr 10, 2019

jsoref closed this May 6, 2019

jsoref deleted the try-restart branch May 6, 2019 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[deb/rpm] try restart on upgrade #20473

[deb/rpm] try restart on upgrade #20473

jsoref commented Jul 4, 2018 •

edited

Loading

elasticmachine commented Jul 4, 2018

timroes commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

jsoref commented Jul 5, 2018

mistic commented Jul 5, 2018

mistic left a comment

mistic Jul 5, 2018

jsoref Jul 5, 2018

mistic Jul 5, 2018

jsoref Jul 5, 2018

jsoref Jul 5, 2018

mistic Jul 5, 2018

jsoref Jul 5, 2018

mistic Jul 5, 2018

elasticmachine commented Jul 5, 2018

jbudz commented Jul 5, 2018

mistic left a comment

jbudz commented Jul 9, 2018

jbudz commented Jul 9, 2018

jbudz commented Jul 9, 2018

mistic commented Jul 10, 2018

jbudz commented Jul 11, 2018

jbudz commented Jul 11, 2018

mistic commented Jul 11, 2018

jbudz left a comment •

edited

Loading

jbudz commented Jul 11, 2018

tylersmalley commented Jul 18, 2018

tylersmalley left a comment

jsoref commented Jul 18, 2018

rashmivkulkarni commented Oct 10, 2018

elasticmachine commented Oct 10, 2018

elasticmachine commented Dec 27, 2018

[deb/rpm] try restart on upgrade #20473

[deb/rpm] try restart on upgrade #20473

Conversation

jsoref commented Jul 4, 2018 • edited Loading

elasticmachine commented Jul 4, 2018

timroes commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

💔 Build Failed

jsoref commented Jul 5, 2018

mistic commented Jul 5, 2018

mistic 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

Choose a reason for hiding this comment

elasticmachine commented Jul 5, 2018

💔 Build Failed

jbudz commented Jul 5, 2018

mistic left a comment

Choose a reason for hiding this comment

jbudz commented Jul 9, 2018

jbudz commented Jul 9, 2018

jbudz commented Jul 9, 2018

mistic commented Jul 10, 2018

jbudz commented Jul 11, 2018

jbudz commented Jul 11, 2018

mistic commented Jul 11, 2018

jbudz left a comment • edited Loading

Choose a reason for hiding this comment

jbudz commented Jul 11, 2018

tylersmalley commented Jul 18, 2018

tylersmalley left a comment

Choose a reason for hiding this comment

jsoref commented Jul 18, 2018

rashmivkulkarni commented Oct 10, 2018

elasticmachine commented Oct 10, 2018

💔 Build Failed

elasticmachine commented Dec 27, 2018

💔 Build Failed

jsoref commented Jul 4, 2018 •

edited

Loading

jbudz left a comment •

edited

Loading