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

feat(raspberry-pi-os): Add Raspberry Pi OS support #5827

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

paulober
Copy link

@paulober paulober commented Oct 16, 2024

This commit adds support for the Raspberry Pi OS debian based distribution. It includes a distro definition, 3 modules and integration into other systems like config generation and docs.

  • I have signed the CLA: https://ubuntu.com/legal/contributors
  • I have added my Github username to tools/.github-cla-signers
  • I have included a comprehensive commit message using the guide below
  • I have added unit tests to cover the new behavior under tests/unittests/
    • Test files should map to source files i.e. a source file cloudinit/example.py should be tested by tests/unittests/test_example.py
    • Run unit tests with tox -e py3
  • I have kept the change small, avoiding unnecessary whitespace or non-functional changes.
  • I have added a reference to issues that this PR relates to in the PR message
  • I have updated the documentation with the changed behavior.
    • If the change doesn't change the user interface and is trivial, this step may be skipped.
    • Cloud-config documentation is generated from the jsonschema.
    • Generate docs with tox -e doc.

Proposed Commit Message

<type>(raspberry-pi-os): <summary>  Add support for Raspberry Pi OS

This commit adds support for the Raspberry Pi OS distro.
It includes a distro integration and detection including
four additional configuration modules:
- cc_raspberry_pi
- cc_netplan_nm_patch

Additional Context

@tdewey-rpi, @XECDesign, @ghollingworth, @will-v-pi

Test Steps

Raspberry Pi OS Images with cloud-init can be found here.

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@github-actions github-actions bot added the documentation This Pull Request changes documentation label Oct 16, 2024
@holmanb holmanb added the CLA signed The submitter of the PR has signed the CLA label Oct 16, 2024
@paulober paulober force-pushed the raspios-support branch 2 times, most recently from c475d72 to 683ffc8 Compare October 17, 2024 08:34
@paulober paulober marked this pull request as ready for review October 17, 2024 08:41
@paulober
Copy link
Author

Is there an option if a setting for rpi_interfaces has been set, to tell the power_state_change module that a reboot is required?

@tdewey-rpi
Copy link

Some additional context:

This PR (and the linked GitHub PRs from other repos) are part of a wider desire to move to using cloud-init as the customisation scheme for Raspberry Pi OS, with associated infrastructure in Raspberry Pi Imager to ensure we maintain feature parity (if not an increased feature set) with the current 'firstrun' style customisation scheme.

Going forward, assuming this PR is merged and our migration proceeds along happily enough, my plan is to recommend to each distro vendor supplying OS images to the Raspberry Pi Imager that they also adopt cloud-init as their customisation scheme. This may result in additional PRs adding distro features to cloud-init in a similar manner to this PR - but should absolutely improve the experience for end users across the board.

@@ -305,6 +320,11 @@ system_info:
{% elif variant == "openmandriva" %}
network:
renderers: ['network-manager', 'networkd']
{% elif variant == "raspberry-pi-os" %}
network:
dhcp_client_priority: [dhclient]
Copy link
Member

Choose a reason for hiding this comment

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

Upstream dhclient is abandoned. Is raspberry-pi-os's plan to support it forever?

Choose a reason for hiding this comment

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

This was included as an error, and dhclient is not actually used by Raspberry Pi OS - instead, we use network-manager's built in DHCP client. I believe this has been corrected in the latest version of this PR.

{% elif variant == "raspberry-pi-os" %}
network:
dhcp_client_priority: [dhclient]
renderers: ['netplan', 'network-manager', 'networkd', 'eni']
Copy link
Member

Choose a reason for hiding this comment

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

Which of these does raspberry-pi-os support and plan to use?

Choose a reason for hiding this comment

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

Which of these does raspberry-pi-os support and plan to use?

In the updated PR, this has been reduced to netplan & network-manager, both of which we want to offer.

@paulober paulober force-pushed the raspios-support branch 2 times, most recently from fcd247a to 9bb2b18 Compare October 18, 2024 10:42
@holmanb
Copy link
Member

holmanb commented Nov 12, 2024

There are some merge conflicts now.

@paulober
Copy link
Author

There are some merge conflicts now.

Resolved.

Copy link

github-actions bot commented Dec 5, 2024

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Dec 5, 2024
@paulober
Copy link
Author

paulober commented Dec 5, 2024

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

@TheRealFalcon

@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Dec 5, 2024
This commit adds support for the Raspberry Pi OS debian distribution.
It includes a distro definition, 3 modules and integration into other
systems like config generation.

Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
Signed-off-by: paulober <[email protected]>
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. I left some first pass inline comments.

In general, I would like to consolidate the config and modules as much as possible. When it comes to the config, let's put anything raspberry-pi specific under a rpi or similar top-level key. E.g.,

rpi:
  enable_rpi_connect: true
  interfaces:
    ...
  piwiz:
    ...

Do all the modules need to run at the boot stages they've been added? If not, can any of the modules be combined? E.g., cc_rpi_connect.py runs a single command. Can that command not be incorporated into the cc_rpi_userdata.py module?

@@ -0,0 +1,129 @@
# Copyright (C) 2024, Raspberry Pi Ltd.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this file is necessary. Cloud-init writes 50-cloud-init.yaml, but as far as I know, all network-manager -> netplan files start with '90-'. Doesn't this mean any changes made in network-manager will automatically override the default netplan config? Are you aware of cases where this doesn't work?

Copy link
Author

@paulober paulober Dec 11, 2024

Choose a reason for hiding this comment

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

If I remember correctly, the problem was if you edit the netplan generated NetworkManager (created by cloud-init) config in a desktop GUI the updated config will be written back to netplan with a generated UUID and not touching the original file. This ment changes made by a user are lost after a reboot because the cloud-init netplan config will always be preferred and overrides the NM generated one. By triggering the save operation for every cloud-init generated config after first boot and then removing the original file this issue can be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious if things are different on raspberry-pi-os, but on my desktop at least, I see the network manager files get written with a '90-' first, e.g., 90-NM-141e654c-21b3-4e52-9505-e3c94be335c0.yaml so anything in that file would override what is in 50-cloud-init.yaml. If it doesn't work this way, I would think this would be a netplan bug, or the patch was applied incorrectly. Is this something you can check to see if you get different results?

Copy link
Author

Choose a reason for hiding this comment

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

Ok I just checked without the netplan_nm_patch, and yes the updated config is written to 90-NM... and also visible in the UI. But after a reboot the 50-cloud-init will take over => changes are not effective.

systemd/cloud-init-main.service.tmpl Show resolved Hide resolved
After=networking.service
{% if variant == "raspberry-pi-os" %}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this section needed? After=NetworkManager... is already a few line below.

Copy link
Author

Choose a reason for hiding this comment

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

I will have to dig into this again. But I think it was need because I've had huge problems getting the cloud-init network stage run after network manager is available but not to late so it wont get skipped.

Before=sshd-keygen.service
Before=sshd.service
Before=systemd-user-sessions.service
{% if variant in ["ubuntu", "unknown", "debian"] %}
{% if variant in ["ubuntu", "unknown", "debian", "raspberry-pi-os"] %}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think raspberry-pi-os is needed in this section. Based on the line below you don't want Before=sysinit.target and the two after that are part of DefaultDependencies. Can you add raspberry-pi-os to the line about default dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the advice. I'll test it.

LOG.error("Failed to configure serial console: %s", e)


def enable_ssh(cfg: Config, enable: bool) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the separate tool to enable SSH? Cloud-init has built in SSH support. Is there a reason we can't use that? If so, is there a reason that module can't be expanded to do what this tool is doing?

@@ -156,6 +165,9 @@ cloud_config_modules:
{% endif %}
- locale
{% endif %}
{% if variant == "raspberry-pi-os" %}
Copy link
Member

Choose a reason for hiding this comment

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

rpi_interfaces was also defined in the network stage. Does this module need to run twice? Why?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it doesn't. I'll remove the duplicate.

@@ -35,6 +35,8 @@
"lxd",
"mcollective",
"mounts",
"netplan-nm-patch",
Copy link
Member

Choose a reason for hiding this comment

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

We only need the _ version of all the keys. The - versions exist in other modules for backwards compatibility only.

Copy link
Author

Choose a reason for hiding this comment

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

Ok

@@ -0,0 +1,228 @@
# Copyright (C) 2024, Raspberry Pi Ltd.
Copy link
Member

@TheRealFalcon TheRealFalcon Dec 10, 2024

Choose a reason for hiding this comment

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

Much of this module is a bit strange to me. We're using a one-shot first-boot system configuration service (cloud-init) to configure how a one-shot first-boot system configuration service (piwiz) should work. Why would people want to use both in tandem? Does piwiz do anything that cloud-init currently can't? If not, then why not just use cloud-init for everything? If no user data is passed, I can see still starting piwiz so it can be used interactively, but if a user provides cloud-init user data, I see no reason to then also use a separate configuration service.

In particular, I think allowing username and password configuration multiple ways is very confusing. Cloud-init will create a default user automatically that can be overridden to provide a different user and password. Why would we then also expose additional cloud-init user data to allow piwiz to set the same thing? Let's have one configuration option for applying these things.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for raising these points. The userconf-pi scripts handle more than just the setup wizard. They also rename the default user shipped with Raspbian OS and manage other configuration tasks that are specific to the OS. Overwriting the user setup logic directly in the distribution's Python script seemed more complex.

The module runes even if not configuration is supplied to ensure the scripts are launched correctly, especially since the systemd hook is disabled on builds with cloud-init, allowing for this behavior to be modified.

That said, your suggestion to streamline this makes sense. I agree that having multiple methods to configure the username and password can be confusing. While piwiz provides interactive setup capabilities, cloud-init is designed for automated configuration. Ideally, there should be a single clear configuration pathway.

If I find the time, I’ll look into the feasibility of removing the userconf module or aligning it more closely with cloud-init. This will likely require some adjustments on the OS side, but it’s worth exploring. I’ll revisit this and update you once I have a concrete solution.

@TheRealFalcon TheRealFalcon self-assigned this Dec 10, 2024
Copy link

github-actions bot commented Jan 7, 2025

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025
@paulober
Copy link
Author

paulober commented Jan 7, 2025

Hi @TheRealFalcon, do you have any recommendation on how I can build cloud-init deb pkgs for testing. My current build script always results in (and I had to modify a requirements.txt as it always tries to locate python3-pyserial instead of python3-serial which should normally be auto-renamed by the config in the debs.json if I understand correctly):

dpkg-buildpackage: info: host architecture arm64
         debian/rules clean
        Can't exec "debian/rules": Permission denied at /usr/bin/dpkg-buildpackage line 834.
        dpkg-buildpackage: error: debian/rules clean subprocess failed with unknown status code -1
Stderr: debuild: fatal error at line 1182:
        dpkg-buildpackage -us -uc -ui failed

my build.sh:

#!/bin/bash
cd cloud-init
VARIANT=raspberry-pi-os
make deb VARIANT=raspberry-pi-os
dpkg-deb -R cloud-init_all.deb package_dir
make render-template VARIANT=raspberry-pi-os | tail -n +2 > package_dir/etc/cloud/cloud.cfg

SYSTEM_DIR="./package_dir/lib/systemd/system"
GENERATORS_DIR="./package_dir/lib/systemd/system-generators"

# Iterate over all .tmpl files in the ./systemd directory
for tmpl_file in ./systemd/*.tmpl; do
    # Get the base name of the file without the directory and extension
    base_name=$(basename "$tmpl_file" .tmpl)

    # Check if the file is the generator file
    if [ "$base_name" = "cloud-init-generator" ]; then
        # Render the template and copy it to the system-generators directory
        output_file="$GENERATORS_DIR/$base_name"
        ./tools/render-template --variant="${VARIANT}" "$tmpl_file" > "$output_file"
        echo "Rendered generator template $tmpl_file to $output_file"
    else
        # Render the template and copy it to the system directory
        output_file="$SYSTEM_DIR/$base_name"
        ./tools/render-template --variant="${VARIANT}" "$tmpl_file" > "$output_file"
        echo "Rendered service template $tmpl_file to $output_file"
    fi
done

make clean_packaging
dpkg-deb -b package_dir cloud-init-custom.deb
rm -rf package_dir
cp cloud-init-custom.deb ~/cloud-init/pi-gen/stage2/04-cloud-init/files
cd ..

Platform: Debian 12 arm64

Many thanks,
Paul

@TheRealFalcon
Copy link
Member

@paulober , for throwaway test debs, we have a script for that. Usage for me usually looks something like

DEB_BUILD_OPTIONS=nocheck packages/bddeb -d

Would that work for you?

@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2025
@paulober
Copy link
Author

@paulober , for throwaway test debs, we have a script for that. Usage for me usually looks something like

DEB_BUILD_OPTIONS=nocheck packages/bddeb -d

Would that work for you?

Thank's for the recommendation. The issue seemed to be that I cannot build cloud-init deb pkgs if I'm working as root user on the build server so cloning it again in a normal user account and applying the patches solved it.

@github-actions github-actions bot removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed The submitter of the PR has signed the CLA documentation This Pull Request changes documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants