Skip to content
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

storaged: support creating btrfs snapshots #20562

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jelly
Copy link
Member

@jelly jelly commented Jun 7, 2024

Btrfs supports creating a snapshot of a subvolume, either readonly or write-able. Unlike creating subvolumes, it doesn't make sense to put a snapshot of a subvolume in the subvolume itself users likely have a special snapshots subvolume where they collect their snapshots.

Open questions:

  • Unlike subvolumes we don't want to create a snapshot in the parent directory, I have given the UI an option to specify a full path, this is good and bad. Good because it is flexible, bad because it would allow you to specify a path to a NFS mount, xfs partition or something else weird. Then again it isn't too bad, note that snapshots can be created under a subvolume or normal directory in btrfs. (Users usually have a snapshot subvolume directory where they put them either /snapshots or /.snapshots`, this is not a well-known directory)
  • Do we offer mount options? You can obviously mount them like subvolumes as they are one, so we can offer that flexibility

Follow up / issues:

  • We don't identify snapshots, we just show it as subvolume
  • We don't and likely can't identify read only snapshots, this requires major udisks work.

Support creating btrfs snapshots

Screenshot from 2024-06-07 11-36-05

Snapshot creation dialog:

image

@jelly jelly added no-test For doc/workflow changes, or experiments which don't need a full CI run, release-note labels Jun 7, 2024
@jelly jelly requested review from garrett and mvollmer June 7, 2024 09:44
@garrett
Copy link
Member

garrett commented Jun 11, 2024

When you're creating snapshots, you do not want to mount them, or set up auto mounting? I don't understand why mounting is half the dialog.

Snapshots of subvolumes are intended as "cheap" "backups". I don't even remember mounting or unmounting them when using them before in the past. They also cannot cross into other subvolumes. I don't think they're even treated like subvolumes in a traditional manner.

Isn't btrfs snapshotting basically the following:

  • the volume you're wanting to snapshot
  • the location within that same subvolume that you're snapshotting
  • if it's a read-only or read-write snapshot
  • optionally, quota group(s)

Nothing related to mounting or even unmounting.

(Whenever I've made a snapshot, I'm able to go into that snapshot immediately without having to do anything special.)

If someone really wants to mount a snapshot instead of going into it, they can do that with the standard mount dialog afterward, right?

@garrett
Copy link
Member

garrett commented Jun 11, 2024

So I was thinking we can make snapshotting more like a snapshotting task and less like "random generic subvolume". This would include some nice stuff where it makes snapshotting super easy without "crap work" that someone would have to do each time, and instead have the computer remember and figure out that stuff for them, with good and opinionated defaults.

Here's what that could look like:

create snapshot dialog

The text in the side says:

  • Save the snapshot location for each subvolume in local storage, so when someone comes back to create a new snapshot, it'll be the same as before
  • Remember the last selection for snapshot name too
  • Selecting custom shows a text entry

@Conan-Kudo
Copy link

If you're trying to snapshot the parent subvolumes (e.g. root and home in Fedora's default setup), you will want to consider putting those snapshots in the parent volume that's not normally mounted (that is, the actual base btrfs volume).

Put more concretely, in Fedora's setup, we have the following setup:

Unmounted base volume
|
|- root
|- home

You could do something like this:

Unmounted base volume
|
|- root
|- home
|- .snapshots
|-|
|-|- root-20240615

This would allow you to have the snapshots not visible most of time, and if you need it for recovery or boot-to-snapshot purposes, it's easily available.

@jelly
Copy link
Member Author

jelly commented Jun 17, 2024

If you're trying to snapshot the parent subvolumes (e.g. root and home in Fedora's default setup), you will want to consider putting those snapshots in the parent volume that's not normally mounted (that is, the actual base btrfs volume).

Put more concretely, in Fedora's setup, we have the following setup:

Unmounted base volume
|
|- root
|- home

You could do something like this:

Unmounted base volume
|
|- root
|- home
|- .snapshots
|-|
|-|- root-20240615

This would allow you to have the snapshots not visible most of time, and if you need it for recovery or boot-to-snapshot purposes, it's easily available.

The Cockpit storage page (currently) shows all subvolumes not based on what is mounted (that is an interesting idea though). There is a draft PR to hide snapshots from the overview page and list them under the subvolume you made a snapshot of. #20234

@mvollmer
Copy link
Member

mvollmer commented Jul 16, 2024

I have given the UI an option to specify a full path, this is good and bad.

Is this a path in the filesystem? I think it should be the name of the parent subvolume.

We could have a dropdown for the parent subvol, and then the radio for the name of the new snapshot.

@jelly
Copy link
Member Author

jelly commented Jul 16, 2024

  • Snapshot location is remembered per subvolume
  • Directory Autocomplete (filter files), with an option to create a new subvolume to put snapshots. (a helper text should show that)
  • Validate if specify a location which is not on the same btrfs volume
  • Allow force overriding an existing snapshot, for example only selection the date

Open question is, do we add an option for subvolumename + date?

@jelly jelly force-pushed the create-btrfs-snapshot branch from 2b1674a to 517ef66 Compare July 18, 2024 14:54
jelly added 6 commits July 25, 2024 14:09
Btrfs supports creating a snapshot of a subvolume, either readonly or
write-able. Unlike creating subvolumes, it doesn't make sense to put a
snapshot of a subvolume in the subvolume itself users likely have a
special `snapshots` subvolume where they collect their snapshots.
@jelly jelly force-pushed the create-btrfs-snapshot branch from 517ef66 to 0babff9 Compare July 26, 2024 13:36
TextInput("subvolume", _("Subvolume"),
{
value: subvol.pathname,
disabled: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is confusing, imo. Usually we put the subject of a dialog into the title, like "Permanently delete subvolume foo/bar?" so I would expect "Create snapshot of foo/bar" as the title instead of this disabled text field.

My immediate thought when seeing it in the dialog was "why is it disabled? what can I do to enable it?" and there are no good answers to those questions, I guess.

The mockups have a static text instead of a disabled textinput, which avoids those questions. I think that's much better.

I like this new pattern of putting the subject of a dialog into the list of input fields, as a static text. However, it's still a new pattern and it feels random to introduce it here, and make this dialog different from all others in this regard.

];

const get_local_storage_snapshots_locs = () => {
const localstorage_snapshot_data = localStorage.getItem(localstorage_key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the location of the most recently made snapshot? By looking at the list of snapshots of this subvolume. That makes this independent from browser storage.

placeholder: cockpit.format(_("Example, $0"), "/.snapshots"),
explanation: (<>
<p>{_("Snapshots must reside within the same btrfs volume.")}</p>
<p>{_("When the snapshot location does not exist, it will be created as btrfs subvolume automatically.")}</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why not as a directory? Also, non existing parents should be created as well, no?

value: subvol.pathname,
disabled: true,
}),
TextInput("snapshots_location", _("Snapshots location"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a autocomplete thing, offering the mount points of all mounted subvolumes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run, release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants