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

Honor match directives when installing in a gap using layout.mode: use_gap #2083

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Sep 13, 2024

Autoinstall directives allow to specify match directives when using guided use_gap mode:

  storage:
    layout:
      name: direct
      mode: use_gap
      match:
        install-media: true

However, match directives are currently ignored ; unlike when using the reformat_disk mode.
Make sure they are honored.

LP:#2080608

@ogayot
Copy link
Member Author

ogayot commented Sep 13, 2024

Testing done using:

storage:
  layout:
    name: direct
    mode: use_gap
    match:
      devpath: "*vdc"

and two empty disks: /dev/vdb (12GiB) and /dev/vdc (8GiB)

  • Without the change, the installer picks up /dev/vdb because it is the largest disk, effectively ignoring the match directive.
  • With the change, the installer picks up /dev/vdc, honoring the match directive.

The behavior looks correct to me.

On the other hand, I believe, that it will change the behavior when no match directives are specified ; from

  • pick the largest gap in the largest disk; to
  • pick the largest gap, not necessary in the largest disk

is it acceptable or should we revisit the logic? Revisiting the logic might be a matter of doing:

-            bootable_disks = self.get_bootable_matching_disks(match)
-            gap = gaps.largest_gap(bootable_disks)
+            bootable_disk = self.get_bootable_matching_disk(match)
+            gap = gaps.largest_gap(bootable_disk)

@ogayot ogayot marked this pull request as ready for review September 13, 2024 14:00
Copy link
Collaborator

@dbungert dbungert 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 but I'd like some test automation to go with. subiquity/server/controllers/tests/test_filesystem.py has some tests that stretch the definition of Unit test but I think are effective at testing this area, and it has some use_gap cases already.

@dbungert
Copy link
Collaborator

pick the largest gap, not necessary in the largest disk

That was arguably what I had in mind, so I'd call this a bug fix. Would be worth release noting, to clarify that the use_gap behavior is more intentional and can now be better controlled.

Instead of using inconsistent type hints for match directives, we now
declare a MatchDirective type and use it everywhere.

Signed-off-by: Olivier Gayot <[email protected]>
disk_for_match either returns a matching disk ; or returns None. Ensure
that the return type hint is "Optional".

Signed-off-by: Olivier Gayot <[email protected]>
When using the use_gap mode in autoinstall, one may want to ensure that
we use a gap on a specific disk. Unfortunately, the match directives
were silently ignored.

Let's make sure they are honored.

LP: #2080608

Signed-off-by: Olivier Gayot <[email protected]>
@ogayot
Copy link
Member Author

ogayot commented Sep 18, 2024

Looks good but I'd like some test automation to go with. subiquity/server/controllers/tests/test_filesystem.py has some tests that stretch the definition of Unit test but I think are effective at testing this area, and it has some use_gap cases already.

I spent a bit of time doing that. I would like to cover more scenarios but it's a start..

Copy link
Collaborator

@dbungert dbungert left a comment

Choose a reason for hiding this comment

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

I see you have more testing in mind, merge when you think it's ready.

@ogayot
Copy link
Member Author

ogayot commented Sep 18, 2024

I see you have more testing in mind, merge when you think it's ready.

Thanks! I'm going to hit merge but will revisit this in a follow-up PR.

@ogayot ogayot merged commit 95ccb9c into canonical:main Sep 18, 2024
12 checks passed
@ogayot ogayot deleted the matcher-use-gap branch September 18, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants