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

TSM client #61312

Merged
merged 3 commits into from Jul 18, 2019
Merged

TSM client #61312

merged 3 commits into from Jul 18, 2019

Conversation

@Yarny0
Copy link
Contributor

@Yarny0 Yarny0 commented May 11, 2019

Motivation for this change

Tivoli Storage Manager is IBM's backup solution. This pull request brings the client software and a NixOS module to use the client for regular system backups. Note that the client also provides an API to be linked against.
This solftware was renamed to IBM Spectrum Protect recently, but the previous name prevails in file names of the software as well as in package names of other distros. A I would expect possible users to look for the old name, I stick to it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS: x86_64-linux
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) (no such tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip" (none)
  • 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)
  • Assured whether relevant documentation is up to date: there is none
  • Fits CONTRIBUTING.md.

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented May 13, 2019

Updated the branch following the suggestions of #59891 . Also added some examples to module options.

options.services.tsmBackup.includeExclude.default;
environment.systemPackages = [ tsmPkg ];
environment.etc."${etcDir}/cli.dsm.sys".text = dsmSysText;
systemd.services.tsm-systembackup = service;
Copy link
Contributor

@flokli flokli May 14, 2019

please inline the service here

preFixup = ''
# Symlinks to dms.sys outside of the package:
ln --verbose --symbolic --no-target-directory \
"/etc/${etcDir}/cli.dsm.sys" $out/dsm_dir/dsm.sys
Copy link
Contributor

@flokli flokli May 14, 2019

I'm not sure if I like this passing around of etcDir too much - I guess we can just create symlinks to /etc/tsm-client and use that in the module consistently - no need to make the location overridable.

--prefix PATH : "${lib.strings.makeBinPath [ procps jdk ]}:$out/dsm_dir" \
--set DSM_DIR $out/dsm_dir \
--set-default DSM_CONFIG /dev/null \
--run 'export DSM_LOG=''${DSM_LOG-$HOME}'
Copy link
Contributor

@flokli flokli May 14, 2019

what does DSM_LOG do? The "''" also looks like a typo…

Copy link
Contributor

@flokli flokli May 16, 2019

Inlining your comment…

Regarding DSM_LOG, this variable tells the software where to write logfiles to. It must point to a directory where the invoking user has write access. The way it is declared in the wrapper allows the user to override it at will. However, if the user doesn't touch it, the wrapper lets it point to the user's HOME directory, as that is the only directory where it is very likely the invoking user has write access to. I wanted to use --set-default to implement the override effect, but makeWrapper uses single quotes to enclose the argument in the resulting wrapper script which prevents $HOME from being resolved. Therefore I implemented a command that's similar to --set-default but actually uses the value of $HOME here.

I guess if DSM_LOG is not set, we log to whatever is defined in errorlogname from dsmSysText, and this is a way to override it for manual operation?

Copy link
Contributor

@charles-dyfis-net charles-dyfis-net May 25, 2019

@flokli ''${...} ensures that the ${...} is passed to bash, not interpreted by nix. It's thus necessary for any kind of parameter expansion that can't be done in shell without curly brackets, including the expansion-with-default-on-unset PE done here (used to default to $HOME should there be no DSM_LOG variable).

(Edit: Oh, I see this was already discussed elsewhere, in a thread not attached to the per-line comment).

# https://www.ibm.com/support/knowledgecenter/en/SSEQVQ_8.1.7/client/c_sched_rtncode.html
serviceConfig.SuccessExitStatus = "4 8";
serviceConfig.ExecStart = "${tsmPkg}/bin/dsmc backup";
serviceConfig.StateDirectory = etcDir;
Copy link
Contributor

@flokli flokli May 14, 2019

This will create /var/lib/tsm-client.

Can you explain a bit what lands where, and why HOME is set like above?

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented May 15, 2019

  • Service is inlined (assertion also, but remaining stuff is so involved, so it's probably better comprehendable as separate declaration).
  • I also realized that the service should be created even if no autoTime is set as it would be useful to permit starting the service manually.
  • I hardcoded tsm-client as directory in /etc and /var/lib everywhere now. I have to add that I made up this name; there is no "official" name for these directories provided by IBM (at least none that I heard of).
  • Regarding DSM_LOG, this variable tells the software where to write logfiles to. It must point to a directory where the invoking user has write access. The way it is declared in the wrapper allows the user to override it at will. However, if the user doesn't touch it, the wrapper lets it point to the user's HOME directory, as that is the only directory where it is very likely the invoking user has write access to. I wanted to use --set-default to implement the override effect, but makeWrapper uses single quotes to enclose the argument in the resulting wrapper script which prevents $HOME from being resolved. Therefore I implemented a command that's similar to --set-default but actually uses the value of $HOME here.
  • The construct ''${ is needed to escape the string ${ as nix would try to resolve it as an antiquotation. Prefixing with two single quotes is actually the recommended way (given in the Nix manual) to escape the ${ construct.
  • Regarding the directories /var/lib/tsm-client/*: The client needs a directory where it stores the password it uses to connect to the server; that directory is declared in the configuration file (in /etc/tsm-client/cli.dsm.sys). The client also needs a directory where it stores certificates received from the server on the first connection (and more files that I cannot explain); it stores those files somewhere in $HOME. As all those files are created at some point, then must be kept around for future invocations, I considered them to be state and I put them in /var/lib/tsm-client. I have to add that my statements above derive in parts from the documentation of IBM, in parts from personal experience of tsm's behaviour I couldn't find documentation for.
  • Thanks, @flokli, for all the efforts you invest in my pull requests. I really appreciate that!

Copy link
Contributor

@flokli flokli left a comment

some more comments, shaping up well already :-)

IBM Spectrum Protect (Tivoli Storage Manager, TSM)
client system configuration and backup service
'';
ip6 = lib.options.mkEnableOption
Copy link
Contributor

@flokli flokli May 16, 2019

Suggested change
ip6 = lib.options.mkEnableOption
enableIPv6 = lib.options.mkEnableOption

The default value excludes compressed
formats from being recompressed by TSM.
To disable the default value,
you have to use <function>mkForce</function>.
Copy link
Contributor

@flokli flokli May 16, 2019

Can't the user just pass an empty string?

default = "";
example = ''
encryptiontype AES256
encryptkey save
Copy link
Contributor

@flokli flokli May 16, 2019

if the config file format stays flat like this, what about letting the user pass an attrset, and convert it to the TSM config syntax, with something similar to toKeyValue from lib/generators.nix? (with space as separator)

(see NixOS/rfcs#42 for why)

**** Do not edit!
**** This file is generated by NixOS configuration.
servername systembackup
commmethod ${if cfg.ip6 then "v6tcpip" else "tcpip"}
Copy link
Contributor

@flokli flokli May 16, 2019

Suggested change
commmethod ${if cfg.ip6 then "v6tcpip" else "tcpip"}
commmethod ${if cfg.enableIPv6 then "v6tcpip" else "tcpip"}

It's a shame the client doesn't just use getaddrinfo and getnameinfo, and IPv6 needs to explicitly be enabled :-/

description = "IBM Spectrum Protect (Tivoli Storage Manager) System Backup";
environment.DSM_CONFIG = "/dev/null";
# TSM needs a HOME dir to store certificates.
environment.HOME = "/var/lib/${serviceConfig.StateDirectory}/home";
Copy link
Contributor

@flokli flokli May 16, 2019

serviceConfig isn't defined here - just hardcode it to /var/lib/tsm-client/home, or even better, /var/lib/tsm-client, if there's no reason to put it inside another subfolder.

inclexcl ${pkgs.writeText "dsm.sys-inclexcl" cfg.includeExclude}
compression yes
passworddir /var/lib/tsm-client/password/
errorlogname /var/log/tsm-client.log
Copy link
Contributor

@flokli flokli May 16, 2019

Can this be configured to log via syslog or to stdout/stderr instead?

That way, we wouldn't need to worry about rotating the logs, and could easily look at the logs from the systemd journal (and handle log forwarding there as well, if desired).

Copy link
Contributor

@flokli flokli May 16, 2019

After some quick googling, the TSM client seems to log to syslog and stderr "when it's not able to log into a file" - I guess this translates to when the log destination isn't configured, too?

environment.DSM_CONFIG = "/dev/null";
# TSM needs a HOME dir to store certificates.
environment.HOME = "/var/lib/${serviceConfig.StateDirectory}/home";
serviceConfig.ExecStartPre = "${pkgs.coreutils}/bin/mkdir --parents $HOME";
Copy link
Contributor

@flokli flokli May 16, 2019

This can be accomplished by either setting HOME to /var/lib/tsm-client as suggested above, or by setting serviceConfig.StateDirectory = "tsm-client tsm-client/home";.

pkgs/tools/backup/tsm-client/default.nix Show resolved Hide resolved
--prefix PATH : "${lib.strings.makeBinPath [ procps jdk ]}:$out/dsm_dir" \
--set DSM_DIR $out/dsm_dir \
--set-default DSM_CONFIG /dev/null \
--run 'export DSM_LOG=''${DSM_LOG-$HOME}'
Copy link
Contributor

@flokli flokli May 16, 2019

Inlining your comment…

Regarding DSM_LOG, this variable tells the software where to write logfiles to. It must point to a directory where the invoking user has write access. The way it is declared in the wrapper allows the user to override it at will. However, if the user doesn't touch it, the wrapper lets it point to the user's HOME directory, as that is the only directory where it is very likely the invoking user has write access to. I wanted to use --set-default to implement the override effect, but makeWrapper uses single quotes to enclose the argument in the resulting wrapper script which prevents $HOME from being resolved. Therefore I implemented a command that's similar to --set-default but actually uses the value of $HOME here.

I guess if DSM_LOG is not set, we log to whatever is defined in errorlogname from dsmSysText, and this is a way to override it for manual operation?

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented May 20, 2019

I did some research on the syntax of dsm.sys and some tests. The youngest update of the branch is based on the results.

First off, some notes on server options:

  • dsm.sys contains sections per server, so-called "stanzas", similar to "Host" sections in ssh_config. Each stanza has a name, followed by that stanza's configuration. In contrast to ssh, users may only connect to servers declared in the system-wide dsm.sys file.
  • To represent the stanza structure, this pull request's tsm module provides the option tsmClient.servers, containing an attrset that maps stanza names to their respective configuration attrsets (similar to users.users with user configuration). The stanza's options include some of the most important or commonly used options (server, port, ...), furthermore the option extraConfig containing another attrset that is directly mapped into key-value-pairs in the respective stanza.
  • The declaration of stanzas with tsmClient.servers happens independently of an optional systemd service that might invoke dsmc. Depending on the usecase, these stanzas and the TSM client in general may be used for things other than pure backup operations. Therefore, the global configuration option is now called tsmClient instead of tsmBackup.
  • To use a stanza for regular systemd-controlled system backup, the user may provide the name of the stanza to the option tsmClient.service.servername.

I also tried to feed logging into syslog/journal, ultimately to no avail:

  • There are several ways to control where the client writes its log output to:
    • The dsm.sys option errorlogname takes a filename where logs are to be written to.
    • The environment variable $DSM_LOG takes a filename where logs are to be written to. If $DSM_LOG ends with a slash, it is treated as a directory name and the base name dsmerror.log will be appended automatically.
    • If none is given, TSM acts as if $DSM_LOG points to the current working directory. Consequently, $DSM_LOG/dsmerror.log will be used.
  • TSM tries to create the logging directory if it does not exist.
  • If TSM cannot create its log file/dir, it aborts.
  • To summarize, I failed to convince TSM to use syslog. However, the systemd service writes detailed output to stdout/stderr in addition to the logfile.
  • As TSM uses a (sensible) default for $DSM_LOG, I removed the default from the wrapper scripts.
  • TSM creates several logfiles. I observed dsminstr.log, citScanOutput.xml being created in the same directory that also contains dsmerror.log. As not all filenames document their ancestry clearly, I decided to use /var/lib/tsm-client/ as logging directory of the system backup service. Putting these files directly into /var/log (as did the outdated versions of this pull request) would litter that directory with confusingly-named log files.

Further notable changes:

  • It turned out that dsmc works well with $DSM_CONFIG unset, so I removed this variable from the wrapper scripts as well.
  • The backup systemd service uses /var/lib/tsm-client as $HOME directly.
  • Some options changed their names to be more consistent.
  • The docs on the commmethod stanza key explain that TSM uses IPv4 or IPv6 (depending on dns lookup result) if v6tcpip is selected. So that's now the default as I guess it's what most users would like.
  • The default value of inclucdeExclude is removed as the mkForce-comment seemed to be too confusing. Compression is off by default, anyway.
  • Several assertions check for inconsistencies (e.g. default servername not in stanza attrset). In particular, as TSM ignores upper and lower case in server/stanza names and option names, several checks are in place to prevent duplicate definition of server/stanza names and option names via differing capitalization.
  • I learned that stanza names cannot have more than 64 characters and network ports (used by TSM) cannot be higher than 32767. The option declarations reflect these restrictions.

@Yarny0 Yarny0 mentioned this pull request May 30, 2019
10 tasks
@flokli
Copy link
Contributor

@flokli flokli commented Jun 2, 2019

@spacefrogg could you take a look at this? You seem to be familiar with this ecosystem ;-)

Copy link
Contributor

@spacefrogg spacefrogg left a comment

Thank you for your effort! Looks fine for most part but binary wrapping should be changed to comply with nixos necessities.

, stdenv
, fetchurl
, gnutar
, jdk # optional (needed for `dsmj` GUI application)
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Does it support any (also conceivable future) jdk? Do we need version restriction here?

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Good point! The documentation https://www-01.ibm.com/support/docview.wss?uid=swg21052223#Version%208.1 asks for "JRE 7 or JRE 8", so I'm now using "jdk8".

For certain functionality, it also recommends "the acl package" and "libdevmapper.so v1.01 or higher".

  • Grepping through the result of the build process and using the strings utility shows that there are indeed some setfacl and getfacl command lines in binary files. While I never tested/used any acl functionality of TSM, I guess it's wiser to put {s,g}etfacl in the PATH. The increase in closure size should be minimal and it might help other users.
  • The only file referencing libdevmapper.so seems to be opt/tivoli/tsm/client/ba/bin/plugins/libPiIMG.so (apart from many language-specific help files). However, autopatchelf doesn't complain about a missing library. Adding lvm2 to builtInputs also does not generate a dependency on lvm2. So I'm ignoring this (optional) dependency for now.

for debfile in *.deb
do
ar -xv "$debfile"
tar --verbose --xz --extract --file=data.tar.xz
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Please remove all --verbose flags. They clutter the logs without providing useful information.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Done.

ln --verbose --symbolic --no-target-directory \
"/etc/tsm-client/cli.dsm.sys" $out/dsm_dir/dsm.sys
ln --verbose --symbolic --no-target-directory \
"/etc/tsm-client/api.dsm.sys" $out/dsmi_dir/dsm.sys
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Please remove. Nix packages must not depend on outside state. See nixos module for resolution.

Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

This implies that all external binaries that use TSM via its libraries must also set environment variables appropriately. Maybe, add a note about this at the top of this derivation.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Please remove. Nix packages must not depend on outside state. See nixos module for resolution.

Please see my general comment.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

This implies that all external binaries that use TSM via its libraries must also set environment variables appropriately. Maybe, add a note about this at the top of this derivation.

There is a comment about the client system-options file dsm.sys at the top of the derivation. I extended the comment to hint on DSMI_DIR and possible further environment variables.

rm --verbose "$bin"
makeWrapper "$target" "$bin" \
--prefix PATH : "${lib.strings.makeBinPath [ procps jdk ]}:$out/dsm_dir" \
--set DSM_DIR $out/dsm_dir
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Remove wrapper, here. This will be done in nixos module.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Please see my general comment.

} // lib.attrsets.optionalAttrs (config.includeExclude!="") {
inclexcl = ''"${pkgs.writeText "inclexcl.dsm.sys" config.includeExclude}"'';
}
);
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

This only works if options' effects are independent of the order in which they appear. Is that true for this config file?

It is normally easier (on future maintenance), less error prone and less likely to cause user frustration to not abstract the config file format at all and leave configuration to the user than to abstract only parts of the config file language.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

This only works if options' effects are independent of the order in which they appear. Is that true for this config file?

I don't know -- see below for a resolution.

It is normally easier (on future maintenance), less error prone and less likely to cause user frustration to not abstract the config file format at all and leave configuration to the user than to abstract only parts of the config file language.

I wouldn't consider that a problem here as it concerns two different configuration files with different syntax. Abstracting the "inclexcl" file would certainly be quite hard.

However, I agree with the objection above: the order of options in dsm.sys is a problem. I haven't found a statement about ordering options in dsm.sys in the doucmentation. Coarsely looking for order-dependent options didn't give me any suspects, but there are too many options to be sure.

Completely "unabstracting" the dsm.sys seems to be hard as well as we still need to place some options there for the system backup. I was tempted to change the type of the "extraConfig" option to contain "lines" of text instead of an "attrsOf" strings. That way users could just juse mkBefore and mkAfter to add lines in arbitrary order around those generated by the predefined config keys. That approach, however, precludes the user from overwriting or suppressing config lines generated by the predefined config keys. In light of this, I added another config option "text" that contains the textual result of "extraConfig" that forms the stanza. So, if the user wants to overwrite/suppress a config line that is generated by a predefined option, s/he can use "extraConfig"; if the user wants to add lines in specific order, s/he can use "text".

This solution might appear a bit over-engineered, but it's the best one I could come up with: It provides predefined options for common configuration keys and still gives the user full control over the content of each server stanza.

Copy link
Contributor

@spacefrogg spacefrogg Jun 14, 2019

You can still use lines to achieve your goal. A user can completely overwrite generated content by setting the variable with mkForce.

};
systemd.services.tsm-backup = lib.modules.mkIf (cfgSvc.servername!=null) {
description = "IBM Spectrum Protect (Tivoli Storage Manager) Backup";
environment.DSM_CONFIG =
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Not needed with wrapped binaries. Also, please add a config option for writing a dsm.opt. I specify more options than the servername in there. In this implementation, I couldn't communicate this.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Are you sure that is needed? While I couldn't find an explict statement, my understanding of the documentation is that each option in dsm.opt can also be used in dsm.sys. A noteable -- but not crucial -- exception are the servername and defaultserver options, but those options just control which server stanza in dsm.sys is to be used.

If my idea of the inner workings of the TSM client is correct, this leaves us with two scenarios, neither of which requires dsm.opt to be defined in the NixOS module:

  • The system backup is completely controlled by the settings in dsm.sys. Any options in a hypothetical dsm.opt might as well be stated in dsm.sys.
  • Users of the command line client want to define additional options that the system administrator didn't add to the system-wide dsm.sys. They can do so in their own dsm.opt file and provide that one to the client with the -optfile=/path/to/dsm.opt command line option, or with the DSM_CONFIG environment variable.

@spacefrogg: Can you check if your use cases fit in the scenarios described above and whether they can be handled without dsm.opt or with a user-provided dsm.opt, respectively? I'm curious if I missed something and there is actually a use case (e.g. an option I overlooked) that must be stated in dsm.opt for the system backup service.

Note: Researching on the interplay of dsm.sys and dsm.opt led me to the -se command line option. This option can fully replace the dsm.opt file for the backup systemd service, so I removed the file in the recent incarnation of the pull request at hand.

# for exit status description see
# https://www.ibm.com/support/knowledgecenter/en/SSEQVQ_8.1.7/client/c_sched_rtncode.html
serviceConfig.SuccessExitStatus = "4 8";
serviceConfig.ExecStart = "${pkgs.tsm-client}/bin/dsmc backup";
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Make the arguments configurable. I use incr, here, so this seems to depend on usage scenario. :)
Also, refer to the wrapped binary instead of the original.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

I added the services.tsmClient.service.command option now, that should do the trick.

for link in $out/lib/* $out/bin/*
do
target=$(readlink "$link")
test "$(cut -b -6 <<< "$target")" == "../../"
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Please use proper if [ ... ]; then ... fi construct, here.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Done.

pkgs/tools/backup/tsm-client/default.nix Show resolved Hide resolved
, jdk # optional (needed for `dsmj` GUI application)
, procps
, zlib
, makeWrapper
Copy link
Contributor

@spacefrogg spacefrogg Jun 7, 2019

Dito. Remove.

Copy link
Contributor Author

@Yarny0 Yarny0 Jun 11, 2019

Please see my general comment.

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jun 11, 2019

Thanks for your suggestions, @spacefrogg. It is really good to see that I will not be the sole user of that package! I applied most of your requested changes.

Considering your suggestion to create a wrapper script in a NixOS module, I beg to differ. Since creating that wrapper concerns several of your per-line requests, I will concentrate my response to that concept here.

I am convinced that the symlink to dsm.sys in /etc from within the package is a better approach than reassembling the DSM_DIR (and adding dsm.sys) outside of the core package, for the following reasons:

  • It is certainly right that a reference to /etc constitutes a dependency on state outside of the package. However, this is standard practice in most packages containing system software in nixpkgs. For instance, most server software that handles logins reference /etc/pam.d somewhere inside their binaries to find out how to authenticate users and alike. While such software references /etc via a hardcoded path inside some binary in most cases (or is patched to do so, e.g. fuse), using a symlink is no different. In particular, a symlink causes no further problems.
  • According to IBM's documentation (https://www.ibm.com/support/knowledgecenter/en/SSEQVQ_8.1.7/client/c_cfg_spevunix.html), the directory pointed to by DSM_DIR must contain most of the executables, dsm.sys, and "resource files". Those, in turn, encompass the language directories (EN_US and similar), but probably more. All of those files are simply static data, with dsm.sys being the lone exception. Therefore, it's no problem to let the environment variable point to the directory inside the package. In contrast, reassembling the DSM_DIR outside of the package is complicated, occupies additional space, and is prone to missing files. For instance, the example wrapper for the NixOS module is only symlinking EN_US thereby losing support for other languages.
  • Removing the wrappers from the tsm-client package would render that package useless without the NixOS module, e.g., if used on other distributions where Nix is installed, but NixOS modules can't be used. Users would likely have to write a similar wrapper by hand.
  • I expect this to be even more of a problem for API users, as software linking against the tsm-client certainly brings no script for rebuilding the DSMI_DIR, leaving that task again for the user.
  • Adding other packages to the PATH via a wrapper in the derivation is a common method in nixpkgs -- grepping in nixpkgs for makeWrapper yields many examples for this, git and vim amongst them. Removing the wrapper from the tsm-client package cuts the dependency on procps and jdk for non-NixOS users. The unwrapped binaries actually do depend on outside state as they will invoke whatever happens to be in PATH. I can't imagine a use case where it is necessary to call the command line utilities without procps or (optionally) java in the PATH.
  • Adding DSM_CONFIG and DSM_LOG to the wrapper does make it behave the same whether called by systemd or by hand, but there is no need for that. In fact, the environment variables are used to modify the behaviour of dsmc. If the user wants to connect to a different server, s/he will likely provide her/his own dsm.opt file.
  • In contrast, providing config files to systemd services by adding environment variables like DSM_CONFIG is a standard method. Most software uses command line options for this (like sshd -C /path/to/config-file) but it is the same principle. (Note that I replaced the environment variable with the -se command line option now.)

On a sidenote: I used to have a similar setup on my machines -- I reassembled the DSM_DIR (the directory where my package's symlink dsm_dir points to) in /etc/tsm-client and -- from time to time -- had to adjust my copy of the directory as it turned out I missed some important files in the package's directory. I think that now I understand much better how IBM wants that directory to be used. Based on my understanding, the symlink to /etc appears to me to be a solution that is much more robust and less complicated than reassembling the DSM_DIR directory by script or wrapper.

@spacefrogg
Copy link
Contributor

@spacefrogg spacefrogg commented Jun 14, 2019

@Yarny0 Thank you for your effort! I understand your reasoning and will address each of your remarks in turn. Please note, that I am in no authority over merging this PR or demanding any changes of you. My code reviews are based on my experience and current understanding of how nixpkgs PRs are supposed to be designed to acceptable to people that actually do have the authority. So, feel free to ignore me.

  • No file-based references to outside a nixpkg. This is not negotiable. We try very hard to stick to this rule. Even if you find a package breaking this rule. This one is a normal use-case and does not demand such exception. I am quite sure the upstream hydra server does reject this package as it is right now.
  • TSM is of exceptionally poor design (as many closed-source applications) as it does not properly differentiate between static and runtime data. You don't need to copy anything around, and my suggested wrapper script was not doing this at all. You create a tree of links with the proper files in it, which costs virtually nothing. If you look closely, the structures of my suggested approach and your solution slightly differ: In my solution, all configuration files are first generated into the /nix/store and then referenced by the wrapper. So, still nothing points to outside the store. With a heavyweight config-abstraction you could even make sure that the tsm-client never breaks, as it never sees a broken config file. In your approach, you link to modifiable data outside the store and hope nothing goes wrong. Why is that important even if anything works today? Well, in general, you could not vouch for TSM to not deprecate a config option or note choke on a perfectly reasonable config file in the future. Abstraction through modules gives you a way to properly migrate or warn/inform users. Depending on usable external state does not.
  • This is true but is a price we are willing to pay. The reasoning goes as follows: This application is broken by design, as it is only useful with a proper configuration file, which is not taken from a reasonable location by default. The approach to get there in nixos is via wrappers in modules. People outside nixos have to write their own wrappers to get to there. No shortcuts that break nixos. This is actually true for most server software on nixos. The apache package, for instance, is completely useless outside nixos for this very reason.
  • Why should software linking against tsm-client need that? Anyways, linking software against a library is a package maintainer's task not an ordinary user task. We expect package maintainers to take the burden. (Which you could lighten by putting up a comment explaining how to properly link against TSM including wrappers.)
  • True. Inside nixos the package is never used directly, see apache for another example, but only through a nixos module. For outside nixos you can use the runtimeDependencies attribute to unconditionally add the packages to the closure. You might actually want to keep the wrapper for dsmj (making it double wrapped in the end) and only add procps to runtimeDependencies.
  • True. If you look closely to my suggested wrapper, it allows the respective environment variables to be overridden from the outside at call time. Still, you should at least pin DSM_LOG, as I found it annoying that it writes the log to your current directory. So again, TSM comes without sane defaults.
  • True. I have nothing against providing the data via environment variables in systemd directly. It was merely a shortcut, once you have the proper values in the wrapped binaries anyway.

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jun 17, 2019

Dear @spacefrogg, I will concentrate on the question of the (im)possibility of the symlink into /etc. That question is probably paramount to the design of this pull request.

No file-based references to outside a nixpkg. This is not negotiable. We try very hard to stick to this rule. Even if you find a package breaking this rule. This one is a normal use-case and does not demand such exception. I am quite sure the upstream hydra server does reject this package as it is right now.

I'm not sure if your argument goes against a symlinks in particular or against any kind of reference that points outside of the Nix store. Please forgive if I'm bringing forward unnecessary arguments. Anyway, here's my stance on references that point out of the Nix store:

  • It is common for packages to reference files outside of /nix/store. vim, bash, tmux, ssh{,d}, git, and many more consult configuration files in /etc when invoked. Those references are no problem as their targets are properly documented or at least common knowledge.
  • A symlink -- while being more prominent -- has the same effect as a simple file containing a reference. Binaries can use and follow a symlink like they follow a path that is denoted elsewhere in the package.
  • Symlinks to targets beyond the Nix store are also common practice in nixpkgs, see e.g. linux-pam, spacefm, roundcube, bluez, openfire, and more. Most of those point to out-of-package configuration files in /etc, thereby accomplishing exactly what I want to accomplish with tsm-client. Hydra seems to have no problems with these packages as long as the license is acceptable.
  • Last but not least, I simply can't spot a problem with NixOS that the symlink would provoke, as long as it points to a sane and fixed location. It is common and apparently legitimate for packages in nixpkgs to reference files with configuration, state or runtime data outside of the Nix store.

Is there any documentation or otherwise documented consensus that explicitely forbids -- or frowns upon -- references pointing out of the Nix store? What is the rational to avoid such references in general? Although I still consider myself a novice w.r.t. Nix packaging, the ban on outside references stands in stark contrast to everything in nixpkgs I've seen so far. On the other hand, not using the symlink would hamstring many of my use cases. So please understand my skepticism and excuse my insistence.

While looking for statements about the (in)validity of references pointing out of /nix/store, I found a script that checks for references to the build directory (<nixpkgs/pkgs/build-support/setup-hooks/audit-tmpdir.sh>), as such references could pose serious problems. However, neither this script nor any other one I looked at checks for references pointing outside of the store in general.

[...] In your approach, you link to modifiable data outside the store and hope nothing goes wrong. Why is that important even if anything works today? Well, in general, you could not vouch for TSM to not deprecate a config option or note choke on a perfectly reasonable config file in the future. Abstraction through modules gives you a way to properly migrate or warn/inform users. Depending on usable external state does not.

The safety and comfort of a NixOS module is independent of the usage of /etc. This pull request uses a module to place the config file in /etc (similar to other software, e.g. sshd), providing the abstraction of the module. Using a wrapper and sidestepping /etc does not give any benefit over the symlink to a config file in /etc if that file is generated by the NixOS module. On non-NixOS systems -- and even on NixOS if root explicitely chooses to do so -- root may also provide a handmade config file. This would be a conscious decision by root, and I consider this possibility an improvement as it facilitates usage of tsm-client on non-NixOS systems. Of course, root has to ensure that the file is correct, but this is no different from other software using config files in /etc (irrespective of the distribution).

This is true but is a price we are willing to pay. The reasoning goes as follows: This application is broken by design, as it is only useful with a proper configuration file, which is not taken from a reasonable location by default. The approach to get there in nixos is via wrappers in modules. People outside nixos have to write their own wrappers to get to there. No shortcuts that break nixos. This is actually true for most server software on nixos. The apache package, for instance, is completely useless outside nixos for this very reason.

Do you refer to Apache's httpd? I'm not sure it fully compares to the problem at hand: httpd accepts a config file on its command line, and it is a server process not ment to be invoked by ordinary users. However, I tried it, and httpd from nixpkgs runs fine without a wrapper script -- even when called by non-root (on a port beyond 1024) and even on a non-NixOS machine. I'd like to reach a similar solution with tsm-client: The binaries should work "out of the box", without any wrapper scripts but just the configuration file being present, irrespective of the underlying distribution.

Why should software linking against tsm-client need that? Anyways, linking software against a library is a package maintainer's task not an ordinary user task. We expect package maintainers to take the burden. (Which you could lighten by putting up a comment explaining how to properly link against TSM including wrappers.)

The software would need it as TSM's API needs the DSMI_DIR environment variable containing a properly prepared dsm.sys (like the command-line client that needs a DSM_DIR). A package maintainer, not knowing the content of dsm.sys in advance, would face the same dilemma and would likely end up with the same symlink solution that I prefer, since this solution doesn't require the package to be rebuild whenever the configuration file dsm.sys needs modification (assuming that the maintainer uses another Nix recipe to prepare the package linking against tsm-client).

True. Inside nixos the package is never used directly, see apache for another example, but only through a nixos module. For outside nixos you can use the runtimeDependencies attribute to unconditionally add the packages to the closure. You might actually want to keep the wrapper for dsmj (making it double wrapped in the end) and only add procps to runtimeDependencies.

Why would I add procps to runtimeDependencies? TSM doesn't link against procps (not sure if that would be possible at all), but TSM actually calls the executable ps, so ps must be in the PATH. The wrappers in this pull request accomplish exactly that.

Let me add and emphasize that I'm very greatful of this discussion, @spacefrogg. Irrespective of the outcome of this particular question, I'm aiming at a solution that covers as much use cases as possible, in a way that is as user/admin friendly as possible. So I would be very inclined if the version that is finally merged covers your use cases as well as mine. I'm still convinced that the symlink to /etc is a dependable solution, nevertheless (or maybe for exactly that reason), I'd like to know if the pull request in its current version could also cover your use cases.

@spacefrogg
Copy link
Contributor

@spacefrogg spacefrogg commented Jun 18, 2019

No file-based references to outside a nixpkg.

I'm not sure if your argument goes against a symlinks in particular or against any kind of reference that points outside of the Nix store.

My argument goes against what I wrote just above. File-based references, i.e., softlinks and hardlinks.

  • It is common for packages to reference files outside of /nix/store. vim, bash, tmux, ssh{,d}, git, and many more consult configuration files in /etc when invoked. Those references are no problem as their targets are properly documented or at least common knowledge.

These are not references of the same kind. The main difference between the two is in expectations. Resolving a configuration file from inside a running program is in the responsibility of the program itself. Providing a filesystem without broken links is not. You may not like this distinction, but I, myself, regard this as common consensus (by mere observation) among Linux distribution maintaining folks.

  • A symlink -- while being more prominent -- has the same effect as a simple file containing a reference. Binaries can use and follow a symlink like they follow a path that is denoted elsewhere in the package.

Maybe, but this similarity is not relevant. See my previous remark.

  • Symlinks to targets beyond the Nix store are also common practice in nixpkgs, see e.g. linux-pam, spacefm, roundcube, bluez, openfire, and more. Most of those point to out-of-package configuration files in /etc, thereby accomplishing exactly what I want to accomplish with tsm-client. Hydra seems to have no problems with these packages as long as the license is acceptable.

True, and the term you are looking for is purity, impure package and descendants. We try to keep packages pure as much and as often as possible. A package should not reference anything outside the known scope of the nix package manager with very few exceptions, e.g., runtime data (usually databases of any kind). And even that is a concession to practicality. I give you the very idea of NixOS:

  1. The system is (ideally) stateless.
  2. Consequently, a /nix/store path to a system configuration uniquely identifies the complete closure of the system, its applications, their configuration.
  3. Consequently, calling an application from the closure of a particular system configuration always yields the same result (when executed on the same machine).
  4. This even holds when a different system configuration was active in the meantime.

The consequence from 3. is, that applications cannot have state, which is impractical, which is why local application state is treated separately. But even then, look at the postgres NixOS module and you will find that it uses versioned directories for its application state. This way, you at least have the possibility to downgrade again, should the database upgrade mess up your end-user application. On a 'normal' Linux distribution you would have literally messed up when you upgraded without making backups first.

As a sidenote: With pam you have picked a special case of package impurity, that is really hard to solve due to its basic service it provides to a system. You need it everywhere under insanely varied circumstances. The others look like bad quality packages (without having looked into it). roundcube is definitely a case for bad quality software design, as it expects to be allowed to write into its own code base. This is the case for a lot of so-called "web applications".

  • Last but not least, I simply can't spot a problem with NixOS that the symlink would provoke, as long as it points to a sane and fixed location. It is common and apparently legitimate for packages in nixpkgs to reference files with configuration, state or runtime data outside of the Nix store.

Okay, now to the other side of the story... As mentioned, NixOS allows application state, although it strives for purity through statelessness. If it would have been practically achievable and sustainable (i.e., maintainable), there would be no /etc directory. You can see that we strive very hard to get rid of the statefulness of /etc by the way we literally generate a complete shadow tree and copy everything to the real directory on each system startup and each system configuration change. Allowing references to /etc is a concession to broken software (well, no longer valid design assumptions) and practicality. We avoid statefulness in /etc (configuration files in general) as much as possible by:

  • Directly referencing the constructed configuration file in the /nix/store via command-line switches etc.,
  • Wrapping binaries, so that they point to the right incarnation of the configuration file or, at least, by
  • Making /etc/<configfile> a symlink to a /nix/store path.

We even support copying the file from the /nix/store to /etc and setting its owner/mode bits on each start if an application really does not accept a symlink (for god's sakes!). Look at the openafs module for an instance of this. Also look at the cups module for the fireworks we burn to provide a writable state directory, yet a stateless configuration, all in the same directory. And if that still is not enough to please your application, you can use an activation script to execute code on every boot or system configuration change to do something useful.

So... stepping back, looking at the infrastructure NixOS actually provides you with to avoid statefulness. No, breaking this rule lightly and uncalled for is not acceptable.

Is there any documentation or otherwise documented consensus that explicitely forbids -- or frowns upon -- references pointing out of the Nix store? What is the rational to avoid such references in general? Although I still consider myself a novice w.r.t. Nix packaging, the ban on outside references stands in stark contrast to everything in nixpkgs I've seen so far. On the other hand, not using the symlink would hamstring many of my use cases. So please understand my skepticism and excuse my insistence.

I can only refer you to IRC or the discourse forum for further guidance. But anyway, could you please concisely list which of your use cases it would hinder to not set up the symlink but to follow the approach I showed you? I think, I haven't understood your problem domain, yet.

The safety and comfort of a NixOS module is independent of the usage of /etc. This pull request uses a module to place the config file in /etc (similar to other software, e.g. sshd), providing the abstraction of the module. Using a wrapper and sidestepping /etc does not give any benefit over the symlink to a config file in /etc if that file is generated by the NixOS module. On non-NixOS systems -- and even on NixOS if root explicitely chooses to do so -- root may also provide a handmade config file. This would be a conscious decision by root, and I consider this possibility an improvement as it facilitates usage of tsm-client on non-NixOS systems. Of course, root has to ensure that the file is correct, but this is no different from other software using config files in /etc (irrespective of the distribution).

I hope, I could prove to you, that your conclusions are wrong, here. Sidestepping /etc is exactly what makes tsm-client in incarnation X differ from its incarnation Y. X references the configuration from X, and Y from Y. In your case, both would refer to the configuration of whatever system configuration is active at the time, e.g., Z. This brakes the consistency guarantees you may expect (and we strive to provide) from a NixOS system configuration.

The software would need it as TSM's API needs the DSMI_DIR environment variable containing a properly prepared dsm.sys (like the command-line client that needs a DSM_DIR). A package maintainer, not knowing the content of dsm.sys in advance, would face the same dilemma and would likely end up with the same symlink solution that I prefer, since this solution doesn't require the package to be rebuild whenever the configuration file dsm.sys needs modification (assuming that the maintainer uses another Nix recipe to prepare the package linking against tsm-client).

But it is the very responsibility of a package maintainer to correctly instate the configuration of a package that depends on another one. If we could automate that task, we would do so. If it is as easy as providing every descendent with the right DSMI_DIR environment variable, it's even a no-brainer (maybe not trivial to write, just the architectural idea is). You could write a package setup hook in tsm-client that does all the magic to the application binaries whenever you add tsm-client to the descendant's buildInputs. That is exactly what happens when using cmake or autobuild or... In this sense, linking to /etc from inside the /nix/store breaks a second rule. A nix package should be described declaratively. You describe the result you want to achieve and let nix care for how to get there. You reference content, not locations. The locations that provide you with the content are resolved by nix.

Why would I add procps to runtimeDependencies? TSM doesn't link against procps (not sure if that would be possible at all), but TSM actually calls the executable ps, so ps must be in the PATH. The wrappers in this pull request accomplish exactly that.

Because it adds procps to the runtime closure of tsm-client. So, whenever somebody installs tsm-client, procps also get's installed. Thus, procps is always in PATH for tsm-client. And yes, the wrappers do that. It is an optimization to avoid double-wrapping but not a necessary thing to do. Actually, calling tsm-client is in no way performance critical (in the sense of "is usually called 1000 times per second"). Just go ahead, leave your wrappers (except DSM_DIR) and wrap again in the nixos module.

I'm still convinced that the symlink to /etc is a dependable solution, nevertheless (or maybe for exactly that reason), I'd like to know if the pull request in its current version could also cover your use cases.

I am not, but I think, it would cover my use cases.

I am always willing to share my insights.

@Yarny0 Yarny0 force-pushed the tsm-client branch 2 times, most recently from 943cba5 to 5b51e3a Jun 24, 2019
@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jun 24, 2019

Thanks for your response that helped me understand your objections more clearly. I updated the request in order to tweak some minor details (explained below). However, irrespective of other packaging details, I am still confused by the interplay of symlinks and purity:

My argument goes against what I wrote just above. File-based references, i.e., softlinks and hardlinks.

vim, bash, tmux, ssh{,d}, git, and many more consult configuration files in /etc when invoked.

These are not references of the same kind. The main difference between the two is in expectations. Resolving a configuration file from inside a running program is in the responsibility of the program itself. Providing a filesystem without broken links is not.

This leaves me with the impression that it is considered alright when a program looks into /etc to find configuration as long as there is no symlink (or hardlink) involved. However, this one contests the impression:

A package should not reference anything outside the known scope of the nix package manager with very few exceptions, e.g., runtime data (usually databases of any kind). And even that is a concession to practicality. I give you the very idea of NixOS:

  1. The system is (ideally) stateless.
  2. Consequently, a /nix/store path to a system configuration uniquely identifies the complete closure of the system, its applications, their configuration.
  3. Consequently, calling an application from the closure of a particular system configuration always yields the same result (when executed on the same machine).
  4. This even holds when a different system configuration was active in the meantime.

[...]

I hope, I could prove to you, that your conclusions are wrong, here. Sidestepping /etc is exactly what makes tsm-client in incarnation X differ from its incarnation Y. X references the configuration from X, and Y from Y. In your case, both would refer to the configuration of whatever system configuration is active at the time, e.g., Z. This brakes the consistency guarantees you may expect (and we strive to provide) from a NixOS system configuration.

These definitions are independent of the usage of symlinks/hardlinks. In particular, by this definition, the programs listed above (and many more) suffer impurity. E.g. the command ssh hostname still depends on whatever the administrator has written in /etc/ssh/ssh_config. By that standard, even Nix itself is impure as it consults /etc/nix/nix.conf.

I still can't grasp why dependence on outside state is acceptable unless a symlink is involved. While a filesystem with a "broken symlink" (as criticised above) isn't nice, it won't do any additional harm when compared to a missing config file that is referenced without passing through a symlink. For instance:

  • sshd works alright if a proper configuration file is placed in /etc/ssh/sshd_config as documented by the man page. If it is missing, sshd says so and fails.
  • dsmc on classical Linux distributions works alright if a proper configuration file is placed in TSM's directory structure as ordered by the documentation. If it is missing, dsmc says so and fails.
  • dsmc as declared in this pull request works alright if a proper configuration file is placed in /etc/tsm-client/ as denoted in the package's description. If it is missing, dsmc says so and fails.

All those behaviours are certainly impure to the degree that local configuration is not included in the package itself. Yet, it matches most users' expectations, and it works flawlessly on NixOS and on non-NixOS systems. How the config file is discovered in the end is an implementation detail that is of no interest to the admin as long as where the file is to be placed is documented.

To aid the admin, I added a hint about the configuration file's placement in the longDescription of the package. Also, I expanded the module to create the API's configuration file as well.

We avoid statefulness in /etc (configuration files in general) as much as possible by:

  • Directly referencing the constructed configuration file in the /nix/store via command-line switches etc.,
  • Wrapping binaries, so that they point to the right incarnation of the configuration file or, at least, by
  • Making /etc/ a symlink to a /nix/store path.

We even support copying the file from the /nix/store to /etc and setting its owner/mode bits on each start if an application really does not accept a symlink (for god's sakes!). Look at the openafs module for an instance of this. Also look at the cups module for the fireworks we burn to provide a writable state directory, yet a stateless configuration, all in the same directory. And if that still is not enough to please your application, you can use an activation script to execute code on every boot or system configuration change to do something useful.

So... stepping back, looking at the infrastructure NixOS actually provides you with to avoid statefulness. No, breaking this rule lightly and uncalled for is not acceptable.

This leaves me confused. This pull request uses the method advertised in the third bullet point of yours to spare the admin the hassle of having to prepare the config file by hand. So, if you want to ensure that TSM's config file is in the Nix store, this pull request satisfies your needs. By using the NixOS module in this pull request, you will have a config file that is generated in the store and then referenced by a symlink in /etc. Note that only on non-NixOS systems, where the NixOS module is not applicable, the actual place of the configuration file is in /etc; but even there the administrator may still decide to symlink back into the Nix store from /etc.

I can only refer you to IRC or the discourse forum for further guidance. But anyway, could you please concisely list which of your use cases it would hinder to not set up the symlink but to follow the approach I showed you? I think, I haven't understood your problem domain, yet.

As I'm still not sure how to balance practicability and purity here, I will consult Discourse.

Regarding the use cases I'd like to cover, they represent most combinations of

  1. (a) NixOS and (b) Nix on non-NixOS
  2. (a) command line client and (b) linking aginst the API
  3. (a) called by root or systemd and (b) called by non-root user

in addition to

  1. the config file contains several server stanzas
  2. the config file sees regular changes

The combination 2b+3a is of no real interest to me, the combination 2a+3b is used occasionally only. All other combinations are very important.

Talking about expectations, non-NixOS systems are a concern for me in particular, as admins are used to just modifying the config file and expecting the change to be active immediately (like adding a certificate to ssh_known_hosts is for ssh). It would likely be a source of constant mishaps if the admin and/or users would have to remember to rebuild wrapper packages whenever the config changes. Moreover, it would deprive users of the possibility to simply use nix-env to install a usable tsm-client (again, as is normal with ssh and most other tools, including also Apache's httpd, as discussed earlier).

But it is the very responsibility of a package maintainer to correctly instate the configuration of a package that depends on another one.
[...]
You could write a package setup hook in tsm-client that does all the magic to the application binaries whenever you add tsm-client to the descendant's buildInputs.

dsm.sys is a file the administrator of a system has to provide, not the package maintainer. The package maintainer can only design the package such that providing a config file is as easy as possible for the administrator. So while using a setup hook to supply descendent packages with DSMI_DIR-creating wrappers is a beautiful concept, we would ultimately wind up again with the question of the source/placement of the configuration file: Either baked into a wrapper package in the store, or discovered in a documented place outside of the store via a symlink.

Why would I add procps to runtimeDependencies? TSM doesn't link against procps (not sure if that would be possible at all), but TSM actually calls the executable ps, so ps must be in the PATH.

Because it adds procps to the runtime closure of tsm-client. So, whenever somebody installs tsm-client, procps also get's installed. Thus, procps is always in PATH for tsm-client.

When trying what you suggested, dsmc responded sh: ps: command not found. And this is consistent with the nixpkgs manual: runtimeDependencies are added to the RPATH of executables. This is completely unrelated to the PATH environment variable. So I will stick to adding procps to the PATH with makeWrapper (together with the optional packages jdk8 and acl).

Yet I want to thank you for reminding me of the runtimeDependencies argument. As mentioned earlier in this discussion thread, TSM requires "libdevmapper.so v1.01 or higher" for "image backup and restore functions for LVM2 volumes". autoPatchElf doesn't detect any dependency on libdevmapper.so by itself, so adding lvm2 (which contains libdevmapper) to buildInputs wouldn't make a difference. I suspect TSM accesses libdevmapper only when needed, then likely through dlopen and dlsym. Like adding acl to the PATH to enable ACL functions, adding lvm2 to runtimeDependencies should therefore enable LVM2 volume functions. I updated the pull request to do so (and added null as default for optional packages), although I can't test it. As with acl, I expect the increase in closure size to be too small to warrant a separate tsm-client-with-lvm package.

I am not, but I think, it would cover my use cases.
I am always willing to share my insights.

Thanks for you offer, and moreover thanks for the assessment of your use cases. This feeds me with optimism regarding this pull request.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jun 24, 2019

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

https://discourse.nixos.org/t/symlink-to-etc-from-within-a-package-vs-purity/3257/1

@spacefrogg
Copy link
Contributor

@spacefrogg spacefrogg commented Jun 24, 2019

Executive summary, see below for details:

  • Make the dsmc nixpkg provide wrapped binaries that point to the configuration in /etc and that set DSMC_DIR to a location in the nix store. Do not put a link to outside the nix store in there.
  • Make a nixos module that consistently creates the configuration file pointed to by the wrapper. On NixOS, this is the way, dsmc is installed, not via nix-env.

This should suite both, NixOS and non-NixOS cases without relying on this broken symlink.

These are not references of the same kind. The main difference between the two is in expectations. Resolving a configuration file from inside a running program is in the responsibility of the program itself. Providing a filesystem without broken links is not.

This leaves me with the impression that it is considered alright when a program looks into /etc to find configuration as long as there is no symlink (or hardlink) involved. However, this one contests the impression:

It is considered acceptable, because it is not sustainable to make all applications work without hardcoded configuration files. Maybe it helps to look at it from a different angle. You actually don't care where a configuration file is stored on the harddrive. What you do care about is, making the application access the right configuration when it is run. Having a hardcoded location for a configuration file, is a broken, old-school way of providing this functionality. Whenever you can tell your application where to find the right configuration at calltime, you don't need a hardcoded location. This is what we do on NixOS, if possible. There are usage scenarios, where it is important for a user to know what's inside a configuration file. But this is an abuse of hardcoded paths for providing a stable interface altogether. Ideally you'd have a way to ask your application or operating system for providing this information.

Again, the question is, why are hardcoded paths a broken interface? Answer, they lack the notion of multiple (and different) consistent application states. This cannot be done using hardcoded paths. This is the reason why there is a separate /nix/store with weird paths that are not under user control.

[...]
These definitions are independent of the usage of symlinks/hardlinks. In particular, by this definition, the programs listed above (and many more) suffer impurity. E.g. the command ssh hostname still depends on whatever the administrator has written in /etc/ssh/ssh_config. By that standard, even Nix itself is impure as it consults /etc/nix/nix.conf.

Yes, they are independent of symlinks/hardlinks. On NixOS, programs generally do not suffer from impurity, because, to stay with you example, the administrator cannot write into /etc/ssh/ssh_config. It is a symlink into the nix store and, thus, (re-)generated by activating a particular system configuration. The same is true for Nix. If you read my statements closely, you will see, that I never stated that the way NixOS implements /etc was flawed or stateful or violates above definitions.

I still can't grasp why dependence on outside state is acceptable unless a symlink is involved. While a filesystem with a "broken symlink" (as criticised above) isn't nice, it won't do any additional harm when compared to a missing config file that is referenced without passing through a symlink. For instance:

What I wanted to convey with my previous remarks, was the following: Nothing you say is plain wrong, but on a NixOS system, we want more guarantees. You are arguing towards not providing these guarantees in the way that I seem acceptable (aka. what I think the NixOS community finds acceptable). Nothing more, nothing less.

  • sshd works alright if a proper configuration file is placed in /etc/ssh/sshd_config as documented by the man page. If it is missing, sshd says so and fails.

You're comparing apples and oranges, here. On a NixOS system this is exactly not how you use sshd. On a NixOS system you activate its module and it provides two things for you:

  • The running service, whatever is necessary for it; stuff in PATH, systemd service etc.
  • A configuration file that is consistent with the administrators configuration and the necessities of this particular sshd version. For instance, for sometime, the module added an option to the configuration file that prevented a known security issue with sshd, thus, providing safe defaults.

This said, if you want a PR to be merged into NixOS, you should at least accept that this is the way software is used. No ad hoc hacking in /etc, no stateful installation of software. And, this is how you wrote your PR. So, everything is fine on that side of the story. :)

  • dsmc as declared in this pull request works alright if a proper configuration file is placed in /etc/tsm-client/ as denoted in the package's description. If it is missing, dsmc says so and fails.

This is not good enough. If possible and sustainable, dsmc, particularly because it is more likely to be used as a regular service than a time-to-time user application, should be used via a proper nixos module. It never happens that dsmc on NixOS does not have a proper configuration file. This has nothing to do whether it is residing in /etc or not. This also makes providing nix-env support a low priority. Providing the wrapper from inside the nixos module does not do any harm. So, I fail to see why you argument so hard against it... I hope you see, that it at least does provide the benefits I described earlier.

All those behaviours are certainly impure to the degree that local configuration is not included in the package itself. Yet, it matches most users' expectations, and it works flawlessly on NixOS and on non-NixOS systems. How the config file is discovered in the end is an implementation detail that is of no interest to the admin as long as where the file is to be placed is documented.

True, not good enough. NixOS provides functionality, not inconsistent stuff that must be made consistent as an afterthought. This is where almost all other operating systems fail today.

This leaves me confused. This pull request uses the method advertised in the third bullet point of yours to spare the admin the hassle of having to prepare the config file by hand....

True, I never argued against this part of your implementation. I merely shared my insights into the architecture of NixOS with you. I argued against putting a broken symlink into the nix store.

Regarding the use cases I'd like to cover, they represent most combinations of

  1. (a) NixOS and (b) Nix on non-NixOS
  2. (a) command line client and (b) linking aginst the API
  3. (a) called by root or systemd and (b) called by non-root user

in addition to

  1. the config file contains several server stanzas
  2. the config file sees regular changes

The combination 2b+3a is of no real interest to me, the combination 2a+3b is used occasionally only. All other combinations are very important.

Thanks for sharing.

Talking about expectations, non-NixOS systems are a concern for me in particular, as admins are used to just modifying the config file and expecting the change to be active immediately (like adding a certificate to ssh_known_hosts is for ssh). It would likely be a source of constant mishaps if the admin and/or users would have to remember to rebuild wrapper packages whenever the config changes. Moreover, it would deprive users of the possibility to simply use nix-env to install a usable tsm-client (again, as is normal with ssh and most other tools, including also Apache's httpd, as discussed earlier).

The expectation of ad hoc hacking configuration is not the focus of NixOS. If possible without violating purity, you may write the package in a way to also provide useful non-NixOS behaviour. If not, non-NixOS is not the focus, and folks there have to find a way around the issues. For your particular case, there is no such problem if you follow my suggestions. Non-NixOS systems can happily use dsmc with a wrapper that points to the config file in /etc.

dsm.sys is a file the administrator of a system has to provide, not the package maintainer. The package maintainer can only design the package such that providing a config file is as easy as possible for the administrator.

This is true only for non-NixOS systems. On NixOS, both, the application and the configuration are provided consistently at the same time. It is, thus, in the responsibility of the package maintainer to provide the necessary nixos module mechanics. Other distributions like Debian also expect from you to provide sane default configuration for a service you want to integrate in their system.

[...] So while using a setup hook to supply descendent packages with DSMI_DIR-creating wrappers is a beautiful concept, we would ultimately wind up again with the question of the source/placement of the configuration file: Either baked into a wrapper package in the store, or discovered in a documented place outside of the store via a symlink.

True. The decision has been made, wrap the binaries. No symlinks pointing outside the store. This decision hinges on filesystems not providing multiple consistent states on hardcoded paths.

Why would I add procps to runtimeDependencies? TSM doesn't link against procps (not sure if that would be possible at all), but TSM actually calls the executable ps, so ps must be in the PATH.

Because it adds procps to the runtime closure of tsm-client. So, whenever somebody installs tsm-client, procps also get's installed. Thus, procps is always in PATH for tsm-client.

When trying what you suggested, dsmc responded sh: ps: command not found. And this is consistent with the nixpkgs manual: runtimeDependencies are added to the RPATH of executables. This is completely unrelated to the PATH environment variable. So I will stick to adding procps to the PATH with makeWrapper (together with the optional packages jdk8 and acl).

Good, good! Do this! I was not sure myself. :)

Yet I want to thank you for reminding me of the runtimeDependencies argument. As mentioned earlier in this discussion thread, TSM requires "libdevmapper.so v1.01 or higher" for "image backup and restore functions for LVM2 volumes". autoPatchElf doesn't detect any dependency on libdevmapper.so by itself, so adding lvm2 (which contains libdevmapper) to buildInputs wouldn't make a difference. I suspect TSM accesses libdevmapper only when needed, then likely through dlopen and dlsym. Like adding acl to the PATH to enable ACL functions, adding lvm2 to runtimeDependencies should therefore enable LVM2 volume functions. I updated the pull request to do so (and added null as default for optional packages), although I can't test it. As with acl, I expect the increase in closure size to be too small to warrant a separate tsm-client-with-lvm package.

Sounds good!

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jul 1, 2019

The pull request received a major overhaul:

  • The package now accepts paths to the config files as arguments.
  • The module uses these arguments to have the package point directly to the config file created by the module.

Details of changes in the package:

  • The package -- internally -- consists of an "unwrapped" derivation and a "wrapper" derivation. The "wrapper" derivation is ultimatelly returned/used.
  • The "unwrapped" derivation is similar to the old package. However, no dsm.sys is added to the installation directories (where DSM_DIR and DSMI_DIR have to point to) at all. Binaries are left untouched (not enclosed in wrappers). Hence, the "unwrapped" derivation can't be used directly.
  • The "wrapper" derivation is a collection of symlinks into the "unwrapped" one. To each installation directory, the builder also adds symlinks that point to the config file paths that have been provided as build arguments. Wrapper scripts are created to help the executables find the DSM_DIR installation directory. Whenever the path to the configuration file changes, only the "wrapper" derivation will be rebuild.
  • If installed with nix-env -i on other distros, no paths can be set. In that case, the paths default to /etc/..., as before.

Details of changes in the module:

  • pkgs.writeText is used to create the config file. Its path is passed to pkgs.tsm-client as config file path. The resulting package is used in the system environment and for the backup service. /etc is no longer touched or referenced.
  • The text of the config file is also made available via an additional option. API users can use the module, possibly define additional servers, and feed the configuration into their own API-linking packages, in order to sidestep /etc as well.
  • (Not related to the /etc issue): To enable users to install the tsm-client-withGui package with the module, I added a package option.

Dear @spacefrogg,

first of all, thanks again for your hints towards a better implementation. I revised the pull request based on ideas I received on discourse and on what I distilled from your latest response. Could you have a look at the current revision to judge whether the design satisfies purity requirements?

I tried to understand and follow your suggestions, but I'm still not sure if I graps your goals correctly. In case the current design is not acceptable, this is, essentially, where I'm struggling to follow your response:

On NixOS, programs generally do not suffer from impurity, because, to stay with you example, the administrator cannot write into /etc/ssh/ssh_config. It is a symlink into the nix store and, thus, (re-)generated by activating a particular system configuration.

Just to be sure so we're not talking cross purposes: In my initial design the files /etc/tsm-client/{cli,api}.dsm.sys were, like /etc/ssh/ssh_config (and /etc/ssh/sshd_config for the server) generated by the module in my PR and hence yielded symlinks pointing from /etc back into the store as well. So, assuming ssh/sshd are based on well-designed modules, I can't see where my approach deviates.

Copy link
Contributor

@spacefrogg spacefrogg left a comment

Very neat! Thank you very much, @Yarny0, for your efforts! I still have some requests, but the overall architecture is very much in the right spirit, now. :)

Regarding your last remark: This difference was, that you wanted to put a, probably broken, symlink into the nix store. This is something, sshd doesn't do.

pkgs/tools/backup/tsm-client/default.nix Show resolved Hide resolved
, gnutar
, makeWrapper
, procps
, runCommand
Copy link
Contributor

@spacefrogg spacefrogg Jul 2, 2019

Not used, remove.

, autoPatchelfHook
, buildEnv
, fetchurl
, gnutar
Copy link
Contributor

@spacefrogg spacefrogg Jul 2, 2019

I believe this would be unnecessary if you would depend on stdenv instead of stdenv.cc.cc later. This way, you make sure to use the tar package from stdenv. So, unless you need a tar feature, that only gnutar has, this would be more general.

I am not sure about the impact on autoPatchelfHook, though...

# The `-se` option must come after the command.
# The `-optfile` option suppresses a `dsm.opt`-not-found warning.
serviceConfig.ExecStart =
"${tsmClientPkg}/bin/dsmc ${cfgSvc.command} -se='${cfgSvc.servername}' -optfile=/dev/null";
Copy link
Contributor

@spacefrogg spacefrogg Jul 2, 2019

Is the dsm.opt not-found guard still necessary?

Copy link
Contributor Author

@Yarny0 Yarny0 Jul 4, 2019

Yes. It merely hides a warning, but a long and confusing one.

{ config, lib, pkgs, ... }:

let

Copy link
Contributor

@spacefrogg spacefrogg Jul 2, 2019

Please use

inherit (lib) mkOption, strMatching,...;
inherit (builtins) ...;

lines, here. Using full-qualified names throughout the code affects readability a lot.

Copy link
Contributor Author

@Yarny0 Yarny0 Jul 4, 2019

Applied, with two exceptions

  • I still refer to the sub-namespaces in lib, like inherit (lib.options) mkOption. I faintly recall having read somewhere that putting all library elements in the lib namespace was a bad idea (I can't find it and I might be wrong about this). While I guess this will never be reverted, I don't want to stand in the way of doing so.
  • I didn't add an inherit line for libraries with only one element to be imported. In such cases I feel short code weight more than readability (but that's probably a matter of taste).

IBM Spectrum Protect (Tivoli Storage Manager, TSM)
client system-options file "dsm.sys"
used by TSM command line applications
'';
Copy link
Contributor

@spacefrogg spacefrogg Jul 2, 2019

Intertwining enable and servername this way is unexpected and confusing. The semantics you described warrant, that you split the module in two (code may stay in the same file). One programs.tsmClient.enable that draws in the right wrapped binary, and one services.tsmClient.enable which enables the service, forcing that servername has to be configured and activating programs.tsmClient.enable.

Rationale: The way you constructed this module, the tsm-client binaries are expected to work without configuration. Then you should put them into programs instead of services. The user's expectation is that, when (s)he does services.*.enable, the service gets enabled. This silently fails now, if servername is not set. I know, that you correctly documented this behavior. Still, this is unexpected and, thus, confusing.

EDIT: typo

@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jul 4, 2019

Dear @spacefrogg, I updated the pull request and applied your requests (and responded to non-request comments). Two further remarks are in order:

  • IBM published a new TSM client about a week ago (8.1.8.0). The pull request has been updated to use the latest version.
  • The module has been split into a "program" module and a "service" module, as requested. The "program" module creates the final, wrapped, tsm client package based on the module's configuration. Since the "service" module needs the wrapped package to call the executable dsmc, the "program" module makes the wrapped package available via the new wrappedPackage option.

Copy link
Contributor

@spacefrogg spacefrogg left a comment

Looks ready to merge from my side. @Infinisil @flokli

Yarny0 added 3 commits Jul 15, 2019
IBM Spectrum Protect (former name: Tivoli Storage Manager)
provides a single point of control for backup and recovery.
This package contains the client software, that is,
a command line client and linkable libraries.

This commit adds two packages to nixpkgs:
The TSM client software contains a Java GUI
that, naturally, requires Java to be installed.
To keep the closure size low, we provide the packages
`tsm-client` and `tsm-client-withGUI`.
The former comes without the Java GUI.

While the product has been renamed, its old name is still
alive in filenames and as package name used by other distros.
This commit brings a module that installs the
IBM Spectrum Protect (Tivoli Storage Manager)
command-line client together with its
system-wide client system-options file `dsm.sys`.
Based on the programs/tsm-client module,
this commit introduces a systemd service that uses the
tsm-client to create regular backups of the machine.
@Yarny0
Copy link
Contributor Author

@Yarny0 Yarny0 commented Jul 15, 2019

Tiny update: By looking at xfig in https://nixos.org/nixos/packages.html#xfig I realized that XML tags are no good in longDescription.

@flokli
Copy link
Contributor

@flokli flokli commented Jul 18, 2019

looks really nice so far, thanks for the hard work!

@flokli flokli merged commit 9d339e3 into NixOS:master Jul 18, 2019
13 checks passed
@flokli
Copy link
Contributor

@flokli flokli commented Jul 18, 2019

I saw Docbook tags being used in NixOS module descriptions - I assumed that was also possible for package descriptions - cc @grahamc

@grahamc
Copy link
Member

@grahamc grahamc commented Jul 18, 2019

It seems thaht a subset of the XML is supported for options: https://github.com/NixOS/nixos-homepage/blob/master/nixos/options.tt#L279 but is not applied to packages. It'd be nice to port this over and support it in both!

@flokli
Copy link
Contributor

@flokli flokli commented Jul 20, 2019

It would be lovely if this could be integrated into NixOS/nixos-homepage#209 (or a followup PR after that)

@Yarny0 Yarny0 deleted the tsm-client branch Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants