Skip to content
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

nixos/tomcat: add purifyOnStart option #44303

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

pvgoran
Copy link
Contributor

@pvgoran pvgoran commented Aug 1, 2018

With this option enabled, before creating file/directories/symlinks in baseDir according to configuration, old occurences of them are removed.

This prevents remainders of an old configuration (libraries, webapps, you name it) from persisting after activating a new configuration.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Aug 1, 2018
@@ -185,6 +188,10 @@ in
after = [ "network.target" ];

preStart = ''
# Delete the existing base directory, if any
# (we don't want remainders of old configuration!)
rm -rf ${cfg.baseDir}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not also delete logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it would, and it's definitely not wanted.

@pvgoran pvgoran changed the title nixos/tomcat: Delete baseDir before (re-)creating it nixos/tomcat: Clean up baseDir before filling it Aug 1, 2018
@pvgoran
Copy link
Contributor Author

pvgoran commented Aug 1, 2018

The new version specifically keeps logs in place. Not sure if find is the best way to do it (with all the options), but it works fine.

@pvgoran
Copy link
Contributor Author

pvgoran commented Aug 1, 2018

Now, it is possible that people added their own files into baseDir and expect them to stay there after restart or reconfiguration; this patch will break their configuration. However, leaving bits of old configuration (especially old libraries or old webapps) may be even worse. Not sure how to handle this.

This PR, while it very much makes sense by itself, is a pre-requisite of another PR which would add management of the tomcat-users.xml file.

# Delete everything but logs from the existing base directory
# (we don't want remainders of old configuration!)
[[-e ${cfg.baseDir} ]] \
&& find ${cfg.baseDir} -mindepth 1 -maxdepth 1 -not -name logs -exec rm -rf '{}' '+'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do it more explicit?

rm -rf ${cfg.baseDir}/{conf,virtualhosts,temp,lib,shared/lib,webapps,work}

Are there any files in those directory that should be not deleted? I am not an expert when it comes to tomcat related applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do it more explicit?

Yes, this can work as well. Though I would also include bin which is a symlink created by the script. And work needs to be excluded (see below).

Are there any files in those directory that should be not deleted?

In fact, there are. The session files, named SESSIONS.ser by default, can exist inside the work directory, and they shouldn't be deleted.

On the other hand, work serves as a cache for compiled JSP files, and in certain corner cases Tomcat can improperly use the cached files from an old deployment. But these corner cases should be very uncommon (if at all possible), so probably it doesn't make much spending code lines on handling them.

@joachifm
Copy link
Contributor

joachifm commented Aug 6, 2018

If deleting specific stuff is too clunky, an alternative is to write state into versioned dirs and use symlinks to point to whatever is current. That way old stuff can be garbage collected manually --- the tradeoff obviously being accumulating lots of junk over time & the overhead of managing the links ...

@pvgoran
Copy link
Contributor Author

pvgoran commented Aug 9, 2018

@Mic92 I replaced removal of "everything except logs" with removal of specific directories/symlinks.

@pvgoran
Copy link
Contributor Author

pvgoran commented Aug 9, 2018

@joachifm Did you mean versioning the whole tomcat directory? This would prevent loss of old files, but it's a solution that is way too "heavy" to be practical, and it doesn't prevent the main problem at hand: unexpected change of behaviour of the startup procedure.

@pvgoran
Copy link
Contributor Author

pvgoran commented Sep 23, 2018

For some reason, the previous commit actually did not include the advertised changes ("I replaced removal of "everything except logs" with removal of specific directories/symlinks"), now this is corrected by
fefcd78.

Any chance for merging this soon?

@7c6f434c
Copy link
Member

Just in case — @danbst do you want to comment on the list of directories to clean?

@danbst
Copy link
Contributor

danbst commented Oct 22, 2018

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

3 similar comments
@danbst
Copy link
Contributor

danbst commented Oct 22, 2018

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

@danbst
Copy link
Contributor

danbst commented Oct 22, 2018

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

@danbst
Copy link
Contributor

danbst commented Oct 22, 2018

@pvgoran @7c6f434c
I think all good, but only enable if services.tomcat.purifyOnStart = true; is set (option to be created). Tomcat management is essentially stateful (the whole startup script is an example of this madness) and so many rely on this. For example, I have mutable webapp deploy to webapps dir - after this PR my webapp would be removed on tomcat restart (I can't put webapp into configuration.nix)

@pvgoran
Copy link
Contributor Author

pvgoran commented Oct 22, 2018

@danbst Makes sense. This new option is supposed to be disabled by default, right?

@7c6f434c
Copy link
Member

OK, so GitHub sends notifications correctly but displays the new comments one in five reloads or something. Yay eventual consistency, I guess. I can merge once the option is added, please ping me then.

@NixOS NixOS deleted a comment from danbst Oct 22, 2018
@danbst
Copy link
Contributor

danbst commented Oct 22, 2018 via email

@danbst
Copy link
Contributor

danbst commented Oct 22, 2018 via email

@7c6f434c
Copy link
Member

7c6f434c commented Oct 22, 2018

«Once the option is added» was precisely about purifyOnStart

With this option enabled, before creating file/directories/symlinks in baseDir
according to configuration, old occurences of them are removed.

This prevents remainders of an old configuration (libraries, webapps, you name
it) from persisting after activating a new configuration.
@pvgoran pvgoran changed the title nixos/tomcat: Clean up baseDir before filling it nixos/tomcat: add purifyOnStart option Oct 29, 2018
@pvgoran
Copy link
Contributor Author

pvgoran commented Oct 29, 2018

@danbst @Mic92 I added the purifyOnStart option to protect against removing things by default.

Now that it's in place, I'm thinking about going back to deleting everything (except the specifically preserved subdirectories) from baseDir, instead of deleting explicitly listed items. What do you think about it?

@7c6f434c
Copy link
Member

What is more likely, that they add one more directory we want to keep or that they add one more directory we want to clean?

Intuitively I would consider an explicit removal of known-problematic subdirectories (with a comment about what and why we still keep) the cleanest approach.

@pvgoran
Copy link
Contributor Author

pvgoran commented Oct 29, 2018

What is more likely, that they add one more directory we want to keep or that they add one more directory we want to clean?

Both of these look improbable to me, if we are talking about Tomcat developers adding something. NixOS developers, on the other hand, could add something; in this case, I think it will be rather something we want to clean.

But this is not how I see this choice. I think it's more about what to do with unknown files which were not created by NixOS: keep them or remove them? The currently implemented approach is "keep", but personally I'd prefer "remove".

@7c6f434c
Copy link
Member

7c6f434c commented Oct 29, 2018 via email

@danbst
Copy link
Contributor

danbst commented Oct 29, 2018

@pvgoran yes, you can remove everything. Those who don't want this behavior should remove purifyOnStart = true; declaration.
EDIT: maybe leave logs?

@7c6f434c

Is there a need to add an option to extend the whitelist?

Don't think so. Tomcat is pretty much stabilized software, which hasn't changed conceptually for ages.

@pvgoran
Copy link
Contributor Author

pvgoran commented Oct 30, 2018

What is your mental model of where these files would actually (probably) come from? Is there a need to add an option to extend the whitelist?

It could be something like custom logs, or data files/directories created by web applications. In this case, an explicit whitelist would be beneficial. However, I don't think this use case will appear (or at least appear frequently), so it's not worth adding an option now, until someone actually needs/requests it.

maybe leave logs?

Yes, and also work which (by default) contains session files.

@Mic92 Previously I switched from the "whitelist" approach (delete everything except selected directories) to the "blacklist" approach (delete only known directories that we create) by your request. Now, do you agree with going back to the "whitelist" approach with introduction of the purifyOnStart option?

@7c6f434c
Copy link
Member

7c6f434c commented Oct 30, 2018 via email

@pvgoran
Copy link
Contributor Author

pvgoran commented Oct 30, 2018

To me that sounds like an argument for a blacklist (if anything extra is created it is custom logs or data files, and we keep the known logs and session data files). But I won't use the module anyway.

The thing is, this use case is just a speculation on my part, "what if someone does this". I have no evidence someone would actually need to organize files like this.

@pvgoran
Copy link
Contributor Author

pvgoran commented Nov 4, 2018

I implemented the "whitelist" approach (removing of all baseDir contents except for logs and work) in #49729. I keep this PR as it is in case it will be preferred.

My preference is the "whitelist" approach (#49729), and I don't see strong arguments against it, now that the purifyOnStart option was added.

@Mic92 @7c6f434c @danbst Please review this and commit one of these two PRs, at last.

@7c6f434c
Copy link
Member

7c6f434c commented Nov 4, 2018

@danbst I am OK with the current state of either PR, I marginally prefer this one, and I will merge whichever you ask me to merge (please do a direct mention in that comment).

@danbst
Copy link
Contributor

danbst commented Nov 4, 2018

@7c6f434c @pvgoran on second thought, I'd prefer this one, "blacklist" approach. Because we allow users to change baseDir, someone can set it to /home/XXX, and eventually add purifyOnStart. Which will be uh-oh. With current approach the disaster won't be that large, as with "whitelist" approach.

@7c6f434c 7c6f434c merged commit 6b8252d into NixOS:master Nov 4, 2018
@7c6f434c
Copy link
Member

7c6f434c commented Nov 4, 2018

I do not claim arguments for blacklist are strong, it is all about reasonable arguments on both sides. And now this bikeshed is freshly painted. Thanks to everyone.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/nixops-and-tomcat/1472/2

@aanderse aanderse mentioned this pull request Apr 5, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants