mgr/Zabbix: Various fixes to Zabbix module #19452
Conversation
wido
force-pushed
the
mgr-zabbix-runtimerror
branch
from
bf614e9
to
327a542
Compare
|
The reason I added 'bugfix' as a label is that without this fix the Zabbix module doesn't start after setting zabbix_host and restarting the manager. |
There was a problem hiding this comment.
This looks ok to me, but I'm not sure if we're trying to converge on logging errors vs throwing exceptions. IIRC there is a PR in flight (or merged?) that will raise a health alert if a module throws an exception from serve(); dropping the exceptions here would lose that.
|
@liewegas Ah I see, you mean where I dropped the RuntimeErrors. I can add those back easily. Just need to know what the convention is. |
|
The PR with the error handling is #19235 It introduces a behaviour where exceptions from serve() or init() cause a health check to fail + get reported. However, it also prevents the module running until the mgr restarts, so it's meant for fatal/unexpected errors. Modules are also able to raise their own health checks with MgrModule.set_health_checks, so maybe that's the right thing to do for runtime-fixable conditions like a bad configuration option? |
|
The set_health_checks bits are already in master, so I think it would be neat to try using those to raise a friendly health message to tell the user to set their zabbix config when it's missing/invalid. For anything really fatal, just raise the exception and it'll be handled more gracefully when #19235 merges. |
Users can still override this parameter, but by default the fsid of the cluster will be used to send data to Zabbix. This makes it even easier to use the Zabbix module. Signed-off-by: Wido den Hollander <wido@42on.com>
The module refused to run/start if not all configuration was properly set This commit makes sure the module is initialized properly and allows it to run much more smoothly. Signed-off-by: Wido den Hollander <wido@42on.com>
Just to make sure more logging can be done when needed. Signed-off-by: Wido den Hollander <wido@42on.com>
wido
force-pushed
the
mgr-zabbix-runtimerror
branch
from
327a542
to
9f525b2
Compare
|
@jcsp I've brought back the RunTimeError exceptions again. How does it look now? |
|
We may want to revisit the runtimeerrors in the future, but clearly it's not a regression for this PR so 👍 |
|
I see that the docs fail to build, but that's due to a 404 during the test itself. Reading package lists... E: Failed to fetch http://mirror.cs.uchicago.edu/ubuntu-toolchain-r/dists/xenial/main/binary-i386/Packages 404 Not Found E: Failed to fetch http://mirror.yandex.ru/mirrors/launchpad/ubuntu-toolchain-r/dists/xenial/main/i18n/Translation-en 404 Not Found [IP: 213.180.204.183 80] E: Some index files failed to download. They have been ignored, or old ones used instead. Build step 'Execute shell' marked build as failure |
Signed-off-by: Wido den Hollander <wido@42on.com>
wido
force-pushed
the
mgr-zabbix-runtimerror
branch
from
6b9cc29
to
d7a09ff
Compare
|
I pushed again to trigger the checks. They succeeded now. |
This pull request contains multiple improvements and fixes for the Zabbix Manager Module: