storage: Anaconda mode #19352

mvollmer · 2023-09-19T14:18:20Z

Anaconda code: https://github.com/mvollmer/anaconda-webui/tree/cockpit-storage

Demo: https://www.youtube.com/watch?v=x3KfKJhBxD0

Next steps after this PR:

Discourage the user from actually mounting new filesystems by only offering "Format only" in the format dialog.

mvollmer · 2023-09-20T12:18:59Z

@jkonecny12, please check this out! (start with the demo)

M4rtinK · 2023-09-21T11:10:08Z

doc/anaconda.md

+However, Cockpit (via UDisks2) will still write the new mount point
+configuration into the real /etc/fstab (_not_ /sysroot/etc/fstab).
+
+Anaconda will read the real /etc/fstab and reverse engineer the


@vojtechtrefny @VladimirSlavik @poncovka I think Blivet has some support for fstab parsing, right ? I just wonder what else could be there in fstab & if we can't get into a situation where we can't tell what we want to forward to the target system & what now.

Blivet doesn't have support for parsing fstab yet, but it is planned. But I think it would be better to not write to fstab -- in the "anaconda" mode Cockpit could simply write the mount points to a custom configuration file, all we need in Anaconda is a simple device -> mountpoint "mapping", we don't need the full fstab entry. We could avoid a lot of potential issues by not writing to fstab on the live image (which is not empty by the way).

We (now speaking as a udisks developer) could also add a new option/type to AddConfigurationItem to be able to write to a fstab in a different location than the default /etc/fstab.

We (now speaking as a udisks developer) could also add a new option/type to AddConfigurationItem to be able to write to a fstab in a different location than the default /etc/fstab.

I think that would only help if UDisks2 would also watch and read that different location. If modifying the real /etc/fstab is not acceptable, I think I would rather change Cockpit Storage to maintain its own "virtual fstab" (in Browser storage?) and use that instead of the UDisks2 API.

We could avoid a lot of potential issues by not writing to fstab on the live image (which is not empty by the way).

What issues do you see?

I can't think of any, tbh. Cockpit Storage will of course not corrupt /etc/fstab, if that is one of your concerns.

Cockpit now stores a mountpoint -> device map for Anaconda.

I think that would only help if UDisks2 would also watch and read that different location.

I guess we could do that, but that would be something too anaconda-specific and I am not sure if @tbzatek would be happy about it.

What issues do you see?

My main issue is with the existing entries in fstab and potential for Anaconda to mix these up with the "installer" entries created by Cockpit.

I am also not sure how well Cockpit (or UDisks to be more precise) handles btrfs subvolumes in fstab, but that's a different story.

There must be Python code for parsing fstab files

Anaconda has some functions for parsing fstab and we are working on more generic support in blivet (see storaged-project/blivet#1119)

but Anaconda probably also needs to look into udev to find the block device that holds a filesystem with a given UUID.

Blivet has this covered.

Can the "simple device" also be a device mapper device, like "/dev/dm-2"? Can Anaconda figure out from there that this is a LVM2 logical volume, for example?

Yes, we (blivet) can identify a device by name, uuid, all the /dev symlinks, basically everything.

In [2]: b.devicetree.resolve_device("/dev/dm-2") Out[2]: LVMLogicalVolumeDevice instance (0x7f53ec3957d0) -- name = fedora_aida-root status = True id = 78 ...

I think that would only help if UDisks2 would also watch and read that different location.

I guess we could do that, but that would be something too anaconda-specific and I am not sure if @tbzatek would be happy about it.

What issues do you see?

My main issue is with the existing entries in fstab and potential for Anaconda to mix these up with the "installer" entries created by Cockpit.

Does Anaconda already look into fstab, or are you referring to Anaconda getting confused during my proposed 'reverse engineering'? If the latter, I think this becomes moot now that Cockpit provides Anaconda with a separate mount-point -> device mapping.

I am also not sure how well Cockpit (or UDisks to be more precise) handles btrfs subvolumes in fstab, but that's a different story.

Once Cockpit supports btrfs, it will handle the subvolumes correctly, see #16408. But currently it is indeed pretty bad at it.

Can the "simple device" also be a device mapper device, like "/dev/dm-2"? Can Anaconda figure out from there that this is a LVM2 logical volume, for example?

Yes, we (blivet) can identify a device by name, uuid, all the /dev symlinks, basically everything.

Nice!

I think that would only help if UDisks2 would also watch and read that different location.

I guess we could do that, but that would be something too anaconda-specific and I am not sure if @tbzatek would be happy about it.

@tbzatek would not be so happy with this I'm afraid.

What issues do you see?

My main issue is with the existing entries in fstab and potential for Anaconda to mix these up with the "installer" entries created by Cockpit.

I have no idea how entries from multiple sources will be visualized (on the D-Bus interface). Besides, the whole RFE seems a little off the UDisks project target.

However, if something like this is about to be needed, we need to think in bigger picture. There are various files that we watch and/or modify. In some cases where these files are consumed by other libraries we pull in - a problem with different prefix in the build time sometimes causes unexpected issues.

Which brings me into another question - should this be considered with (not actually planned yet) mount namespace support? Would this be interesting for your use case?

Feel free to detach this conversation to a separate ticket.

[...] Besides, the whole RFE seems a little off the UDisks project target.

Our idea was that we don't want to change UDisks2 at all, or run it in a special way. All should be done in the top-most UI layers of Cockpit.

I still think this is achievable.

Cockpit would write to /etc/fstab (via the UDisks API) for its own bookkeeping benefits. Nobody else needs to pay any attention to it.

doc/anaconda.md

pkg/storaged/client.js

pkg/storaged/anaconda.jsx

pkg/storaged/pages/other.jsx

pkg/storaged/pages/drive.jsx

pkg/storaged/create-pages.jsx

test/verify/check-storage-stratis

pkg/storaged/client.js

martinpitt

Some remaining tiny nits, but from my POV this is basically ready to go. Then @KKoukiou can start playing around with it from the main-builds COPR.

test/verify/check-storage-anaconda

martinpitt

🚀 as this remains fully malleable for the time being.

mvollmer · 2023-12-13T10:16:46Z

Upps, I have found a bug: leaving the "Mount point" field empty results in "/" being used as the mount point. Instead it should keep the meaning of "no mount point specified" and either be an error, or lead to skipping of fstab updates.

mvollmer · 2023-12-13T11:26:43Z

Changes since last review:

client.add_mount_point_prefix = (dir) => {
    const mpp = client.anaconda?.mount_point_prefix;
-    if (mpp) {
+    if (mpp && dir != "") {
        if (dir == "/")
            dir = mpp;
        else
            dir = mpp + dir;
    }
    return dir;
};

And the bug fix in storage: Only action failures should disable the apply button

mvollmer · 2023-12-13T11:27:20Z

Pixels need to updated.

martinpitt

i386 unit test has some trouble, retried.

mvollmer · 2023-12-13T12:56:12Z

pkg/storaged/client.js

+        client.anaconda = null;
+    }
+
+    console.log("ANACONDA", client.anaconda);


Hmm, this runs unconditionally... it's better to remove it I guess.

cockpituous · 2024-01-02T12:37:03Z

pkg/storaged/block/format-dialog.jsx

@@ -242,6 +247,10 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended,
    if (old_opts == undefined)
        old_opts = initial_mount_options(client, block);

+    old_dir = client.strip_mount_point_prefix(old_dir);
+    if (old_dir === false)
+        return Promise.reject(_("This device can not be used for the installation target."));


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:03Z

pkg/storaged/client.js

+    } catch {
+        console.warn("Can't parse cockpit_anaconda configuration as JSON");
+        client.anaconda = null;


These 3 added lines are not executed by any test. Details

cockpituous · 2024-01-02T12:37:03Z

pkg/storaged/client.js

+
+    if (dir && mpp) {
+        if (dir.indexOf(mpp) != 0)
+            return false;


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/client.js

+    const mpp = client.anaconda?.mount_point_prefix;
+    if (mpp && dir != "") {
+        if (dir == "/")
+            dir = mpp;


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/client.js

+        if (dir == "/")
+            dir = mpp;
+        else
+            dir = mpp + dir;


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/dialog.jsx

+        "stratis-pool-member": _("member of Stratis pool"),
+        mounted: _("Filesystem outside the target"),


These 2 added lines are not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/dialog.jsx

+            if (use.usage == "mounted") {
+                location = client.strip_mount_point_prefix(location);
+                if (location === false)
+                    location = _("(Not part of target)");


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/filesystem/filesystem.jsx

+    if (mount_point) {
+        mp_text = client.strip_mount_point_prefix(mount_point);
+        if (mp_text == false)
+            return null;


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/filesystem/mounting-dialog.jsx

@@ -47,6 +47,10 @@ export function mounting_dialog(client, block, mode, forced_options) {
    const [old_config, old_dir, old_opts, old_parents] = get_fstab_config(block, true);
    const options = old_config ? old_opts : initial_tab_options(client, block, true);

+    const old_dir_for_display = client.strip_mount_point_prefix(old_dir);
+    if (old_dir_for_display === false)
+        return Promise.reject(_("This device can not be used for the installation target."));


This added line is not executed by any test. Details

cockpituous · 2024-01-02T12:37:04Z

pkg/storaged/stratis/pool.jsx

@@ -107,7 +110,7 @@ function create_fs(pool) {
                       }),
            SelectOne("at_boot", _("At boot"),
                      {
-                          value: "nofail",
+                          value: client.in_anaconda_mode() ? "local" : "nofail",


This added line is not executed by any test. Details

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Sep 19, 2023

mvollmer force-pushed the storage-mount-point-prefix branch 2 times, most recently from 3480ea7 to a1f76ce Compare September 21, 2023 10:28

mvollmer changed the title ~~storage: Anaconda mount point prefix~~ storage: Anaconda mode Sep 21, 2023

M4rtinK reviewed Sep 21, 2023

View reviewed changes

doc/anaconda.md Show resolved Hide resolved

mvollmer force-pushed the storage-mount-point-prefix branch 2 times, most recently from 004ffbb to 7592ccb Compare September 22, 2023 09:42

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

pkg/storaged/client.js Fixed Show fixed Hide fixed

pkg/storaged/client.js Fixed Show fixed Hide fixed

mvollmer force-pushed the storage-mount-point-prefix branch 3 times, most recently from 037df3d to ce99530 Compare September 22, 2023 12:25

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

pkg/storaged/anaconda.jsx Fixed Show fixed Hide fixed

pkg/storaged/anaconda.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the storage-mount-point-prefix branch from ce99530 to 714697f Compare September 22, 2023 12:30

mvollmer force-pushed the storage-mount-point-prefix branch from 714697f to 1608eb9 Compare October 5, 2023 12:50

mvollmer force-pushed the storage-mount-point-prefix branch from 1608eb9 to 0928a0c Compare October 19, 2023 10:53

mvollmer temporarily deployed to gh-cockpituous October 19, 2023 10:53 — with GitHub Actions Inactive

github-advanced-security bot found potential problems Oct 19, 2023

View reviewed changes

pkg/storaged/pages/other.jsx Fixed Show fixed Hide fixed

pkg/storaged/pages/drive.jsx Fixed Show fixed Hide fixed

pkg/storaged/create-pages.jsx Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Oct 19, 2023

View reviewed changes

test/verify/check-storage-stratis Fixed Show fixed Hide fixed

mvollmer force-pushed the storage-mount-point-prefix branch from 0928a0c to 7a8fcd0 Compare November 23, 2023 08:56

github-advanced-security bot found potential problems Nov 23, 2023

View reviewed changes

pkg/storaged/client.js Fixed Show fixed Hide fixed

mvollmer force-pushed the storage-mount-point-prefix branch 2 times, most recently from fc2eaea to a1208d9 Compare November 28, 2023 08:02

mvollmer force-pushed the storage-mount-point-prefix branch 5 times, most recently from e252280 to f55a870 Compare December 5, 2023 12:05

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Dec 5, 2023

mvollmer force-pushed the storage-mount-point-prefix branch from c4e15df to 9c04721 Compare December 12, 2023 08:12

mvollmer mentioned this pull request Dec 12, 2023

storage: "Create" menu shows group header without networked storage items below it #19735

Closed

mvollmer force-pushed the storage-mount-point-prefix branch from 9c04721 to 45201d0 Compare December 12, 2023 09:24

martinpitt requested changes Dec 12, 2023

View reviewed changes

test/verify/check-storage-anaconda Outdated Show resolved Hide resolved

test/verify/check-storage-anaconda Show resolved Hide resolved

test/verify/check-storage-anaconda Show resolved Hide resolved

mvollmer force-pushed the storage-mount-point-prefix branch from 45201d0 to f8bec94 Compare December 12, 2023 13:09

mvollmer requested a review from martinpitt December 13, 2023 07:13

mvollmer mentioned this pull request Dec 13, 2023

storage: Add new integration layer between anaconda and cockpit storage rhinstaller/anaconda-webui#72

Merged

9 tasks

martinpitt previously approved these changes Dec 13, 2023

View reviewed changes

mvollmer added needswork no-test For doc/workflow changes, or experiments which don't need a full CI run, and removed needswork labels Dec 13, 2023

mvollmer dismissed martinpitt’s stale review via b396123 December 13, 2023 11:22

mvollmer force-pushed the storage-mount-point-prefix branch from f8bec94 to b396123 Compare December 13, 2023 11:22

mvollmer force-pushed the storage-mount-point-prefix branch 2 times, most recently from 5f933e4 to 8eb1cc7 Compare December 13, 2023 11:53

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Dec 13, 2023

mvollmer force-pushed the storage-mount-point-prefix branch from 8eb1cc7 to cbae721 Compare December 13, 2023 11:54

mvollmer requested a review from martinpitt December 13, 2023 11:54

martinpitt approved these changes Dec 13, 2023

View reviewed changes

mvollmer commented Dec 13, 2023

View reviewed changes

mvollmer added 2 commits January 2, 2024 13:12

storage: Only action failures should disable the apply button

9ec9bfb

storage: Anaconda mode

0a02f8c

mvollmer force-pushed the storage-mount-point-prefix branch from cbae721 to 0a02f8c Compare January 2, 2024 11:12

cockpituous reviewed Jan 2, 2024

View reviewed changes

mvollmer added the unrelated-test-failure label Jan 2, 2024

mvollmer merged commit 5a3c197 into cockpit-project:main Jan 2, 2024
92 of 94 checks passed

martinpitt mentioned this pull request Jan 2, 2024

Allow samba-dcerpcd read public files fedora-selinux/selinux-policy#1989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Anaconda mode #19352

storage: Anaconda mode #19352

mvollmer commented Sep 19, 2023 •

edited

Loading

mvollmer commented Sep 20, 2023 •

edited

Loading

M4rtinK Sep 21, 2023

vojtechtrefny Sep 21, 2023

vojtechtrefny Sep 21, 2023 •

edited

Loading

mvollmer Sep 22, 2023

mvollmer Sep 22, 2023

mvollmer Sep 22, 2023

vojtechtrefny Sep 23, 2023

mvollmer Sep 25, 2023

tbzatek Sep 25, 2023

mvollmer Sep 26, 2023

martinpitt left a comment

martinpitt left a comment

mvollmer commented Dec 13, 2023

mvollmer commented Dec 13, 2023

mvollmer commented Dec 13, 2023

martinpitt left a comment

mvollmer Dec 13, 2023

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

cockpituous Jan 2, 2024

		"stratis-pool-member": _("member of Stratis pool"),
		mounted: _("Filesystem outside the target"),

storage: Anaconda mode #19352

storage: Anaconda mode #19352

Conversation

mvollmer commented Sep 19, 2023 • edited Loading

mvollmer commented Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechtrefny Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Dec 13, 2023

mvollmer commented Dec 13, 2023

mvollmer commented Dec 13, 2023

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Sep 19, 2023 •

edited

Loading

mvollmer commented Sep 20, 2023 •

edited

Loading

vojtechtrefny Sep 21, 2023 •

edited

Loading