-
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
Btrfs subvolume creation #19849
Btrfs subvolume creation #19849
Conversation
8de1263
to
8e2c223
Compare
8e2c223
to
2dfecb5
Compare
2dfecb5
to
4979304
Compare
d9be999
to
0fd0594
Compare
0fd0594
to
b2bc543
Compare
pkg/storaged/btrfs/subvolume.jsx
Outdated
if (!mounted && subvol.id == 5) | ||
create_excuse = _("Subvolume needs to be mounted"); | ||
if (mounted && opt_ro) | ||
create_excuse = _("Subvolume needs to be mounted rw"); |
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.
Let's not say "rw" but "Subvolume needs to be writable".
Also, I think creation is possible if one of the parents is mounted rw, and get_mount_point_in_parent takes that into account.
So what about this:
if (!mount_point_in_parent) {
if (!mounted)
create_excuse = _("Subvolume needs to be mounted");
if (!mounted && subvol.id == 5)
create_excuse = _("Subvolume needs to be mounted");
if (mounted && opt_ro)
create_excuse = _("Subvolume needs to be mounted writable");
}
Thus, we only talk to the user in terms of the current subvolume and never about parents.
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.
Hmm I don't think this is true. mount_point_in_parent
returns the mount_point for the current parent and doesn't become true. It breaks the current test_case for mounted && opt_ro
.
/dev/sda on /run/ro type btrfs (ro,relatime,seclabel,ssd,space_cache=v2,subvolid=264,subvol=/ro)
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.
It breaks the current test_case
There is a test case where we expect that creation inside the subvolume "ro" (mounted at "/run/ro") should be disabled, but with my proposed changes, it is enabled. Right?
This happens because the root subvol is still mounted read-write at "/run/butter" and creation of subvolumes of "ro" can be done inside the mount point "/run/butter/ro".
(I think, I'll check this 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.
See my "PROPOSAL" commit.
if (name.length > 255) | ||
return _("Name cannot be longer than 255 characters."); | ||
if (name.includes('/')) | ||
return cockpit.format(_("Name cannot contain the character '/'.")); |
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.
What about subvolumes in directories? Should we allow those to be created in Cockpit?
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.
Well, we don't offer listing directories so how would a user be able to create such?
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.
By giving a name like "dir/dir/subvol" when creating a new one, maybe. Cockpit would do the necessary "mkdir -p".
But I agree that this might be confusing.
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.
Yeah, we could but I honestly found this a bit confusing:
btrfs subvolume create -p foo/bar/ding
ID 264 gen 145 parent 256 top level 256 path root/root/foo/bar/ding
So foo/bar
are just normal directories. Not super obvious if you ask me.
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.
Oh, btrfs itself has a -p option, neat! But, yeah, no strong opinions here.
68fa567
to
8d2067d
Compare
"Subvolume needs to be mounted") | ||
b.click(self.dropdown_toggle(root_sel)) | ||
|
||
self.click_dropdown(self.card_row("Storage", name=label) + " + tr", "Mount") |
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.
There is self.click_card_dropdown
which probably works here and is cleaner.
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.
See the PROPOSAL commit.
Allow a user to create a new subvolume if the parent subvolume is mounted or any of it's parent subvolumes is mounted. We don't use btrfs's CreateSubvolume as it uses the first mount point from the block device, so with multiple subvol's mounted it is not possible to predict / influence on what mount point it will be created.
7e8c062
to
3021c69
Compare
if (!mount_now || vals.at_boot == "never") { | ||
mount_options.push("noauto"); |
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 2 added lines are not executed by any test. Details
if (vals.mount_options.ro) | ||
mount_options.push("ro"); | ||
if (vals.at_boot == "never") | ||
mount_options.push("x-cockpit-never-auto"); |
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 (vals.at_boot == "never") | ||
mount_options.push("x-cockpit-never-auto"); | ||
if (vals.at_boot == "nofail") | ||
mount_options.push("nofail"); |
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 (vals.at_boot == "nofail") | ||
mount_options.push("nofail"); | ||
if (vals.at_boot == "netdev") | ||
mount_options.push("_netdev"); |
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 (vals.at_boot == "netdev") | ||
mount_options.push("_netdev"); | ||
|
||
const name = (subvol.pathname == "/" ? vals.name : subvol.pathname + "/" + vals.name); |
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
const name = (subvol.pathname == "/" ? vals.name : subvol.pathname + "/" + vals.name); | ||
mount_options.push("subvol=" + name); | ||
if (vals.mount_options.extra) | ||
mount_options.push(vals.mount_options.extra); |
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
|
||
let mount_point = vals.mount_point; | ||
if (mount_point[0] != "/") | ||
mount_point = "/" + mount_point; |
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
{ | ||
dir: { t: 'ay', v: encode_filename(mount_point) }, | ||
type: { t: 'ay', v: encode_filename("btrfs") }, | ||
opts: { t: 'ay', v: encode_filename(mount_options.join(",") || "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
if (mount_now) { | ||
return client.mount_at(block, mount_point); | ||
} else | ||
return Promise.resolve(); |
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
] | ||
}), | ||
mount_options(false, false), | ||
at_boot_input(client.in_anaconda_mode() ? "local" : "nofail"), |
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
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.
Thanks!
Support creating and optionally mounting subvolumes. Subvolumes can only be created when any parent in the tree is mounted.