-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic Service Restart when Service is Enabled (Debian) #4530
Conversation
Taking a look. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4530 +/- ##
=======================================
Coverage 92.17% 92.17%
=======================================
Files 192 192
Lines 6297 6297
=======================================
Hits 5804 5804
Misses 493 493 ☔ View full report in Codecov by Sentry. |
Test: Use 2.10.0 as base, generate 2.11.0 using this PR commits
|
Test: Use 2.10.0 as base, generate 2.11.0 using this PR commits
|
Test: Use 2.10.0 as base, generate 2.11.0 using this PR commits
|
Additional test: install 2.11.0 with this PR commit from scratch, no prior installation, expect no changes.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wieso-itzi ,
Thanks very much for the contribution.
I have tested the change with some local builds and works well.
Please check the comments I send here.
Let me know your opinions on those.
Thanks.
echo "### You can start opensearch-dashboards service by executing" | ||
echo " sudo systemctl start opensearch-dashboards.service" | ||
# Restart the service after an upgrade | ||
if [ "$1" == "configure" ] && [ "$2" != "" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this line to:
if [ "$1" = "configure" ] && [ -n "$2" ]; then
[ foo = bar ]
is POSIX correct.[ foo == bar ]
is a bashism and should be avoided.-n
True if the length of string is nonzero, so it should be better than just check[ "$2" != "" ]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to posix compliance and the usage of "-n" looks good to me, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. @wieso-itzi if you commit above changes for both opensearch and opensearch-dashboards, I will approve this PR right after.
Thanks.
scripts/pkg/build_templates/opensearch-dashboards/deb/debian/postinst
Outdated
Show resolved
Hide resolved
echo " sudo systemctl start opensearch-dashboards.service" | ||
# Restart the service after an upgrade | ||
if [ "$1" == "configure" ] && [ "$2" != "" ]; then | ||
if command -v systemctl >/dev/null && systemctl is-enabled opensearch-dashboards.service >/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the unit is enabled but not running, we don't want the upgrade to start it.
If the unit is disabled but the service is running, we want the upgrade to restart it.
if command -v systemctl >/dev/null && systemctl is-enabled opensearch-dashboards.service >/dev/null; then | |
if command -v systemctl >/dev/null && systemctl is-active opensearch-dashboards.service >/dev/null; then |
Same for the other services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the prerm
script stop the service, so this would never work. What a mess 😒 … My vote would be to NOT stop the services at the prerm
stage, and systemctl try-restart opensearch-dashboards.service
in the postinst
stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
@peterzhuamazon what are your thoughts on this? Technically I could implement it 2 ways:
- Don't stop the service in prerm and preinst, when the command line arguments to those scripts show the package is being upgraded, then check if the service is active (or maybe both active and enabled) in postinst and restart it.
- Stop the service in prerm/preinst but create a file (e. g. /tmp/opensearch-upgrade-flag), when the command line arguments show the package is being upgraded, then in postinst check if the file exists and only start the service if the file exists (and maybe additionally if the service is enabled).
Keep in mind, that if this is implemented, testing will either require to create 2 new package versions with the updated maintainer scripts or to replace /var/lib/dpkg/info/opensearch.prerm with the updated version before an upgrade is attempted.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we dont stop the service in prerm, then it will result in a case where service is already started but necessary files get deleted, which might even cause corrupt indices in data.
If option 2, what is the way to decide if that is a upgrade? Or just removal? The problem with this approach is, if someone issue a removal of version 1, then later they just want to install version 2 from scratch, this /tmp file will treat install from scratch as upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @bbarani @prudhvigodithi into discussion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we dont stop the service in prerm, then it will result in a case where service is already started but necessary files get deleted, which might even cause corrupt indices in data.
If option 2, what is the way to decide if that is a upgrade? Or just removal? The problem with this approach is, if someone issue a removal of version 1, then later they just want to install version 2 from scratch, this /tmp file will treat install from scratch as upgrade.
The way to differentiate between an upgrade or removal in prerm is to check the passed command line arguments. If a package is removed the first command line argument is "remove", if a package is upgraded it is "upgrade". The tmp-file itself would be removed in postinst immediately after starting the service (if the file existed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we dont stop the service in prerm, then it will result in a case where service is already started but necessary files get deleted, which might even cause corrupt indices in data.
Ah! Not a good idea then.
The concept of a "flag file" (option 2) seems really fragile indeed.
Nothing is fine, everything is terrible 😨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then under these circumstances I'm afraid I don't know about a way to implement the functionality for a restart if the service was active before upgrading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smortex what would be your ideal solution to this problem? Can you elaborate so we can look for optimal solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a solution to this problem: Debian manage service a certain way, has all the tooling to make this straightforward, but we want a different behavior and it is a pain.
The current code that check if the service is enabled and stop / start it if so does not really make sense but is probably the least-worse that can be done.
…x of chmown commands to prevent warning messages Signed-off-by: wieso-itzi <[email protected]>
… use '-n' to check if a variable is unequal to an empty string Signed-off-by: wieso-itzi <[email protected]>
Hi @peterzhuamazon , I've now made the 2 changes in the if-statements that check whether postinst is run on an upgrade for both opensearch and opensearch-dashboards. Before merging this please consider the changes @smortex proposed and whether this may be the more preferred behaviour. Thanks |
Hi, I have some discussion about this with my team members, and from my point of view I still think the current implementation with However, I do see arguments to support Thanks. |
Two approaches on restart service during an upgrade:
Thanks. |
There will be conflicts as I am merging some other deb/rpm changes. |
Signed-off-by: Peter Zhu <[email protected]>
I restore the If there is no other suggestions, I think we can stay on this solution and get it merged next week. cc: @bbarani |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @wieso-itzi for the contribution here! I will follow up with the same changes to RPM, and also the updated msgs and release notes later. Thanks! |
Thanks all for your collaboration and feedback! |
Signed-off-by: wieso-itzi <[email protected]> Signed-off-by: Peter Zhu <[email protected]> Co-authored-by: wieso-itzi <[email protected]> Co-authored-by: Peter Zhu <[email protected]>
Description
This change makes it so that the opensearch services are started after upgrades on Debian if they were enabled.
In addition I changed the syntax in the postinst scripts for the chown command from "chown user.group" to "chown user:group" (this changes nothing about the functionality but the former will produce warning messages -> "chown: warning: '.' should be ':'")
Issues Resolved
#3891
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.