-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: Allow creation of swap and maintain fstab entries #19843
Conversation
fa2fa91
to
1143d40
Compare
@marusak For excellent swap support also in the normal Cockpit mode, we would need to write entries for swap devices into fstab, maintain their "noauto" option depending on whether the device is active, do some "misswapping" warnings, and remove the entry during teardown. That's entirely possible, but might not be worth the effort. Opinions? |
1143d40
to
f90ef17
Compare
In my own opinion I would like this to be fully supported in Cockpit (even if it is dying tech most distro's nowadays only use zram/zswap). Keeping the amount of custom Anaconda features low would a good thing to strive for in my opinion. |
Yes, that's the right thing to do(tm), let's do it. |
a81d732
to
3ab49c7
Compare
3ab49c7
to
80fc371
Compare
db745eb
to
6aab047
Compare
Some issues I found:
|
Oh, you mean there is a JavaScript exception? Good catch! Fixed! |
That's not possible, stratis only has filesystems, no? |
6aab047
to
70e1404
Compare
Ah ok, then nvm! |
Yup, blocks_ptable doesn't exist for a plain /dev/sda. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one comment about a potential missing test.
b4f40dc
to
ffd6b83
Compare
Just realized this is release note worthy so marked it as such. |
test/verify/check-storage-swap
Outdated
m.execute(f"mkswap -f {disk}") | ||
b.click(self.card_button("Swap", "Start")) | ||
b.wait_text(self.card_desc("Swap", "Used"), "0") | ||
self.assertIn("defaults", m.execute(f"findmnt --fstab -n -o OPTIONS {disk}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check seems to be a bit racy fails on both Arch and debian-testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, clicking "Start" will first activate the Swap and then add the fstab entry. So it is feasible that the "Used" number appears before the fstab entry. I changed this into a testlib.wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately one part of the test seems to be flaky.
ffd6b83
to
e6fc1c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, re-triggered rhel-8-10-distropkg/storage hopefully it was just a busy test vm.
@@ -423,6 +429,8 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, | |||
else if (trigger == "type") { | |||
if (dlg.get_value("type") == "empty") { | |||
dlg.update_actions({ Variants: action_variants_for_empty }); | |||
} else if (dlg.get_value("type") == "swap") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
@@ -450,6 +458,12 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, | |||
partition_type = "21686148-6449-6e6f-744e-656564454649"; | |||
} | |||
|
|||
if (type == "swap") { | |||
partition_type = (block_ptable && block_ptable.Type == "dos" | |||
? "0x82" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
if (keep_keys) { | ||
const content_block = client.blocks_cleartext[path]; | ||
return client.blocks_swap[content_block.path]; | ||
} else if (is_encrypted(vals)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 added lines are not executed by any test. Details
return client.blocks_swap[content_block.path]; | ||
} else if (is_encrypted(vals)) | ||
return (client.blocks_cleartext[path] && | ||
client.blocks_swap[client.blocks_cleartext[path].path]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
if (type == "swap" && mount_now) | ||
return (client.wait_for(() => block_swap_for_block(path)) | ||
.then(block_swap => block_swap.Start({}))); | ||
if (is_encrypted(vals) && (is_filesystem(vals) || type == "swap") && !mount_now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
@@ -284,6 +284,12 @@ export function get_resize_info(client, block, to_fit) { | |||
grow_needs_unmount: false | |||
}; | |||
shrink_excuse = _("VDO backing devices can not be made smaller"); | |||
} else if (client.blocks_swap[block.path]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
["fstab", { | ||
dir: { t: 'ay', v: encode_filename("none") }, | ||
type: { t: 'ay', v: encode_filename("swap") }, | ||
opts: { t: 'ay', v: encode_filename(noauto ? "noauto" : "defaults") }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
@@ -910,6 +911,15 @@ export function get_active_usage(client, path, top_action, child_action, is_temp | |||
enter_unmount(children[c], c, false); | |||
enter_unmount(block, mp, true); | |||
}); | |||
} else if (swap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
Storage: improved support for swap
Cockpit can now format block devices as swap and will maintain a fstab entry for them.