-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add a config check before restarting mimir #198
Conversation
Did you consider using
Something like (have not tested this):
|
@hakong thats a new option to me! :D I'll try it out, but even so this will stop execution if the config file is wrong - I could have gone more complex into backing up the file before changing it but I just wanted to start simple. |
You can back up with the
|
So all in all something like:
|
Hello @panfantastic, It would be great if you utilize
Also, could you please use the Fully Qualified Collection Name (FQCN) for the meta module, like this: - name: Flush handlers after deployment
ansible.builtin.meta: flush_handlers Additionally, please move the flush handler above the task |
Thanks for the feed back! I really appreciate it! There isn't a verify-config option as far as I can see for alloy so I'll assume you mean the same structure. I"ll send an update to the PR shortly. |
I've been trying to submit the change to my fork but Github Actions is failing for some odd reason. I'm pretty tired now so will call it a night and try and pick this back up tomorrow. |
Regarding validation, I don't have the place to test it at the moment, but I believe you can simply follow the steps outlined in this documentation. Just make sure you're using I'm unsure why the restart feature has been removed, but each configuration template requires a trigger handler to restart Mimir once the configuration is changed. In my honest opinion, having a backup for the template module is unnecessary. The role should template what is in the inventory and always reflect the state of the file. It's up to you to use Git to maintain proper inventory changes, and due to this, I believe the backup should be removed. |
The alternative view is that you shouldn't leave the system in a broken state such that you can't restart the service - so a change that alters a config should be reversible - i.e. you make a broken change to a config, then the system should replace the good config back rather than break the config on the system. This has never been easy to accomplish in ansible - unless it is possible now. |
I believe everything will remain clean. If you make any configuration changes and validation fails, the real config file won't be replaced with the faulty configuration. A restart is only triggered by |
I neglected to read the detail on %s - I think you are correct. |
It looks like the github actions are now passing on my fork. I've not tested this against my test cluster yet, but if you are ok to send it for another stage of accepting the submission then lets try that. |
In my opinion, the last thing that needs to be addressed is the modification of the validation line. validate: "mimir -modules --config.file=%s" I removed the |
Setting up mimir was very k8s heavy in suggestion - I don't use k8s so missed that section of the documentation. |
I think it can be merged now. |
Hi
This modifies the mimir restart logic to ensure the configuration is valid before issuing the restart of the service. Flushing handlers to ensure it is restarted before validating the service is running/responsive.