fix(apache.service.running): prevent recursive requisite #391
Conversation
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
If Apache fails to start, on a non-systemd system, we dump 20 lines of logs, which seems like enough to give a good hint of what went wrong. When available, however, we instead use `journalctl -xe`. `journalctl -x` annotates each log message, resulting in a large amount of context for each line. `journalctl -e` implicitly allows a cap of 1000 lines of *log* to be printed, which is further multiplied by the annotations. IME the annotations aren't actually useful for Apache restarts (*Apache's* output doesn't have any, so it's just a lot of explanation of what systemd is doing). Rather than cluttering up Salt's output, this change makes `journalctl` consistent with non-systemd log dumping: 20 lines max, with no context. An admin can easily run `journalctl -xe` themselves if they actually need the annotations or more lines.
This is a follow-on to PR saltstack-formulas#388: - When the main Apache config file changes, reload Apache - Installing modules only needs the module dir to exist, it doesn't need the entire config. Tightening up this requisite avoids potential circular dependencies.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[build]Changes related to the build system[chore]Changes to the build process or auxiliary tools and libraries such as documentation generation[ci]Changes to the continuous integration configuration[feat]A new feature[fix]A bug fix[perf]A code change that improves performance[refactor]A code change that neither fixes a bug nor adds a feature[revert]A change used to revert a previous commit[style]Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)Secondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE?No.
Related issues and/or pull requests
This fixes the same issue as #388, but slightly more completely.
Describe the changes you're proposing
The fixes circular requisite errors when using the formula. Most of the change isn't mine (see the commit authors), but I added some tweaks, hence the new PR.
In terms of how the change works, mostly copying from my comment on #388
apache/config/file.sls are all justs/require_in/watch_in/`. AIUI the watch requisite implies the require requisite, so these are required either way, this just means that we restart Apache if they change.file.directorystates. I think if Apache previously crashed,service.runningwould rerun anyway, and it's unlikely that Apache would run but incorrectly without these directories, so I don't see whywatch_inis needed. OTOH, it's probably harmless.require_inis getting combined with thewatch_in). This seems like an improvement, though it's hard to see how it would improve a circular dep.watch_infor the main config file. This seems like it would be pretty important to getting appropriate config-reloading behavior.)apache-service-running*states lose theirwatch(and usually require, which AIUI is redundant) onsls: {{ sls_config_file }}, which AFAICT is just the wholeapache.config.filefrom earlier in the PR. Effectively, in combination with the earlier changes, I think this is replacing a broad dependency from the running states on the whole config file sls (inrunning.sls) with narrow dependencies from the running states to each of the config file states (infile.sls). My guess is thatrequire: sls: whateverrequires everything in that SLS, including the includes, and weinclude{{ sls_service_running}}, which would make a loop.-reloadand-runninglose theirwatch/requireentirely (there's no consistentwatch_infor them), but they'reaftertheservice: apache-service-running. I can't find a definition ofafter, but my guess is this functionally means we needapache-service-running's prereqs, which is good enough. (Also these shouldn't run all that much.)apache/config/modules/install.slsto use a finer-grained requisite there (based on danny-smit's patch)I also reduce the amount of logging dumped to salt when Apache restarts -- the old parameters could really clutter the output, which was very apparent when circular deps meant it kept failing to restart.
Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).pillar.example.Testing checklist
state_top).Additional context