GC: always detect and unmount leftovers #1856

steveej · 2015-12-09T14:08:32Z

This covers unmounting of recursive bind-mounts, as well as the overlay mounts.
It also has the advantage of not attempting to unmount something that is
not there, e.g. if a reboot happened and the mounts are gone anyway.

iaguis · 2015-12-09T16:51:58Z

rkt/gc.go

+		stage1TreeStoreID, err := p.getStage1TreeStoreID()
+		if err != nil {
+			stderr("error getting stage1 treeStoreID: %v", err)
+			return


We want to resist this error too.

Yeah, if we can't get the treeStoreID we should still be able to force-GC the pod.

Gotcha. PR updated

blixtra · 2015-12-10T16:52:51Z

rkt/gc.go

+		}
+
+		// unmount all leftover mounts
+		stdout("GC of leftover mounts for pod %q...", p.uuid)


Do we really need to output this every time? Just an implementation detail, IMHO.

we don't need this stdout, I think the previous stdout in emptyExitedGarbage() was enough.

alban · 2015-12-11T11:39:19Z

I would like to validate that the shared mounts and then make-rprivate+umount works in practice when there are several pods.

Ideally, when the number of "fly" pods running increase, the number of mounts on the host namespace increases linearly and not exponentially. I would like that to be tested by starting e.g. ~10 "fly" pods.
When starting "fly" pod1 and then "fly" pod2, I would like to check that they can be GC'ed in any order.

alban · 2015-12-11T12:06:21Z

The reason I am asking these two tests is because I think the number of mounts will increase exponentially with the number of pods (or Fibonacci? I am not sure):

zero pods

/ (root)
└─ /data

one pod

/ (root)
├─ /data
└─ pod1
   └─ host-root
      └─ data

two pods

/ (root)
├─ /data
├─ pod1
│  └─ host-root
│     └─ data
└─ pod2
   └─ host-root
      ├─ data
      └─ pod1
         └─ host-root
            └─ data

three pods: too big tree to draw.

If it is indeed the case, then I think we should check @philips ' suggestion with rkt run-direct: without chroot and without bind mounts, but just a chdir.

alban · 2015-12-11T13:23:03Z

The number of mount points seems to grow quickly, from my test script:

$ sudo ./try.sh
Pod 1: 37 mounts
Pod 2: 185 mounts
Pod 3: 1517 mounts
Pod 4: 66785 mounts

steveej · 2015-12-11T17:04:31Z

@alban The user provided volumes are mounted with just MS_BIND so they won't recurse submounts. The main problem is that if you mount / into the container recursively the mounts will be multiplied explosively due to directories that again like /var/lib/rkt which again contain the recursively mounted /.

This is not only a problem of scaling but also of containers blocking each other in GC. I've solved this problem by unmounting all instances of a pod's rootfs, also if they live in a different pod's rootfs due to recursive mounts. I've left this code in even though we don't use recursive binds now, since we might provide the option to configure this per mount and will need it then.

xiang90 · 2015-12-13T02:20:41Z

rkt/gc.go

+			stderr("Skipping stage1 GC")
+		} else {
+			stage1RootFS := s.GetTreeStoreRootFS(stage1TreeStoreID)
+			if err := stage0.GC(p.path(), p.uuid, stage1RootFS, globalFlags.Debug); err != nil {


err is a shadowed variable. When you enable go vet --shadow, it will compliant.

Thanks. Should we enable the --shadow flag in our testscript?

I don't know.

I thought that shadowing with if err := ...; err != nil { was idiomatic in Golang.

alban · 2015-12-14T16:15:07Z

I've left this code in even though we don't use recursive binds now, since we might provide the option to configure this per mount and will need it then.

It would be better to add that code when we actually need it because we are not sure it's going to be added.

alban · 2015-12-14T16:25:04Z

rkt/gc.go

+		}
+
+		// unmount all leftover mounts
+		if err := stage0.ForceMountGC(p.path(), p.uuid.String()); err != nil {


Suggestion: rename function to stage0.MountGC().

alban · 2015-12-14T16:57:56Z

stage0/gc.go

 		}

-		if strings.HasPrefix(mountPoint, path) {
+		for i, s := range lineResult {
+			s := s[0]


If you use strings.Split, no need to do this s := s[0].

alban · 2015-12-14T17:09:12Z

Since it does not use recursive binds, this branch could be simplified: no need to complicate the parsing of /proc/pid/mountinfo for now to parse the optional fields and we can keep the fmt.Sscanf as it is.

steveej · 2015-12-16T15:40:20Z

@alban

Since it does not use recursive binds, this branch could be simplified: no need to complicate the parsing of /proc/pid/mountinfo for now to parse the optional fields and we can keep the fmt.Sscanf as it is.

Assuming it is the stage1 that may still construct recursive bind mounts which we need to detect properly to be able to clean it up. WRT the stage1 fly this is needed for the recursive mounts for /dev, /sys and /proc. Also, users might still instruct recursive mounts manually, since passing MS_REC has the same results just a more comfortable invocation ;-)

iaguis · 2015-12-17T10:16:48Z

stage0/gc.go

+		mode       string
+	)
+
+	//	lineRegex := regexp.MustCompile(`[^ ]+`)


If strings.Split works, you can remove this comment :)

alban · 2015-12-17T11:08:42Z

A rebase would be good for the next iteration :)

This is for unmounting recursive bind-mounts, as well as the overlay mounts. It also has the advantage of not attempting to unmount something that is not there, e.g. if a reboot happened and the mounts are gone anyway.

alban · 2015-12-17T13:03:53Z

I think it looks good! @krnowak are you still reviewing it or is it good to you too?

krnowak · 2015-12-17T13:09:10Z

No no, I'm done with it. LFAD if green.

GC: always detect and unmount leftovers

alban · 2015-12-17T14:12:02Z

👍

steveej added needs/review do not merge and removed do not merge labels Dec 9, 2015

steveej mentioned this pull request Dec 9, 2015

initial implementation of rkt fly as stage1 #1833

Merged

4 tasks

iaguis reviewed Dec 9, 2015
View reviewed changes

steveej mentioned this pull request Dec 10, 2015

GC option to only remove CNI IP address ? #1835

Open

alban added this to the v0.14.0 milestone Dec 10, 2015

blixtra reviewed Dec 10, 2015
View reviewed changes

krnowak added needs/some-changes and removed needs/review labels Dec 11, 2015

xiang90 reviewed Dec 13, 2015
View reviewed changes

alban reviewed Dec 14, 2015
View reviewed changes

alban assigned steveej Dec 14, 2015

alban mentioned this pull request Dec 16, 2015

stage1-implementors-guide.md: Add filesystem layout assumptions. #1832

Merged

steveej added needs/review and removed needs/some-changes labels Dec 16, 2015

iaguis reviewed Dec 17, 2015
View reviewed changes

GC: always detect and unmount leftovers

9f876b1

This is for unmounting recursive bind-mounts, as well as the overlay mounts. It also has the advantage of not attempting to unmount something that is not there, e.g. if a reboot happened and the mounts are gone anyway.

alban added a commit that referenced this pull request Dec 17, 2015

Merge pull request #1856 from steveeJ/gc-rework

d8a98bb

GC: always detect and unmount leftovers

alban merged commit d8a98bb into rkt:master Dec 17, 2015

steveej deleted the gc-rework branch December 17, 2015 14:36

steveej mentioned this pull request Dec 17, 2015

rkt fly status #1889

Open

12 tasks

alban mentioned this pull request Dec 28, 2015

Unable to GC some containers #1922

Open

alban removed the needs/review label Jan 18, 2016

euank mentioned this pull request Sep 9, 2016

"sharing" the rkt data directory across a shared bindmount breaks rkt rm #3181

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GC: always detect and unmount leftovers #1856

GC: always detect and unmount leftovers #1856

steveej commented Dec 9, 2015

iaguis Dec 9, 2015

alban Dec 9, 2015

iaguis Dec 10, 2015

steveej Dec 10, 2015

blixtra Dec 10, 2015

alban Dec 10, 2015

alban commented Dec 11, 2015

alban commented Dec 11, 2015

alban commented Dec 11, 2015

steveej commented Dec 11, 2015

xiang90 Dec 13, 2015

steveej Dec 16, 2015

alban Dec 16, 2015

alban commented Dec 14, 2015

alban Dec 14, 2015

alban Dec 14, 2015

alban commented Dec 14, 2015

steveej commented Dec 16, 2015

iaguis Dec 17, 2015

alban Dec 17, 2015

alban commented Dec 17, 2015

alban commented Dec 17, 2015

krnowak commented Dec 17, 2015

alban commented Dec 17, 2015

GC: always detect and unmount leftovers #1856

GC: always detect and unmount leftovers #1856

Conversation

steveej commented Dec 9, 2015

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

alban commented Dec 11, 2015

alban commented Dec 11, 2015

alban commented Dec 11, 2015

steveej commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented Dec 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented Dec 14, 2015

steveej commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented Dec 17, 2015

alban commented Dec 17, 2015

krnowak commented Dec 17, 2015

alban commented Dec 17, 2015