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

Duplicate /dev mounts #242

Closed
jlowellwofford opened this issue May 12, 2020 · 12 comments
Closed

Duplicate /dev mounts #242

jlowellwofford opened this issue May 12, 2020 · 12 comments

Comments

@jlowellwofford
Copy link

jlowellwofford commented May 12, 2020

When a node is vnfs provisioned, it ends up with two entries for the /dev mount in /proc/mounts (etc). After digging a bit, this happens because provision/initramfs/capabilities/provision-vnfs/70-devtree mounts dev in $NEWROOT/dev, but the one in the initramfs remains.

For almost everything, having a duplicate /dev mount doesn't hurt anything. I ran into an issue specifically with running libvirt on a warewulf provisioned node, where the duplicate /dev entry confuses libvirt (specifically, because of a not-so-robust routine used in libvirt, it fails to mount /dev/pts in the domain namespace).

Here's one way this could be managed:

  1. I suspect provision/initramfs/capabilities/provision-vnfs/70-devtree may no longer be needed. It has the ability to populate a /dev for linux that doesn't support devtmpfs, but devtmpfs has been in the kernel for the last decade. Maybe just get rid of it.
  2. Since initramfs/init handles mounting /proc, /sys, and /dev initially, it makes sense to do the following:
mount --move /proc "$NEWROOT/proc"
mount --move /sys "$NEWROOT/sys"
mount --move /dev "$NEWROOT/dev"

exec switch_root "$NEWROOT" /sbin/init

I've tested this, and it resolved this issue for a centos7 vnfs image.

Happy to create a PR if this seems reasonable.

@jmstover
Copy link
Contributor

Hrmm... Looks like there's a few issues .... if we keep 70-devtree ... we should wrap that mount with a check on a mountpoint /dev call to see if somethings already mounted before trying to mount it again.

The /dev mount in init should be moved up to before the dmesg -n 1 >/dev/null 2>&1 call ... so we aren't creating a empty /dev/null file.

@bensallen ... You're the last I see that made changes here. What are your thoughts on this?

@jlowellwofford
Copy link
Author

Can we revive this? I'm happy to throw together a PR if there's some consensus on how.

@jmstover
Copy link
Contributor

I have some ideas ... there's an if block in provision/initramfs/init where it mounts /dev ... that block should be moved up in the init file prior to any of the attempted > /dev/null calls.

We also have: provision/initramfs/capabilities/provision-vnfs/70-devtree

I theory this file just needs to have the cp -rap call in it. Copying the initramfs /dev to the new system root /dev. Might be possible to just mount a devtmpfs at $NEWROOT/dev. I can't remember off the top of my head what the behavior is at that point when you switch to the new root.

@bensallen
Copy link
Member

If we want to maintain the old behavior of supporting kernels without devtmpfs, as we do currently, can we just umount /dev right before switch_root? What about umount /sys and /proc as well, so the init that we're switch_root'ing to can take care of however it sees fit?

@bensallen
Copy link
Member

Also agreed that https://github.com/warewulf/warewulf3/blob/development/provision/initramfs/init#L49 should move up above the dmesg setting console level to 1.

@jmstover
Copy link
Contributor

jmstover commented Aug 11, 2020

the init that we're switch_root'ing to can take care of however it sees fit?

The init shouldn't be dependent on them already being mounted should it? From a boot initramfs?

can we just umount /dev right before switch_root?

That should work. We're actually mounting it twice... once in init and then in 70-devtree

@bensallen
Copy link
Member

The init shouldn't be dependent on them already being mounted should it? From a boot initramfs?

Coreutils' switch_root appear to move /dev, /proc, /sys, /run if they're mounted. We might as well too as busybox's switch_root doesn't do this.

That should work. We're actually mounting it twice... once in init and then in 70-devtree

Right this was done to follow the original legacy mdev + cp method of populating /dev. We don't support any kernel's that old at this point (RHEL6 support is dropped for example). Thus as Lowell suggest we could just simplify this whole thing, drop 70-devtree completely. Do the three mount --move right before the switch root.

Any case you can think of where someone might be futzing with $NEWROOT/dev via postscript? As this change would force said futzing to using /dev instead.

@jlowellwofford
Copy link
Author

It seems like a pretty odd move to futz with $NEWROOT/dev. I can't think of any realistic use-cases for it with modern kernels. Maybe just call that "unsupported"?

And yeah, completely agree that it needs to move up. I should have caught that the first time around.

@bensallen
Copy link
Member

bensallen commented Aug 11, 2020

Yea agreed, so I think we should:

  1. Remove 70-devtree.
  2. Add to right before the switch_root
mount --move /proc "$NEWROOT/proc"
mount --move /sys "$NEWROOT/sys"
mount --move /dev "$NEWROOT/dev"
  1. Move the mount -t devtmpfs block above the dmesg in init.
  2. Do we also remove the mdev fallback lines if mount -t devtmpfs fails?

@jmstover
Copy link
Contributor

jmstover commented Aug 11, 2020

Do we also remove the mdev fallback lines if mount -t devtmpfs fails?

I say if we're cleaning house here, then go a head and remove it and just mark devtmpfs as a requirement.

Any case you can think of where someone might be futzing with $NEWROOT/dev via postscript?

No... but it wouldn't surprise me for someone to want to use mknod to do something in the /dev structure. Do something in an earlier ##-* script and have 70-devtree copy it over. I'm content to have that be moved to a runtime script from wwgetscript without a concrete example.

@jlowellwofford
Copy link
Author

PR #255 attempts to implement this as discussed.

@bensallen
Copy link
Member

Closing with #255 merged.

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

No branches or pull requests

3 participants