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

Btrfs snapshot support #20234

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jelly
Copy link
Member

@jelly jelly commented Mar 28, 2024

Show snapshots of a subvolume in the detail view.

image

@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Mar 28, 2024
@jelly jelly mentioned this pull request Apr 3, 2024
@jelly
Copy link
Member Author

jelly commented Apr 3, 2024

@mvollmer I thought a bit on how we could implement filtering:

We could come up with a system where we don't have to hardcode the "filters" in but we could register filters for pages which want that.

register_filter(display_name, property_name)

This registers a filter which lives in pages.jsx, then we use these filter with property_name and filter_properties.

Alternatively, we just hardcode filters:

PAGE_FILTER_SNAPSHOT = 'snapshot'

Page/Card can have filter_properties which contains the key we are interested for with the value. State can come later.

I'm not thinking of introducing something like a filter component. But maybe it is interesting to have something usable.

@mvollmer
Copy link
Member

mvollmer commented Apr 4, 2024

@mvollmer I thought a bit on how we could implement filtering:

I think this is a bit too much machinery. Let's just do what we need, and nothing more.

@jelly
Copy link
Member Author

jelly commented Apr 4, 2024

Alright, after the discussion today in standup:

  • First pass, show snapshots under a subvolume
  • Second pass, hide snapshots

@jelly
Copy link
Member Author

jelly commented May 23, 2024

@garrett question from @mvollmer did we decide on the design yet?

So my idea is:

  • Hide snapshots on the overview
  • List snapshots of a subvolume in the subvolume detail page.

@garrett
Copy link
Member

garrett commented May 23, 2024

@jelly: Yes, I think that's what we want. Good, simple summary.

I think we (you and/or @mvollmer and I) agreed on that a while ago somewhere? Perhaps in a video call? Perhaps in an issue? I don't remember where, but if we all agree on that, then that's great.

To restart: I definitely do not want them on the overview (and never have) and I think the details pages are the right places for the snapshots.

@mvollmer mvollmer self-requested a review May 31, 2024 07:37
@mvollmer
Copy link
Member

mvollmer commented May 31, 2024

So the listing of snapshots on the page of their origin in a crossref table works. Let's nail down when to hide snapshots in the regular child tables.

What about this: snapshots are only included in a child table when they are on the top-level. Otherwise they are replaced with a row that summarizes them "(20 snapshots)". Clicking on that row will go to the parent of those snapshots.

Here is a made-up example (without that rule applied):

image

Here, all the home-123 and images-123 subvolumes are snapshots. They would be omitted from the table and replaced with "(13 snapshots)".

But when going to the "snapshots" subvolume, they would be listed like they are now, because they are on the top-level in that table:

image

And of course they would also be listed on the page of their origin:

image

In addition the page for a snapshot subvolume should have a link back to its origin:

image

Heere the could be a description row with "Snapshot origin data/home".

@jelly
Copy link
Member Author

jelly commented May 31, 2024

So the listing of snapshots on the page of their origin in a crossref table works. Let's nail down when to hide snapshots in the regular child tables.

Good point.... I forgot about that...

So on the overview they need to go away, that is a requirement from @garrett

What about this: snapshots are only included in a child table when they are on the top-level. Otherwise they are replaced with a row that summarizes them "(20 snapshots)". Clicking on that row will go to the parent of those snapshots.

What view is this? The filesystem detail page? Can we differentiate the overview and filesystem detail page easily?

Here is a made-up example (without that rule applied):

image

Here, all the home-123 and images-123 subvolumes are snapshots. They would be omitted from the table and replaced with "(13 snapshots)".

Hmm feels a bit magical to me.

But when going to the "snapshots" subvolume, they would be listed like they are now, because they are on the top-level in that table:

image

But then you wouldn't know it is a special subvolume would you? Better asked, how would we implement this:

for subvolume in parent_dir.subvolumes:
       if subvolume.parent__uid == parent_dir.uuid
            show()
      else
           hide()

The problem is a bit that BTRFS is very lenient and you can make everything everywhere without having to give it logical names.

And of course they would also be listed on the page of their origin:

image

Agreed.

In addition the page for a snapshot subvolume should have a link back to its origin:

image

Heere the could be a description row with "Snapshot origin data/home".

Stretch goal, but seems nice!

@mvollmer
Copy link
Member

mvollmer commented May 31, 2024

Better asked, how would we implement this:

for subvolume in parent_dir.subvolumes:
       if subvolume.parent__uid == parent_dir.uuid
            show()
      else
           hide()

So parent_uuid is the origin of the snaphot subvolume, and parent_dir is the parent subvolume, right? In my example, "snapshots" is the parent of "home-1234", and "home" is the origin of "home-1234" (since it was created by "btrfs subvolume snapshot /run/butter/data/home /run/butter/data/snapshots/home-1234").

So your code above would not hide any snapshots in my example, since no snapshot subvolume has the subvolume as both parent and origin. It would only hide snapshots that are directly inside their origins, like the one created by "btrfs subvolume snapshot /run/butter/data/home /run/butter/data/home/snapshot-1".

In my understanding, we want to get rid of snapshots in our tables because there are too many of them and they are usually not interesting. Isn't that the goal?

@mvollmer
Copy link
Member

What about this: snapshots are only included in a child table when they are on the top-level. Otherwise they are replaced with a row that summarizes them "(20 snapshots)". Clicking on that row will go to the parent of those snapshots.

What view is this? The filesystem detail page? Can we differentiate the overview and filesystem detail page easily?

That rule would apply to any ChildrenTable, thus to each page, overview and details.

We can distinguish overview and details pages and we do that already: only the overview has icons, for example), But I propose we don't do that when deciding whether to hide snapshots. If they are distracting on the overview, they are also distracting elsewhere, no? And with the summary line, you can easily find the place that lists them all.

@mvollmer
Copy link
Member

Should we have actions on snapshots when they are listed in the crossrefs table? At least "Delete" seems appropriate.

@jelly
Copy link
Member Author

jelly commented May 31, 2024

Better asked, how would we implement this:

for subvolume in parent_dir.subvolumes:
       if subvolume.parent__uid == parent_dir.uuid
            show()
      else
           hide()

So parent_uuid is the origin of the snaphot subvolume, and parent_dir is the parent subvolume, right? In my example, "snapshots" is the parent of "home-1234", and "home" is the origin of "home-1234" (since it was created by "btrfs subvolume snapshot /run/butter/data/home /run/butter/data/snapshots/home-1234").

So your code above would not hide any snapshots in my example, since no snapshot subvolume has the subvolume as both parent and origin. It would only hide snapshots that are directly inside their origins, like the one created by "btrfs subvolume snapshot /run/butter/data/home /run/butter/data/home/snapshot-1".

No this does indeed not work...

[root@fedora-40-127-0-0-2-2201 ~]# btrfs subvolume list -apuq /
ID 256 gen 838 parent 5 top level 5 parent_uuid -                                    uuid 13c51cda-d57a-0a45-a808-1785eca3bc28 path <FS_TREE>/root
ID 257 gen 838 parent 5 top level 5 parent_uuid -                                    uuid 9d64752d-1de6-2849-baac-696ce79f3a7b path <FS_TREE>/home
ID 258 gen 840 parent 5 top level 5 parent_uuid -                                    uuid bd3edcf1-e9b5-654a-96ec-6e62ee17d924 path <FS_TREE>/var
ID 259 gen 78 parent 258 top level 258 parent_uuid -                                    uuid b3735d93-91e0-1046-98f7-e377fafc1b3c path <FS_TREE>/var/lib/machines
ID 260 gen 836 parent 256 top level 256 parent_uuid -                                    uuid 6b5445f7-2d40-5c45-b914-9eedd47dd5cd path root/snapshots
ID 261 gen 836 parent 260 top level 260 parent_uuid 9d64752d-1de6-2849-baac-696ce79f3a7b uuid cf9b06e1-6141-0b42-b61c-5d2c920e88ab path <FS_TREE>/root/snapshots/today

In my understanding, we want to get rid of snapshots in our tables because there are too many of them and they are usually not interesting. Isn't that the goal?

Yes, but you argued for showing them?

@jelly
Copy link
Member Author

jelly commented May 31, 2024

Should we have actions on snapshots when they are listed in the crossrefs table? At least "Delete" seems appropriate.

Sounds reasonable.

@mvollmer
Copy link
Member

In my understanding, we want to get rid of snapshots in our tables because there are too many of them and they are usually not interesting. Isn't that the goal?

Yes, but you argued for showing them?

Yes, but sparingly. Snapshots are also subvolumes and we need to offer all the functionality of subvolumes for them, somewhere. Maybe people want to mount them, or copy them back to their origin.

@jelly
Copy link
Member Author

jelly commented May 31, 2024

Some conclusions from my side:

  • We want to hide snapshots from the overview (or allow filtering) as it gets too cluttered
  • Show snapshots underneath a subvolume where all snapshots live makes a lot of sense, otherwise I would have to know that the snapshots from my home partition in the UI are shown in that subvolume. This makes it tricky as we want something different for the overview

Note: what if we show a subvolume snapshot as btrfs snapshot instead of btrfs subvolume.

@jelly
Copy link
Member Author

jelly commented May 31, 2024

Conclusion from standup with @mvollmer "Let's merge this and figure out filtering/hiding subvolumes later.

@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 31, 2024
@jelly jelly marked this pull request as ready for review May 31, 2024 16:15
@jelly jelly force-pushed the btrfs-snapshot-support branch 2 times, most recently from 4a25cde to 866349f Compare June 3, 2024 08:19
jelly added 2 commits June 3, 2024 11:46
In btrfs a snapshot is a subvolume with a parent_uuid field filled in
with the uuid of the subvolume of which the snapshot is made.

Note that btrfs allows one to make a snapshot of a snapshot.
Register crossrefs for snapshots so they show under snapshots in the
detail page of a subvolume.
register_crossref({
key: subvol.parent_uuid,
card,
size: mounted && <StorageUsageBar stats={use} short />,
Copy link
Contributor

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.

@jelly jelly requested a review from martinpitt June 3, 2024 11:00
@jelly
Copy link
Member Author

jelly commented Jun 3, 2024

@martinpitt do you mind taking the review over?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants