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

factory: do not mount /lib/{modules,firmware} if there is a drivers tree #238

Merged

Conversation

alfonsosanchezbeato
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato commented Mar 14, 2024

Do not mount /lib/{modules,firmware} in case snapd created a drivers
tree for our kernel snap, as in that case these mounts will be created
by snapd-generator from the base or from the snapd deb package for
hybrid systems. This situation is expected to happen only for UC24+.

@alfonsosanchezbeato
Copy link
Member Author

alfonsosanchezbeato commented Mar 14, 2024

Draft for the moment, please do not merge - This will need to wait for snapd 2.63 to be in stable

@ernestl ernestl requested a review from Meulengracht May 2, 2024 10:09
@Meulengracht Meulengracht marked this pull request as ready for review May 2, 2024 10:28
@Meulengracht Meulengracht marked this pull request as draft May 2, 2024 10:29
@alfonsosanchezbeato alfonsosanchezbeato changed the title factory: do not mount /lib/{modules,firmware} for run mode factory: do not mount /lib/{modules,firmware} if there is a drivers tree May 16, 2024
@alfonsosanchezbeato alfonsosanchezbeato marked this pull request as ready for review May 16, 2024 11:52
@alfonsosanchezbeato
Copy link
Member Author

@valentindavid @Meulengracht this is ready for review now.

@valentindavid
Copy link
Collaborator

@alfonsosanchezbeato Could we have a reference of what PR from snapd will expect this?

@alfonsosanchezbeato
Copy link
Member Author

@alfonsosanchezbeato Could we have a reference of what PR from snapd will expect this?

This change enables using (mounting) a kernel drivers tree when created by snapd. All the needed changes in snapd were already merged. The ways this works is:

  • snapd will generate the drivers tree only for core24/hybrid 24.04 or more modern, as in these cases the systemd generator that creates the mounts for /var/lib/snapd/kernel/... exists
  • With this change, the initramfs will not mount kernel in /lib/{firmware,modules} if the drivers tree exists
  • Instead, the systemd generator from the rootfs will generate the right mounts from the drivers tree to /lib/{firmware,modules}

Hopefully this answers your question.

@valentindavid
Copy link
Collaborator

I suppose this was this one: canonical/snapd#13923

@alfonsosanchezbeato
Copy link
Member Author

I suppose this was this one: snapcore/snapd#13923

And canonical/snapd#13563 and possibly others.

EOF

# Mount /lib/{firmware,modules} only if there is no drivers tree created by snapd
if [ ! -d /run/mnt/data/system-data/var/lib/snapd/kernel ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the situation where this is not created? I think of 2, but I want to make sure I understand.

  • When snapd is too old (will not be the case in uc26 anymore)
  • It is the first boot, or factory reset

Copy link
Member Author

Choose a reason for hiding this comment

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

Could happen for old snapd or base < core24 so snapd does not create the drivers tree. Also on install mode or factory reset, when you have an ephemeral system. But for first boot after installation the drivers tree should be there (see canonical/snapd#13923).

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, does snapd generate /etc/systemd/system/usr-lib-modules.mount? Then this already should have more priority than /run/systemd/generator/usr-lib-modules.mount.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we do not generate that mount in /etc.

In any case, as discussed it would be good to check for the path including the revision (/var/lib/snapd/kernel/<kernel_snap_name>/<snap_revision>/)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, could you explain how the modules are found? Usually, they have to be placed in /lib/modules/$(uname -r).

Copy link
Member Author

Choose a reason for hiding this comment

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

Modules are mounted by the systemd generator (https://github.com/snapcore/snapd/blob/master/cmd/snapd-generator/main.c) that now we include in core24.

Do not mount /lib/{modules,firmware} in case snapd created a drivers
tree for our kernel snap, as in that case these mounts will be created
by snapd-generator from the base or from the snapd deb package for
hybrid systems. This situation is expected to happen only for 24+
systems.
@alfonsosanchezbeato
Copy link
Member Author

@valentindavid I now check for kernel snap name / revision, I think that should handle most corner case situations. And hopefully the change guarantees that you will have a drivers tree compatible with the loaded kernel, and any core24/hybrid 24.10 system has in theory the right generator for these mounts.

I have tested the change for UC24 and hybrid in different scenarios, and seems to work as expected.

Copy link
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

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

Thanks, based on the changes that use the rev directly this looks good

Copy link
Collaborator

@valentindavid valentindavid left a comment

Choose a reason for hiding this comment

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

Looks fine.

@alfonsosanchezbeato
Copy link
Member Author

The tests-snapd tests seem to fail because no eth device is created in the VM. This looks as the same as #246 , so the issue does not seem directly related to the PR.

@alfonsosanchezbeato
Copy link
Member Author

I'd like to set snapd.debug=1 in the kernel command line, but we need canonical/snapd#14077 to land first. Otherwise the tests should pass now.

@alfonsosanchezbeato alfonsosanchezbeato merged commit 1086714 into canonical:main Jun 13, 2024
3 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.

3 participants