fix(sudoers/included): fix idempotence with purge_includedir=True #74
Conversation
|
Hello, thanks for this PR. Using provided Kitchen tests, I simply added Its output seems to indicate that it removes files to recreate them later: But I'm not sure it really remove and recreate the files, because inodes stay the same So inodes are the same which means files are the same, but still modification time has been changed. I also monitored the inodes to understand what happens, an excerpt of the output Without the option to purge the directory |
|
I'm not sure why in your case inodes stay the same (maybe some docker specifics?), but in my case on real server they changes after every In your inotifywait it removes the file, creates temporary file with random name, writes to it, renames it to destination, then changes permissions isn't it? Also a quote from documentation:
AFAIK, it's by design. It looks in requisites to decide what files to exclude from deletion. |
|
I haven't been able to reproduce the inode change, even on a 'real VM', but I see differences in the salt output with your patch: the following part does not appear! Congrats for having found this information on the documentation! I think it could also be used for saltstack-formulas/nginx-formula#266. I'll merge this code now, thanks for your contribution. |
|
🎉 This PR is included in version 0.23.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
I was curious about that. And the explanation turns out to be simple: filesystems reuse inodes. Salt removes and recreates the same amount of files and these files get the same inodes, because salt is doing the same thing during every run. Simple code to test on xfs or ext4 filesystem: stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 test
stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 test
stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 testBut the same code on btrfs or zfs (copy-on-write) filesystem: stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282467 test
removed 'test'
renamed '.temp_test' -> 'test'
282468 test
stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282468 test
removed 'test'
renamed '.temp_test' -> 'test'
282469 test
stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282469 test
removed 'test'
renamed '.temp_test' -> 'test'
282470 testWhen I was testing inodes in the earlier post I was using a LXC container on ZFS filesystem with pretty active IO. But even without IO new files get new inodes on COW filesystems based on my testing. |
|
@stasjok I was just chatting to @javierbertoli and there appears to be an edge case which still fails. I think it was related to this comment: |
It should work as long as required state function is |
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
Describe the changes you're proposing
clean: Trueshould be used withrequirerequisite in order to clean only those files that are not managed by salt.Without this change the formula would clean and recreate all files every time.
If
/etc/sudoers.ddirectory doesn't exist, it will be created withmakedirs: Truewith default permissions, then permissions will be fixed at the end.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