btrfs: auto-detect device #813

AkihiroSuda · 2017-05-06T17:19:22Z

The device is automatically detected via mountinfo (ported from moby/pkg/mount v17.05.0).

No longer need to set plugins.snapshot-btrfs.device manually.

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

import github.com/moby/moby/pkg/mount (v17.05.0-ce) Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

No longer need to set `plugins.snapshot-btrfs.device` manually Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

codecov-io · 2017-05-06T17:58:55Z

Codecov Report

Merging #813 into master will increase coverage by 0.49%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #813      +/-   ##
==========================================
+ Coverage   59.26%   59.76%   +0.49%     
==========================================
  Files           5        5              
  Lines         739      753      +14     
==========================================
+ Hits          438      450      +12     
  Misses        198      198              
- Partials      103      105       +2

Impacted Files	Coverage Δ
snapshot/btrfs/btrfs.go	`55.08% <72.72%> (+2.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e192fa...8cd2182. Read the comment docs.

crosbymichael · 2017-05-08T17:13:42Z

LGTM

stevvooe · 2017-05-08T18:30:59Z

mountinfo/mountinfo.go

+	Optional string
+
+	// Fstype indicates the type of filesystem, such as EXT3.
+	Fstype string


FSType - make sure to follow Go's style guide.

stevvooe · 2017-05-08T18:31:10Z

mountinfo/mountinfo.go

+	Source string
+
+	// VfsOpts represents per super block options.
+	VfsOpts string


stevvooe · 2017-05-08T18:31:51Z

mountinfo/mountinfo.go

+	Mountpoint string
+
+	// Opts represents mount-specific options.
+	Opts string


stevvooe · 2017-05-08T18:32:07Z

mountinfo/mountinfo.go

+}
+
+// GetMounts retrieves a list of mounts for the current running process.
+func GetMounts() ([]*Info, error) {


No getters in Go. Just use Mounts.

stevvooe · 2017-05-08T18:32:42Z

mountinfo/mountinfo.go

+}
+
+// GetMounts retrieves a list of mounts for the current running process.
+func GetMounts() ([]*Info, error) {


Returning a slice to pointers to a record type doesn't make sense. Just use []Info.

stevvooe · 2017-05-08T18:35:46Z

mountinfo/mountinfo_linux.go

+
+// Parse /proc/self/mountinfo because comparing Dev and ino does not work from
+// bind mounts
+func parseMountTable() ([]*Info, error) {


This helper seems redundant.

stevvooe · 2017-05-08T18:37:30Z

mountinfo/mountinfo.go

+	Opts string
+
+	// Optional represents optional fields.
+	Optional string


What Opts and Optional?

Jeez, this madness is direct from the kernel: https://www.kernel.org/doc/Documentation/filesystems/proc.txt.

stevvooe · 2017-05-08T18:38:37Z

mountinfo/mountinfo_linux.go

+// PidMountInfo collects the mounts for a specific process ID. If the process
+// ID is unknown, it is better to use `GetMounts` which will inspect
+// "/proc/self/mountinfo" instead.
+func PidMountInfo(pid int) ([]*Info, error) {


Odd that we would have an exported method that is supported on a single platform. This should just error out for other platforms.

Let's also name this consistently with the other exported function. Like Self and Pid or something like that. Usage will be mountinfo.Pid(pid) and mountinfo.Self().

stevvooe · 2017-05-08T18:38:58Z

mountinfo/mountinfo_windows.go

+
+func parseMountTable() ([]*Info, error) {
+	// Do NOT return an error!
+	return nil, nil


This should probably return an error.

stevvooe · 2017-05-08T18:41:59Z

mountinfo/mountinfo_linux.go

+)
+
+const (
+	/* 36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue


Please link to the place you copypasted this from: https://www.kernel.org/doc/Documentation/filesystems/proc.txt.

Also, please be mindful of copy pasting from other sources without attribution.

stevvooe · 2017-05-08T22:13:53Z

snapshot/btrfs/btrfs.go

 	root   string // root provides paths for internal storage.
 	ms     *storage.MetaStore
 }

+func getBtrfsDevice(root string, mounts []*mountinfo.Info) (string, error) {
+	device := ""


Should this match on the device id, rather than the path?

device id is unreliable on btrfs
https://www.spinics.net/lists/linux-btrfs/msg58908.html

AkihiroSuda · 2017-05-09T02:31:49Z

@stevvooe

The mountinfo package is just copy-pasted from moby/pkg/mountinfo.
I didn't vendor because IIUC we don't want to make containerd dependent on Docker,
but on second thought, it might be ok to import "lego blocks" of Moby.

Let me know whether I should fork mountinfo and modify here according to your review, or just vendor the upstream Moby.

stevvooe · 2017-05-09T20:23:25Z

Let me know whether I should fork mountinfo and modify here according to your review, or just vendor the upstream Moby.

This is fine and we can even port moby to use the better version here.

AkihiroSuda · 2017-05-10T04:54:26Z

updated PR

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

stevvooe · 2017-05-10T15:14:25Z

LGTM

crosbymichael · 2017-05-10T16:53:13Z

LGTM

AkihiroSuda added 3 commits May 6, 2017 16:29

new package: mountinfo

0f21829

import github.com/moby/moby/pkg/mount (v17.05.0-ce) Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

mountinfo: remove unused functions

e37c337

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

btrfs: auto-detect device

d5707d3

No longer need to set `plugins.snapshot-btrfs.device` manually Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the btrfs-zero-config branch from 4881aa9 to d5707d3 Compare May 6, 2017 17:29

stevvooe reviewed May 8, 2017

View reviewed changes

mountinfo/mountinfo.go Outdated

Source string

// VfsOpts represents per super block options.

VfsOpts string

Copy link

Member

stevvooe May 8, 2017

Choose a reason for hiding this comment

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

VFSOpts.

stevvooe reviewed May 8, 2017

View reviewed changes

mountinfo/mountinfo.go Outdated

Mountpoint string

// Opts represents mount-specific options.

Opts string

Copy link

Member

stevvooe May 8, 2017

Choose a reason for hiding this comment

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

Options.

stevvooe reviewed May 8, 2017

View reviewed changes

mountinfo: refactored

8cd2182

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the btrfs-zero-config branch from 6570735 to 8cd2182 Compare May 10, 2017 05:10

stevvooe mentioned this pull request May 10, 2017

Find a place for /pkg moby/moby#32989

Open

22 tasks

crosbymichael merged commit 07fda4b into containerd:master May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

btrfs: auto-detect device #813

btrfs: auto-detect device #813

AkihiroSuda commented May 6, 2017

codecov-io commented May 6, 2017 •

edited

crosbymichael commented May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

stevvooe May 8, 2017

AkihiroSuda May 9, 2017

AkihiroSuda commented May 9, 2017 •

edited

stevvooe commented May 9, 2017

AkihiroSuda commented May 10, 2017

stevvooe commented May 10, 2017

crosbymichael commented May 10, 2017

btrfs: auto-detect device #813

btrfs: auto-detect device #813

Conversation

AkihiroSuda commented May 6, 2017

codecov-io commented May 6, 2017 • edited

Codecov Report

crosbymichael commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented May 9, 2017 • edited

stevvooe commented May 9, 2017

AkihiroSuda commented May 10, 2017

stevvooe commented May 10, 2017

crosbymichael commented May 10, 2017

codecov-io commented May 6, 2017 •

edited

AkihiroSuda commented May 9, 2017 •

edited