-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
switch-to-configuration-ng: fix with nix-upgrade #343709
Conversation
This was working previously, so I assume the perl implementation didn't had this issue. |
cc @jmbaur |
The mount can be busy and the update fails.
@@ -1293,7 +1293,8 @@ won't take effect until you reboot the system. | |||
units_to_reload.insert(unit.to_string(), ()); | |||
record_unit(RELOAD_LIST_FILE, &unit) | |||
} | |||
} else { | |||
// systemd creates a transient mount unit for /nix/store, that we shouldn't try to unmount | |||
} else if !matches!(mountpoint.as_str(), "/nix/store") { |
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.
Why is this needed here and not in the perl one?
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 know. It wasn't needed when we introduced the test. Was the perl version taking transient mount units into account?
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 think stc
shouldn't touch any transient units. Same reason why transient units have IgnoreOnIsolate=yes
. Otherwise you'll kill things like people's graphical applications in GNOME and kill their docker containers
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.
Probably get_active_units should skip those units entirely?
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.
The dbus interface doesn't seem to return IgnoreOnIsolate
directly for the dbus call used, but it may be accessible through some other command.
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 should be a Transient=yes
property exposed in dbus. Maybe we can use that?
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.
Same as IgnoreOnIsolate, it's GetUnit()
in https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.systemd1.html
so additional dbus call is needed.
It's just not clear to me how to do this with the generated dbus interface:
https://gist.github.com/Mic92/37f1789f9bfc91fa7376845442fa6d80
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 feel familiar enough with this library. Closing this pull request instead. Probably a release blocker if it indeed can break automounts and docker container.
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.
Otherwise you'll kill things like people's graphical applications in GNOME
@arianvp Won't those be user units, and thus unaffected?
This is a NixOS / systemd thing that doesn't require review from the Nix team. It seems pretty harmless. I suppose the kernel is clever enough to "close" the file system when the only mount is read-only or something? Or maybe we're just relying on the kernel to finish up nicely during its final procedure before shutting down the all the remaining hardware. I'm not an expert on this, but as far as Nix is concerned, sounds good to me. |
The nix-team only got requested here, because I had a nix-related patch in the pull request before. |
Opened an issue instead: #343975 |
Description of changes
See https://hydra.nixos.org/build/271655940/nixlog/29 for the failure
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.