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

New module for managing btrfs quota #54728

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@shateq
Copy link
Contributor

shateq commented Apr 2, 2019

SUMMARY

This pull request adds btrfs filesystem quota management functionality that is currently not covered by any of existing modules.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

btrfs_quota

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 2, 2019

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/system/btrfs_quota.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/system/btrfs_quota.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test validate-modules [explain] failed with 9 errors:

lib/ansible/modules/system/btrfs_quota.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got ['Pawel Szatkowski (pszatkowski.byd@gmail.com)']
lib/ansible/modules/system/btrfs_quota.py:0:0: E305 DOCUMENTATION.version_added: required key not provided @ data['version_added']. Got None
lib/ansible/modules/system/btrfs_quota.py:0:0: E307 version_added should be '2.8'. Currently None
lib/ansible/modules/system/btrfs_quota.py:0:0: E312 No RETURN provided
lib/ansible/modules/system/btrfs_quota.py:0:0: E316 ANSIBLE_METADATA.metadata_version: not a valid value for dictionary value @ data['metadata_version']. Got '2.7'
lib/ansible/modules/system/btrfs_quota.py:0:0: E324 Argument 'max_excl' in argument_spec defines default as ('none') but documentation defines default as (None)
lib/ansible/modules/system/btrfs_quota.py:0:0: E324 Argument 'max_rfer' in argument_spec defines default as ('none') but documentation defines default as (None)
lib/ansible/modules/system/btrfs_quota.py:0:0: E324 Argument 'qgroup' in argument_spec defines default as ('none') but documentation defines default as (None)
lib/ansible/modules/system/btrfs_quota.py:61:13: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/system/btrfs_quota.py:61:13: error EXAMPLES: syntax error: found character '@' that cannot start any token

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 2, 2019

@AugustusKling @ColOfAbRiX @EvanK @LinusU @Mogztter @MorrisA @abulimov @adejoux @ahtik @Akasurde @azaghal @bachradsusi @bgurney-rh @dankeder @davidobrien1985 @davixx @dirtyharrycallahan @dougluce @dsummersl @fishman @flynn1973 @gforster @giovannisciortino @goozbach @groks @haad @hryamzik @indrajitr @jamescassell @jasperla @jbenden @jdauphant @jhoekx @jpdasma @jsumners @jtyr @kairoaraujo @kevensen @kyleabenson @lberruti @marvin-sinister @mator @mattjeffery @matze @mcv21 @molekuul @mpdehaan @mulby @natefoo @nibalizer @obourdon @ovcharenko @pilou- @pmarkham @pyykkis @ramooncamacho @rhaido @risaacson @ryan_sb @saito-hideki @saranyasridharan @scathatheworm @sebastiendarocha @sfromm @srvg @tacatac @tdtrask @tmshn @troy2914 @wtcross @xen0l

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@jamescassell
Copy link
Contributor

jamescassell left a comment

Some naming/docs suggestions. I haven't tested the code.

- Qgroup ID (0/257) or subvolume path (@/opt)
default: none
required: false
max_rfer:

This comment has been minimized.

Copy link
@jamescassell

jamescassell Apr 3, 2019

Contributor
Suggested change
max_rfer:
max_reference:

Also would be good to further describe below or link to docs.

This comment has been minimized.

Copy link
@shateq

shateq Apr 3, 2019

Author Contributor

OK, I can put link to btrfs project wiki article that explains what those limits are.

Regarding to naming suggestion, in my opinion if someone is familiar with btrfs quota, he will know what max_rfer means. Btw, this is how this value is being displayed while listing:

root@pawel-VirtualBox:~/btrfs_subvolume# btrfs qgroup show -re /mnt
qgroupid         rfer         excl     max_rfer     max_excl 
--------         ----         ----     --------     -------- 
0/5          16.00KiB     16.00KiB         none         none 
0/257        16.00KiB     16.00KiB         none         none 
0/258        16.00KiB     16.00KiB         none         none 
0/262        16.00KiB     16.00KiB         none         none
- Max reference limit
default: none
required: false
max_excl:

This comment has been minimized.

Copy link
@jamescassell

jamescassell Apr 3, 2019

Contributor
Suggested change
max_excl:
max_exclusive:

This comment has been minimized.

Copy link
@shateq

shateq Apr 3, 2019

Author Contributor

Same comment as for max_rfer, I don't think it's necessary, but if community likes it, I will update :)

state:
description:
- Enable or disable quota for btrfs filesystem
choices: [enabled, disabled]

This comment has been minimized.

Copy link
@jamescassell

jamescassell Apr 3, 2019

Contributor

would present and absent be good state names? i.e., is this something that is turned on or off per-filesystem, or is it a set of policies that can be configured on a more granular level? (for example some policies present, others absent) (I'm not familiar w/ these quotas)

This comment has been minimized.

Copy link
@jamescassell

jamescassell Apr 3, 2019

Contributor

otherwise, many modules have a separate 'enabled' true/false, like enabling a service but not (necessarily) starting it

This comment has been minimized.

Copy link
@shateq

shateq Apr 3, 2019

Author Contributor

@jamescassell Quota support is turned on/off per-filesystem. It must be enabled first, otherwise it's not possible to set limits.

Regarding to present and absent states, well that's a good point. It's something that current code is not doing. It doesn't create or delete qgroups, because 0-level qgroups are automatically created/deleted for each btrfs subvolume and I wrote this module for that use case - I wanted have controll over subvolumes data.

However qgroup mechanism is much more richfull. It allows to create quota groups hierrarchy in parent-child relation. It means that you can define higher level qgroups (1-level, 2-level, etc.) that contains lower level qgroups (children). Limit set on parent also limits children. It's well explained in this withepaper.

Nevertheless the most frequent use case is to limit subvolumes data only, but if module should use whole potential of qgroups then indeed present and absent states should be added as well as parent option for setting relation to parent qgroup.

needs/privileged
needs/root
shippable/posix/group1
skip/rhel/8.0/3

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 10, 2019

Member
Suggested change
skip/rhel/8.0/3
skip/rhel8.0

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 10, 2019

Member

Fixing this will allow the tests to run on RHEL 7.6.

Since the tests are marked needs/privileged they will not run on any of our Docker containers.

@ansibot ansibot added the stale_ci label Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.