Skip to content
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

Don't retry DB errors for 5m on daemon startup #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yhabteab
Copy link
Member

See the inline comments for more context.

@yhabteab yhabteab added the bug Something isn't working label Jun 21, 2024
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jun 21, 2024
// e.g. on Debian based systems, and since the unit type is set to `notify`, make sure you don't block the
// installation if no database is configured yet.
dbCtx, cancelFunc := context.WithTimeout(ctx, 5*time.Second)
defer cancelFunc()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This defer seems useless. In case of a logger.Fatalf, the logger exits the program with a call to os.Exit and otherwise, cancelFunc will be called directly.

@julianbrost
Copy link
Collaborator

When installing Icinga Notifications via our packages, the systemd service will be started automatically e.g. on Debian based systems, and since the unit type is set to notify, make sure you don't block the installation if no database is configured yet.

This change somewhat defeats the purpose of the retry mechanism. It exists in part to account for delayed database startup.

Even with this change, the service wouldn't start properly, it would just fail earlier. So wouldn't this still result in an error on package installation?

What is the point in automatically starting a service if it can't work without configuring it first? Not sure what Debian's packaging policies say about this but to me not automatically starting a service sounds like the only reasonable thing in that situation.

@yhabteab
Copy link
Member Author

Even with this change, the service wouldn't start properly, it would just fail earlier. So wouldn't this still result in an error on package installation?

I don't think so, yes, of course the service wouldn't actually start properly, but it shouldn't interfere with the package installation in any way. Btw, this is how Debian starts the service in the end deb-systemd-invoke start 'icinga-notifications.service' >/dev/null || true.

What is the point in automatically starting a service if it can't work without configuring it first? Not sure what Debian's packaging policies say about this but to me not automatically starting a service sounds like the only reasonable thing in that situation.

Well, I don't think Debian will automatically start your systemd service out of nowhere, but only if you include this small but magical comment #DEBHELPER# in your postinst script.

@julianbrost
Copy link
Collaborator

Even with this change, the service wouldn't start properly, it would just fail earlier. So wouldn't this still result in an error on package installation?

I don't think so, yes, of course the service wouldn't actually start properly, but it shouldn't interfere with the package installation in any way. Btw, this is how Debian starts the service in the end deb-systemd-invoke start 'icinga-notifications.service' >/dev/null || true.

Depends on how you define interfere. It would still block it for 5 seconds. Also, there's still stderr, so there might still be an error message. And it will leave behind a failed unit, so systemctl status will show the system to be in a degraded state.

What is the point in automatically starting a service if it can't work without configuring it first? Not sure what Debian's packaging policies say about this but to me not automatically starting a service sounds like the only reasonable thing in that situation.

Well, I don't think Debian will automatically start your systemd service out of nowhere, but only if you include this small but magical comment #DEBHELPER# in your postinst script.

Well yes and no. If you omit #DEBHELPER# from a custom postinst script, the auto-generated commands won't make it to the final package. However, if there was no custom postinst, it should still add one with just the auto-generated commands. You can tweak the behavior of what debhelper generates, see dh_installsystemd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants