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

DietPi-Imager | Add option to add FAT partition after rootfs to simplify setup #6602

Merged
merged 13 commits into from
Sep 10, 2023

Conversation

dirkhh
Copy link
Contributor

@dirkhh dirkhh commented Sep 6, 2023

This isn't complete, yet (MBR only, only tested on one board), but I felt that this would be enough to maybe get some feedback on the code and the overall idea for implementation.

What this does right now is that if the corresponding environment variable ADD_DOS_PART is set, at image creation time a 4MB DOS partition named DIETPISETUP (yay for 11 character limit) is created at the end of the disk image and dietpi.txt and dietpi-wifi.txt are copied onto that partition.

During first boot, when such a partition is found, those two files (and ONLY those two files - not sure if that is the right choice, but it seemed better that way) get copied back over the ones that were in the root filesystem.

This way a user on a Mac or a Windows system can very easily make changes to the settings - I think the most important use case will be wifi setup on headless systems... and then of course fully automated setup.

Again, I'm curious to hear comments about everything. Coding style, decisions about how this is enabled, where I inserted the code, anything. I want to make sure that this is broadly useful and not just a fix for my use case.

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 6, 2023

oh, the first commit was needed in order for me to be able to test it on a board that met my criteria... the firstboot would always fail (apparently there is a kernel update and downloading and unpacking this take a LOT of space)

@MichaIng
Copy link
Owner

MichaIng commented Sep 7, 2023

Ah, another important thing, the actual mount, copy and partition deletion would need to be done here: https://github.com/MichaIng/DietPi/blob/dev/rootfs/var/lib/dietpi/services/fs_partition_resize.sh

This script runs before dietpi-firstboot and does the expansion of the root partition, which cannot work as long as the config partition is still there. But it even makes it easier as the root partition detection code and the final partprobe/partx -u are not needed anymore (as they are done in the expansion script already). So steps would be:

  1. Detect root partition
  2. Check for last 4 MiB FAT partition, an in case mount it, copy over files and remove it
  3. expand root partition
  4. partprobe/partx -u
  5. expand root filesystem

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 7, 2023

Ah, another important thing, the actual mount, copy and partition deletion would need to be done here: https://github.com/MichaIng/DietPi/blob/dev/rootfs/var/lib/dietpi/services/fs_partition_resize.sh

BRILLIANT. That's the piece that was missing. And that explains why the partition didn't get resized.

I will change that as well. Depending on a few errands I have to run today you should have an update to the PR within the next 24h

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 8, 2023

so this needed a bit more work than your comments might make one believe.
Yes, of course GPT is supported by sfdisk, but of course it needs a different partition type.
Reusing the loop device also required a bit of acrobatics to get it to find the new partition (I don't particularly think that this is any easier or more obvious than what I have, but it works, and that's what matter).
I switched to the "hey, everywhere else we assume the root partition is the last one in the partition table and on disk" approach which simplified things a bit.
Also, I did fix the syntax suggestions, etc.
I think this is pretty complete, but because I'm at the barn with my kids I didn't get a chance to test the resulting image on device. I hope I'll get to it this evening.

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

This is now tested with a both mbr and gpt and seems to work fine.
The big question of course is... how will this become useful to Joe Average User.
I, of course, think that this is something that we might consider enabling by default; for someone who doesn't need the ability to set up the dietpi{,-wifi}.txt file prior to boot, this has negligible cost (a few kB in larger compressed image size, a minuscule slowdown in boot time. But for anyone setting up a wifi only device this is a huge help.

And yes, of course it's a no op when build an image that already has a FAT /boot partition.

Anyway, I think this is ready to be merged.

@MichaIng
Copy link
Owner

MichaIng commented Sep 9, 2023

I, of course, think that this is something that we might consider enabling by default

Definitely. I'd keep it as opt-in in dietpi-imager but enable it by default in dietpi-build for all images, excluding:

  • all images with [[ $boot_size != 0 && $boot_fstype == 'fat'* ]]
  • VM images, which usually come as appliance or virtual disk images which one usually cannot mount anyway before having it important into the virtualizer already?
  • Container images, because, well, currently they are more for our internal use, and when used for LXC containers or similar, one has a Linux host already and is able to mount Linux filesystems.
  • x86_64 UEFI and installer images (both [[ $ITYPE == 'Installer' ]]), as those are wrapped into a Clonezilla ISO where one has no direct access. And of course for those in theory we could accept config files on the EFI partition instead (copy from /boot/efi).

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

Excellent, I'll add that to the exclusions I already have (which is just the first one)

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

A couple of comments while I work on the second change you requested...
The two changes to the rootfs size should be dropped... This seems to be a general issue if there is a kernel update since the image we start from was cut. The two where I added more space just happen to be the ones I tested with (as the NanoPi has a dos partition table and the OrangePi uses gpt. But I can reproduce this with others as well. So this really isn't related to this PR.
Normally I'd force push this, but I think you said you collapse all of this into one commit, so I will just add one at the end that undoes those changes and look at you for a more generic solution to have more space available at build time. I'm actually surprised that you haven't run into this yourself, yet.
Also, I will go for a smaller size - 1MB is plenty. And add README-Dietpi-setup.txt
The one thing I don't know what to do about is the filesystem name... Is there a better choice than DIETPISETUP that is all caps and 11 or fewer characters?

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

Ok, I think I have once again addressed all the feedback.
Let me know what else you'd like to see improved.

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

"final" (ha... right...) cleanups to make it look more consistent with the rest. Collapsed into a single commit this is nice and small and tidy now.

@MichaIng
Copy link
Owner

MichaIng commented Sep 9, 2023

The two changes to the rootfs size should be dropped... This seems to be a general issue if there is a kernel update since the image we start from was cut.

I think this is caused by the recent major kernel upgrade by Armbian, respectively the first APT repo kernel/bootloader package upgrades/syncs since February. Those Linux builds and/or the initramfs' seem to take significantly more space. So yes, let's drop the change here and instead I'll do a test with all images anyway before merging the 7z => xz change, and then raise all rootfs sizes where necessary.

The one thing I don't know what to do about is the filesystem name... Is there a better choice than DIETPISETUP that is all caps and 11 or fewer characters?

I think it is fine. How high is the chance that someone creates a partition labelled "DIETPISETUP" with a FAT filesystem at the end of the image, not intended for this feature. Let's keep it like that.

@MichaIng MichaIng added this to the v8.22 milestone Sep 9, 2023
@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 9, 2023

The two changes to the rootfs size should be dropped... This seems to be a general issue if there is a kernel update since the image we start from was cut.

I think this is caused by the recent major kernel upgrade by Armbian, respectively the first APT repo kernel/bootloader package upgrades/syncs since February. Those Linux builds and/or the initramfs' seem to take significantly more space.

yes, they seem to take something like an extra 150M...

So yes, let's drop the change here and instead I'll do a test with all images anyway before merging the 7z => xz change, and then raise all rootfs sizes where necessary.

Excellent. I already pushed a commit to this branch that reverts them.
You do collapse everything to one commit, right? :)

The one thing I don't know what to do about is the filesystem name... Is there a better choice than DIETPISETUP that is all caps and 11 or fewer characters?

I think it is fine. How high is the chance that someone creates a partition labelled "DIETPISETUP" with a FAT filesystem at the end of the image, not intended for this feature. Let's keep it like that.

That was my thinking. Excellent. Then I think I have no outstanding tasks here. YAY.

@MichaIng MichaIng changed the title Add fat partition after root partition to simplify setup DietPi-Imager | Add option to add FAT partition after rootfs to simplify setup Sep 10, 2023
Sometimes the buids are running out of space when doing a kernel update.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
So far this only handles dos partition tables.
When ADD_DOS_PART is set to 1, add a small 4MB fat filesystem as the
last partition to this image, and copy the two DietPi config files to
this partition.
Now even people who don't have access to a Linux system can very easily
mount the DIETPISETUP partition, edit the two files and get the setup
they want. This is especially important for headless devices connecte
via WiFi - where this initial setup is necessary for them to connect to
the network in the first place.

This commit only creates that new partition. The setup logic still needs
to be updated so that it mounts that partition (when present) and uses
the config files on that partition instead of the ones provided
elsewhere in the image.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
This is the second part of the logic. If we have the DIETPISETUP partition then
copy the dietpi.txt and dietpi-wifi.txt files from there into the /boot directory
and delete the DIETPISETUP partition (in order to allow the resizing of the root
filesystem to work).

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
We were checking for the mbr type of 'c'. Instead of checking for that
and the GUID for a generic MS data partition, make things easier and
just check that it's a vfat filesystem. With that, the 4MB, and the
subsequent check for the label this appears to be enough belts and
suspenders.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
And collect all the code for this in one spot.
Putting this all in one line is of course possible, but also much harder
to read, understand, and maintain, IMHO.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
Also copy the dietpiEnv.txt file. The others don't appear to have
default content, so I just mention them in the readme.

Frankly, the text in the readme probably needs some editing.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
These were needed for me to be able to test things locally, but don't
belong in this PR.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
comparing to dev this removes an odd whitespace change; it also tightens
some of the code to be more in line with the rest of the file, and
remove some remaining debug output that otherwise just clutters the user
experience when manually building an image.

Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
@MichaIng MichaIng force-pushed the add-fat branch 2 times, most recently from 27686ca to 4c023fd Compare September 10, 2023 20:37
- DietPi-Imager | Turn ADD_DOS_PART env variable into a CLI flag. Do not add the additional partition by default, but only if requested
- DietPi-Imager | Coding: Re-use FP_SOURCE_IMG and IMAGE_SIZE variables for adding FAT patition; only raise source image size if required
- DietPi-Imager | Copy as well dietpiEnv.txt and optional config/automation files/scripts to setup FAT partition if present
- DietPi-FS_partition_resize | Allow FAT setup partitions of any size and only copy files if newer: Reason is that users with a Linux host may have edited/created files on the ext4 partition and we do not want to overwrite these
- DietPi-Build | Make skipping the additional FAT setup partition a CLI flag, and pass the new CLI flag to DietPi-Imager
- DietPi-Build | When creating multiple image editions for the same device, always remove the added FAT setup partition before booting the system or passing it to DietPi-Imager again
- CHANGELOG | Add entry about added FAT setup partition
@MichaIng
Copy link
Owner

MichaIng commented Sep 10, 2023

I did a few changes:

  • Moved from env var to CLI flag, which I generally prefer. Consistently all DietPi-Imager env vars should become CLI options/flag in a dedicated PR.
  • The setup partition can now have any size. We might want to extend this is the future and I think the fact that it is FAT + the label are sufficiently unique.
  • Some minor coding/refactor, aiming to reduce doubled command calls

But the two more important changes:

  • Users who flash the image from a Linux host might not even recognise the added FAT partition and instead edit the file(s) on the rootfs directly. In this case of course we must not overwrite the files there. I am not sure whether this is fully safe/reliable, but I added the -u flag to the copy command. At least if the system time during image generation was earlier than on the host when editing the files, this should work. Probably you have a better/safer idea, but this is the best I could come up with for now.
  • We generate multiple editions of the same image by times, editing dietpi.txt or even boot the image and run installs, then pass it again through the imager. Currently the imager does not check for & re-use an existing setup partition but would just add a new one, and of course within dietpi-build that setup partition would block expanding the filesystem as well. So for now it is just removed within dietpi-build in those cases and re-added every time in dietpi-imager. We could come up with a smarter solution for this which does not imply repeatedly adding and removing the setup partition, but for now this should work.

Merging this now, so I am able to test it via GitHub Actions directly.

@MichaIng MichaIng merged commit 63c84fc into MichaIng:dev Sep 10, 2023
@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 10, 2023

Thanks for those changes.
Yes, I'd love to see (and will be happy to help) such a move towards CLI switches. So much less chance for odd mistakes and strange outcomes.

For the "user edited files in root partition" my preferred fix would be to store a hash of the unedited file as part of the imaging.
If the file in the root filesystem doesn't match the hash and the file on the fat partition does, don't copy.

The interesting question of course is what to do when neither match 🤷‍♂️

@MichaIng
Copy link
Owner

MichaIng commented Sep 10, 2023

Indeed, the hash would be a safer approach. I'm just a little hesitating to add another 2-3 files to our images and making it another little more complex 😅.

I would only check whether the files on the ext4 partition do not match the hash, and in case skip the copy, and do not check the ones on the FAT partition at all. So if both do not match, the ones on the ext4 partition are kept. But it is also not perfectly failsafe indeed, which is probably simply impossible.

I think for now I'll leave it like it is, expecting not too off system clocks. For the images we generate it is definitely correct, and it is very unlikely that a Windows/macOS system where those images were downloaded to afterwards has a system time from before the image generation, after those images have been flashed. If really someone runs into this issue, we can reconsider.

@MichaIng
Copy link
Owner

MichaIng commented Sep 12, 2023

Btw, tested it on an OPi 5 Plus, and after I replaced the fs_partition_resize.sh script (I forgot to set the branch for the target image) it worked very well. On Windows, after flashing with Rufus, the FAT partition pops up, and with the label "DIETPISETUP" one can hardly miss that. Then with the readme and the dietpi.txt inside, it is kinda obvious. This also saves me from mounting the USB device (in case SD card reader) to a VM before flashing images for automation testing, or the need to attach a serial console for WiFi setup of headless SBCs.

I was actually hoping that WSL2 will solve this at least for Windows users, but mounting USB devices is not possible without a VirtualHere-like IP based device sharing from Windows host to WSL guest, which requires additional setup within the guest. And even then, mounting SD cards via shared USB device (card reader) is not possible, which means that the most relevant use case for us does not work, and most others are at least difficult, more difficult than spinning up some VMware/VirtualBox VM to mount and edit the USB device.

So yes, a great time saver, with no native alternative to expect any time soon.

@dirkhh
Copy link
Contributor Author

dirkhh commented Sep 12, 2023

I am thrilled that this is working as well as I had hoped.
Thanks for the collaboration (and all the subsequent fixes since you merged it).

@MichaIng
Copy link
Owner

and all the subsequent fixes since you merged it

Most of them caused by my last commit here, which I merged untested so that I can test via our CI instead 😄. But yep, learned a few things, that also FAT16 has a minimal size above 1 MiB, and that sfdisk (correctly) requires 34 tailing free sectors for the backup partition table, when adding a GPT partition.

@StephanStS
Copy link
Collaborator

@dirkhh: Big thanks for this exciting and valuable feature. I used it many times so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants