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

grml-live: fix falldown from c78f58bfee79af61276e3e91457a1b0faa55bbf5. #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ionic
Copy link
Contributor

@Ionic Ionic commented Mar 25, 2021

While deleting the bind-mount destination of mirror directories is fine
in theory, we ended up deleting empty base directories as well.

This is obviously bad, so let's avoid removing base directories by:

  • keeping a copy of all base directories (this should be updated
    periodically, but updates to base directories happen so rarely that
    it hardly matters)
  • wrapping the functionality of rmdir -p in a new function that makes
    sure to stop on base directories, no matter whether empty or not.

Since we use that functionality for directories in a chroot (from the
outside), additional code to handle chroots is necessary.

With that, base directories should now be correctly protected from
removal.

While deleting the bind-mount destination of mirror directories is fine
in theory, we ended up deleting empty base directories as well.

This is obviously bad, so let's avoid removing base directories by:
  - keeping a copy of all base directories (this should be updated
    periodically, but updates to base directories happen so rarely that
    it hardly matters)
  - wrapping the functionality of rmdir -p in a new function that makes
    sure to stop on base directories, no matter whether empty or not.

Since we use that functionality for directories in a chroot (from the
outside), additional code to handle chroots is necessary.

With that, base directories should now be correctly protected from
removal.
@Ionic
Copy link
Contributor Author

Ionic commented Mar 25, 2021

The list of protected base directories looks a bit ugly, but it's the shortest form I could come up with.

If you don't like it, I can expand and re-wrap it. I can't just rewrap it without expanding, though.

@mika
Copy link
Member

mika commented Mar 26, 2021

Uff, many thanks for working on a fix for this, but tbh, I'm a bit afraid of having to maintain this. :-/
What exactly is the problem? The empty directory that's left behind when using the MIRROR_DIRECTORY?
Would it be an option to only run something like rmdir --ignore-fail-on-non-empty "${CHROOT_OUTPUT}/${MIRROR_DIRECTORY}" instead? 🤔

@Ionic
Copy link
Contributor Author

Ionic commented Mar 26, 2021

I've outlined the issue in this comment in grml-live#103.

In short, the issue is that using rmdir -p to remove the bind-mount location for MIRROR_DIRECTORY removes empty directories that are part of the base image, which is arguably bad.

Instead, we could just revert the rmdir -p change, but that'll keep empty directories in the image that aren't part of any package.

The safe_rmdir_p wrapper makes sure that base directories are not removed, which is probably always a good idea. The list of base directories is a bit difficult to maintain (though I could expand and reformat it), but frankly, this list is unlikely to change for another 10 years.

Other alternatives include reimplementing MIRROR_DIRECTORY so as to not just bind-mount the outer directory into the chroot with the same directory structure, but rather bind-mounting it to a specific directory like /lib/live/mount/mirror-directory and only rmdiring that (without -p). That would change the MIRROR_DIRECTORY semantics, though. Crucially, people would need to know to update their sources.list.d entries within the FAI configuration or otherwise will experience build failures all of a sudden.

mika added a commit that referenced this pull request Apr 24, 2021
This reverts commit c78f58b.

While deleting the bind-mount destination of mirror directories is fine
in theory, we end up deleting empty base directories as well, which
is unexpected and unwanted behavior.

See #104 for the related
discussion, this issue needs further attention yet before we can
include this in a new release of grml-live.
@mika
Copy link
Member

mika commented Apr 24, 2021

I tried to look into this and I think your suggestion to re-implement the MIRROR_DIRECTORY handling is the way to go for us.

The MIRROR_DIRECTORY handling is quite error prone and also unhandy already, so it makes sense to rework this. It would actually be nice, if grml-live could also take care of handling apt's source list setup as well, so it's not necessary to manually create something like /etc/grml/fai/config/files/etc/apt/sources.list.d/local-packages.list/CUSTOM, which then even refers to a path that's no longer available in the chroot once grml-live finished its execution.

I think it's fine to change the MIRROR_DIRECTORY semantics, as long as we properly document this change. I personally don't think many folks are using this, and tbh - I didn't use it either for ages, tbh. :) Also if we handle the sources.list configuration for users on the fly, they don't even need to touch the configuration at all.

Does this make sense? What do you think, @Ionic?

For the time being, I reverted the rmdir -p ... change, so we aren't blocked for a new grml-live release.

@Ionic
Copy link
Contributor Author

Ionic commented May 9, 2021

I tried to look into this and I think your suggestion to re-implement the MIRROR_DIRECTORY handling is the way to go for us.

The MIRROR_DIRECTORY handling is quite error prone and also unhandy already, so it makes sense to rework this.

Having a known end location for MIRROR_DIRECTORY would be good since we could easily remove this without having to worry about side-effects. I guess that would be the cleanest way to handle this, yeah.

I think it's fine to change the MIRROR_DIRECTORY semantics, as long as we properly document this change. I personally don't think many folks are using this, and tbh - I didn't use it either for ages, tbh. :)

Changed semantics are the only risk involved with that approach. I've used MIRROR_DIRECTORY extensively, since that allowed me to put custom/modified Debian packages (mostly grml ones that I modified/tested, but also a customized Debian kernel) there for FAI's sources.list.d mechanism to pick them up while building the image. Unless users just want to respin an image, they won't be able to just release new modified versions of grml or base Debian packages, so the mechanism is probably used quite a lot. :)

It would actually be nice, if grml-live could also take care of handling apt's source list setup as well, so it's not necessary to manually create something like /etc/grml/fai/config/files/etc/apt/sources.list.d/local-packages.list/CUSTOM, which then even refers to a path that's no longer available in the chroot once grml-live finished its execution. Also if we handle the sources.list configuration for users on the fly, they don't even need to touch the configuration at all.

Yeah, that's a bit cumbersome. We could make MIRROR_DIRECTORY create such entries automatically if they are fixed anyway and also delete them in the cleanup phase.

The problem with that approach, though, that apt is a bit... weird.

In my setup, I wanted to modify grml packages without increasing their version number (mostly out of laziness). I thought that I could select the packages to be installed via pinning, favoring my local repository. What I didn't know until that point was that apt only uses pinning information for version selection - but the order of source entries is used to determine where to download packages from. Thus, after a bit of trial and error, I had to make sure that my local mirror was the first entry in the global sources.list file. The sources.list.d mechanism wouldn't have worked for this setup, since it's ordered at a later time.

Long story short: there isn't really a "one size fits all" solution here, sadly. Most users won't need such advanced setups, though.

Maybe we should also introduce a new variable called something like MIRROR_DIRECTORY_AUTOADD, defaulting to 1/true that does what you had in mind. If set to 0/false, the mirror directory will be mounted at a fixed location, but not added to the sources list - for advanced setups.

Does this make sense? What do you think, @Ionic?

For the time being, I reverted the rmdir -p ... change, so we aren't blocked for a new grml-live release.

Yep, fine.

clexanis pushed a commit to clexanis/grml-build that referenced this pull request Dec 12, 2022
This reverts commit c78f58b.

While deleting the bind-mount destination of mirror directories is fine
in theory, we end up deleting empty base directories as well, which
is unexpected and unwanted behavior.

See grml#104 for the related
discussion, this issue needs further attention yet before we can
include this in a new release of grml-live.
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.

2 participants