Prometheus Alertmanager Fixes#218
Merged
Merged
Conversation
Moving alertmanager logs to /var/log as this is more conventional
Moving prometheus logs to /var/log as this is more conventional
We can remove a lot of tasks as they can be achieved by already used tasks. In the old changes the following tasks run: - Check if the binaries exist (stat) - Download the binaries (get_url) - Extract the binaries (unarchive) - Create the binary directory (file) - Move the extracted binaries into the folder (shell) In the new changes the following tasks run: - Unarchive - Check the downloaded files exists with "creates" - Download the files with "src" as a URL - Extract the files to destination "/tmp" - Copy - Copy the extracted files into the binaries directory - Create that directory if it doesn't exist
See previous commit
The alert will trigger after 30 seconds not 10. Updated the description to reflect that
khalford
force-pushed
the
prom_alert_fixes
branch
2 times, most recently
from
26e8b58 to
4735e4f
Compare
jose-caballero
previously approved these changes
May 23, 2025
AlexCK-STFC
suggested changes
Jun 2, 2025
AlexCK-STFC
left a comment
AlexCK-STFC
left a comment
Member
There was a problem hiding this comment.
Mostly LGTM, couple of questions ^
AlexCK-STFC
suggested changes
Jun 2, 2025
Reducing the permission scope of the alertmanager and prometheus logs. The public do not need read access to the logs. Any user who would need to read the logs should already be in the prometheus/alertmanager group.
When running the playbook for a second time, if the binaries were already removed from `/tmp` (e.g. system reboot) then the binaries will be re-downloaded and extracted even if `/opt/<application>` already exists. We can fix this by checking if `/opt/<application>` exists instead and only run the copy module if the unarchive module made changes.
AlexCK-STFC
previously approved these changes
Jun 2, 2025
When a task is run based on a previous task causing a change on the target host it should be run as a handler. Moving the handler tasks into handlers to fix the lint warning. https://ansible.readthedocs.io/projects/lint/rules/no-handler/#problematic-code
AlexCK-STFC
approved these changes
Jun 2, 2025
Dmitry-Popovichev
approved these changes
Jun 3, 2025
Dmitry-Popovichev
left a comment
Dmitry-Popovichev
left a comment
Contributor
There was a problem hiding this comment.
LGTM, btw with "mode" you can type it as an octal number, i.e mode: 0774 or as a string mode: "774" and ansible will do the conversion. Doesn't really matter either way but just something I found today while reviewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Includes various fixes and improvements for Prometheus and Alertmanager
/opt/<service>into/var/log/<service>