Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

Avoid exposing the host's special mounts #294

Closed
wants to merge 11 commits into from
Closed

Avoid exposing the host's special mounts #294

wants to merge 11 commits into from

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Feb 24, 2016

This should avoid the kind of problem encountered in #293.

Currently, this breaks InstallGrub_1_99 and InstallGrub_2, which need to be fixed to call install-grub from outside the chroot.

@nbraud
Copy link
Contributor Author

nbraud commented Feb 24, 2016

Actually, I'm not sure calling install-grub inside the chroot is the right idea.

@nbraud
Copy link
Contributor Author

nbraud commented Feb 24, 2016

@andsens The latest commit I added rely on nbd having been loaded with max_part being 1.
Also, I wasn't able to check whether that indeed lets install-grub run successfully. I will check tomorrow, in case you don't have time by then.

@nbraud
Copy link
Contributor Author

nbraud commented Feb 24, 2016

@andsens Slightly unrelated, but do you think the “fake /dev” approach would be relevant for debootstrap too?

@andsens
Copy link
Owner

andsens commented Feb 24, 2016

@andsens Slightly unrelated, but do you think the “fake /dev” approach would be relevant for debootstrap too?

It wouldn't. Whoever coded that part was the right kind of lazy about it. debootstrap just extracts a very special tar.gz directly into /dev and calls it a day. It's a file you really shouldn't try extracting anywhere else :-)

Actually, I'm not sure calling install-grub inside the chroot is the right idea.

On of the reasons for doing this might be a leftover from grub 1.99. When run on the host, the grub helper scripts make all kinds of assumptions that you can't configure your way around. More importantly though is that you have to run grub inside the guest. What if the host boots from extlinux or a different version of grub?

@andsens
Copy link
Owner

andsens commented Feb 24, 2016

In the previous comment I forgot to mention how impressive this PR is. That is some really cool stuff!!
I love how it demystifies the whole /dev magic :-)

However, before merging, this will need some heavy testing and a lot more comments.

@nbraud
Copy link
Contributor Author

nbraud commented Feb 25, 2016

Whoever coded that part was the right kind of lazy about it. debootstrap just extracts a very special tar.gz directly into /dev and calls it a day.

Ok, I misunderstood your comment in #293 and thought that debootstrap actually mounts a devfs there.

this will need some heavy testing and a lot more comments.

Definitely. That's just a first stab at it because I got incredibly annoyed by #293.
I guess it gave me incentive to fix it, though :-)

@nbraud
Copy link
Contributor Author

nbraud commented Feb 25, 2016

@andsens I added some commits, now the grub2 install works.
Grub1.99 should work too, but I didn't check

@nbraud
Copy link
Contributor Author

nbraud commented Feb 29, 2016

@andsens Did you have the opportunity to try this?

@andsens
Copy link
Owner

andsens commented Feb 29, 2016

Hoooly crap. How much documentation did you have to sift through to get this right? Wow.
I have not had the time to test this yet. Sorry. Maybe we could pull in some help? (I'll definitely test it myself as well, though).
Paging @JamesBromberger, @myhro, @srivasta and @zmarano. Could you guys try this out? This PR has the potential to fix loads of annoying errors and could make the bootstrapping process a lot more robust to failure. It also removes unnecessary interaction with the host and could potentially provide a more "clean" system.

makedev(43, nbd_max_part * i + j+1))

path = '/dev/mapper/nbd%ip%i' % (i,j+1)
if os.path.exists(path):
Copy link
Owner

Choose a reason for hiding this comment

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

Under which circumstances would this path not be a link? Shouldn't we consider that an error? We expect something to be there, but it's not, which means somethings fishy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I couldn't find a good reference on the internals of device-mapper.
Basically, I'm unaware of any case where the files in /dev/mapper are not symlinks (except control), I'm just being extra-cautious.

@nbraud
Copy link
Contributor Author

nbraud commented Feb 29, 2016

How much documentation did you have to sift through to get this right?

It wasn't quite that bad ;-)
Also, I just woke up and realised that a PID namespace might be the right answer for /proc, so I added that before going back to sleep.

I have not had the time to test this yet.

No problem, it's not quite urgent. I guess one would have to be extra unlucky to hit #293.

@myhro
Copy link
Collaborator

myhro commented Mar 1, 2016

@andsens this is on my TODO list. Unfortunately I've been sick (it's true what they say about Zika virus) in the past few days and the time I was able to stand on the PC was used for work-related stuff. :-(

@andsens andsens mentioned this pull request Mar 3, 2016

# The nbd devices
os.makedirs(join(dev, 'mapper'), 0o755)
nbd_max_part = int(info.volume._module_param('nbd', 'max_part'))
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks on EC2, since the _module_param part only works for QEMU volumes.

@andsens
Copy link
Owner

andsens commented Mar 13, 2016

I'm having trouble getting grub2 to install:

[remote] Installing grub 2
[remote] Executing: chroot /target/5fec2bf1/root grub-install /dev/xvdf
[remote] Installing for i386-pc platform.
[remote] grub-install: error: failed to get canonical path of `/dev/mapper/xvdf2'.
[remote] Command 'chroot /target/5fec2bf1/root grub-install /dev/xvdf' returned non-zero exit status 1

@nbraud
Copy link
Contributor Author

nbraud commented Mar 14, 2016

@andsens Could I have the manifest which makes it fail?

@andsens
Copy link
Owner

andsens commented Mar 14, 2016

Ah, yes of course. Though it isn't a manifest. It's the gpt_grub_stable ec2 test:
tox -e system tests.system.ec2_ebs_hvm_tests:test_gpt_grub_stable

Or via code:

#!/usr/bin/env python

from tests.system.ec2_ebs_hvm_tests import test_gpt_grub_stable
from bootstrapvz.base.main import setup_loggers

setup_loggers({'--log': '-', '--color': 'default', '--debug': True})
test_gpt_grub_stable()

You'll need a build-servers.yml for the test to be able to run.

@andsens
Copy link
Owner

andsens commented Apr 2, 2016

Hehe, soo, that was easy:

for name, partition in get_partitions().items():
    mknod(join(dev, name), 0o666 | stat.S_IFBLK, makedev(int(partition['major']), int(partition['minor'])))

I think you can even replace the nbd stuff with this (minus the symlinks). The code simply re-creates all the block devices mentioned in /proc/partitions, which you can fetch with get_partitions().

@nbraud
Copy link
Contributor Author

nbraud commented Apr 6, 2016

@andsens Thanks for having a look, I was unable to make the time those last weeks.

I will see about using your snippet and testing.

@andsens
Copy link
Owner

andsens commented Jun 5, 2016

OK, so I have tried getting this to work, but there are a few problems:

  • After isolating the process it seems python can't start new threads. It fails with thread.error: can't start new thread in /usr/lib/python2.7/threading.py:745
  • Some tasks use unmount() (e.g. the prebootstrapped plugin) to do things to the volume. After remounting /dev is of course not populated any longer, so we need to either re-create it or create /dev outside root and mount --bind it into root

I have improved on your method by simply walking through /dev and re-creating everything in it, you can check it out here.
It's on the special-devices branch.

I am closing this PR, let me know if you find a solution to these problems. I'd love to get this working, but there is just too much other stuff for me to work on.

@andsens andsens closed this Jun 5, 2016
@nbraud
Copy link
Contributor Author

nbraud commented Jun 5, 2016

@andsens Duplicating the entirety of /dev still comes with the risk that the build/installation of the guest might clobber something in the host (for instance during grub-install).

I also didn't have time lately to dedicate to this PR, but when I do I will try sending you separate PRs.
I realise that trying to work simultaneously on /dev, /proc and /sys here didn't help making it very manageable.

P.S.: Actually, it seems everything except /proc isolation made it to your branch (modulo my previous caveat with duplication the entirety of /dev)

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

Successfully merging this pull request may close these issues.

None yet

3 participants