linux-system-roles / storage

Ansible role for linux storage management
https://linux-system-roles.github.io/storage/
MIT License
104 stars 59 forks source link

feat: Support for creating volumes without a FS #400

Closed japokorn closed 1 year ago

japokorn commented 1 year ago

Currently whenever volume is created without fs_type specification, a filesystem of default type (i.e. xfs) is automatically put on it. This change allows user to prevent FS creation by using "unformatted" value as a fs_type option. In the same manner it also allows to remove an existing FS. To be able to do that the safe mode has to be off.

Enhancement:

Reason:

Result:

Issue Tracker Tickets (Jira or BZ if any):

codecov[bot] commented 1 year ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (fccfa38) 13.67% compared to head (cd84a91) 13.66%. Report is 4 commits behind head on main.

:exclamation: Current head cd84a91 differs from pull request most recent head c320742. Consider uploading reports for the commit c320742 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #400 +/- ## ========================================== - Coverage 13.67% 13.66% -0.01% ========================================== Files 8 8 Lines 1733 1734 +1 Branches 79 79 ========================================== Hits 237 237 - Misses 1496 1497 +1 ``` | [Flag](https://app.codecov.io/gh/linux-system-roles/storage/pull/400/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles) | Coverage Δ | | |---|---|---| | [sanity](https://app.codecov.io/gh/linux-system-roles/storage/pull/400/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles) | `16.54% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/linux-system-roles/storage/pull/400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles) | Coverage Δ | | |---|---|---| | [library/blivet.py](https://app.codecov.io/gh/linux-system-roles/storage/pull/400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles#diff-bGlicmFyeS9ibGl2ZXQucHk=) | `0.00% <0.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

richm commented 1 year ago

What happens now if the fs_type is not specified? e.g. if you use

        storage_pools:
          - name: foo
            disks: "{{ unused_disks }}"
            volumes:
              - name: test1
                size: "{{ volume_size }}"

what happens? Does the role do nothing? Does it remove the fs? Does it create a volume with no fs?

What happens when a user has a playbook with the above, which was working and creating file systems with fs_type xfs, and they upgrade the storage role, and run the playbook again? The user expects that the behavior will not change - that is, it should create fs_type xfs. We cannot easily break that, especially if the default will be destructive, or will just silently fail.

I think a better option to avoid breaking the api is to use a special value for fs_type e.g. fs_type: NOFS which will create the volume with no fs, or remove the fs on an existing volume.

japokorn commented 1 year ago

Using None is in this case recognized and processed differently than when fs_type is not used at all.

For idempotency reasons we already have to be able to identify the situations when some variable is omitted. When that happens, any value found (on possibly already existing device) should remain unchanged.

Otherwise, for example running the role over pre-existing encrypted volume without encryption option in the task would force default absent value on it and change the device.

richm commented 1 year ago

The way the current role (current contents of main, not this PR) works is this: if fs_type is omitted, or has the value of null, an fs is created with a type of xfs (on RHEL 8.9 manage nodes anyway). I don't think we should change this behavior as it would break the existing API. I have verified this by using the test tests_create_disk_then_remove.yml and using various different values for, or omitting, fs_type.

vojtechtrefny commented 1 year ago

I think a better option to avoid breaking the api is to use a special value for fs_type e.g. fs_type: NOFS which will create the volume with no fs, or remove the fs on an existing volume.

We are currently using unformatted in blivet-gui and empty in udisks so these would be also options. There is a (old and probably never really used) filesystem called NoFS so we should probably avoid using that.

japokorn commented 1 year ago

Using ansible 'false' equivalents might be interchangeable with None, so based on discussion with vtrefny I have changed the string from None to all case variations of unformatted. I added this to README and also modified the commit message accordingly.

richm commented 1 year ago

ok - I also did a test with

        storage_pools:
          - name: foo
            disks: "{{ unused_disks }}"
            volumes:
              - name: test1
                size: "{{ volume_size }}"
                fs_type: unformatted

i.e. create a volume without an FS - it worked fine