Skip to contentSkip to conversation

RFC: Split do_rootfs into image specific tasks?

DISCUSSION Richard Purdie 3 participants 6 messages Dec 28, 2015 – Jan 6, 2016

lore
Share & tools
lore
#0 lore raw

I've heard complaints from people trying to create more interesting image types about how hard it is to understand the rootfs/image generation code and that its a pain to develop/test/debug.

Having looked at it myself, the internal construction of shell functions which then gets passed into a multiprocessing pool is rather convoluted and it places rather odd constraints on when variables are expanded. Its therefore no wonder people find it confusing/complex.

I've sent three patches to the OE-Core list which illustrate how we could split the code in lib/oe/image into separate tasks, one per image type. This removes the need for the simple task graph code and defers to the bitbake task management code to handle this instead.

This is a good step forward in splitting up the monolithic code and starting to make it more accessible to people. It should also make it easier for people to hook in other tasks and processes into the rootfs code.

Incidentally, the reason this code was all combined originally was due to limitations of fakeroot where if you exited the session, you lost permissions data. With pseudo this constraint was removed.

We did start to rework the rootfs/image code previously and got so far with untangling it however we did prioritise some performance tweaks over splitting into separate tasks and in hindsight, this was a mistake and should have been done the other way around. That work was suspended due to changes in the people working on the project but this split has always been intended, now is the time to finish it IMO.

The code I've sent does work but is not finished yet and has only had basic testing. Things that remain to be done:

* Handling of IMAGE_GEN_DEBUGFS needs reimplementing on the new code structure
* Some variable dependencies on do_rootfs need moving to do_image
* References to do_rootfs need auditing to see if they should now be do_image_complete
* Changes need documenting with migration notes.
* The layout of image.bbclass is pretty horrid and needs cleanup, along with moving functions to the right places
* We should only calculate the rootfs size once, not once per image type.

This work could also be a stepping stone towards further changes to the rootfs backends. Right now its pretty backwards compatible but we could change the API a bit and create the image types as specific tasks right from the start rather than hand crafting shell functions as we do now for example. I believe there would be further benefits in debugging and ease of understanding of the codebase if we did that.

I guess the real question is whether people like this direction and how much should we change the API in order to improve things? Could we afford a pretty heavy rewrite in this area to give a good end result?

Cheers,

Richard

#1 lore raw

To put this in a different why which people might be able to relate to, this changes:

do_rootfs

into:

do_rootfs
do_image
do_image_ext4
do_image_tar
do_image_complete

(for IMAGE_FSTYPES = "tar.bz2 ext4")

Cheers,

Richard

#2 lore raw
In reply to Richard Purdie
Quoted 3 lines · Richard Purdie

I've heard complaints from people trying to create more interesting image types about how hard it is to understand the rootfs/image generation code and that its a pain to develop/test/debug.

[...]

Quoted 3 lines

I guess the real question is whether people like this direction and how much should we change the API in order to improve things? Could we afford a pretty heavy rewrite in this area to give a good end result?

I think it is indeed a step in the right direction. Having to re-create the rootfs directory each time one makes a change to an image creation class or image configuration made experimenting with such changes awkward and slow.

However, the question remains how far this rewrite should go. The current semantic model of an "image" is essentially "rootfs + boot loader + kernel", and the current "image" recipes define both the content of the rootfs and additional parameter for turning that one rootfs into an image.

That's arguably the most common scenario, but it is only a special case. Other image layouts may have multiple partitions, each with certain files. For example, there might be a read-only rootfs, an overlay partition for the rootfs and a data partition for user files (for the sake of the argument, assume that it needs to be pre-populated with certain files).

In my opinion, the proposed changes allow implementing a more general image class. Such a class would have to create additional tasks (like do_overlayfs and do_userfs where it installs files based on some additional recipe variables), then in do_image_foobar combine these additional directories into the full disk image.

This can be done without using any existing code, but probably there are overlaps in functionality and opportunities for code reuse. For example, converting a directory into a ext2/ext3/ext4/btrfs file system is a common problem. Having that in a Python utility method would be useful (but not needed immediately).

The "populate rootfs" code may also be a candidate for moving into an utility method, although it may be harder to determine exactly which variables it depends on and might not even be needed.

So, bottom line, I think it makes sense to move ahead as proposed and refine later in cooperation with distros trying to implement their own special disk layout.

Signature · 6 lines
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
#3 lore raw
Quoted 5 lines · Patrick Ohly

On Mon, 2015-12-28 at 13:00 +0000, Richard Purdie wrote:
I think it is indeed a step in the right direction. Having to re -create the rootfs directory each time one makes a change to an image creation class or image configuration made experimenting with such changes awkward and slow.

Agreed, that alone probably makes this change worthwhile in some ways.

Quoted 14 lines

However, the question remains how far this rewrite should go. The current semantic model of an "image" is essentially "rootfs + boot loader + kernel", and the current "image" recipes define both the content of the rootfs and additional parameter for turning that one rootfs into an image.

That's arguably the most common scenario, but it is only a special case.
Other image layouts may have multiple partitions, each with certain files. For example, there might be a read-only rootfs, an overlay partition for the rootfs and a data partition for user files (for the sake of the argument, assume that it needs to be pre-populated with certain files).

Quoted 5 lines

In my opinion, the proposed changes allow implementing a more general image class. Such a class would have to create additional tasks (like do_overlayfs and do_userfs where it installs files based on some additional recipe variables), then in do_image_foobar combine these additional directories into the full disk image.

That is the intent, someone should be able to much more easily tap into and do other things with the generated data. It works better with rm_work for example too, since things don't get cleaned up until after do_image_complete.

Quoted 7 lines

This can be done without using any existing code, but probably there are
overlaps in functionality and opportunities for code reuse. For example,
converting a directory into a ext2/ext3/ext4/btrfs file system is a common problem. Having that in a Python utility method would be useful (but not needed immediately).

I'd be more than happy to see a function library of these kinds of useful things. This change sets the scene for more changes to the image_types.bbclass, we could for example change the IMAGE_CMD variables into tasks and drop IMAGE_CMD entirely (at least in principle, I'm not saying we should/shouldn't).

Quoted 3 lines

The "populate rootfs" code may also be a candidate for moving into an utility method, although it may be harder to determine exactly which variables it depends on and might not even be needed.

If you look at the code after the recent changes, the do_rootfs code is already well abstracted into the lib/oe/rootfs.py and associated files as a module.

Quoted 3 lines

So, bottom line, I think it makes sense to move ahead as proposed and refine later in cooperation with distros trying to implement their own special disk layout.

Thanks for the feedback. I think this is a good first step and then we can consider if any subsequent ones make sense.

Cheers,

Richard

#4 lore raw
In reply to Richard Purdie

Hi Richard,

Quoted 51 lines · On Mon, 28 Dec 2015 13:00:40 Richard Purdie wrote:

I've heard complaints from people trying to create more interesting image types about how hard it is to understand the rootfs/image generation code and that its a pain to develop/test/debug.

Having looked at it myself, the internal construction of shell functions which then gets passed into a multiprocessing pool is rather convoluted and it places rather odd constraints on when variables are expanded. Its therefore no wonder people find it confusing/complex.

I've sent three patches to the OE-Core list which illustrate how we could split the code in lib/oe/image into separate tasks, one per image type. This removes the need for the simple task graph code and defers to the bitbake task management code to handle this instead.

This is a good step forward in splitting up the monolithic code and starting to make it more accessible to people. It should also make it easier for people to hook in other tasks and processes into the rootfs code.

Incidentally, the reason this code was all combined originally was due to limitations of fakeroot where if you exited the session, you lost permissions data. With pseudo this constraint was removed.

We did start to rework the rootfs/image code previously and got so far with untangling it however we did prioritise some performance tweaks over splitting into separate tasks and in hindsight, this was a mistake and should have been done the other way around. That work was suspended due to changes in the people working on the project but this split has always been intended, now is the time to finish it IMO.

The code I've sent does work but is not finished yet and has only had basic testing. Things that remain to be done:

* Handling of IMAGE_GEN_DEBUGFS needs reimplementing on the new code structure
* Some variable dependencies on do_rootfs need moving to do_image
* References to do_rootfs need auditing to see if they should now be do_image_complete
* Changes need documenting with migration notes.
* The layout of image.bbclass is pretty horrid and needs cleanup, along with moving functions to the right places
* We should only calculate the rootfs size once, not once per image type.

This work could also be a stepping stone towards further changes to the rootfs backends. Right now its pretty backwards compatible but we could change the API a bit and create the image types as specific tasks right from the start rather than hand crafting shell functions as we do now for example. I believe there would be further benefits in debugging and ease of understanding of the codebase if we did that.

Quoted 3 lines

I guess the real question is whether people like this direction and how much should we change the API in order to improve things? Could we afford a pretty heavy rewrite in this area to give a good end result?

I like this a lot. I'd idly considered this model before (as I'm sure others had) as it simplifies a lot of the multi-format image construction handling, improves incremental performance, and the added granularity makes it slightly easier to see what the build system is doing. Obviously there is some minor cleanup work to be done on the patchset as you've noted.

The only "issue" I can think of is that bitbake -c rootfs will no longer build the image files as it did previously; however I'm not sure I'd structure things any differently to avoid that though. I suspect people will fairly easily adapt to this change as long as we publicise it effectively.

Cheers,
Paul

Signature · 2 lines
Paul Eggleton
Intel Open Source Technology Centre
#5 lore raw
Quoted 10 lines · Paul Eggleton

I like this a lot. I'd idly considered this model before (as I'm sure others
had) as it simplifies a lot of the multi-format image construction handling,
improves incremental performance, and the added granularity makes it slightly
easier to see what the build system is doing. Obviously there is some minor
cleanup work to be done on the patchset as you've noted.

Quoted 7 lines

The only "issue" I can think of is that bitbake -c rootfs will no longer build
the image files as it did previously; however I'm not sure I'd structure things
any differently to avoid that though. I suspect people will fairly easily adapt
to this change as long as we publicise it effectively.

Thanks for the feedback. Just as a heads up for people, I posted an updated patch set to the OE-Core list. This one has various issues fixed (including the todos) and is ready for wider testing.

Cheers,

Richard