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

Add Btrfs quotas to the Partitioner UI #1165

Merged
merged 2 commits into from
Nov 22, 2020

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Nov 17, 2020

Problem

We want to add support for Btrfs quotas to the Partitioner. See Jira entry SLE-7742

This is the second part of https://trello.com/c/Ypt6X2VA/2118-3-partitioner-support-for-btrfs-quotas (the first part was #1163).

Solution

  • In the form to edit a Btrfs file system (not to be confused with the form to edit a block device), a new "Enable Subvolumes Quota" is added.
    Click here to see two screenshots about itquota1
    quota2
  • In the form to edit a Btrfs subvolume, a "Limit Size" widget is added.
    Click here to see two screenshots about itquota3
    quota4
    Note: the help text was extended in 167bcff
  • The columns in the Btrfs section of the Partitioner are reorganized to show that limit and also, if known, the space used by each subvolume.
    Click here to see two screenshots about itquota5
    quota6b
    Note: the help texts were slightly modified in 167bcff
  • As a bonus, when subvolumes with a quota are displayed in a general table with more devices, the size limit is displayed in the column "Size".

Testing

  • Manually tested
  • Added unit tests

@ancorgs ancorgs force-pushed the ui_snap_experiment2 branch 13 times, most recently from bb66e1b to 949c861 Compare November 20, 2020 14:33
@ancorgs ancorgs marked this pull request as ready for review November 20, 2020 15:33
#
# @return [DiskSize]
def fallback_referenced_limit
subvolume&.former_referenced_limit || filesystem.blk_devices.first.size
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure whether to use the first block device is a good idea in case of a multi-device Btrfs. Why not something like filesystem.blk_devices.map(&:size).min ?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, what happens when the limit is bigger than the size of the filesystem? Is there something like thin provisioning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing like thin provisioning. But that's not a problem here. It's a max quota in a subdirectory. It implies no promises about that subdirectory being bigger than the container file system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, adding a max quota of 100GiB in a subdirectory of a partition that is only 50GiB will give me no extra space. :-)

src/lib/y2partitioner/actions/controllers/filesystem.rb Outdated Show resolved Hide resolved
src/lib/y2partitioner/actions/controllers/filesystem.rb Outdated Show resolved Hide resolved
src/lib/y2partitioner/dialogs/btrfs_subvolume.rb Outdated Show resolved Hide resolved
def validate
return true if value

Yast::Popup.Error(_("The size entered is invalid."))
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: I understood we prefer to use Yast2::Popup in the new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, for very simple cases like this, the old API is more straightforward... and we have another 27 usages of Yast::Popup in the Partitioner anyways.

src/lib/y2partitioner/widgets/help.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 97.605% when pulling 167bcff on ancorgs:ui_snap_experiment2 into 5caf727 on yast:btrfs_subvols.

@ancorgs ancorgs merged commit d4cadc6 into yast:btrfs_subvols Nov 22, 2020
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.

3 participants