storage: Grow Stratis blockdevs #19079

mvollmer · 2023-07-06T12:49:57Z

Storage: Support for growing block devices of a Stratis pool

Cockpit can now grow logical volumes that are used as block devices in a Stratis pool. Also, if a Stratis block device grows for any reason, Cockpit will notify you about this and can extend the pool to use all of it.

martinpitt · 2023-08-23T03:03:25Z

Tests still need work. The coverage complaints above also point out some test gaps. Moving to draft for now to clean up the PR review list. Thanks!

mvollmer · 2023-08-23T09:13:29Z

Tests still need work. The coverage complaints above also point out some test gaps.

What do you have in mind concretely?

mvollmer · 2023-08-23T09:25:00Z

pkg/storaged/lvol-tabs.jsx

@@ -110,6 +114,14 @@ function lvol_and_fsys_resize(client, lvol, size, offline, passphrase) {
                return Promise.reject(_("VDO backing devices can not be made smaller"));
            else
                return Promise.resolve();
+        } else if (stratis_bdev) {


Coverage of if statements is weird. The lines after the if (the consequent) are executed, so I don't know how the if itself is not executed. We might want filter them all out, like we do with else lines.

mvollmer · 2023-08-23T09:25:46Z

pkg/storaged/lvol-tabs.jsx

+            } else if (size - crypto_overhead < Number(stratis_bdev.TotalPhysicalSize))
+                return Promise.reject(_("Stratis blockdevs can not be made smaller"));


This is a safe-guard and is not normally triggerable from the UI.

mvollmer · 2023-08-23T09:26:54Z

pkg/storaged/stratis-details.jsx

+        return;
+
+    for (const p in client.stratis_pools) {
+        const blockdevs = client.stratis_pool_blockdevs[p] || [];


The safety fallback || [] is not executed, but that doesn't normally trigger.

mvollmer · 2023-08-23T09:27:03Z

pkg/storaged/stratis-details.jsx

@@ -185,9 +197,28 @@ export const StratisPoolDetails = ({ client, pool }) => {
                      pool.ClevisInfo[0] && // pool has consistent clevis config
                      (!pool.ClevisInfo[1][0] || pool.ClevisInfo[1][1][0] == "tang")); // not bound or bound to "tang"
    const tang_url = can_tang && pool.ClevisInfo[1][0] ? JSON.parse(pool.ClevisInfo[1][1][1]).url : null;
+    const blockdevs = client.stratis_pool_blockdevs[pool.path] || [];


safety fallback

mvollmer · 2023-08-23T09:27:27Z

pkg/storaged/lvol-tabs.jsx

@@ -185,6 +197,13 @@ function get_resize_info(client, block, to_fit) {
                    grow_excuse = cockpit.format(_("$0 filesystems can not be made larger."),
                                                 block.IdType);
            }
+        } else if (client.blocks_stratis_blockdev[block.path] && client.features.stratis_grow_blockdevs) {


if wonkiness

mvollmer · 2023-08-23T09:29:17Z

pkg/storaged/lvol-tabs.jsx

@@ -331,6 +350,10 @@ function lvol_shrink(client, lvol, info, to_fit) {
        if (vdo)
            shrink_size = vdo.physical_size + crypto_overhead;

+        const stratis_bdev = client.blocks_stratis_blockdev[content_path];
+        if (stratis_bdev)
+            shrink_size = Number(stratis_bdev.TotalPhysicalSize) + crypto_overhead;


We could try to get this.

martinpitt · 2023-08-23T10:51:50Z

pkg/storaged/stratis-details.jsx

@@ -185,10 +197,29 @@ export const StratisPoolDetails = ({ client, pool }) => {
                      pool.ClevisInfo[0] && // pool has consistent clevis config
                      (!pool.ClevisInfo[1][0] || pool.ClevisInfo[1][1][0] == "tang")); // not bound or bound to "tang"
    const tang_url = can_tang && pool.ClevisInfo[1][0] ? JSON.parse(pool.ClevisInfo[1][1][1]).url : null;
+    const blockdevs = client.stratis_pool_blockdevs[pool.path] || [];


For such belt-and-suspenders code paths you could add a // not-covered: OS error or something similar?

mvollmer · 2023-08-23T12:45:43Z

For such belt-and-suspenders code paths you could add a // not-covered: OS error or something similar?

TBH, I'd rather ignore the complaints here than annotate the code... The machinery will shut up about this until we change the line again, and I'd say we actually want it to shout at us when we change the line, instead remaining silent because of a annotation that might be incorrect now. shrug

martinpitt

Thanks! I'm almost happy with this. Nice demo video, too!

pkg/storaged/lvol-tabs.jsx

martinpitt · 2023-08-25T06:44:05Z

pkg/storaged/lvol-tabs.jsx

@@ -185,6 +197,13 @@ function get_resize_info(client, block, to_fit) {
                    grow_excuse = cockpit.format(_("$0 filesystems can not be made larger."),
                                                 block.IdType);
            }
+        } else if (client.blocks_stratis_blockdev[block.path] && client.features.stratis_grow_blockdevs) {


I do understand this one -- on Fedora 38, the stratis_grow_blockdevs flag is always True, so it complains about not exercising the "false" path?

martinpitt · 2023-08-25T06:47:09Z

pkg/storaged/stratis-details.jsx

+    for (const p in client.stratis_pools) {
+        const blockdevs = client.stratis_pool_blockdevs[p] || [];
+        if (blockdevs.some(bd => bd.NewPhysicalSize[0] && Number(bd.NewPhysicalSize[1]) > Number(bd.TotalPhysicalSize)))
+            enter_warning(p, { warning: "unused-blockdevs" });


What's that magic string? It's not in main, and it doesn't appear anywhere else in this patch. Do you perhaps mean "unused-space"?

The exact string doesn't matter, this is just to get a warning triangle next to the pool in the "Devices" panel on the Overview.

test/verify/check-storage-stratis

martinpitt · 2023-08-25T08:46:12Z

oh uh, new bug found?

same error on rhel 9, so not just a glitch.

martinpitt · 2023-08-25T11:35:09Z

unrelated flake, probably due to the sync package loading that @allisonkarlitskaya and I want to address ASAP.

martinpitt

Thanks! I'll let you land this, especially wrt. coordination with #19238 . I suggest to land that other one first, unless you are sure they won't conflict.

Just like we already did for filesystems and VDO.

The dialogs do this already, so let the buttons and menu items do it as well. This helps with naughty patterns.

cockpituous · 2023-08-25T14:41:00Z

pkg/storaged/lvol-tabs.jsx

@@ -110,6 +114,15 @@ function lvol_and_fsys_resize(client, lvol, size, offline, passphrase) {
                return Promise.reject(_("VDO backing devices can not be made smaller"));
            else
                return Promise.resolve();
+        } else if (stratis_bdev) {


This added line is not executed by any test. Details

cockpituous · 2023-08-25T14:41:01Z

pkg/storaged/lvol-tabs.jsx

+                const pool = client.stratis_pools[stratis_bdev.Pool];
+                return pool.GrowPhysicalDevice(stratis_bdev.Uuid).then(std_reply);
+            } else if (delta < 0)
+                return Promise.reject(_("Stratis blockdevs can not be made smaller"));


This added line is not executed by any test. Details

cockpituous · 2023-08-25T14:41:01Z

pkg/storaged/lvol-tabs.jsx

@@ -185,6 +198,13 @@
                    grow_excuse = cockpit.format(_("$0 filesystems can not be made larger."),
                                                 block.IdType);
            }
+        } else if (client.blocks_stratis_blockdev[block.path] && client.features.stratis_grow_blockdevs) {


This added line is not executed by any test. Details

cockpituous · 2023-08-25T14:41:01Z

pkg/storaged/stratis-details.jsx

+        return;
+
+    for (const p in client.stratis_pools) {
+        const blockdevs = client.stratis_pool_blockdevs[p] || [];


This added line is not executed by any test. Details

cockpituous · 2023-08-25T14:41:01Z

pkg/storaged/stratis-details.jsx

@@ -621,9 +633,28 @@
                      pool.ClevisInfo[0] && // pool has consistent clevis config
                      (!pool.ClevisInfo[1][0] || pool.ClevisInfo[1][1][0] == "tang")); // not bound or bound to "tang"
    const tang_url = can_tang && pool.ClevisInfo[1][0] ? JSON.parse(pool.ClevisInfo[1][1][1]).url : null;
+    const blockdevs = client.stratis_pool_blockdevs[pool.path] || [];


This added line is not executed by any test. Details

mvollmer mentioned this pull request Jul 6, 2023

pool.GrowPhysicalDevice doesn't trigger change notification for blockdev.TotalPhysicalSize stratis-storage/stratisd#3371

Closed

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from a02880a to 4d66d1a Compare July 6, 2023 13:35

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

mvollmer force-pushed the storage-stratis-grow-blockdevs branch 2 times, most recently from 1232dad to 5b4a3bf Compare July 7, 2023 12:07

mvollmer added the release-note label Aug 9, 2023

mvollmer force-pushed the storage-stratis-grow-blockdevs branch 2 times, most recently from a958246 to ace1a34 Compare August 11, 2023 11:59

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

mvollmer marked this pull request as ready for review August 11, 2023 11:59

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from ace1a34 to 0b7a383 Compare August 11, 2023 12:08

martinpitt marked this pull request as draft August 23, 2023 03:03

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from 0b7a383 to a429377 Compare August 23, 2023 09:05

mvollmer commented Aug 23, 2023

View reviewed changes

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from a429377 to b1db542 Compare August 23, 2023 10:37

martinpitt reviewed Aug 23, 2023

View reviewed changes

mvollmer mentioned this pull request Aug 23, 2023

naughty: Add pattern for #5090 on rhel-9-3 cockpit-project/bots#5139

Merged

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from b1db542 to f05215f Compare August 23, 2023 12:24

mvollmer added the blocked Don't land until something else happens first (see task list) label Aug 23, 2023

mvollmer mentioned this pull request Aug 24, 2023

Refresh Arch for Stratis 3.5.8 cockpit-project/bots#5144

Merged

1 task

mvollmer removed the blocked Don't land until something else happens first (see task list) label Aug 24, 2023

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from f05215f to b242f4c Compare August 24, 2023 11:40

mvollmer marked this pull request as ready for review August 24, 2023 11:40

mvollmer requested a review from martinpitt August 24, 2023 17:09

martinpitt requested changes Aug 25, 2023

View reviewed changes

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from b242f4c to fda8c7c Compare August 25, 2023 08:20

mvollmer requested a review from martinpitt August 25, 2023 08:20

mvollmer mentioned this pull request Aug 25, 2023

naughty: Make patterns for #5090 less specific cockpit-project/bots#5155

Merged

martinpitt previously approved these changes Aug 25, 2023

View reviewed changes

mvollmer added 4 commits August 25, 2023 16:27

storage: Don't swallow resize errors

c3da802

storage: Resize Stratis blockdevs together with logical volumes

06cd6cf

Just like we already did for filesystems and VDO.

storage: Detect unused space in Stratis pools in general

b8e978c

storage: Write more error messages to the Browser console

2df3b91

The dialogs do this already, so let the buttons and menu items do it as well. This helps with naughty patterns.

mvollmer dismissed martinpitt’s stale review via 2df3b91 August 25, 2023 13:29

mvollmer force-pushed the storage-stratis-grow-blockdevs branch from fda8c7c to 2df3b91 Compare August 25, 2023 13:29

martinpitt approved these changes Aug 25, 2023

View reviewed changes

cockpituous reviewed Aug 25, 2023

View reviewed changes

mvollmer merged commit 93f6974 into cockpit-project:main Aug 25, 2023
92 checks passed

mvollmer mentioned this pull request Sep 1, 2023

Stratis feature for cockpit GUI to increase stratis pool #17804

Closed

martinpitt added release-note and removed release-note labels Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Grow Stratis blockdevs #19079

storage: Grow Stratis blockdevs #19079

mvollmer commented Jul 6, 2023 •

edited

Loading

martinpitt commented Aug 23, 2023

mvollmer commented Aug 23, 2023

mvollmer Aug 23, 2023

mvollmer Aug 23, 2023

mvollmer Aug 23, 2023

mvollmer Aug 23, 2023

mvollmer Aug 23, 2023

mvollmer Aug 23, 2023

martinpitt Aug 23, 2023

mvollmer commented Aug 23, 2023

martinpitt left a comment

martinpitt Aug 25, 2023

martinpitt Aug 25, 2023

mvollmer Aug 25, 2023

martinpitt commented Aug 25, 2023 •

edited

Loading

martinpitt commented Aug 25, 2023

martinpitt left a comment

cockpituous Aug 25, 2023

cockpituous Aug 25, 2023

cockpituous Aug 25, 2023

cockpituous Aug 25, 2023

cockpituous Aug 25, 2023

		} else if (size - crypto_overhead < Number(stratis_bdev.TotalPhysicalSize))
		return Promise.reject(_("Stratis blockdevs can not be made smaller"));

storage: Grow Stratis blockdevs #19079

storage: Grow Stratis blockdevs #19079

Conversation

mvollmer commented Jul 6, 2023 • edited Loading

Storage: Support for growing block devices of a Stratis pool

martinpitt commented Aug 23, 2023

mvollmer commented Aug 23, 2023

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 Aug 23, 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

martinpitt commented Aug 25, 2023 • edited Loading

martinpitt commented Aug 25, 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

mvollmer commented Jul 6, 2023 •

edited

Loading

martinpitt commented Aug 25, 2023 •

edited

Loading