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/cockroachdb: simplify dataDir management, tweaks #51535

Merged
merged 1 commit into from Dec 5, 2018

Conversation

thoughtpolice
Copy link
Member

This cleans up the CockroachDB expression, with a few suggestions from
@aszlig.

However, it brought up the note of using systemd's StateDirectory=
directive, which is a nice feature for managing long-term data files,
especially for UID/GID assigned services. However, it can only manage
directories under /var/lib (for global services), so it has to introduce
a special path to make use of it at all in the case someone wants a path
at a different root.

While the dataDir directive at the NixOS level is occasionally useful,
I've gone ahead and removed it for now, as this expression is so new,
and it makes the expression cleaner, while other kinks can be worked out
and people can test drive it.

CockroachDB's dataDir directive, instead, has been replaced with
systemd's StateDirectory management to place the data under
/var/lib/cockroachdb for all uses.

There's an included RequiresMountsFor= clause like usual though, so if
people want dependencies for any kind of mounted device at boot
time/before database startup, it's easy to specify using their own
mount/filesystems clause.

This can also be reverted if necessary, but, we can see if anyone ever
actually wants that later on before doing it -- it's a backwards
compatible change, anyway.

Signed-off-by: Austin Seipp aseipp@pobox.com

Motivation for this change
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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This cleans up the CockroachDB expression, with a few suggestions from
@aszlig.

However, it brought up the note of using systemd's StateDirectory=
directive, which is a nice feature for managing long-term data files,
especially for UID/GID assigned services. However, it can only manage
directories under /var/lib (for global services), so it has to introduce
a special path to make use of it at all in the case someone wants a path
at a different root.

While the dataDir directive at the NixOS level is _occasionally_ useful,
I've gone ahead and removed it for now, as this expression is so new,
and it makes the expression cleaner, while other kinks can be worked out
and people can test drive it.

CockroachDB's dataDir directive, instead, has been replaced with
systemd's StateDirectory management to place the data under
/var/lib/cockroachdb for all uses.

There's an included RequiresMountsFor= clause like usual though, so if
people want dependencies for any kind of mounted device at boot
time/before database startup, it's easy to specify using their own
mount/filesystems clause.

This can also be reverted if necessary, but, we can see if anyone ever
actually wants that later on before doing it -- it's a backwards
compatible change, anyway.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
chown -R ${cfg.user} ${cfg.dataDir}
fi
'';
unitConfig.RequiresMountsFor = "/var/lib/cockroachdb";
Copy link
Member

Choose a reason for hiding this comment

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

From systemd.exec(5):

Units with WorkingDirectory=, RootDirectory=, RootImage=, RuntimeDirectory=, StateDirectory=, CacheDirectory=, LogsDirectory= or ConfigurationDirectory= set automatically gain dependencies of type Requires= and After= on all mount units required to access the specified paths. This is equivalent to having them listed explicitly in RequiresMountsFor=.

"0.25" both represent 25% of the available system memory. The values
"1000000000" and "1GB" both represent 1 gigabyte of memory.
decimal-point number, or any bytes-based unit. For example,
<literal>"25%"</literal>, <literal>"0.25"</literal> both represent
Copy link
Member

Choose a reason for hiding this comment

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

Hm, maybe remove the double quotes as well, as <literal/> are already formatted in a different way in the manual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do they actually put quotes, though? I should have double checked, I just left them to make it a bit more clear they're of string type.

country=us,region=us-west,datacenter=us-west-1b,rack=12
country=ca,region=ca-east,datacenter=ca-east-2,rack=4

planet=earth,province=manitoba,colo=secondary,power=3
</literal>
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that be one single line? Like eg. if used for the example attribute of mkOption:

mkOption {
  type = types.nullOr types.str;
  default = null;
  example = "country=us,region=us-west,datacenter=us-west-1b,rack=12,"
          + "country=ca,region=ca-east,datacenter=ca-east-2,rack=4,"
          + "planet=earth,province=manitoba,colo=secondary,power=3";
  description = ''
    ...
  '';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, maybe. I'll double check this too. <example> doesn't seem to work though (some kind of nested <para> conflict, I think?) but I'll look around for this, too.

Copy link
Member

@aszlig aszlig Dec 5, 2018

Choose a reason for hiding this comment

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

<example/> works within <para/>, but you'll need a <title/> or <info/> child. <informalexample/> might be better suited here. Or as said: It's maybe even better to use the example attribute of mkOption.

@thoughtpolice thoughtpolice deleted the nixos/cockroachdb-tweaks branch January 12, 2019 00:12
@infinisil
Copy link
Member

@thoughtpolice Can you wait until people have reviewed your PRs before merging them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants