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.

Sign up for GitHub

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

Jump to bottom

Fstab support (rebased to 3.9) #1119

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Fstab support (rebased to 3.9) #1119

merged 4 commits into from
Nov 1, 2023

Conversation

japokorn
Copy link
Contributor

@japokorn japokorn commented Mar 22, 2023 •
edited
Loading

  • added support for blivet to directly modify fstab based on blivet actions
  • added tests

@japokorn japokorn changed the title 3.8 devel fstab Fstab support (rebased to 3.8) Mar 22, 2023
@japokorn japokorn mentioned this pull request Mar 22, 2023
Closed
@japokorn
Copy link
Contributor Author

japokorn commented Mar 22, 2023 •
edited
Loading

Summary of replies to review comments from #1093:

  • changed API to avoid using confusing arguments
  • added docstrings
  • get_device_by_specs method renamed to get_device and reworked (unclear parts were dropped or changed)
  • added methods for better access; it is now possible to iterate over fstab entries, print, and compare them directly
  • extended tests
  • other small changes

@japokorn
Copy link
Contributor Author

blivet/formats/fs.py: change_mountpoint intentionally left blank
The point of this is to be able to revert any mountpoint changes in case the action gets cancelled. Since it is just a property value change, it does not need any code but functionality remains. I am open to suggestions but this came to me as a simple solution that utilizes structures already in place.

@japokorn japokorn force-pushed the 3.8-devel-fstab branch 9 times, most recently from 2956abe to 04d9581 Compare March 29, 2023 10:49
@vojtechtrefny
Copy link
Member

image

These tests run using GitHub Actions and not in our CI, the missing dependencies need to be resolved in the specific workflow configuration.

@japokorn japokorn force-pushed the 3.8-devel-fstab branch 13 times, most recently from 5976394 to 27b0e04 Compare April 6, 2023 10:22
@japokorn japokorn force-pushed the 3.8-devel-fstab branch 2 times, most recently from 9b78410 to 43876a6 Compare April 11, 2023 10:46
@japokorn japokorn requested a review from dwlehman June 29, 2023 13:27
dwlehman
dwlehman reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@dwlehman dwlehman left a comment

Choose a reason for hiding this comment

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

This looks okay to me overall. Some of the stuff in get_device around swap files or bind mounts makes me a bit nervous. I might prefer to leave that out unless we also have test coverage.

blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
@japokorn japokorn force-pushed the 3.8-devel-fstab branch 3 times, most recently from 6db5e36 to b1a5d28 Compare September 1, 2023 13:06
vojtechtrefny
vojtechtrefny reviewed Sep 5, 2023
View reviewed changes
Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Just a few observations based on trying to use blivet.fstab.FSTabManager as a standalone module without blivet.

blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py

entry.file = None
if device.format.mountable:
entry.file = device.format.mountpoint
Copy link
Member

Choose a reason for hiding this comment

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

What if the device doesn't have a mount point set? I don't think that None is a valid value in fstab:

In [13]: f.entry_from_device(nvme0n1p1)
Out[13]: UUID=3E7F-3333         efi     None    0       0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is meant to be able to produce incomplete entries as well (these are used for looking up existing entries). There is a sanity check in FSTabManager.write that prevents invalid entries to be written into the file in case of misuse.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, please make a note about this in the docstring, thanks.

blivet/fstab.py Outdated Show resolved Hide resolved
blivet/fstab.py Outdated Show resolved Hide resolved
blivet/fstab.py Outdated Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Show resolved Hide resolved
blivet/fstab.py Outdated Show resolved Hide resolved
blivet/fstab.py Outdated Show resolved Hide resolved
@vojtechtrefny vojtechtrefny mentioned this pull request Sep 23, 2023
Merged
@japokorn japokorn force-pushed the 3.8-devel-fstab branch 2 times, most recently from 7d2e09b to a741460 Compare September 27, 2023 12:50
@japokorn japokorn force-pushed the 3.8-devel-fstab branch 3 times, most recently from 76c28ad to 5f881f9 Compare October 17, 2023 10:30
@japokorn
Fstab support
3258334 
 - added support for blivet to directly modify fstab based
on blivet actions
 - added tests and unit tests
@japokorn
Incorporated review comments
0514047 
- changed API
- other fixes
@japokorn
Added support for user defined values in fstab
6fa0170 
- Allows to set prefered spec type in fstab by either setting
blivet.fstab.spec_type for 'global' effect or device.format.fstab.spec_type for individual device.
Allowed values are "UUID", "PARTUUID", "LABEL", "PARTLABEL" or "PATH".
When selected value is not available, it uses other one (defaults to
UUID).

- Allows to set freq, passno and mntops for individual devices to be
  written in fstab. Done by setting device.format.fstab.freq/passno/mntops values respectively.
@japokorn
fixed fstab.read issue
2778634 
(to be squashed)
@japokorn japokorn changed the base branch from 3.8-devel to 3.9-devel October 23, 2023 13:10
@vojtechtrefny vojtechtrefny changed the title Fstab support (rebased to 3.8) Fstab support (rebased to 3.9) Oct 27, 2023
vojtechtrefny
vojtechtrefny approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thank you.

@japokorn japokorn merged commit a40a3ad into storaged-project:3.9-devel Nov 1, 2023
9 of 11 checks passed
@vojtechtrefny vojtechtrefny mentioned this pull request Nov 2, 2023
Closed
japokorn added a commit to japokorn/blivet that referenced this pull request Nov 2, 2023
@japokorn
Added missing fstab object to SwapSpace
5f8e8a1 
Anaconda tests revealed missing FSTabOptions object in SwapSpace class.
This issue is related to the recent merge of fstab rework PR storaged-project#1119 which
added this object to FS class. SwapSpace does not inherit from FS and
was overlooked.

This change adds the object.
@japokorn japokorn mentioned this pull request Nov 2, 2023
Merged
japokorn added a commit to japokorn/blivet that referenced this pull request Nov 6, 2023
@japokorn
Added missing fstab object to SwapSpace
d7186c3 
Anaconda tests revealed missing FSTabOptions object in SwapSpace class.
This issue is related to the recent merge of fstab rework PR storaged-project#1119 which
added this object to FS class. SwapSpace does not inherit from FS and
was overlooked. This change adds the missing object.

It also adds logic that skips all fstab related operations if fstab file
is not to be written (i.e. fstab.dest_file is None). The reason for this is to
eliminate the risk of potential issues caused by unused component.

The test for above has been added as well.
japokorn added a commit to japokorn/blivet that referenced this pull request Nov 6, 2023
@japokorn
Added missing fstab object to SwapSpace
179381c 
Anaconda tests revealed missing FSTabOptions object in SwapSpace class.
This issue is related to the recent merge of fstab rework PR storaged-project#1119 which
added this object to FS class. SwapSpace does not inherit from FS and
was overlooked. This change adds the missing object.

It also adds logic that skips all fstab related operations if fstab file
is not to be written (i.e. fstab.dest_file is None). The reason for this is to
eliminate the risk of potential issues caused by unused component.

The test for above has been added as well.
@japokorn japokorn mentioned this pull request Feb 13, 2024
Closed
@vojtechtrefny vojtechtrefny mentioned this pull request May 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwlehman dwlehman dwlehman left review comments

@vojtechtrefny vojtechtrefny vojtechtrefny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@japokorn @vojtechtrefny @dwlehman