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

support falling back to a bridge kernel #2089

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

mwhudson
Copy link
Collaborator

This needs a lot more commentary and tests but its the end of my week and I think the logic is basically correct and I welcome any comments you have before I get back to it on Monday.

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 reasonable so far

size: 1066115072
type: fsimage-layered
variant: server
bridge_kernel_name: linux-generic-brg-22.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bit is inconsistent with the spec

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah. the spec is also a bit inconsistent with latest terminology. I'll try to move the spec and this branch towards a consistent approach.

class SourceCatalog:
version: int
sources: typing.List[CatalogEntry]
bridge_kernel_name: typing.Optional[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

In particular I want to pull in the default kernel in here as well, so we aren't specifying them in different ways in livecd-rootfs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I wondered about that but we already have a way to specify a default kernel, /etc/subiquity/kernel-meta-package (and similar). Should we work towards dropping that in favour of recording the default in the catalog? How would this interact with the server .2+ ISOs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh i see the spec covers this, good reading skills there Michael.

@@ -47,3 +47,7 @@ class InstallerChannels(CoreChannels):
# step is after logfiles have been copied to the system, so should be used
# sparingly and only as absolutely required.
PRE_SHUTDOWN = "pre-shutdown"
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminder note for real comments since this looks like a stub.

@mwhudson
Copy link
Collaborator Author

OK pushed a new version that has some tests and comments and is probably a bit better in most ways. Two things though:

  1. I haven't thought about how this is going to interact with a kernel indicated by autoinstall or oem stuff (the latter probably not being a real concern in practice but I should think about it)
  2. I think it would be better factored to replace the DRIVERS_LISTED event I add here with a DRIVERS_DECIDED event and move all the slightly painful logic around what happens if you say you want drivers before you know if there are any (which is what can happen on desktop) into the driver controller rather than having the kernel controller deal with it.

I'll try to work on those tomorrow.

@mwhudson mwhudson marked this pull request as ready for review October 3, 2024 23:34
@mwhudson
Copy link
Collaborator Author

mwhudson commented Oct 3, 2024

I think this is conceptually sound now, and possibly ready for merging? It could stand to gain some more tests I think.

@mwhudson mwhudson requested a review from dbungert October 3, 2024 23:36
@dbungert dbungert merged commit 4a5bf37 into canonical:main Oct 15, 2024
12 checks passed
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