Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Conversation

@Random-Liu
Copy link
Member

Use linux device number from mount.Lookup to look for uuid.

This may be able to fix #325.

And I also see the Ubuntu used by GKE, rootfs is mounted from /dev/root, which doesn't have real corresponding device file.

With this fix, we get device number from mount directly, and find uuid with the device number. This doesn't assume that the mount source has to be a existing file.

Signed-off-by: Lantao Liu [email protected]

@Random-Liu
Copy link
Member Author

@ijc Could you take a look?

@Random-Liu Random-Liu added this to the v1.0.0-alpha.1 milestone Oct 9, 2017
@abhi
Copy link
Member

abhi commented Oct 9, 2017

Looks fine to me.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

// getDeviceUUID gets device uuid for a given path.
func (c *criContainerdService) getDeviceUUID(path string) (string, error) {
info, err := c.os.LookupMount(path)
mount, err := c.os.LookupMount(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

LookupMount has a comment in it:

  // Note that m.{Major, Minor} are generally unreliable for our purpose here
  // https://www.spinics.net/lists/linux-btrfs/msg58908.html

But we are now using exactly those fields below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we don't seem to rely on these any more or less than we did before, so I guess this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

@Random-Liu @ijc Interesting.. if "generally unreliable" maybe we should do it both ways and compare results then return the best result. Just thinking out loud and now worrying about what was meant by generally unreliable.

@Random-Liu Random-Liu merged commit d50c610 into containerd:master Oct 10, 2017
@Random-Liu Random-Liu deleted the fix-fs-uuid branch October 10, 2017 23:06
@metahertz
Copy link

Thanks for taking this one on @Random-Liu!
Will test if this fixes #325 and report back.

@metahertz
Copy link

metahertz commented Oct 15, 2017

Hey @Random-Liu
Sadly this doesn't fix the ZFS issues in #325. For the same reason as the previous code, ZFS manages it's own volumes/filesystems/mounts etc, so the rest of the linux kernel (IIUC) doesn't know which disks are part of a ZFS pool (think LVM/RAID/etc) and which aren't.

If dealing with ZFS, the "truest" way to get a unique identifier which will ALWAYS identify that pool is the GUID property of the pool itself, so for the given mounts (from #325)

tmpfs on /var type tmpfs (rw,nosuid,nodev,noexec,relatime,size=1023524k,mode=755)
testpool/nextnas/containerd-vol on /var/lib/containerd/io.containerd.snapshotter.v1.zfs type zfs (rw,xattr,noacl)
testpool on /var/zpool type zfs (rw,xattr,noacl)
testpool/nextnas on /var/zpool/nextnas type zfs (rw,xattr,noacl)

the "pool name" is always the first word before the first / for a zfs mount, in our case, testpool.
Running the ZFS userspace tools;

zpool get guid testpool

gives us a UUID which will always identify that pool.
UUID/nextnas would always identify the other mount (as it's in the pool 'testpool').

image

Error received for current master of cri-containerd as follows;

screen shot 2017-10-15 at 17 30 56

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getDeviceUUID doesn't work for ZFS

6 participants