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

OSTree task improvements #713

Merged

Conversation

agners
Copy link
Contributor

@agners agners commented Apr 27, 2020

Hi, this are smaller improvements to the OSTree image tasks. Most notably it changes how /home folder is stored to align with what Fedora CoreOS is doing and what upstream OSTree today uses as the modern approach.

Stefan Agner added 5 commits April 27, 2020 07:42
The environment variable OSTREE_BOOT_PARTITION is only used when using
GRUB. Move the export into the if statement. Also add a comment why
manually adding /boot/loader{.0} directory is necessary.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
The current code clears tmp and then creates a symlink inside it to
/sysroot/tmp:
tmp
└── tmp -> sysroot/tmp

This is likely a mistake and the root tmp should have pointed to
sysroot/tmp. However, since /tmp is mounted as a tmpfs anyways, we can
get rid of all this logic.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Instead of copying the files to be commited to the ostree just use a
hardlink tree. This improves performance and wasts less diskspace.

When using this method the root directory has already the correct
permission bits set. Also get rid of the unnecessary sync. This halfs
the execution time of the do_image_ostree tasks in my measurments.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
The home directory currently are commited to the OSTree, presumably
to then use it for the deployment. However, we do have access to the
original rootfs in the OSTree deployment tasks (do_image_ota) hence
transferring the files "via OSTree" is not necessary. We do already
carry over some files from the original OE rootfs to /var/sota.
Follow this approach for /var/local and /home as well.

The home will still be stored in the sysroot as documented in
https://ostree.readthedocs.io/en/latest/manual/adapting-existing/.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Instead of using the double indirection mode /home -> /var/rootdirs/home
-> /sysroot/home move the home directory physically into /var/rootdirs.
This allows to use the --modern flag when initializing the file system.

The "old" style is still supported, and does make sense in case the home
directories need to be shared between multiple deployments. Since
multiple deployments is not a use case in meta-updater use the /var
approach. See also: ostreedev/ostree#2085.

The modern flag also gets rid of dev, proc, root, run, sys and tmp. All
of them have been empty and unused.

Note: This change cannot be pushed through updates as this is an initial
deployment setting. Only devices provisioned with images built with this
change applied will use the new layout. Updates will continue to work
on both systems as the symlink from the deployment stays the same (first
indirection is still /home -> /var/rootdirs/home).

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
@pattivacek
Copy link
Collaborator

Thanks for putting this together! It looks like a lot of good work. I'm reviewing it now and testing it in CI, but it might take just a bit of time to make sure that everything works like we expect.

@pattivacek
Copy link
Collaborator

CI is unfortunately broken apparently because Yocto's certs expired, which also means I can't test manually. Once that is resolved we'll retry.

I've reviewed the logic, though, and I don't see any obvious concerns. Everything looks like an improvement and update to modern OSTree usage.

BTW, I agree that the tmp handling logic we had in place was just a mistake. The code was committed in the earliest days of this project and has probably never been looked at too closely again until now. Thanks for catching that!

@agners
Copy link
Contributor Author

agners commented Apr 28, 2020

CI is unfortunately broken apparently because Yocto's certs expired, which also means I can't test manually. Once that is resolved we'll retry.

Hehe, yeah our CI got tripped by it today too :-)

I've reviewed the logic, though, and I don't see any obvious concerns. Everything looks like an improvement and update to modern OSTree usage.

BTW, I agree that the tmp handling logic we had in place was just a mistake. The code was committed in the earliest days of this project and has probably never been looked at too closely again until now. Thanks for catching that!

Yeah I was assuming that. Thanks for confirming.

@agners
Copy link
Contributor Author

agners commented Apr 28, 2020

@patrickvacek added two more commits, one of which addresses more or less what @eu-smirnov in #687 tried to achieve. If you prefer to have those two separate still I can pop them and create a separate merge request.

@pattivacek
Copy link
Collaborator

added two more commits, one of which addresses more or less what @eu-smirnov in #687 tried to achieve. If you prefer to have those two separate still I can pop them and create a separate merge request.

Again, looks like good stuff! I probably would prefer if the new stuff were in a separate PR, though. It's a lot to test, and all of this has to be tested manually, since CI doesn't really cover the interesting parts here. Does your change to ostree_target_hash address #412?

@lbonn
Copy link
Contributor

lbonn commented Apr 29, 2020

The changes look good and well motivated in their commit messages, thanks!

@agners agners force-pushed the master-ostree-improvements branch from f3f634d to fb058bb Compare April 29, 2020 08:22
@agners
Copy link
Contributor Author

agners commented Apr 29, 2020

@patrickvacek ok, dropped off the last two patches! Will create a separate pull request for those.

@agners
Copy link
Contributor Author

agners commented Apr 29, 2020

@patrickvacek and no, it does not address #412.

@eu-siemann
Copy link
Contributor

Looks good to me!

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

I've tested the following:

  • Update from old layout to new
  • Update from new layout to old
  • Update from new to new

And looked around the affected directories to make sure I didn't see anything unexpected. Everything looks good, just as I anticipated. Thanks again for this!

@pattivacek pattivacek merged commit 82d7e04 into advancedtelematic:master May 5, 2020
@agners
Copy link
Contributor Author

agners commented Jun 8, 2020

Just wanted to point out that using the new /home location also allows to use ro /sysroot (see ostreedev/ostree#2086).

@agners agners deleted the master-ostree-improvements branch June 8, 2020 17:42
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.

4 participants