[Nix-dev] Re: [Nix-commits] SVN commit: nix - 17690 - rob - in nixos/trunk/modules: . services services/backup

[Nicolas Pierron]

Hi rob,

Thanks for this idea of backups.  I have a some comments on your patch
which may benefit you and others about what should be done or not with
the module syntax ...

On Wed, Oct 7, 2009 at 13:55, Rob Vermaas <rob.vermaas at gmail.com> wrote:
> Added: nixos/trunk/modules/services/backup/sitecopy-backup.nix
> ===================================================================
> --- nixos/trunk/modules/services/backup/sitecopy-backup.nix                             (rev 0)
> +++ nixos/trunk/modules/services/backup/sitecopy-backup.nix     2009-10-07 11:55:36 UTC (rev 17690)
> @@ -0,0 +1,109 @@
> +{pkgs, config, ...}:
> +
> +let
> +  inherit (pkgs.lib) mkOption mkIf singleton concatStrings;
> +  inherit (pkgs) sitecopy;
> +
> +  stateDir = "/var/spool/sitecopy";
> +
> +  sitecopyCron = backup : ''
> +    ${if backup ? period then backup.period else config.services.sitecopy.period} root ${sitecopy}/bin/sitecopy --storepath=${stateDir} --rcfile=${stateDir}/${backup.name}.conf --update ${backup.name}
> +  '';

I think an ionice could very interesting here, because you usually
don't want to damage other services when a backup is made.

> +in
> +
> +{

Goog, you are using the module syntax introduced by Eelco Dolstra, which is

{
  options= { ... };
  config = { ... };
}

As this syntax is clear enough and separate the option declarations
from the option definitions, you can omit the comments for the
"interface" and the "implementation".

> +  ###### interface
> +
> +  options = {
> +
> +    services.sitecopy = {

As well as others, you should probably create a backup attribute in
which the other "fileSystem", "mysql" and "postgresql" are declared.

  services.backup.mysql = {
    ...
  };

instead of

  services.mysqlBackup = {
    ...
  };

Camel style is nice when you don't have multiple options sharing an
identical part.  In our case the "backup" suffix has enough meaning to
gets its own path inside services.

> +      enable = mkOption {
> +        default = false;
> +        description = ''
> +          Whether to enable sitecopy backups of specified directories.
> +        '';
> +      };
> +
> +      period = mkOption {
> +        default = "15 04 * * *";
> +        description = ''
> +          This option defines (in the format used by cron) when the
> +          sitecopy backup are being run.
> +          The default is to update at 04:15 (at night) every day.
> +        '';
> +      };
> +
> +      backups = mkOption {
> +        default = [];
> +        description = ''
> +           List of attributesets describing the backups.
> +           E.g. { name = "test";
> +                  local = "/tmp/backup";
> +                  remote = "/staff-groups/ewi/st/strategoxt/backup/test";
> +                  server = "webdata.tudelft.nl";
> +                  protocol = "webdav";
> +                  https = true ;
> +                };

Examples should be defined in the example attribute and they should
have a type which is consistent with the default option.

  mkOption {
    default = []; # default is an empty list.
    example = [ # must be a list of something ...
       ... # something
    ];
    # to ensure that you have the right content, you should check it
by adding a type.
    # if the attribute sets are too complex, you should replace it by
"types.optionSet"
    # see fileSystems options to get more details.
    type = types.list types.attrs;
    description = " ... ";
  };

> +           Username/password are extracted from ${stateDir}/sitecopy.secrets at activation

Filenames should be embedded inside filename nodes.

<filename>${stateDir}/sitecopy.secrets</filename>

> +           time. The secrets file lines should have the following structure:
> +
> +             <server> <username> <password>

Such description will break the manual generation because the content
of option description is a XML-Docbook syntax.  Thus you should escape
these "<" & ">".

> +
> +        '';
> +      };
> +
> +    };
> +
> +  };
> +
> +
> +  ###### implementation
> +
> +  config = mkIf config.services.sitecopy.enable {
> +    environment.systemPackages = [ sitecopy ];
> +
> +    services.cron = {
> +      systemCronJobs = pkgs.lib.optional
> +          config.services.sitecopy.enable

You don't need "pkgs.lib.optional" because you already have a "mkIf"
on top of your configuration.

> +          (concatStrings (map sitecopyCron config.services.sitecopy.backups));
> +    };
> +
> +
> +    system.activationScripts.postgresqlBackup =

Is there too much copy&paste here?  That is supposed to be the
sitecopy expression.

> +      pkgs.stringsWithDeps.noDepEntry ''
> +          mkdir -m 0700 -p ${stateDir}
> +          chown root ${stateDir}
> +          touch ${stateDir}/sitecopy.secrets
> +          chown root ${stateDir}/sitecopy.secrets
> +
> +          ${pkgs.lib.concatStrings (map ( b: ''
> +              unset secrets
> +              unset secret
> +              secrets=`grep '^${b.server}' ${stateDir}/sitecopy.secrets | head -1`
> +              secret=($secrets)
> +              cat > ${stateDir}/${b.name}.conf << EOF
> +                site ${b.name}
> +                server ${b.server}
> +                protocol ${b.protocol}
> +                username ''${secret[1]}
> +                password ''${secret[2]}
> +                local ${b.local}
> +                remote ${b.remote}
> +                ${if b.https then "http secure" else ""}
> +              EOF
> +              chmod 0600 ${stateDir}/${b.name}.conf
> +              if ! test -e ${stateDir}/${b.name} ; then
> +                echo " * Initializing sitecopy '${b.name}'"
> +                ${sitecopy}/bin/sitecopy --storepath=${stateDir} --rcfile=${stateDir}/${b.name}.conf --initialize ${b.name}
> +              else
> +                echo " * Sitecopy '${b.name}' already initialized"
> +              fi
> +            '' ) config.services.sitecopy.backups
> +         )}
> +
> +      '';
> +  };
> +
> +}

Again, thanks for this idea of backups.

Cheers,

-- 
Nicolas Pierron
http://www.linkedin.com/in/nicolasbpierron
Donald Knuth - I can't go to a restaurant because I keep looking at
the fonts on the menu.