140edd2 breaks btrfs subvolumes needed for boot on my machine

[GenericNerdyUsername]

/etc/nixos#nixosConfigurations.nixos.config.fileSystems before and after the commit.

My disk-config.nix:

{ ... }:
{
  disko.devices = {

    disk = {
      nvme = {
        type = "disk";
        device = "/dev/disk/by-id/nvme-WDS250G3X0C-00SJG0_191334805406";
        content = {
          type = "table";
          format = "gpt";
          partitions = [
            {
              name = "ESP";
              start = "1MiB";
              end = "256MiB";
              fs-type = "fat32";
              bootable = true;
              content = {
                type = "filesystem";
                format = "vfat";
                mountpoint = "/boot/efi";
              };
            }
            {
              name = "root";
              start = "256MiB";
              end = "100%";
              content = {
                type = "luks";
                name = "crypted";
                extraFormatArgs = [ "--pbkdf argon2id" ];
                extraOpenArgs = [ "--allow-discards" ];
                content = {
                  type = "lvm_pv";
                  vg = "pool";
                };
              };
            }
          ];
        };
      };
    };

    lvm_vg.pool = {
      type = "lvm_vg";
      lvs = {
        root = {
          size = "210GiB";
          content = {
            type = "btrfs";
            subvolumes = {
              "/rootfs".mountpoint = "/";
              "/home".mountOptions = [ "compress=zstd" ];
              "/nix".mountOptions = [ "compress=zstd" "noatime" "noacl" ];
            };
          };
        };
        swap = {
          size = "20GiB";
          content = {
            type = "swap";
          };
        };
      };
    };

  };
}

I'm pretty sure the problem is that disko.devices.lvm_vg.pool.lvs.root.content.subvolumes.rootfs has a mountpoint and the others don't, but I feel at least an eval warning is needed here if there's a breaking change.

[Lassulus]

ah, it was hard to find out in which cases we should emit a warning, sorry for that. generally people are encouraged to check the upgrade-guide

[Mic92]

I wonder if we should make some sort of releases so that people can actually read the upgraded guides before upgrading.

[eliandoran]

@GenericNerdyUsername , thanks for the bug report. I've just had the same issue today so it helped me find the solution quickly.

@Mic92 , I think it's a great idea. I've been using disko on all my devices for a few months now and I was completely unaware of the upgrade guide.

On my side I have an upgrade.sh script which just does a nix flake update, so disko is automatically updated each time I update my system as well so I was in for a bit of a surprise.

If there are breaking changes, such as this one, it would be really nice to enforce the user to make a conscious choice if they want to update. Releases sounds like a good way to do it.

In this particular case, I would say that this might have been avoided by making auto-mount opt-out instead of opt-in (e.g. subvolumes.foo.autoMount = false) for the subvolumes that the user does not want to mount. Especially since I believe that users most likely want to mount their drives.

[lilyinstarlight]

In this particular case, I would say that this might have been avoided by making auto-mount opt-out instead of opt-in (e.g. subvolumes.foo.autoMount = false) for the subvolumes that the user does not want to mount. Especially since I believe that users most likely want to mount their drives.

I was the one who originally added subvol mounts and made it happen by default, but it's different behavior from all of the other types and it's not explicit what is being mounted and what isn't. I've since regretted that decision and endorsed the breaking change (might as well rip the bandage off now rather than later when even more people use disko)

We definitely should do more to communicate to users when breaking changes occur, though (e.g. semver, warnings, etc)

[nrabulinski]

I just got bit by it and it really scared me. The warning did not show up until I manually loaded the configuration in the nix repl and inspected all the disko options after finally figuring out that my nix store was missing from config.fileSystems.
I'm not sure when/how you emit the warning but deploy-rs happily deployed the config for me without showing any warnings. It may be deploy-rs' fault for not propagating the warning but IMO such breaking changes should actually break the build and not silently fail.

I'd like something either akin to stateVersion or just breaking changes actually failing and not emitting a warning.

[iFreilicht]

As this issue is over a year old, it's unlikely that anyone will hit this again, and as I understand we can't really fix it either, as that would also be a breaking change.

However, I think your suggestions regarding handling breaking changes are very valid, so I opened #833 for this. Let's continue the discussion there :)