-
Notifications
You must be signed in to change notification settings - Fork 45
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
Feature: permit running anything-sync-daemon as an unprivileged user #85
base: master
Are you sure you want to change the base?
Conversation
along with a `numtide/devshell`-based development shell providing a command to lint and format shell and Nix code (with, respectively, shellcheck and alejandra).
96b0a42
to
d310643
Compare
Hi @tomeon, I really like the changes that you have been doing ( just a bird's eye review based on your commits ). Unfortunately , I haven't been able to spend much time to give a detailed review as you already know the changes are humongous and it's not exactly possible to review but I do see that you are active and doing some good improvements based on your commits. I will see if I can review few of the stuff but I can't commit at this stage. Although, I would like to know a few things. Is there any breaking change in this full PR w.r.t the current asd version and something that you would like to summarise based on that? The debug capability handling along with fixes and added nix os support is always welcomed but the only thing that I am mostly worried about is the compatibility with the current asd version. I see that asd is a very critical piece of software and people have actually suffered data losses in the past due to the refactor and the syncing process, if you know anything that you have done to drastically alter that process ( along with changing any names for the backup folders or anything), I would recommend giving a summary of it and maybe explaining the changes here ( or if possible pointing to those specific commits along with the explanation ) as that'd help us see the scope of the upgradation process from current asd to the new one. Also, I see that most of your testing has been NixOS based, is there anything that you have done for arch or is there some difference in that? I haven't really ventured into NixOS so feel free to impart your knowledge |
Hello @manorit2001 -- thank you very much for your feedback.
If you subscribe to Hyrum's Law, then I'm afraid there are breaking changes in just about every commit 😉 . More seriously: a few of my changes are liable to break some setups, particularly the changes having to do with loading
I am working on a
I hadn't done anything for Arch Linux, though, inspired by your mention of it, I've added a Vagrant configuration that spins up an Arch Linux machine and deploys Part of what I like about NixOS systems tests is how the testing framework is distributed with However, I am happy to look into rigorous testing on Arch Linux, too. Haven't found any analog of |
e61966c
to
8d0010d
Compare
I have added the changelog. |
Also, latest run is passing; see here. |
15bfa99
to
6bbed5a
Compare
6bbed5a
to
b3656bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tomeon, apologies for the delay but you know it's a big PR so had to see what all changes are being done, I still can't cover it thoroughly but trying to gauge the changes being done in this, have some questions as well. Why is shellcheck ci removed btw?
Makefile
Outdated
# set to 1 to prevent attempting to stop asd before installation | ||
SKIP_STOP = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although added but I am not sure when it'll be set to 1
actually, the way to upgrade asd should always disable asd before updating for good sanity upgrades as it can happen that the updated asd unsync might not be able to handle the changes and would break the sync that would've happened by previous asd version but I believe this can be a good thing for testing where there wouldn't be a lot of breaking changes but just wanted to let you know about the rational behind stopping asd before upgrades
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used this for manual testing; will remove it.
$(INSTALL_DATA) init/asd-resync.service "$(DESTDIR)$(INITDIR_SYSTEMD_SYSTEM)/asd-resync.service" | ||
$(INSTALL_DATA) init/asd-resync.timer "$(DESTDIR)$(INITDIR_SYSTEMD_SYSTEM)/asd-resync.timer" | ||
$(INSTALL_DIR) "$(DESTDIR)$(INITDIR_SYSTEMD_USER)" | ||
$(INSTALL_DATA) init/asd.service "$(DESTDIR)$(INITDIR_SYSTEMD_USER)/asd.service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't both the system level and user level asd.server conflict with each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so; at least, not without the user somehow configuring them to conflict with one another. Under their default configurations, the system daemon and user-specific daemons:
- Use different systemd unit files,
- Use different configuration files,
- Use different daemon files,
- Use different lock files,
- Run as different users,
- Etcetera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, if that is the case then should be okay, wanted to know if these created any troubles or not in your manual testing. If no visible troubles then should be okay.
README.md
Outdated
|
||
Anything-sync-daemon (asd) is a tiny pseudo-daemon designed to manage user | ||
defined dirs in tmpfs and to periodically sync back to the physical disc | ||
(HDD/SSD). This is accomplished via a symlinking step and an innovative use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not symlinking step anymore, I believe it has been changed to bind mounts but a stale documentation, maybe you can update that as well. Can see the arch wiki to be updated as well as your description of Usage.md
so maybe fix the stale documents also from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; happy to handle these items.
Consult the man page or the wiki page: https://wiki.archlinux.org/index.php/Anything-sync-daemon | ||
|
||
Consult the `asd(1)` man page (available [here](/USAGE.md)) or | ||
[the Arch Linux `asd` wiki page](https://wiki.archlinux.org/index.php/Anything-sync-daemon). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you've added a nix package or something maybe provide that path also here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a Nix package, but it is distributed as a part of this repository and there is no external documentation specific to the NixOS package. I could link to the package definition itself? That is, link to nix/packages.nix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, you can skip it that then, I was wondering if this gives information about what all linux variants are supported and in first glance of the readme, it doesn't look like nix is supported, maybe we can explicitly mention it then.
|
||
# NAME | ||
|
||
`anything-sync-daemon` - Symlinks and syncs user specified dirs to RAM thus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again symlinking here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
: A boolean variable controlling whether to use "version 1" paths or not. When | ||
set to a false value, `asd` will use old-style defaults for its daemon file | ||
(`/run/asd`), lock file (`/run/asd-lock`), and configuration file | ||
(`/etc/asd.conf`). **NOTE** that this variable is ignored when `asd` is | ||
running as a non-`root` user. Defaults to disabled (effectively, | ||
`ASDNOV1PATHS=0`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the paths when this is not enabled? Also, I think renaming this to ASD_BACKWARD_COMPAT
would be good ig as it seems like it is something similar to that, this name is a bit confusing for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the paths when this is not enabled?
I'll make sure to enumerate these in the documentation. For the systemd asd.service
unit, the locations are as follows:
- Daemon file, in order of precedence from highest to lowest:
a. the value of theDAEMON_FILE
environment variable, if set and non-empty,
b.${ASDRUNDIR}/asd.run
, ifASDRUNDIR
is set and non-empty,
c.${RUNTIME_DIRECTORY%%:*}/asd.run
, ifRUNTIME_DIRECTORY
is set and non-empty (note thatRUNTIME_DIRECTORY
is a list of colon-separated directories set by systemd on the basis of theRuntimeDirectory
directive from the[Service]
section of a systemd unit), and, finally,
d./run/asd/asd.conf
. - Lock file: pretty much the same as the daemon file, with these modifications: the basename is
asd.lock
, and there is no equivalentDAEMON_FILE
-style environment variable that the user can set to an arbitrary path. - Configuration file:
/etc/asd/asd.conf
.
I think renaming this to ASD_BACKWARD_COMPAT would be good ig as it seems like it is something similar to that, this name is a bit confusing for me.
I think that ASD_BACKWARD_COMPAT
is too general; there are any number of ways in which anything-sync-daemon
might become backward-incompatible in the future, which implies on or more of the following:
- The meaning of
ASD_BACKWARD_COMPAT
would change over time (the behaviors it controls would differ), - Users would face an all-or-nothing decision: all backward-compatible behaviors, or none, or
- We'd have to choose new variable names to control new backward-compatibility-or-not behavior.
I chose ASDNOV1PATHS
for the following reasons:
- Preexisting
ASD*
variables all eschew underscores and are all quite terse (though, for what it's worth, in general I like separating semantically-distinct elements with underscores), - The
NO
bit helps emphasize that this is opt-in behavior and that by default you get the old path scheme, and is less liable to confuse afterasd
fully deprecates the old path scheme (ASDNOV1PATHS=no
will cease to have any effect, whereasASDNOV1PATHS=yes
will, in a sense, remain in effect forever). - The
V1
bit scopes the effect to a particular path scheme (that is, the first one), and - The
PATHS
bit scopes the effect to a particular realm of concern (that is, paths significant toasd
).
However, I'd be happy to consider alternatives that satisfy at least some of the desiderata just listed. What do you think about something like ASD_DISABLE_V1_FILE_PATH_SCHEME_COMPAT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why was there a need for this change? Does this help adding support for running asd as an unprivileged user? Basically I want to understand if there was a need or if this a stylistic change only.
|
||
graysky (graysky AT archlinux DOT us) | ||
|
||
# MAINTAINER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a contributors section and add your name as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
' "$0" "${1?}" | ||
} | ||
|
||
use_v1_paths() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about why we are changing paths btw, what benefits do we get and what paths are we ending up changing?
There is one test that I am particularly worried about first of all with this PR, it is basically the upgrade process, if possible could you check if we use the current asd version, stop asd ( this will happen as the part of upgradation process ) and then update your asd version to the new one with same configs as it was previously, I want to know that nothing breaks as a part of that. W.r.t inspec or serverspec, I don't have any strong preference as I haven't worked on them, I would recommend you to choose whatever feels scalable to you for the project, would see the changes based on that. |
and expand the `mkFlake` `systems` argument to more supported Linux architectures.
for managing systemwide and per-user anything-sync-daemon installations.
that exercises various anything-sync-daemon features and behaviors, including OverlayFS support and crash-recovery backups.
that prints additional context like the source file and current line number when Bash execution tracing is in effect.
rather than inlining nearly-identical `case` statements for both and `USE_OVERLAYFS` and `USE_BACKUPS`.
`diag` (simple echo to stderr) and `ediag` (`echo -e` to stderr). The intention is to replace uses of `echo` and `echo -e` that write to stdout.
but instead define the `debug` function so that it is a NOP when `DEBUG` is disabled.
in preparation for supporting running anything-sync-daemon as a non-root user.
that is, `set -euo pipefail`.
as we'll be bailing out anyway due to `set -e`. Quote `help trap`: A SIGNAL_SPEC of ERR means to execute ARG each time a command's failure would cause the shell to exit when the -e option is enabled.
with the `have_dep` function and its verbose wrapper `have_dep_diag`. This de-duplicates code in `dep_check`, and also permits code re-use when checking whether `flock` is available toward the beginning of the script. Additionally, clean up the OverlayFS version check in `dep_check` by (a) using the arithmetic comparison operator `(( ... ))` and (b) consolidating to a single conditional statement. Finally, don't bail out at the first failed dependency, but instead store the non-zero exit status and continue. This way, the code will report all missing dependencies in one go.
by (a) handling `FLOCKER` being unset, (b) respecting the choice of Bash executable by prepending `"$BASH"` to the re-executed command line, (c) propagating the current shell options by passing `"-$-"`, and (d) using portable `flock` options (omit `--verbose`, use `-x` rather than `-e`).
and replace it with explicit error messaging mentioning the configuration file.
via the `source-path=SCRIPTDIR` directive.
using the `:=` "default-assignment" parameter expansion operator.
to aid in diagnosing issues with asd's mounts
to avoid getting prettified output.
to avoid getting back data for multiple mounts when we just want one.
to better delineate what's being run
to provide additional diagnostics (e.g. exit status) and to avoid clobbering `xtrace` if it's already enabled.
when caching them for later synchronization, thus better handling funky filenames.
before shutting off the resync service.
in an attempt to speed up GitHub Actions by running the checks concurrently. Involves some not-terribly-attractive monkeypatching, with attendant stubbing/casting/etc. to satisfy MyPy.
and explain that it should not be used unless `ASDNOV1PATHS` is enabled.
for spawning an Arch Linux machine and provisioning `asd`.
by selecting the first of the colon-separated paths. This is necessary, as systemd supports multiple directories specified as `ConfigurationDirectory=` and `RuntimeDirectory=`.
[skip ci]
as asd now uses bind-mounts rather than symlinks for exposing the tmpfs-es that it manages.
in preparation for creating a Home Manager module that re-uses some code from the NixOS module.
b3656bf
to
39e0f19
Compare
39e0f19
to
f0435de
Compare
Plus a bunch of other fixes and features. Note that this changeset includes major refactorings; I offer this PR with the expectation that its scope is too large to effectively review and therefore not reasonable to merge. However, I've done my best to test and document the changes, and I think that they are genuinely useful, so if you've got the inclination to review this -- many thanks in advance!
Summary of changes:
asd
as an unprivileged user. This includes installing a systemd user unit, and loadingasd.conf
from a different location than/etc/asd.conf
. I've also added the scriptasd-mount-helper
, based onpsd-overlay-helper
, to facilitate mounting and unmounting as a non-root
user.anything-sync-daemon
Nix package (nix/packages.nix
).nix/nixos-module.nix
) for configuring and running system and user instances ofasd.service
,asd-resync.service
, andasd-resync.timer
.nix/checks.nix
) that verify the correct operation of system and userasd
services.nix/devshell.nix
), including theshellcheck
shell linter andalejandra
Nix expression language formatter.shellcheck
workflow, as an equivalent job is included in the Nix checks).doc/asd.1
to Markdown (inUSAGE.md
) and generate the manpage from that Markdown. Also expand the documentation to cover environment variables, and add some relevant links.DIR
,BACKUP
,TMP
, and so on).I'm still ironing out the GitHub Actions workflow; currently it is quite slow (at least, the NixOS systems tests are). But you can see a recent run here.
Thanks again for your consideration!