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

smokeping service improvements #144127

Merged
merged 8 commits into from
Nov 1, 2021

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Nov 1, 2021

Motivation for this change

Upstreaming here multiple improvements to the smokeping service that we use in our production setup.

Please see the commit messages for the individual motivations.

Things done

CC maintainer: @erictapen
CC recent contributors: @trofi @primeos @aanderse @fadenb @ixmatus @cransom

The bash wrapper process served no purpose, and systemd directly controlling the
processes is more reliable / more responsive to systemctl commands.
Copy link
Member

@erictapen erictapen left a comment

Choose a reason for hiding this comment

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

Thanks for upstreaming this. Would you also be interested in adding yourself as maintainer to the module?

@nh2
Copy link
Contributor Author

nh2 commented Nov 1, 2021

Would you also be interested in adding yourself as maintainer to the module?

OK, done!

@nh2 nh2 force-pushed the smokeping-service-improvements branch from b3a68ba to efaa7c3 Compare November 1, 2021 16:38
Copy link
Contributor

@fadenb fadenb left a comment

Choose a reason for hiding this comment

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

Quick glance: Looks good :)

Copy link
Member

@erictapen erictapen left a comment

Choose a reason for hiding this comment

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

LGTM as well. Could you squash the commits a bit? Maybe one for the host option, one for imgUrl stuff and one for the systemd service changes? One commit for everything would also be fine for me. Nice commit messages btw.

Allows setting smokeping to not listen on the public Internet.
This avoids a common problem:

Until now, port forwarding to multiple hosts running smokeping did not work;
they all show the data of the first smokeping instance.
That ws because the image URLs generated by smokeping are absolute
(`imgurl` setting).
Consequently, if you ran
  ssh node-1 -L 8081:localhost:8081
  ssh node-2 -L 8081:localhost:8082
  ssh node-3 -L 8081:localhost:8083
and try to open http://localhost:8081, http://localhost:8082 and
http://localhost:8083, they all would show the images of node-1!

Using a relative `imgurl` fixes that.
As per smokeping docs on `imgurl`:

> Either an absolute URL to the `imgcache` directory or one relative to the
> directory where you keep the SmokePing cgi.
Details on NixOS/nixops#1063 (comment).

`partOf` makes that if `smokeping.service` is stopped, `thttpd.service` will
be stopped as well.
(But not that `thttpd` will be started when `smokeping` is started).

Once `thttpd.service` is stopped that way, `Restart = always` will not apply.

When the smokeping config options are changed, NixOS's `switch-configuration.pl`
will stop `smokeping` (whit shuts down thttpd due to `partOf`), and then restart
smokeping; but this does not start thttpd.
As a result, thttpd will be off after changing the config, which isn't desired.

This commit fixes it by removing the `partOf`, which makes `Restart` work
as expected.
This makes switch-configuration fail if something is wrong with it,
which is desired especially for NixOps deployments.
In general, NixOS services are configured such that by default
they are not exposed to the Internet for security, see NixOS#100192.
@nh2 nh2 force-pushed the smokeping-service-improvements branch from efaa7c3 to a65e7f0 Compare November 1, 2021 21:46
@nh2
Copy link
Contributor Author

nh2 commented Nov 1, 2021

Could you squash the commits a bit?

Ah, I left an unintentional fixup! commit in, that's fixed now.

Beyond that, the commits are already self-contained, that is, they work independently of each other, so that if one makes a problem in the future, it can be easily reverted.

@erictapen
Copy link
Member

Mh yeah, makes sense.
Thank you!

@erictapen erictapen merged commit 29f4f71 into NixOS:master Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants