From cfd08d52f507898db38cb58947920a7c4c8a8756 Mon Sep 17 00:00:00 2001 From: Marius Vollmer Date: Mon, 19 Aug 2024 11:57:12 +0300 Subject: [PATCH 1/2] storage: More precise ignoring of "other mounts" We used to ignore all other mounts when the first one is for the root filesystem. But filesystems can have more than one mount and we should look at each one individually. This also prepares the code for more cases. --- pkg/storaged/filesystem/mismounting.jsx | 16 +++++++++++----- test/verify/check-storage-btrfs | 11 ++++++----- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pkg/storaged/filesystem/mismounting.jsx b/pkg/storaged/filesystem/mismounting.jsx index 4e092465b0d..410d79cd2d1 100644 --- a/pkg/storaged/filesystem/mismounting.jsx +++ b/pkg/storaged/filesystem/mismounting.jsx @@ -45,13 +45,22 @@ export function check_mismounted_fsys(backing_block, content_block, fstab_config if (!(block_fsys || dir)) return; + function ignore_mount(m) { + // We don't complain about the rootfs, it's probably + // configured somewhere else, like in the bootloader. + if (m == "/") + return true; + + return false; + } + const mounted_at = get_mount_points(client, block_fsys, subvol); const split_options = parse_options(opts); const opt_noauto = extract_option(split_options, "noauto"); const opt_noauto_intent = extract_option(split_options, "x-cockpit-never-auto"); const opt_systemd_automount = split_options.indexOf("x-systemd.automount") >= 0; const is_mounted = mounted_at.indexOf(dir) >= 0; - const other_mounts = mounted_at.filter(m => m != dir); + const other_mounts = mounted_at.filter(m => m != dir && !ignore_mount(m)); const crypto_backing_noauto = get_cryptobacking_noauto(client, backing_block); let type; @@ -68,10 +77,7 @@ export function check_mismounted_fsys(backing_block, content_block, fstab_config else if (is_mounted && opt_noauto && !opt_noauto_intent && !opt_systemd_automount) type = "no-mount-on-boot"; } else if (other_mounts.length > 0) { - // We don't complain about the rootfs, it's probably - // configured somewhere else, like in the bootloader. - if (other_mounts[0] != "/") - type = "mounted-no-config"; + type = "mounted-no-config"; } if (type) diff --git a/test/verify/check-storage-btrfs b/test/verify/check-storage-btrfs index c9d32703f03..985c49b90e1 100755 --- a/test/verify/check-storage-btrfs +++ b/test/verify/check-storage-btrfs @@ -522,11 +522,12 @@ class TestStorageBtrfs(storagelib.StorageCase): b.click(self.card_button("btrfs subvolume", f"Mount automatically on {mount_point} on boot")) b.wait_not_present(self.card_button("btrfs subvolume", f"Mount automatically on {mount_point} on boot")) - # No warnings on main page for either subvolumes - b.go("#/") - b.wait_visible(self.card("Storage")) - b.wait_not_present(self.card_row("Storage", name=subvol) + ' .ct-icon-exclamation-triangle') - b.wait_not_present(self.card_row("Storage", name="/") + ' .ct-icon-exclamation-triangle') + # No warnings for either subvolume + b.go(f"#/") + self.click_card_row("Storage", name=disk1) + b.wait_visible(self.card("btrfs filesystem")) + b.wait_not_present(self.card_row("btrfs filesystem", name=subvol) + ' .ct-icon-exclamation-triangle') + b.wait_not_present(self.card_row("btrfs filesystem", name="/") + ' .ct-icon-exclamation-triangle') def testLuksEncrypted(self): m = self.machine From 4fba4639cdd151a23691ce48dfcf26d4d08b5c32 Mon Sep 17 00:00:00 2001 From: Marius Vollmer Date: Mon, 9 Sep 2024 12:22:49 +0300 Subject: [PATCH 2/2] storage: Reimplement btrfs polling with benefits The subvolume polling is now done by a long-running monitor program that only reports changes back to Cockpit. This reduces traffic in general and allows us to do additional clever things. The monitor program can optionally keep all btrfs filesystems mounted in a "secret" location, so that we can list the subvolumes of all btrfs filesystems, even those that are not officially mounted otherwise. This mode is only enabled in Anaconda mode: It is important there since filesystems are normally not mounted when preparing storage for Anaconda, and outside of Anaconda mode, keeping long standing secret mounts is deemed to invasive. The program can also carry out btrfs operations (such as creating subvolumes) while temporarily mounting it. This allows Cockpit to always create and delete subvolumes. The UI still only does it when monitoring subvolumes works as well, however. --- pkg/storaged/btrfs/btrfs-tool.py | 276 ++++++++++++++++++++++++ pkg/storaged/btrfs/filesystem.jsx | 23 +- pkg/storaged/btrfs/subvolume.jsx | 95 +++++--- pkg/storaged/btrfs/volume.jsx | 62 ++++-- pkg/storaged/client.js | 201 ++++++++--------- pkg/storaged/crypto/actions.jsx | 31 ++- pkg/storaged/dialog.jsx | 13 +- pkg/storaged/filesystem/mismounting.jsx | 5 + pkg/storaged/utils.js | 9 +- test/verify/check-storage-anaconda | 10 +- test/verify/check-storage-btrfs | 111 +++++----- test/verify/check-storage-luks | 1 + 12 files changed, 583 insertions(+), 254 deletions(-) create mode 100755 pkg/storaged/btrfs/btrfs-tool.py diff --git a/pkg/storaged/btrfs/btrfs-tool.py b/pkg/storaged/btrfs/btrfs-tool.py new file mode 100755 index 00000000000..ae8efaf40da --- /dev/null +++ b/pkg/storaged/btrfs/btrfs-tool.py @@ -0,0 +1,276 @@ +#! /usr/bin/python3 + +# btrfs-tool -- Query and monitor btrfs filesystems +# +# This program monitors all btrfs filesystems and reports their +# subvolumes and other things. +# +# It can do that continously, or as a one shot operation. The tool +# mounts btrfs filesystems as necessary to retrieve the requested +# information, but does it in a polite way: they are mounted once and +# then left mounted until that is no longer needed. Typically, you +# might see some mounts when a Cockpit session starts, and the +# corresponding unmounts when it ends. +# +# This tool can be run multiple times concurrently with itself, and it +# wont get confused. + +import contextlib +import fcntl +import json +import os +import re +import signal +import subprocess +import sys +import time + + +def debug(msg): + # subprocess.check_call(["logger", msg]) + # sys.stderr.write(msg + "\n") + pass + + +TMP_MP_DIR = "/var/lib/cockpit/btrfs" + + +def read_all(fd): + data = b"" + while True: + part = os.read(fd, 4096) + if len(part) == 0: + return data + data += part + + +@contextlib.contextmanager +def mount_database(): + path = TMP_MP_DIR + "/db" + os.makedirs(TMP_MP_DIR, mode=0o700, exist_ok=True) + fd = os.open(path, os.O_RDWR | os.O_CREAT) + fcntl.flock(fd, fcntl.LOCK_EX) + data = read_all(fd) + blob = {} + try: + if len(data) > 0: + blob = json.loads(data) + except Exception as err: + sys.stderr.write(f"Failed to read {path} as JSON: {err}\n") + try: + yield blob + data = json.dumps(blob).encode() + b"\n" + os.lseek(fd, 0, os.SEEK_SET) + os.truncate(fd, 0) + os.write(fd, data) + finally: + os.close(fd) + + +# There is contextlib.chdir in Python 3.11, which we should use once +# it is available everywhere. +# +@contextlib.contextmanager +def context_chdir(path): + old_cwd = os.getcwd() + os.chdir(path) + try: + yield + finally: + os.chdir(old_cwd) + + +def list_filesystems(): + output = json.loads(subprocess.check_output(["lsblk", "--json", "--paths", "--list", "--noheadings", + "--output", "NAME,FSTYPE,UUID,MOUNTPOINTS"])) + filesystems = {} + for b in output['blockdevices']: + if b['fstype'] == "btrfs": + uuid = b['uuid'] + mps = list(filter(lambda x: x is not None, b['mountpoints'])) + real_mps = list(filter(lambda x: not x.startswith(TMP_MP_DIR), mps)) + has_tmp_mp = len(real_mps) < len(mps) + if uuid not in filesystems: + filesystems[uuid] = { + 'uuid': uuid, + 'devices': [b['name']], + 'mountpoints': real_mps, + 'has_tmp_mountpoint': has_tmp_mp + } + else: + filesystems[uuid]['devices'] += [b['name']] + filesystems[uuid]['mountpoints'] += real_mps + filesystems[uuid]['has_tmp_mountpoint'] = filesystems[uuid]['has_tmp_mountpoint'] or has_tmp_mp + return filesystems + + +tmp_mountpoints = set() + + +def add_tmp_mountpoint(db, fs, dev, opt_repair): + global tmp_mountpoints + uuid = fs['uuid'] + if uuid not in tmp_mountpoints: + debug(f"ADDING {uuid}") + tmp_mountpoints.add(uuid) + if uuid in db and db[uuid] > 0: + db[uuid] += 1 + else: + db[uuid] = 1 + if not fs['has_tmp_mountpoint'] and (db[uuid] == 1 or opt_repair): + path = TMP_MP_DIR + "/" + uuid + debug(f"MOUNTING {path}") + os.makedirs(path, exist_ok=True) + subprocess.check_call(["mount", dev, path]) + + +def remove_tmp_mountpoint(db, uuid): + global tmp_mountpoints + if uuid in tmp_mountpoints: + debug(f"REMOVING {uuid}") + tmp_mountpoints.remove(uuid) + if db[uuid] == 1: + path = TMP_MP_DIR + "/" + uuid + try: + debug(f"UNMOUNTING {path}") + subprocess.check_call(["umount", path]) + subprocess.check_call(["rmdir", path]) + except Exception as err: + sys.stderr.write(f"Failed to unmount {path}: {err}\n") + del db[uuid] + else: + db[uuid] -= 1 + + +def remove_all_tmp_mountpoints(): + with mount_database() as db: + for mp in set(tmp_mountpoints): + remove_tmp_mountpoint(db, mp) + + +def force_mount_point(db, fs, opt_repair): + add_tmp_mountpoint(db, fs, fs['devices'][0], opt_repair) + return TMP_MP_DIR + "/" + fs['uuid'] + + +def get_mount_point(db, fs, opt_mount, opt_repair): + if len(fs['mountpoints']) > 0: + remove_tmp_mountpoint(db, fs['uuid']) + return fs['mountpoints'][0] + elif opt_mount: + return force_mount_point(db, fs, opt_repair) + else: + return None + + +def get_subvolume_info(mp): + lines = subprocess.check_output(["btrfs", "subvolume", "list", "-apuq", mp]).splitlines() + subvols = [] + for line in lines: + match = re.match(b"ID (\\d+).*parent (\\d+).*parent_uuid (.*)uuid (.*) path (/)?(.*)", line) + if match: + pathname = match[6].decode(errors='replace') + # Ignore podman btrfs subvolumes, they are an implementation detail. + if "containers/storage/btrfs/subvolumes" not in pathname: + subvols += [ + { + 'pathname': pathname, + 'id': int(match[1]), + 'parent': int(match[2]), + 'uuid': match[4].decode(), + 'parent_uuid': None if match[3][0] == ord("-") else match[3].decode().strip() + } + ] + return subvols + + +def get_default_subvolume(mp): + output = subprocess.check_output(["btrfs", "subvolume", "get-default", mp]) + match = re.match(b"ID (\\d+).*", output) + if match: + return int(match[1]) + else: + return None + + +def get_usages(uuid): + output = subprocess.check_output(["btrfs", "filesystem", "show", "--raw", uuid]) + usages = {} + for line in output.splitlines(): + match = re.match(b".*used\\s+(\\d+)\\s+path\\s+([\\w/]+).*", line) + if match: + usages[match[2].decode()] = int(match[1]) + return usages + + +def poll(opt_mount, opt_repair): + debug(f"POLL mount {opt_mount} repair {opt_repair}") + with mount_database() as db: + filesystems = list_filesystems() + info = {} + for fs in filesystems.values(): + mp = get_mount_point(db, fs, opt_mount, opt_repair) + if mp: + try: + info[fs['uuid']] = { + 'subvolumes': get_subvolume_info(mp), + 'default_subvolume': get_default_subvolume(mp), + 'usages': get_usages(fs['uuid']), + } + except Exception as err: + info[fs['uuid']] = {'error': str(err)} + return info + + +def cmd_monitor(opt_mount): + old_infos = poll(opt_mount, opt_repair=False) + sys.stdout.write(json.dumps(old_infos) + "\n") + sys.stdout.flush() + while True: + time.sleep(5.0) + new_infos = poll(opt_mount, opt_repair=False) + if new_infos != old_infos: + sys.stdout.write(json.dumps(new_infos) + "\n") + sys.stdout.flush() + old_infos = new_infos + + +def cmd_poll(opt_mount): + infos = poll(opt_mount, opt_repair=True) + sys.stdout.write(json.dumps(infos) + "\n") + sys.stdout.flush() + + +def cmd_do(uuid, cmd): + debug(f"DO {uuid} {cmd}") + with mount_database() as db: + filesystems = list_filesystems() + for fs in filesystems.values(): + if fs['uuid'] == uuid: + mp = force_mount_point(db, fs, opt_repair=True) + with context_chdir(mp): + subprocess.check_call(cmd) + + +def cmd(args): + if len(args) > 1: + if args[1] == "poll": + cmd_poll(len(args) > 2) + elif args[1] == "monitor": + cmd_monitor(len(args) > 2) + elif args[1] == "do": + cmd_do(args[2], args[3:]) + + +def main(args): + signal.signal(signal.SIGTERM, lambda _signo, _stack: sys.exit(0)) + try: + cmd(args) + except Exception as err: + sys.stderr.write(str(err) + "\n") + sys.exit(1) + finally: + remove_all_tmp_mountpoints() + + +main(sys.argv) diff --git a/pkg/storaged/btrfs/filesystem.jsx b/pkg/storaged/btrfs/filesystem.jsx index c093297ae37..e32b9dd7136 100644 --- a/pkg/storaged/btrfs/filesystem.jsx +++ b/pkg/storaged/btrfs/filesystem.jsx @@ -27,10 +27,10 @@ import { DescriptionList } from "@patternfly/react-core/dist/esm/components/Desc import { new_card, ChildrenTable, StorageCard, StorageDescription } from "../pages.jsx"; -import { StorageUsageBar, StorageLink } from "../storage-controls.jsx"; -import { btrfs_device_usage, btrfs_is_volume_mounted } from "./utils.jsx"; +import { StorageUsageBar } from "../storage-controls.jsx"; +import { btrfs_device_usage } from "./utils.jsx"; import { btrfs_device_actions } from "./device.jsx"; -import { rename_dialog } from "./volume.jsx"; +import { BtrfsLabelDescription } from "./volume.jsx"; const _ = cockpit.gettext; @@ -52,28 +52,13 @@ export function make_btrfs_filesystem_card(next, backing_block, content_block) { const BtrfsFilesystemCard = ({ card, backing_block, content_block }) => { const block_btrfs = client.blocks_fsys_btrfs[content_block.path]; const uuid = block_btrfs && block_btrfs.data.uuid; - const label = block_btrfs && block_btrfs.data.label; const use = btrfs_device_usage(client, uuid, block_btrfs.path); - // Changing the label is only supported when the device is not mounted - // otherwise we will get btrfs filesystem error ERROR: device /dev/vda5 is - // mounted, use mount point. This is a libblockdev/udisks limitation as it - // only passes the device and not the mountpoint when the device is mounted. - // https://github.com/storaged-project/libblockdev/issues/966 - const is_mounted = btrfs_is_volume_mounted(client, [backing_block]); - return ( - rename_dialog(block_btrfs, label)} - excuse={is_mounted ? _("Btrfs volume is mounted") : null}> - {_("edit")} - } - /> + { block_btrfs && diff --git a/pkg/storaged/btrfs/subvolume.jsx b/pkg/storaged/btrfs/subvolume.jsx index 9fad2f92eff..e025d625f4c 100644 --- a/pkg/storaged/btrfs/subvolume.jsx +++ b/pkg/storaged/btrfs/subvolume.jsx @@ -31,7 +31,7 @@ import { import { StorageUsageBar } from "../storage-controls.jsx"; import { encode_filename, decode_filename, - get_fstab_config_with_client, reload_systemd, extract_option, parse_options, + get_fstab_config_with_client, reload_systemd, flatten, teardown_active_usage, } from "../utils.js"; import { btrfs_usage, validate_subvolume_name, parse_subvol_from_options } from "./utils.jsx"; @@ -44,7 +44,7 @@ import { check_mismounted_fsys, MismountAlert } from "../filesystem/mismounting. import { is_mounted, is_valid_mount_point, mount_point_text, MountPoint, edit_mount_point } from "../filesystem/utils.jsx"; -import client, { btrfs_poll } from "../client.js"; +import client, { btrfs_poll, btrfs_tool } from "../client.js"; const _ = cockpit.gettext; @@ -58,9 +58,14 @@ function subvolume_mount(volume, subvol, forced_options) { mounting_dialog(client, block, "mount", forced_options, subvol); } -function get_mount_point_in_parent(volume, subvol) { - const block = client.blocks[volume.path]; +function get_rw_mount_point(volume, subvol) { + const mount_points = client.btrfs_mounts[volume.data.uuid]; + return mount_points?.[subvol.id]?.rw_mount_points?.[0]; +} + +function get_rw_mount_point_in_parent(volume, subvol) { const subvols = client.uuids_btrfs_subvols[volume.data.uuid]; + if (!subvols) return null; @@ -68,14 +73,12 @@ function get_mount_point_in_parent(volume, subvol) { const has_parent_subvol = (p.pathname == "/" && subvol.pathname !== "/") || (subvol.pathname.substring(0, p.pathname.length) == p.pathname && subvol.pathname[p.pathname.length] == "/"); - if (has_parent_subvol && is_mounted(client, block, p)) { - const [, pmp, opts] = get_fstab_config_with_client(client, block, false, p); - const opt_ro = extract_option(parse_options(opts), "ro"); - if (!opt_ro) { - if (p.pathname == "/") - return pmp + "/" + subvol.pathname; - else - return pmp + subvol.pathname.substring(p.pathname.length); + const parent_rw_mp = get_rw_mount_point(volume, p); + if (has_parent_subvol && parent_rw_mp) { + if (p.pathname == "/") { + return parent_rw_mp + "/" + subvol.pathname; + } else { + return parent_rw_mp + subvol.pathname.substring(p.pathname.length); } } } @@ -130,8 +133,10 @@ function set_mount_options(subvol, block, vals) { }); } -function subvolume_create(volume, subvol, parent_dir) { +function subvolume_create(volume, subvol) { const block = client.blocks[volume.path]; + const parent_dir = (get_rw_mount_point(volume, subvol) || + get_rw_mount_point_in_parent(volume, subvol)); let action_variants = [ { tag: null, Title: _("Create and mount") }, @@ -170,7 +175,14 @@ function subvolume_create(volume, subvol, parent_dir) { // HACK: cannot use block_btrfs.CreateSubvolume as it always creates a subvolume relative to MountPoints[0] which // makes it impossible to handle a situation where we have multiple subvolumes mounted. // https://github.com/storaged-project/udisks/issues/1242 - await cockpit.spawn(["btrfs", "subvolume", "create", `${parent_dir}/${vals.name}`], { superuser: "require", err: "message" }); + if (parent_dir) + await cockpit.spawn(["btrfs", "subvolume", "create", `${parent_dir}/${vals.name}`], { superuser: "require", err: "message" }); + else { + await btrfs_tool(["do", volume.data.uuid, + "btrfs", "subvolume", "create", + subvol.pathname == "/" ? vals.name : subvol.pathname + "/" + vals.name + ]); + } await btrfs_poll(); if (vals.mount_point !== "") { await set_mount_options(subvol, block, vals); @@ -180,9 +192,10 @@ function subvolume_create(volume, subvol, parent_dir) { }); } -function subvolume_delete(volume, subvol, mount_point_in_parent, card) { +function subvolume_delete(volume, subvol, card) { const block = client.blocks[volume.path]; const subvols = client.uuids_btrfs_subvols[volume.data.uuid]; + const mount_point_in_parent = get_rw_mount_point_in_parent(volume, subvol); function get_direct_subvol_children(subvol) { function is_direct_parent(sv) { @@ -224,7 +237,11 @@ function subvolume_delete(volume, subvol, mount_point_in_parent, card) { if (config) configs_to_remove.push(config); - paths_to_delete.push(mount_point_in_parent + sv.pathname.substring(subvol.pathname.length)); + paths_to_delete.push(sv.pathname); + } + + function move_to_parent(pathname) { + return mount_point_in_parent + pathname.substring(subvol.pathname.length); } dialog_open({ @@ -237,8 +254,15 @@ function subvolume_delete(volume, subvol, mount_point_in_parent, card) { await teardown_active_usage(client, usage); for (const c of configs_to_remove) await block.RemoveConfigurationItem(c, {}); - await cockpit.spawn(["btrfs", "subvolume", "delete"].concat(paths_to_delete), - { superuser: "require", err: "message" }); + if (mount_point_in_parent) { + await cockpit.spawn(["btrfs", "subvolume", "delete", + ...paths_to_delete.map(move_to_parent) + ], + { superuser: "require", err: "message" }); + } else { + await btrfs_tool(["do", volume.data.uuid, + "btrfs", "subvolume", "delete", ...paths_to_delete]); + } await btrfs_poll(); navigate_away_from_card(card); } @@ -318,16 +342,15 @@ function make_btrfs_subvolume_page(parent, volume, subvol, path_prefix, subvols) const use = btrfs_usage(client, volume); const block = client.blocks[volume.path]; + const block_fsys = client.blocks_fsys[volume.path]; const fstab_config = get_fstab_config_with_client(client, block, false, subvol); - const [, mount_point, opts] = fstab_config; - const opt_ro = extract_option(parse_options(opts), "ro"); + const [, mount_point] = fstab_config; const mismount_warning = check_mismounted_fsys(block, block, fstab_config, subvol); const mounted = is_mounted(client, block, subvol); const mp_text = mount_point_text(mount_point, mounted); if (mp_text == null) return null; const forced_options = [`subvol=${subvol.pathname}`]; - const mount_point_in_parent = get_mount_point_in_parent(volume, subvol); if (client.in_anaconda_mode()) { actions.push({ @@ -348,34 +371,36 @@ function make_btrfs_subvolume_page(parent, volume, subvol, path_prefix, subvols) }); } - // If the current subvolume is mounted rw with an fstab entry or any parent - // subvolume is mounted rw with an fstab entry allow subvolume creation. + // If the filesystem is mounted anywhere, we know that we are + // showing the real list of subvolumes. (Otherwise only those in + // fstab are shown.) If so, we allow creating new ones and + // deleting existing ones, because we know that those changes will + // be reflected in the UI. However, we don't allow deleting the + // last mounted subvolume, since that would also break the + // subvolume listing. + let create_excuse = ""; - if (!mount_point_in_parent) { - if (!mounted) - create_excuse = _("Subvolume needs to be mounted"); - else if (opt_ro) - create_excuse = _("Subvolume needs to be mounted writable"); + let delete_excuse = ""; + if (!block_fsys || block_fsys.MountPoints.length == 0) + create_excuse = delete_excuse = _("At least one subvolume needs to be mounted"); + else if (block_fsys && block_fsys.MountPoints.length == 1 && + decode_filename(block_fsys.MountPoints[0]) == mount_point) { + delete_excuse = _("The last mounted subvolume can not be deleted"); } actions.push({ title: _("Create subvolume"), excuse: create_excuse, - action: () => subvolume_create(volume, subvol, (mounted && !opt_ro) ? mount_point : mount_point_in_parent), + action: () => subvolume_create(volume, subvol), }); - let delete_excuse = ""; - if (!mount_point_in_parent) { - delete_excuse = _("At least one parent needs to be mounted writable"); - } - // Don't show deletion for the root subvolume as it can never be deleted. if (subvol.id !== 5 && subvol.pathname !== "/") actions.push({ danger: true, title: _("Delete"), excuse: delete_excuse, - action: () => subvolume_delete(volume, subvol, mount_point_in_parent, card), + action: () => subvolume_delete(volume, subvol, card), }); function strip_prefix(str, prefix) { diff --git a/pkg/storaged/btrfs/volume.jsx b/pkg/storaged/btrfs/volume.jsx index 3b4284516e6..d663eed6caf 100644 --- a/pkg/storaged/btrfs/volume.jsx +++ b/pkg/storaged/btrfs/volume.jsx @@ -31,7 +31,7 @@ import { } from "../pages.jsx"; import { StorageUsageBar, StorageLink } from "../storage-controls.jsx"; import { fmt_size_long, validate_fsys_label, should_ignore } from "../utils.js"; -import { btrfs_usage, btrfs_is_volume_mounted } from "./utils.jsx"; +import { btrfs_usage } from "./utils.jsx"; import { dialog_open, TextInput } from "../dialog.jsx"; import { make_btrfs_subvolume_pages } from "./subvolume.jsx"; import { btrfs_device_actions } from "./device.jsx"; @@ -82,7 +82,7 @@ export function make_btrfs_volume_page(parent, uuid) { make_btrfs_subvolume_pages(subvolumes_page, volume); } -export function rename_dialog(block_btrfs, label) { +function rename_dialog(block_btrfs, label, rw_mount_point) { dialog_open({ Title: _("Change label"), Fields: [ @@ -94,36 +94,58 @@ export function rename_dialog(block_btrfs, label) { ], Action: { Title: _("Save"), - action: function (vals) { - return block_btrfs.SetLabel(vals.name, {}); + action: async function (vals) { + if (rw_mount_point) { + await cockpit.spawn(["btrfs", "filesystem", "label", rw_mount_point, vals.name], + { superuser: true }); + const block = client.blocks[block_btrfs.path]; + await block.Rescan({}); + } else + await block_btrfs.SetLabel(vals.name, {}); } } }); } -const BtrfsVolumeCard = ({ card, block_devices, uuid, use }) => { - const block_btrfs = client.blocks_fsys_btrfs[block_devices[0].path]; +export const BtrfsLabelDescription = ({ block_btrfs }) => { const label = block_btrfs.data.label || "-"; - // Changing the label is only supported when the device is not mounted - // otherwise we will get btrfs filesystem error ERROR: device /dev/vda5 is - // mounted, use mount point. This is a libblockdev/udisks limitation as it - // only passes the device and not the mountpoint when the device is mounted. - // https://github.com/storaged-project/libblockdev/issues/966 - const is_mounted = btrfs_is_volume_mounted(client, block_devices); + // We can change the label when at least one filesystem subvolume + // is mounted rw, or when nothing is mounted. + + let rw_mount_point = null; + let is_mounted = false; + const mount_points = client.btrfs_mounts[block_btrfs.data.uuid]; + for (const id in mount_points) { + const mp = mount_points[id]; + if (mp.mount_points.length > 0) + is_mounted = true; + if (mp.rw_mount_points.length > 0 && !rw_mount_point) + rw_mount_point = mp.rw_mount_points[0]; + } + + let excuse = null; + if (is_mounted && !rw_mount_point) + excuse = _("Filesystem is mounted read-only"); + + return rename_dialog(block_btrfs, label, rw_mount_point)} + excuse={excuse}> + {_("edit")} + } + />; +}; + +const BtrfsVolumeCard = ({ card, block_devices, uuid, use }) => { + const block_btrfs = client.blocks_fsys_btrfs[block_devices[0].path]; return ( - rename_dialog(block_btrfs, label)} - excuse={is_mounted ? _("Btrfs volume is mounted") : null}> - {_("edit")} - } - /> + diff --git a/pkg/storaged/client.js b/pkg/storaged/client.js index eba1975f972..b247f9f0024 100644 --- a/pkg/storaged/client.js +++ b/pkg/storaged/client.js @@ -39,6 +39,8 @@ import { export_mount_point_mapping } from "./anaconda.jsx"; import { dequal } from 'dequal/lite'; +import btrfs_tool_py from "./btrfs/btrfs-tool.py"; + /* STORAGED CLIENT */ @@ -200,120 +202,6 @@ client.swap_sizes = instance_sampler([{ name: "swapdev.length" }, { name: "swapdev.free" }, ], "direct"); -export async function btrfs_poll() { - const usage_regex = /used\s+(?\d+)\s+path\s+(?[\w/]+)/; - if (!client.uuids_btrfs_subvols) - client.uuids_btrfs_subvols = { }; - if (!client.uuids_btrfs_usage) - client.uuids_btrfs_usage = { }; - if (!client.uuids_btrfs_default_subvol) - client.uuids_btrfs_default_subvol = { }; - if (!client.uuids_btrfs_volume) - return; - - if (!client.superuser.allowed || !client.features.btrfs) { - return; - } - - const uuids_subvols = { }; - const uuids_usage = { }; - const btrfs_default_subvol = { }; - for (const uuid of Object.keys(client.uuids_btrfs_volume)) { - const blocks = client.uuids_btrfs_blocks[uuid]; - if (!blocks) - continue; - - // In multi device setups MountPoints can be on either of the block devices, so try them all. - const MountPoints = blocks.map(block => { - return client.blocks_fsys[block.path]; - }).map(block_fsys => block_fsys.MountPoints).reduce((accum, current) => accum.concat(current)); - const mp = MountPoints[0]; - if (mp) { - const mount_point = utils.decode_filename(mp); - try { - // HACK: UDisks GetSubvolumes method uses `subvolume list -p` which - // does not show the full subvolume path which we want to show in the UI - // - // $ btrfs subvolume list -p /run/butter - // ID 256 gen 7 parent 5 top level 5 path one - // ID 257 gen 7 parent 256 top level 256 path two - // ID 258 gen 7 parent 257 top level 257 path two/three/four - // - // $ btrfs subvolume list -ap /run/butter - // ID 256 gen 7 parent 5 top level 5 path /one - // ID 257 gen 7 parent 256 top level 256 path one/two - // ID 258 gen 7 parent 257 top level 257 path /one/two/three/four - const output = await cockpit.spawn(["btrfs", "subvolume", "list", "-apuq", mount_point], { superuser: "require", err: "message" }); - const subvols = [{ pathname: "/", id: 5, parent: null }]; - for (const line of output.split("\n")) { - const m = line.match(/ID (\d+).*parent (\d+).*parent_uuid (.*)uuid (.*) path (\/)?(.*)/); - if (m) { - const pathname = m[6]; - // Ignore podman btrfs subvolumes, they are an implementation detail. - if (pathname.includes("containers/storage/btrfs/subvolumes")) { - continue; - } - - // The parent uuid is the uuid of which this subvolume is a snapshot. - // https://github.com/torvalds/linux/blob/8d025e2092e29bfd13e56c78e22af25fac83c8ec/include/uapi/linux/btrfs.h#L885 - let parent_uuid = m[3].trim(); - // BTRFS_UUID_SIZE is 16 - parent_uuid = parent_uuid.length < 16 ? null : parent_uuid; - subvols.push({ pathname, id: Number(m[1]), parent: Number(m[2]), uuid: m[4], parent_uuid }); - } - } - uuids_subvols[uuid] = subvols; - } catch (err) { - console.warn(`unable to obtain subvolumes for mount point ${mount_point}`, err); - } - - // HACK: Obtain the default subvolume, required for mounts in which do not specify a subvol and subvolid. - // In the future can be obtained via UDisks, it requires the btrfs partition to be mounted somewhere. - // https://github.com/storaged-project/udisks/commit/b6966b7076cd837f9d307eef64beedf01bc863ae - try { - const output = await cockpit.spawn(["btrfs", "subvolume", "get-default", mount_point], { superuser: "require", err: "message" }); - const id_match = output.match(/ID (\d+).*/); - if (id_match) - btrfs_default_subvol[uuid] = Number(id_match[1]); - } catch (err) { - console.warn(`unable to obtain default subvolume for mount point ${mount_point}`, err); - } - - // HACK: UDisks should expose a better btrfs API with btrfs device information - // https://github.com/storaged-project/udisks/issues/1232 - // TODO: optimise into just parsing one `btrfs filesystem show`? - try { - const usage_output = await cockpit.spawn(["btrfs", "filesystem", "show", "--raw", uuid], { superuser: "require", err: "message" }); - const usages = {}; - for (const line of usage_output.split("\n")) { - const match = usage_regex.exec(line); - if (match) { - const { used, device } = match.groups; - usages[device] = used; - } - } - uuids_usage[uuid] = usages; - } catch (err) { - console.warn(`btrfs filesystem show ${uuid}`, err); - } - } else { - uuids_subvols[uuid] = null; - uuids_usage[uuid] = null; - } - } - - if (!dequal(client.uuids_btrfs_subvols, uuids_subvols) || !dequal(client.uuids_btrfs_usage, uuids_usage) || - !dequal(client.uuids_btrfs_default_subvol, btrfs_default_subvol)) { - debug("btrfs_pol new subvols:", uuids_subvols); - client.uuids_btrfs_subvols = uuids_subvols; - client.uuids_btrfs_usage = uuids_usage; - debug("btrfs_pol usage:", uuids_usage); - client.uuids_btrfs_default_subvol = btrfs_default_subvol; - debug("btrfs_pol default subvolumes:", btrfs_default_subvol); - client.update(); - } -} - function btrfs_findmnt_poll() { if (!client.btrfs_mounts) client.btrfs_mounts = { }; @@ -327,6 +215,8 @@ function btrfs_findmnt_poll() { for (const fs of mounts.filesystems) { const subvolid_match = fs.options.match(/subvolid=(?\d+)/); const subvol_match = fs.options.match(/subvol=(?[\w\\/]+)/); + const ro = fs.options.split(",").indexOf("ro") >= 0; + if (!subvolid_match && !subvol_match) { console.warn("findmnt entry without subvol and subvolid", fs); break; @@ -338,6 +228,7 @@ function btrfs_findmnt_poll() { pathname: subvol, id: subvolid, mount_points: [fs.target], + rw_mount_points: ro ? [] : [fs.target], }; if (!(fs.uuid in btrfs_mounts)) { @@ -347,6 +238,8 @@ function btrfs_findmnt_poll() { // We need to handle multiple mounts, they are listed separate. if (subvolid in btrfs_mounts[fs.uuid]) { btrfs_mounts[fs.uuid][subvolid].mount_points.push(fs.target); + if (!ro) + btrfs_mounts[fs.uuid][subvolid].rw_mount_points.push(fs.target); } else { btrfs_mounts[fs.uuid][subvolid] = subvolume; } @@ -397,15 +290,91 @@ function btrfs_findmnt_poll() { }); } +function btrfs_update(data) { + if (!client.uuids_btrfs_subvols) + client.uuids_btrfs_subvols = { }; + if (!client.uuids_btrfs_usage) + client.uuids_btrfs_usage = { }; + if (!client.uuids_btrfs_default_subvol) + client.uuids_btrfs_default_subvol = { }; + + const uuids_subvols = { }; + const uuids_usage = { }; + const default_subvol = { }; + + for (const uuid in data) { + if (data[uuid].error) { + console.warn("Error polling btrfs", uuid, data[uuid].error); + } else { + uuids_subvols[uuid] = [{ pathname: "/", id: 5, parent: null }].concat(data[uuid].subvolumes); + uuids_usage[uuid] = data[uuid].usages; + default_subvol[uuid] = data[uuid].default_subvolume; + } + } + + if (!dequal(client.uuids_btrfs_subvols, uuids_subvols) || !dequal(client.uuids_btrfs_usage, uuids_usage) || + !dequal(client.uuids_btrfs_default_subvol, default_subvol)) { + debug("btrfs_pol new subvols:", uuids_subvols); + client.uuids_btrfs_subvols = uuids_subvols; + client.uuids_btrfs_usage = uuids_usage; + debug("btrfs_pol usage:", uuids_usage); + client.uuids_btrfs_default_subvol = default_subvol; + debug("btrfs_pol default subvolumes:", default_subvol); + client.update(); + } +} + +export async function btrfs_tool(args) { + return await python.spawn(btrfs_tool_py, args, { superuser: "require" }); +} + +function btrfs_poll_options() { + if (client.in_anaconda_mode()) + return ["--mount"]; + else + return []; +} + +export async function btrfs_poll() { + if (!client.superuser.allowed || !client.features.btrfs) { + return; + } + + const data = JSON.parse(await btrfs_tool(["poll", ...btrfs_poll_options()])); + btrfs_update(data); +} + +function btrfs_start_monitor() { + if (!client.superuser.allowed || !client.features.btrfs) { + return; + } + + const channel = python.spawn(btrfs_tool_py, ["monitor", ...btrfs_poll_options()], { superuser: "require" }); + let buf = ""; + + channel.stream(output => { + buf += output; + const lines = buf.split("\n"); + buf = lines[lines.length - 1]; + if (lines.length >= 2) { + const data = JSON.parse(lines[lines.length - 2]); + btrfs_update(data); + } + }); + + channel.catch(err => { + throw new Error(err.toString()); + }); +} + function btrfs_start_polling() { debug("starting polling for btrfs subvolumes"); - window.setInterval(btrfs_poll, 5000); client.uuids_btrfs_subvols = { }; client.uuids_btrfs_usage = { }; client.uuids_btrfs_default_subvol = { }; client.btrfs_mounts = { }; - btrfs_poll(); btrfs_findmnt_poll(); + btrfs_start_monitor(); } /* Derived indices. diff --git a/pkg/storaged/crypto/actions.jsx b/pkg/storaged/crypto/actions.jsx index 85df842ff1e..797055192ef 100644 --- a/pkg/storaged/crypto/actions.jsx +++ b/pkg/storaged/crypto/actions.jsx @@ -21,8 +21,8 @@ import cockpit from "cockpit"; import client from "../client"; import { get_existing_passphrase, unlock_with_type } from "./keyslots.jsx"; -import { set_crypto_auto_option } from "../utils.js"; -import { dialog_open, PassInput } from "../dialog.jsx"; +import { set_crypto_auto_option, get_active_usage, teardown_active_usage, block_name } from "../utils.js"; +import { dialog_open, PassInput, init_teardown_usage, TeardownMessage, BlockingMessage } from "../dialog.jsx"; const _ = cockpit.gettext; @@ -63,7 +63,32 @@ export function lock(block) { if (!crypto) return; - return crypto.Lock({}).then(() => set_crypto_auto_option(block, false)); + const name = block_name(block); + const usage = get_active_usage(client, block.path, _("lock")); + + if (usage.Blocking) { + dialog_open({ + Title: cockpit.format(_("$0 is in use"), name), + Body: BlockingMessage(usage) + }); + return; + } + + dialog_open({ + Title: cockpit.format(_("Lock $0?"), name), + Teardown: TeardownMessage(usage), + Action: { + Title: _("Lock"), + action: async function () { + await teardown_active_usage(client, usage); + await crypto.Lock({}); + await set_crypto_auto_option(block, false); + } + }, + Inits: [ + init_teardown_usage(client, usage) + ] + }); } export function std_lock_action(backing_block, content_block) { diff --git a/pkg/storaged/dialog.jsx b/pkg/storaged/dialog.jsx index c534e080d7a..02ff770151c 100644 --- a/pkg/storaged/dialog.jsx +++ b/pkg/storaged/dialog.jsx @@ -243,7 +243,7 @@ import { FormHelper } from "cockpit-components-form-helper"; import { decode_filename, fmt_size, block_name, format_size_and_text, format_delay, for_each_async, get_byte_units, - is_available_block + is_available_block, BTRFS_TOOL_MOUNT_PATH } from "./utils.js"; import { fmt_to_fragments } from "utils.jsx"; import client from "./client.js"; @@ -1245,6 +1245,14 @@ export const TeardownMessage = (usage, expect_single_unmount) => { if (use.block) { const name = teardown_block_name(use); let location = use.location; + + /* Don't show mount points used internally by Cockpit. + * It's fine to tear them down, but we don't want people + * to start worrying about them. + */ + if (location && location.startsWith(BTRFS_TOOL_MOUNT_PATH)) + return; + if (use.usage == "mounted") { location = client.strip_mount_point_prefix(location); if (location === false) @@ -1263,6 +1271,9 @@ export const TeardownMessage = (usage, expect_single_unmount) => { } }); + if (rows.length == 0) + return null; + return (

{_("These changes will be made:")}

diff --git a/pkg/storaged/filesystem/mismounting.jsx b/pkg/storaged/filesystem/mismounting.jsx index 410d79cd2d1..61d126160cf 100644 --- a/pkg/storaged/filesystem/mismounting.jsx +++ b/pkg/storaged/filesystem/mismounting.jsx @@ -27,6 +27,7 @@ import { encode_filename, parse_options, unparse_options, extract_option, reload_systemd, set_crypto_auto_option, get_mount_points, + BTRFS_TOOL_MOUNT_PATH } from "../utils.js"; import { StorageButton } from "../storage-controls.jsx"; @@ -51,6 +52,10 @@ export function check_mismounted_fsys(backing_block, content_block, fstab_config if (m == "/") return true; + // This is the mount point used for monitoring btrfs filesystems. + if (m.startsWith(BTRFS_TOOL_MOUNT_PATH)) + return true; + return false; } diff --git a/pkg/storaged/utils.js b/pkg/storaged/utils.js index 51b082fd120..3b41fc9c848 100644 --- a/pkg/storaged/utils.js +++ b/pkg/storaged/utils.js @@ -25,6 +25,8 @@ import * as timeformat from "timeformat"; const _ = cockpit.gettext; const C_ = cockpit.gettext; +export const BTRFS_TOOL_MOUNT_PATH = "/var/lib/cockpit/btrfs/"; + /* UTILITIES */ @@ -864,6 +866,11 @@ export function get_active_usage(client, path, top_action, child_action, is_temp const [, mount_point] = get_fstab_config_with_client(client, block); const has_fstab_entry = is_temporary && location == mount_point; + // Ignore the secret btrfs mount point unless we are + // formatting (in which case subvol is false). + if (btrfs_volume && subvol && location.startsWith(BTRFS_TOOL_MOUNT_PATH)) + return; + for (const u of usage) { if (u.usage == 'mounted' && u.location == location) { if (is_top) { @@ -881,7 +888,7 @@ export function get_active_usage(client, path, top_action, child_action, is_temp has_fstab_entry, set_noauto: !is_top && !is_temporary, actions: (is_top ? get_actions(_("unmount")) : [_("unmount")]).concat(has_fstab_entry ? [_("mount")] : []), - blocking: client.strip_mount_point_prefix(location) === false, + blocking: client.strip_mount_point_prefix(location) === false && !location.startsWith(BTRFS_TOOL_MOUNT_PATH), }); } diff --git a/test/verify/check-storage-anaconda b/test/verify/check-storage-anaconda index 2305d9637e7..2e621a53184 100755 --- a/test/verify/check-storage-anaconda +++ b/test/verify/check-storage-anaconda @@ -283,11 +283,6 @@ class TestStorageAnaconda(storagelib.StorageCase): self.dialog_apply() self.dialog_wait_close() - # Mount so that we can create subvolumes - b.wait_text(self.card_row_col("Storage", 1, 3), "btrfs filesystem (encrypted)") - self.click_dropdown(self.card_row("Storage", location="/mnt/butter"), "Mount") - self.dialog({}) - # Create two subvolumes self.click_dropdown(self.card_row("Storage", location="/mnt/butter"), "Create subvolume") self.dialog_wait_open() @@ -306,11 +301,10 @@ class TestStorageAnaconda(storagelib.StorageCase): self.dialog_apply() self.dialog_wait_close() - # Unmount and lock, mount point exporting should still work + # Lock, mount point exporting should still work - self.click_dropdown(self.card_row("Storage", location="/mnt/butter"), "Unmount") - self.dialog({}) self.click_dropdown(self.card_row("Storage", name=disk), "Lock") + self.confirm() b.wait_text(self.card_row_col("Storage", 1, 3), "Locked data (encrypted)") self.expectExportedDevice(disk, diff --git a/test/verify/check-storage-btrfs b/test/verify/check-storage-btrfs index 985c49b90e1..3d1acae6fa7 100755 --- a/test/verify/check-storage-btrfs +++ b/test/verify/check-storage-btrfs @@ -50,7 +50,7 @@ class TestStorageBtrfs(storagelib.StorageCase): b.wait_visible(self.card_row("Storage", name="sda")) self.click_dropdown(self.card_row("Storage", name=dev_1), "Format") - self.dialog({"name": "butter", "type": "btrfs", "mount_point": mount_point}) + self.dialog({"name": "butter", "type": "btrfs", "mount_point": mount_point, "mount_options.ro": True}) # disk(s) are part of the volume card b.wait_visible(self.card_row("Storage", location=mount_point)) @@ -58,7 +58,8 @@ class TestStorageBtrfs(storagelib.StorageCase): self.click_card_row("Storage", name="sda") b.wait_text(self.card_desc("btrfs filesystem", "Label"), "butter") - # UDisks does not allow us to change the label of a mounted FS + + # Can't relabel a filesystem that is mounted read-only b.wait_visible(self.card_desc_action("btrfs filesystem", "Label") + ":disabled") # Unmount to change label @@ -66,15 +67,22 @@ class TestStorageBtrfs(storagelib.StorageCase): self.confirm() b.wait_visible(self.card_row("btrfs filesystem", location=f"{mount_point} (not mounted)")) - label = "butter" + label = "margarine" b.click(self.card_desc_action("btrfs filesystem", "Label")) self.dialog({"name": label}) b.wait_text(self.card_desc("btrfs filesystem", "Label"), label) + # Mount writable for the rest of the test self.click_dropdown(self.card_row("btrfs filesystem", name="/"), "Mount") - self.confirm() + self.dialog({"mount_options.ro": False}) b.wait_visible(self.card_row("btrfs filesystem", location=f"{mount_point}")) + # Change label + label = "butter" + b.click(self.card_desc_action("btrfs filesystem", "Label")) + self.dialog({"name": label}) + b.wait_text(self.card_desc("btrfs filesystem", "Label"), label) + # detect new subvolume subvol = "/run/butter/cake" m.execute(f"btrfs subvolume create {subvol}") @@ -165,7 +173,7 @@ class TestStorageBtrfs(storagelib.StorageCase): b.click(self.dropdown_toggle(root_sel)) b.wait_visible(self.dropdown_action(root_sel, "Create subvolume") + "[disabled]") b.wait_text(self.dropdown_description(root_sel, "Create subvolume"), - "Subvolume needs to be mounted") + "At least one subvolume needs to be mounted") b.click(self.dropdown_toggle(root_sel)) self.click_dropdown(root_sel, "Mount") @@ -242,32 +250,15 @@ class TestStorageBtrfs(storagelib.StorageCase): self.click_dropdown(self.card_row("Storage", location=mount_point), "Create subvolume") self.dialog({"name": os.path.basename(ro_subvol), "mount_point": ro_subvol, "mount_options.ro": True}) - # Since /run/butter is still mounted read-write, we can create - # subvolumes of "ro" using that. - - self.click_dropdown(self.card_row("Storage", location=ro_subvol), "Create subvolume") - self.dialog({"name": "bot"}, secondary=True) - b.wait_visible(self.card_row("Storage", name="bot")) - - # But once /run/butter has been unmounted, we can't create - # subvolumes of "ro" anymore. + # We can always create subvolumes as long as a single one is + # mounted, even read-only. self.click_dropdown(self.card_row("Storage", location="/run/butter"), "Unmount") self.confirm() - b.click(self.dropdown_toggle(self.card_row("Storage", location=ro_subvol))) - b.wait_visible(self.dropdown_action(self.card_row("Storage", location=ro_subvol), "Create subvolume") + "[disabled]") - b.wait_text(self.dropdown_description(self.card_row("Storage", location=ro_subvol), "Create subvolume"), - "Subvolume needs to be mounted writable") - b.click(self.dropdown_toggle(self.card_row("Storage", location=ro_subvol))) - # remount as rw, create subvolume and remount as ro to see parents are also checked - m.execute(f""" - mount -o remount,rw /dev/sda {ro_subvol} - btrfs subvolume create {ro_subvol}/readonly - mount -o remount,ro /dev/sda {ro_subvol} - """) - - self.check_dropdown_action_disabled(self.card_row("Storage", name="readonly"), "Create subvolume", "Subvolume needs to be mounted") + self.click_dropdown(self.card_row("Storage", location=ro_subvol), "Create subvolume") + self.dialog({"name": "bot"}, secondary=True) + b.wait_visible(self.card_row("Storage", name="bot")) def testDeleteSubvolume(self): m = self.machine @@ -338,6 +329,23 @@ class TestStorageBtrfs(storagelib.StorageCase): self.dialog({"name": child_subvol}, secondary=True) b.wait_visible(self.card_row("Storage", name=child_subvol)) + # Unmount root subvolume and delete child-subvol. That will use the mountpoint of "subvol" + self.click_dropdown(root_sel, "Unmount") + self.confirm() + + self.click_dropdown(self.card_row("Storage", name=child_subvol), "Delete") + self.confirm() + b.wait_not_present(self.card_row("Storage", name=child_subvol)) + + # Creating inside subvol will also use its mountpoint now + self.click_dropdown(self.card_row("Storage", name=subvol), "Create subvolume") + self.dialog({"name": child_subvol}, secondary=True) + b.wait_visible(self.card_row("Storage", name=child_subvol)) + + # Mount the root subvolume again so that we can delete the lot + self.click_dropdown(root_sel, "Mount") + self.confirm() + self.click_dropdown(self.card_row("Storage", name=subvol), "Delete") self.checkTeardownAction(1, "Device", child_subvol) self.checkTeardownAction(1, "Action", "delete") @@ -365,43 +373,41 @@ class TestStorageBtrfs(storagelib.StorageCase): self.confirm() b.wait_not_present(self.card_row("Storage", location=subvol_mount_point)) - # Cannot delete subvolume when no parent is mounted subvol = "new-subvol" self.click_dropdown(self.card_row("Storage", location=mount_point), "Create subvolume") self.dialog({"name": subvol, "mount_point": subvol_mount_point}) - self.click_dropdown(root_sel, "Unmount") - self.confirm() - self.check_dropdown_action_disabled(self.card_row("Storage", location=subvol_mount_point), "Delete", "At least one parent needs to be mounted writable") - self.click_dropdown(root_sel, "Mount") - self.confirm() - b.wait_visible(self.card_row("Storage", location=mount_point)) + # We can not delete the last mounted subvolume - self.click_dropdown(self.card_row("Storage", location=subvol_mount_point), "Delete") + self.click_dropdown(root_sel, "Unmount") self.confirm() - # Cannot delete read only mounted subvolume children and itself + sel = self.card_row("Storage", location=subvol_mount_point) + b.click(self.dropdown_toggle(sel)) + b.wait_visible(self.dropdown_action(sel, "Delete") + "[disabled]") + b.wait_text(self.dropdown_description(sel, "Delete"), + "The last mounted subvolume can not be deleted") + b.click(self.dropdown_toggle(sel)) + + subvol2 = "subvol2" + subvol2_mount_point = "/run/subvol2" child_subvol = "child-subvol" - self.click_dropdown(self.card_row("Storage", location=mount_point), "Create subvolume") - self.dialog({"name": subvol, "mount_point": subvol_mount_point, "mount_options.ro": True}) - b.wait_visible(self.card_row("Storage", location=subvol_mount_point)) - self.assertIn("ro", m.execute(f"findmnt -s -n -o OPTIONS {subvol_mount_point}")) + self.click_dropdown(self.card_row("Storage", location=mount_point + " (not mounted)"), "Create subvolume") + self.dialog({"name": subvol2, "mount_point": subvol2_mount_point, "mount_options.ro": True}) + b.wait_visible(self.card_row("Storage", location=subvol2_mount_point)) + self.assertIn("ro", m.execute(f"findmnt -s -n -o OPTIONS {subvol2_mount_point}")) - self.click_dropdown(self.card_row("Storage", name=subvol), "Create subvolume") + self.click_dropdown(self.card_row("Storage", name=subvol2), "Create subvolume") self.dialog({"name": child_subvol}, secondary=True) b.wait_visible(self.card_row("Storage", name=child_subvol)) - # Allowed as root is mounted - self.click_dropdown(self.card_row("Storage", name=child_subvol), "Delete") - self.dialog_wait_open() - self.dialog_cancel() - - # Unmount root as we can otherwise delete via root - self.click_dropdown(self.card_row("Storage", location=mount_point), "Unmount") + # Now we can delete "new-subvol" + self.click_dropdown(self.card_row("Storage", location=subvol_mount_point), "Delete") self.confirm() - b.wait_visible(self.card_row("Storage", location=f"{mount_point} (not mounted)")) - self.check_dropdown_action_disabled(self.card_row("Storage", name=child_subvol), "Delete", "At least one parent needs to be mounted writable") + # And also "child-subvol". The readonly-ness of "subvol2" should not matter + self.click_dropdown(self.card_row("Storage", name=child_subvol), "Delete") + self.confirm() def testMultiDevice(self): m = self.machine @@ -523,7 +529,7 @@ class TestStorageBtrfs(storagelib.StorageCase): b.wait_not_present(self.card_button("btrfs subvolume", f"Mount automatically on {mount_point} on boot")) # No warnings for either subvolume - b.go(f"#/") + b.go("#/") self.click_card_row("Storage", name=disk1) b.wait_visible(self.card("btrfs filesystem")) b.wait_not_present(self.card_row("btrfs filesystem", name=subvol) + ' .ct-icon-exclamation-triangle') @@ -552,6 +558,9 @@ class TestStorageBtrfs(storagelib.StorageCase): self.click_dropdown(self.card_row("Storage", name="sda") + " + tr", "Mount") self.dialog({"mount_point": mount_point}) + # Wait for Cockpit's own mount to go away + b.wait(lambda: "/var/lib/cockpit/btrfs" not in m.execute("findmnt")) + m.execute(f""" umount {mount_point} cryptsetup luksClose /dev/mapper/btrfs-test diff --git a/test/verify/check-storage-luks b/test/verify/check-storage-luks index 50ca7bf523f..dbc4d998b2a 100755 --- a/test/verify/check-storage-luks +++ b/test/verify/check-storage-luks @@ -268,6 +268,7 @@ class TestStorageLuks(storagelib.StorageCase): # Lock it b.click(self.card_button("Unformatted data", "Lock")) + self.confirm() b.wait_visible(self.card("Locked data")) # Make it readonly