-
Notifications
You must be signed in to change notification settings - Fork 141
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
enhancement: allows using multiple web listen addresses #213
enhancement: allows using multiple web listen addresses #213
Conversation
b9f18e5
to
febecbc
Compare
febecbc
to
ee4c479
Compare
ee4c479
to
1513a78
Compare
1513a78
to
279bf0d
Compare
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.
Looks like the version of alertmanager used in the alternative molecule test is too old for multiple web listen addresses.
Please add a version check in the template so that we won't attempt to use the unsupported flag on older versions.
We should probably also add support for multiple web listen addresses in the other roles where possible.
prometheus/exporter-toolkit#91
Do you know which version this was introduced? The changelog doesn't say (searched for both listen-address and toolkit) and I really don't wanna browse Edit: Okay, so I did browse the |
279bf0d
to
74574d4
Compare
I think the change was introduced in this PR prometheus/exporter-toolkit#95 so any version which uses exporter-toolkit v0.8.0 or later (prometheus/exporter-toolkit@6b1221e) Can you add multiple web listen address support to the other roles as well? |
74574d4
to
4066fd4
Compare
@gardar thanks, I was looking for exporter-toolkit in this collection 😅 |
Will do. |
4066fd4
to
65111b1
Compare
We already do include a github authentication token that we get from the CI env, which raised the limit substantially #91
Great! It's always good when you can avoid things such as |
Prometheus itself does not yet support multiple web listen addresses, that's why its alternative is failing:
Tested with both 2.40.0 and 2.48.0. I'm cutting it out but I'm leaving the prep in like I did with both |
Signed-off-by: Christian Krause <[email protected]>
Signed-off-by: Christian Krause <[email protected]>
Signed-off-by: Christian Krause <[email protected]>
Signed-off-by: Christian Krause <[email protected]>
Signed-off-by: Christian Krause <[email protected]>
Signed-off-by: Christian Krause <[email protected]>
0e11f6b
to
38f0e7c
Compare
The last one I don't get working is $ ANSIBLE_DIFF_ALWAYS=True molecule test -s alternative -p ubuntu-22.04
...
--- before [373/9543]
+++ after: /home/umcdev/.ansible/tmp/ansible-local-245143asywggq4/tmp4d3313t_/smokeping_prober.service.j2
@@ -0,0 +1,41 @@
+#
+# Ansible managed: Do NOT edit this file manually!
+#
+[Unit]
+Description=Smokeping Prober
+After=network-online.target
+StartLimitInterval=0
+StartLimitIntervalSec=0
+
+[Service]
+Type=simple
+User=smokeping
+Group=smokeping
+PermissionsStartOnly=true
+ExecReload=/bin/kill -HUP $MAINPID
+ExecStart=/usr/local/bin/smokeping_prober \
+ --config.file=/etc/smokeping_prober//probes.yml \
+ --web.listen-address=127.0.0.1:8080 \
+ --web.listen-address=127.0.1.1:8080 \
+
+SyslogIdentifier=smokeping_prober
+KillMode=process
+Restart=always
+RestartSec=5
+
+LockPersonality=true
+NoNewPrivileges=true
+MemoryDenyWriteExecute=true
+PrivateTmp=true
+ProtectHome=true
+RemoveIPC=true
+RestrictSUIDSGID=true
+
+AmbientCapabilities=CAP_NET_RAW
+ProtectControlGroups=true
+ProtectKernelModules=true
+ProtectKernelTunables=yes
+ProtectSystem=strict
+
+[Install]
+WantedBy=multi-user.target
...
INFO Running alternative > verify
INFO Executing Testinfra tests found in /home/umcdev/src/ansible/prometheus/roles/smokeping_prober/molecule/alternative/tests/...
============================= test session starts ==============================
platform linux -- Python 3.11.6, pytest-7.4.3, pluggy-1.3.0
rootdir: /home/umcdev
plugins: testinfra-10.0.0
collected 5 items
tests/test_alternative.py ..... [100%]
============================== 5 passed in 1.79s ===============================
INFO Verifier completed successfully. |
Yes, I recently did a major rewrite of the snmp_exporter config, so configs need to match the version. If you want, I can open a separate PR to make that test cleaner. |
blackbox exporter alternative fails, but only on debian 10: [Unit]
Description=Blackbox Exporter
After=network-online.target
StartLimitInterval=0
StartLimitIntervalSec=0
[Service]
Type=simple
User=blackbox-exp
Group=blackbox-exp
PermissionsStartOnly=true
ExecReload=/bin/kill -HUP $MAINPID
ExecStart=/usr/local/bin/blackbox_exporter \
--config.file=/etc/blackbox_exporter.yml \
--log.level=warn \
--web.listen-address=127.0.0.1:9000 \
--web.listen-address=127.0.1.1:9000 \
SyslogIdentifier=blackbox_exporter
KillMode=process
Restart=always
RestartSec=5
LockPersonality=true
NoNewPrivileges=true
MemoryDenyWriteExecute=true
PrivateTmp=true
ProtectHome=true
RemoveIPC=true
RestrictSUIDSGID=true
AmbientCapabilities=CAP_NET_RAW
ProtectControlGroups=true
ProtectKernelModules=true
ProtectKernelTunables=yes
ProtectSystem=strict
[Install]
WantedBy=multi-user.target
I think the problem is this:
I think systemd on debian 10 is too old that it can't deal with the trailing backslash like the newer versions:
It turns it into
I'm gonna try nukeing the trailing backslash with loop last hacks. |
…sses Signed-off-by: Christian Krause <[email protected]>
…esses Signed-off-by: Christian Krause <[email protected]>
…eflight this is for consistency with the other roles' preflight asserts for when prometheus itself finally supports multiple web listen addresses Signed-off-by: Christian Krause <[email protected]>
…s in preflight this is for consistency with the other roles' preflight asserts for when the exporter itself finally supports multiple web listen addresses Signed-off-by: Christian Krause <[email protected]>
… in preflight this is for consistency with the other roles' preflight asserts for when the exporter itself finally supports multiple web listen addresses Signed-off-by: Christian Krause <[email protected]>
38f0e7c
to
c8a340e
Compare
That's done now, please re-run the tests. I also did that for smokeping 🤞 that this also fixes the tests. If not, I got no idea what to do about smokeping and need help with that. |
All tests passing now so I'm going to go ahead with the merge. Thanks again! |
Awesome! |
Thanks for the merge and the guidance, this helped a lot. |
these variants should work now:
fixes #115