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

revert changes on fstype from mount #2049

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

revert changes on fstype from mount #2049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions subiquity/models/filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,13 +1258,10 @@ def _available(self):
class Mount:
path: str
device: Filesystem = attributes.ref(backlink="_mount", default=None)
fstype: Optional[str] = None
options: Optional[str] = None
spec: Optional[str] = None

@property
def fstype(self):
return self.device.fstype

def can_delete(self):
from subiquity.common.filesystem import boot

Expand Down Expand Up @@ -2318,8 +2315,9 @@ def can_install(self) -> bool:
def should_add_swapfile(self):
mount = self._mount_for_path("/")
if mount is not None:
if not can_use_swapfile("/", mount.fstype):
return False
if mount.type != "zfs":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I really don't want this bit here. I want this as part of the ZFS class.
So to abstract that we can add something to Mount and ZFS that isn't a property that allows retrieving the fstype - like the property originally, but with a different name (and not a property) so serialization isn't broken, and then ZFS can handle the special case.

if not can_use_swapfile("/", mount.device.fstype):
return False
for swap in self._all(type="format", fstype="swap"):
if swap.mount():
return False
Expand Down
18 changes: 18 additions & 0 deletions subiquity/models/tests/test_filesystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,24 @@ def test_render_includes_unmounted_new_partition(self):
self.assertTrue(disk2.id in rendered_ids)
self.assertTrue(disk2p1.id in rendered_ids)

def test_bind_mount(self):
model = make_model()
make_disk(model, path="/dev/vda")
fake_up_blockdata(model)
model.apply_autoinstall_config(
[
{
"id": "tmpfs1",
"type": "mount",
"spec": "none",
"path": "/tmp",
"size": "4194304",
"fstype": "tmpfs",
},
]
)
self.assertEqual(model.render()["storage"]["config"][0]["fstype"], "tmpfs")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try adding something like this:

Suggested change
self.assertEqual(model.render()["storage"]["config"][0]["fstype"], "tmpfs")

Copy link
Author

Choose a reason for hiding this comment

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

Hi @dbungert

Try adding something like this:

Suggested change
self.assertEqual(model.render()["storage"]["config"][0]["fstype"], "tmpfs")

With that code I could reproduce the issue, enabling me to debug further.

I have a suggested addition to the test that illustrates the problem without a curtin invocation - the serialization code seems to not like the property I think?

Indeed, the serialization code skips if the attribute starts with _, which is true in this specific case, and the attribute is not added to the serialization list.

If that's true, maybe it makes more sense to abandon fstype as a property [...]
I'm not sure I fully understand what you mean by this.
If we abandoned fstype as property, then should we revert back the change to fstype: Optional[str] = None ?

and fix the other code that handles the mountlikes to call to a new method that the serialization code is unaware of.

  1. IIUC, I believe you mean this code ? And append the fstype property to r ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, I believe you mean this code ? And append the fstype property to r ?

As the serialization code doesn't look at underscore attributes, or the property apparently, I'm suggesting that we revert fstype from a property like you had originally suggested - but fix the code that breaks on the ZFS codepath.

Copy link
Author

@dnegreira dnegreira Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Try adding something like this:

This is now added on d4e2cf5

As for the ZFS unit test case, I'm not sure how to proceed; if you can point me to a similar test already done or some pointers, I would be grateful.


class TestPartitionNumbering(unittest.TestCase):
def setUp(self):
Expand Down
Loading