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

Use the first fitting gap for ESP, instead of the largest gap #1765

Merged
merged 5 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion subiquity/common/filesystem/boot.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ def get_add_part_plan(device, *, spec, args, resize_partition=None):
# is a bad idea. So avoid putting any sort of boot stuff on a logical -
# it's probably a bad idea for all cases.

gap = gaps.largest_gap(device, in_extended=False)
gap = gaps.gap_with_size(device, size, in_extended=False)
if gap is not None and gap.size >= size and gap.is_usable:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of the logic is redundant now

create_part_plan.gap = gap.split(size)[0]
return create_part_plan
Expand Down
8 changes: 8 additions & 0 deletions subiquity/common/filesystem/gaps.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,14 @@ def largest_gap_size(device, in_extended=None):
return 0


def gap_with_size(device, size, *, in_extended=None):
for pg in parts_and_gaps(device):
Copy link
Collaborator

Choose a reason for hiding this comment

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

gap_with_size sounds like any gap will do that fits the requirement, or maybe even the closest gap (first gap is large, second gap is small but large enough, choose the second). Would you add a docstring clarifying intent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the method name to 'first_gap_with_size'. For this use case, finding the smallest suitable gap might actually make more sense but well. Minor stuff I think.

if isinstance(pg, Gap) and pg.size >= size and pg.is_usable:
if in_extended is None or in_extended == pg.in_extended:
return pg
return None


@functools.singledispatch
def movable_trailing_partitions_and_gap_size(partition):
"""For a given partition (or LVM logical volume), return the total,
Expand Down
74 changes: 74 additions & 0 deletions subiquity/common/filesystem/tests/test_gaps.py
Original file line number Diff line number Diff line change
Expand Up @@ -783,3 +783,77 @@ def test_strings(self):
self.assertEqual(
"TOO_MANY_PRIMARY_PARTS", GapUsable.TOO_MANY_PRIMARY_PARTS.name
)


class TestGapWithSize(GapTestCase):
def test_empty_disk(self):
d = make_disk(size=10 * MiB)
[g1] = gaps.parts_and_gaps(d)
self.assertEqual(g1, gaps.gap_with_size(d, MiB))

def test_half_full(self):
d = make_disk(size=10 * MiB)
make_partition(device=d, size=d.size // 2)
[p1, g1] = gaps.parts_and_gaps(d)
self.assertEqual(g1, gaps.gap_with_size(d, MiB))

def test_half_full_too_big(self):
d = make_disk(size=10 * MiB)
make_partition(device=d, size=d.size // 2)
[p1, g1] = gaps.parts_and_gaps(d)
self.assertIs(None, gaps.gap_with_size(d, 10 * MiB))

def test_one_gap_too_small(self):
self.use_alignment_data(
PartitionAlignmentData(
part_align=10,
min_gap_size=1,
min_start_offset=10,
min_end_offset=10,
primary_part_limit=10,
)
)
# 0----10---20---30---40---50---60---70---80---90---100
# ##### [ p1 ] #####
d = make_disk(size=100)
make_partition(device=d, size=10, offset=20)
[g1, p1, g2] = gaps.parts_and_gaps(d)
self.assertEqual(g2, gaps.gap_with_size(d, 20))

def test_unusable(self):
self.use_alignment_data(
PartitionAlignmentData(
part_align=10,
min_gap_size=1,
min_start_offset=10,
min_end_offset=10,
primary_part_limit=1,
)
)
# 0----10---20---30---40---50---60---70---80---90---100
# ##### [ p1 ] #####
d = make_disk(size=100)
make_partition(device=d, size=10, offset=2)
self.assertIs(None, gaps.gap_with_size(d, 10))

def test_in_extended(self):
self.use_alignment_data(
PartitionAlignmentData(
part_align=10,
min_gap_size=1,
min_start_offset=10,
min_end_offset=10,
primary_part_limit=10,
ebr_space=2,
)
)
# 0----10---20---30---40---50---60---70---80---90---100
# ##### g1 [ p1 (extended) ] g3 #####
# [ p5 ] g2
d = make_disk(size=100)
make_partition(device=d, size=50, offset=20, flag="extended")
make_partition(device=d, size=18, offset=22, flag="logical")
[g1, p1, p5, g2, g3] = gaps.parts_and_gaps(d)
self.assertEqual(g2, gaps.gap_with_size(d, 20))
self.assertEqual(g3, gaps.gap_with_size(d, 20, in_extended=False))
self.assertEqual(g2, gaps.gap_with_size(d, 10, in_extended=True))
5 changes: 4 additions & 1 deletion subiquity/models/tests/test_filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,13 @@ def make_model_and_disk(bootloader=None, **kw):
return model, make_disk(model, **kw)


def make_partition(model, device=None, *, preserve=False, size=None, offset=None, **kw):
def make_partition(
model=None, device=None, *, preserve=False, size=None, offset=None, **kw
):
flag = kw.pop("flag", None)
if device is None:
device = make_disk(model)
model = device._m
if size is None or offset is None:
gap = gaps.largest_gap(device)
if size is None:
Expand Down
Loading