Re: [PATCH v2 6/8] Support updates via systemd-sysupdate

Demi Marie Obenour
2025-11-14T23:16:50Z

[-- Attachment #1.1.1: Type: text/plain, Size: 9725 bytes --]

On 11/14/25 07:14, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
>
>> On 11/13/25 11:44, Alyssa Ross wrote:
>>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>>>
>>>> +# 9. Call systemd-sysupdate to run the actual update.
>>>> +
>>>> +if { mkdir -p -m 0700 /run/updater }
>>>> +s6-setlock /run/update-lock
>>>> +foreground { redirfd -w 2 /dev/null rmdir -- $1 }
>>>> +if { umask 0077 mkdir -p -- $1 }
>>>> +cd $1
>>>> +foreground {
>>>> + # If this exists already that is okay.
>>>> + foreground { redirfd -w 2 /dev/null btrfs subvolume create -- shared }
>>>> +
>>>
>>> Wouldn't it break if there's already stuff in it?
>>
>> No, it works fine in this case. I checked :).
>>
>>> I'd do
>>>
>>> foreground { redirfd -w 2 /dev/null btrfs subvolume delete -- shared }
>>> if { btrfs subvolume create -- shared }
>>>
>>> and then you know you've got an empty subvolume.
>>
>> An empty subvolume isn't good: it means that systemd-sysupdate will
>> redownload an update even when it isn't needed.
>
> When would the update have already been downloaded but not applied?
> Only if there's an error in actually installing it? I'd feel happier
> knowing we were always starting with a clean state.

The existence of the downloaded files is how the update VM knows
"we already have the latest version installed, don't try to download
anything new". The VM doesn't know the actual version installed on
the host, so it can't query this by other means.
>>>> + # Snapshot directory may have files or directories with untrusted names.
>>>> + # Redirect its output to /dev/null to avoid printing them to the console.
>>>> + ifelse -n { redirfd -w 2 /dev/null rm -rf -- snapshot } {
>>>> + foreground { redirfd -w 2 echo "Cannot remove snapshot directory" }
>>>> + exit 1
>>>> + }
>>>
>>> Why not btrfs subvolume delete? It's faster and won't print names.
>>
>> It doesn't distinguish "subvolume doesn't exist" from "problem
>> deleting subvolume". A better solution is to call `rm -f` if `btrfs
>> subvolume delete` failed. That ignores "does not exist" errors,
>> but not other errors.
>
> Right. I had assumed btrfs subvolume create would cause an error if it
> already existed, but you've said it doesn't.

I don't want to create the subvolume over and over, because of the re

>>>> +
>>>> + backtick -E update_vm_id_ {
>>>> + backtick -E id_path { readlink /run/vm/by-name/sys.appvm-updates }
>>>> + basename -- $id_path
>>>> + }
>>>> +
>>>> + multisubstitute {
>>>> + define fsdir /run/vm/by-id/${update_vm_id_}/fs
>>>> + define update_vm_id ${update_vm_id_}
>>>
>>> Why?
>>
>> Avoiding serial substitution.
>
> But this is serial substitution. You're substituting update_vm_id_, and
> then you're doing another substitution of update_vm_id without the
> underscore. Why? Why not the following?
>
> backtick update_vm_id { ... }
> multisubstitute {
> define fsdir ...
> importas -Siu update_vm_id
> }

"serial substitution" means that one substitutes into a string
that has already been substituted into. See the multisubstitute
documentation for why this is bad. I avoided the route you describe
because I wanted to define fsdir and update_vm_id in the same call
to multisubstitute.

>>>> + }
>>>> +
>>>> + # $fsdir is read-only to the guest, but read-write to the host.
>>>> + # Directories bind-mounted into it are read-write to the guest.
>>>> + # See etc/s6-linux-init/run-image/service/vhost-user-fs/template/run
>>>> + # for details.
>>>> +
>>>> + # Set up /etc with what the VM needs. The VM will overlay this
>>>> + # on its own /etc.
>>>> + if { rm -rf -- ${fsdir}/etc }
>>>> + if { umask 022 mkdir -p -- ${fsdir}/updates ${fsdir}/etc/systemd }
>>>> + if { cp -R -- /etc/updatevm/sysupdate.d /etc/updatevm/url-env ${fsdir}/etc }
>>>> + if { cp -- /etc/systemd/import-pubring.gpg ${fsdir}/etc/systemd }
>>>
>>> Why copy rather than bind mount?
>>
>> Target does not exist and I didn't want to bind-mount all of /etc/systemd.
>
> You can touch a file and then bind mount, and still save a copy.

This is a tiny file. I suspect the extra exec is more expensive than
the copy.

>>>> +
>>>> + # If the directory is already mounted, unmount it. This prevents a
>>>> + # confusing error from mount.
>>>> + foreground { redirfd -w 2 /dev/null umount -- ${fsdir}/updates }
>>>> +
>>>> + # Share the update directory with the VM.
>>>> + if { mount --bind -- shared ${fsdir}/updates }
>>>> +
>>>> + # Start the update VM.
>>>> + if { vm-start $update_vm_id }
>>>> +
>>>> + # Wait for the VM to exit.
>>>> + if { s6-svwait -D ${svcdir} }
>>>> +
>>>
>>> It might be more robust to use a transient VM, like we use for
>>> AppImages, so that nothing can restart it. Transient VMs are still
>>> developing though, so it's also fine to say we'll do it this way for now
>>> and adapt it later. This would also save all the filesystem resetting
>>> you're needing to do here.
>>
>> The path to the update directory is user-provided. It's not from
>> the VM's persistent storage.
>
> Ideally (at some point) it isn't user provided, and is the VM's
> transient disk-backed storage, IMO.

I'd prefer that too, but right now this would cause repeated
calls to the updater to download the current version over and over.
That definitely isn't good. I'd prefer to get a basic updater working
first and then improve it over time.

>>>> @@ -17,5 +18,11 @@ let
>>>> callConfig = config: if builtins.typeOf config == "lambda" then config {
>>>> inherit default;
>>>> } else config;
>>>> + finalConfig = default // callConfig config;
>>>> in
>>>> - default // callConfig config;
>>>> + finalConfig // {
>>>> + update-signing-key = builtins.path {
>>>> + name = "signing-key";
>>>> + path = finalConfig.update-signing-key;
>>>> + };
>>>> + }
>>>
>>> What does this do?
>>
>> This ensures that the Nix store path doesn't depend on the name of
>> the update signing key, only its contents.
>
> Interesting. Does that matter, though? It ends up being called
> /etc/systemd/import-pubring.gpg in the image regardless.

Otherwise renaming it would cause a pointless rebuild of a bunchof stuff.

>>>> diff --git a/vm/app/updates.nix b/vm/app/updates.nix
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..d2c1e5fcb35b37c7ed8a173f19b97894a36a7f0c
>>>> --- /dev/null
>>>> +++ b/vm/app/updates.nix
>>>> @@ -0,0 +1,37 @@
>>>> +# SPDX-License-Identifier: MIT
>>>> +# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
>>>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>>>> +
>>>> +import ../../lib/call-package.nix (
>>>> +{ callSpectrumPackage, config, curl, lib, src
>>>> +, runCommand, systemd, writeScript
>>>> +}:
>>>> +
>>>> +let
>>>> + update-url = config.update-url;
>>>> + mountpoint = "/run/virtiofs/virtiofs0";
>>>> + sysupdate-path = "${systemd}/lib/systemd/systemd-sysupdate";
>>>> + runner = writeScript "update-run-script"
>>>> + ''
>>>> + #!/usr/bin/execlineb -P
>>>> + if { mount -toverlay -olowerdir=${mountpoint}/etc:/etc -- overlay /etc }
>>>> + envfile ${mountpoint}/etc/url-env
>>>
>>> Seems like overkill to use an envfile for a single URL?
>>
>> It is indeed overkill, but I'm not aware of a simpler option.
>> There is backtick + cat but that's two programs rather than one.
>
> I think the canonical way would be redirfd + withstdinas, but that's
> also two programs, so if you want to avoid that, perhaps s6-envdir?
> Reading it isn't any simpler but writing it at least doesn't require a
> special tool.

We need sed to generate the .transfer files anyway.

>>>> + importas -i update_url UPDATE_URL
>>>> + if { ${sysupdate-path} update }
>>>> + if { ${curl}/bin/curl -L --proto =http,https
>>>> + -o ${mountpoint}/updates/SHA256SUMS.gpg ''${update_url}/SHA256SUMS.gpg }
>>>> + # systemd-sysupdate recently went from needing SHA256SUMS.gpg to SHA256SUMS.sha256.asc.
>>>> + # I (Demi) have no need if this is intentional or a bug. I also have no idea if this
>>>> + # behavior will stay unchanged in the future. Therefore, create both files and let
>>>> + # systemd-sysupdate ignore the one it isn't interested in.
>>>> + if { ln -f ${mountpoint}/updates/SHA256SUMS.gpg ${mountpoint}/updates/SHA256SUMS.sha256.asc }
>>>
>>> Would be good to figure out why that happened. If we add a comment like
>>> this it's very unlikely to ever get cleaned up.
>>
>> https://github.com/systemd/systemd/issues/39273
>
> "hwdb: drop trailing whitespace"?

https://github.com/systemd/systemd/issues/39723
("systemd-sysupdate checks for SHA256SUMS.sha256.asc when fetching from file:///"),
which was fixed in
<https://github.com/systemd/systemd/commit/aa7574417b86ac0bb7ed492b7cfc872e9ace15d7>
("pull: fix SHA256SUMS fallback for file:// URLs").
systemd v258.1 should have the fix. Does Spectrum use an older nixpkgs?

>>>> + ${curl}/bin/curl -L --proto =http,https
>>>> + -o ${mountpoint}/updates/SHA256SUMS ''${update_url}/SHA256SUMS
>>>> + '';
>>>> +in
>>>> +
>>>> +callSpectrumPackage ../make-vm.nix {} {
>>>> + providers.net = [ "sys.netvm" ];
>>>> + type = "nix";
>>>> + run = "${runner}";
>>>
>>> Might as well inline this.
>>
>> I chose to keep it separate to improve readability.
>
> Okay. I'd find it more readable inlined but it's your code. :)
I changed my mind :).
--
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]