LCOW: Service VM lifetime changes by lowenna · Pull Request #33969 · moby/moby

lowenna · 2017-07-05T23:43:46Z

Signed-off-by: John Howard jhoward@microsoft.com

This changes the LCOW driver to support both global SVM lifetime and per-instance lifetime, and makes the driver thread-safe. It corrects a number of previously existing bugs. It also bumps to the latest version of jhowardmsft/opengcs.

Have validated basic CI internally here (building/pulling/committing/running/...) in both modes and validated there are no regressions from the previous behaviour

@johnstep PTAL.

lowenna · 2017-07-06T02:29:11Z

Fixed golint errors

lowenna · 2017-07-06T04:12:25Z

More golint fixes to follow tomorrow....

lowenna · 2017-07-06T15:56:12Z

go vet errors fixed. Should be green now in CI

vieux · 2017-07-07T15:24:19Z

ping @johnstep :D

johnstep · 2017-07-07T16:28:35Z

@vieux Sorry, I had been reviewing some separately with @jhowardmsft and we got some lock issues addressed. I will finish up the review soon; so far, so good.

johnstep

LGTM on green, after changes from external review; thanks @jhowardmsft for real-time review.

lowenna · 2017-07-07T20:45:31Z

Is green :)

lowenna · 2017-07-08T04:17:35Z

@thaJeztah Who else could review this? (Windows only code)

thaJeztah · 2017-07-08T05:15:26Z

I'll have a look if there's someone available; maybe @StefanScherer is also interested to have a look

lowenna · 2017-07-10T16:56:21Z

Rebased. @darrenstahlmsft Could you take a look?

darstahl

Not a maintainer, but LGTM other than one missing Unlock

darstahl · 2017-07-10T17:17:10Z

daemon/graphdriver/lcow/lcow.go

Debug log indicating mutex is being released, but unless I'm missing something it's still held.

Yes, good catch. Thanks. Updated.

darstahl · 2017-07-10T17:19:51Z

daemon/graphdriver/lcow/lcow.go

nit: This could be a defer unlock above

RE offline discussion, this is fine as is for now.

lowenna · 2017-07-12T01:18:44Z

@thaJeztah @vieux - OK with the above reviews with Windows-eyes on it? I need to build on this code base, hence would like it to get merged sooner, and similarly does Akash for the remote filesystem work. Thanks.

thaJeztah

Started reviewing, and left some comments

thaJeztah · 2017-07-13T03:32:16Z

daemon/graphdriver/lcow/lcow.go

Nit: would be good to have this function return a type; 4 return variables is hard to read

type layerDetails { fileName string fileSize int64 isSandbox bool }

Could even be (if additional details of the file are useful)

type layerDetails struct { fileInfo os.FileInfo isSandbox }

Not necessarily for this PR ^^

Yeah, let me do that in the current branch I'm working in for a future PR which builds on the lifetime changes in this PR. Will avoid a confusing rebase.

thaJeztah · 2017-07-13T04:04:41Z

daemon/graphdriver/lcow/lcow.go

If this is the package description, it should be at the top

Sure, will move to above package.

thaJeztah · 2017-07-13T04:06:02Z

daemon/graphdriver/lcow/lcow.go

Any reason for including this metadata here?

Before the likes of you start reading the comments too closely 🇬🇧

thaJeztah · 2017-07-13T04:07:02Z

daemon/graphdriver/lcow/lcow.go

Don't forget to update the documentation for this

actually, can we change this option to --storage-opt lcow.globalmode=1?

Storage-driver specific options use a <driver>. prefix (see https://github.com/docker/cli/blob/ff2552f7a1073834e79793aeb5dc5a4e486956a4/docs/reference/commandline/dockerd.md#options-per-storage-driver)

Sure will be in next update.

BTW - documentation is going to come later. Let's get it working first.... docs will be a combined effort which @PatrickLang will have to take on the majority of

thaJeztah · 2017-07-13T04:08:25Z

daemon/graphdriver/lcow/lcow.go

Should this be a tmpdir if it's for "scratch"?

See my comment 1 line higher....

thaJeztah · 2017-07-13T04:14:08Z

daemon/graphdriver/lcow/lcow.go

nit: isMounted perhaps? hasBeenMounted is easily mis-interpreted as "it was mounted at some point, but not anymore"

Sure. Updated

thaJeztah · 2017-07-13T06:07:58Z

daemon/graphdriver/lcow/lcow.go

Same here; actually, can you change this option to lcow.globalmode?

Storage-driver specific options use a <driver>. prefix (see https://github.com/docker/cli/blob/ff2552f7a1073834e79793aeb5dc5a4e486956a4/docs/reference/commandline/dockerd.md#options-per-storage-driver)

Also can you make this a boolean instead of 1 / 0, and check the value here? --storage-opt lcow.globalmode=false should disable it. This is important, because we use these inside the daemon.json as well (for which we use booleans)

Yup, lcow.globalmode

BTW - the statement about the prefix isn't universally true - see btrfs.go L569 for example.

Updated to true/false using strconv.ParseBool

Signed-off-by: John Howard <jhoward@microsoft.com> This changes the LCOW driver to support both global SVM lifetime and per-instance lifetime. It also corrects the scratch implementation.

lowenna · 2017-07-13T18:10:23Z

@thaJeztah Comments addressed, update pushed. Will address the return parameters in the follow-up PR once this is merged as mentioned ^^

lowenna · 2017-07-13T20:40:25Z

@thaJeztah and still green 💚

thaJeztah

discussing with @johnstep - we're good merging this; it's clearly WIP code, and this requires refactoring before it's reaching its final stage.

"lgtm"

thaJeztah · 2017-07-13T20:53:17Z

daemon/graphdriver/lcow/lcow.go

 		return err
 	}

-	return d.config.CreateSandbox(filepath.Join(d.dir(id), sandboxFilename), client.DefaultSandboxSizeMB, d.cachedSandboxFile)


any reason you changed this to if err := ... ; err != nil {?

thaJeztah · 2017-07-13T21:29:22Z

daemon/graphdriver/lcow/lcow.go

+	logrus.Debugf("%s: releasing cacheMutex. Ref count has dropped to zero", title)
+	d.cacheMutex.Unlock()
+
+	// To reach this point, the reference count has dropped to zero. If we have


This is confusing, because entry.refCount will never actually reach zero

I'll update the comment. To something more like there are no more references if we have reached this point. Is that what you mean?

Yes, or "don't" return after decrementing, but checking if referenceCount reaches 0. May want to combine that with the refactoring; for now, updating the comment should do

thaJeztah · 2017-07-13T21:52:51Z

daemon/graphdriver/lcow/lcow.go

-		logrus.Debugf("%s %s: refCount decremented to %d", title, id, entry.refCount)
-		d.cacheMu.Unlock()
+		logrus.Debugf("%s: releasing cache item for decrement and early get-out as refCount is now %d", title, entry.refCount)
+		entry.Unlock()


I realise there's a lot of debugging / WIP code in here still; but all the locking makes it quite easy to make slip-ups at some point.

Once we reach a bit more mature code; should we refactor, and separate out the whole cache into something more standalone?

// GetRefCount returns the reference count. func (e * cacheEntry) GetRefCount() int { e.Lock() defer e.Unlock() return e.refCount } // GetRefCount decrements and returns the reference count. func (e * cacheEntry) DecrementRefCount() int { e.Lock() defer e.Unlock() if e.refCount > 0 { e.refCount-- } return e.refCount }

That's fair. Yes, the debugging stuff is deliberately paranoid right now. Once it's stable, I can make changes such as this.

Signed-off-by: John Howard <jhoward@microsoft.com> This changes the graphdriver to perform dynamic sandbox management. Previously, as a temporary 'hack', the service VM had a prebuilt sandbox in it. With this change, management is under the control of the client (docker) and executes a mkfs.ext4 on it. This enables sandboxes of non-default sizes too (a TODO previously in the code). It also addresses moby#33969 (comment) Requires: - go-winio: v0.4.3 - opengcs: v0.0.10 - hcsshim: v0.5.28

Signed-off-by: John Howard <jhoward@microsoft.com> This changes the graphdriver to perform dynamic sandbox management. Previously, as a temporary 'hack', the service VM had a prebuilt sandbox in it. With this change, management is under the control of the client (docker) and executes a mkfs.ext4 on it. This enables sandboxes of non-default sizes too (a TODO previously in the code). It also addresses moby#33969 (comment) Requires: - go-winio: v0.4.3 - opengcs: v0.0.12 - hcsshim: v0.6.x

Signed-off-by: John Howard <jhoward@microsoft.com> This changes the graphdriver to perform dynamic sandbox management. Previously, as a temporary 'hack', the service VM had a prebuilt sandbox in it. With this change, management is under the control of the client (docker) and executes a mkfs.ext4 on it. This enables sandboxes of non-default sizes too (a TODO previously in the code). It also addresses moby/moby#33969 (comment) Requires: - go-winio: v0.4.3 - opengcs: v0.0.12 - hcsshim: v0.6.x Upstream-commit: 8c279ef Component: engine

GordonTheTurtle added the status/0-triage label Jul 5, 2017

lowenna force-pushed the jjh/lifetime branch from e645314 to 399aec6 Compare July 6, 2017 02:28

lowenna added status/2-code-review and removed status/0-triage labels Jul 6, 2017

lowenna force-pushed the jjh/lifetime branch 5 times, most recently from 4917dfa to d1d1a76 Compare July 6, 2017 20:02

lowenna force-pushed the jjh/lifetime branch 2 times, most recently from 71278e8 to bb64e86 Compare July 7, 2017 18:16

johnstep approved these changes Jul 7, 2017

View reviewed changes

lowenna force-pushed the jjh/lifetime branch from bb64e86 to 77e0980 Compare July 10, 2017 16:56

darstahl reviewed Jul 10, 2017

View reviewed changes

lowenna force-pushed the jjh/lifetime branch from 77e0980 to 965a31b Compare July 10, 2017 18:47

darstahl approved these changes Jul 10, 2017

View reviewed changes

PatrickLang mentioned this pull request Jul 11, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

thaJeztah requested changes Jul 13, 2017

View reviewed changes

LCOW: Service VM lifetime changes

76b0d48

Signed-off-by: John Howard <jhoward@microsoft.com> This changes the LCOW driver to support both global SVM lifetime and per-instance lifetime. It also corrects the scratch implementation.

lowenna force-pushed the jjh/lifetime branch from 965a31b to 76b0d48 Compare July 13, 2017 18:09

thaJeztah approved these changes Jul 13, 2017

View reviewed changes

thaJeztah merged commit f22cecf into moby:master Jul 13, 2017

lowenna mentioned this pull request Jul 18, 2017

LCOW: Dynamic sandbox management #34170

Merged

thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018

Conversation

lowenna commented Jul 5, 2017

Uh oh!

lowenna commented Jul 6, 2017

Uh oh!

lowenna commented Jul 6, 2017

Uh oh!

lowenna commented Jul 6, 2017

Uh oh!

vieux commented Jul 7, 2017

Uh oh!

johnstep commented Jul 7, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Jul 7, 2017

Uh oh!

lowenna commented Jul 8, 2017

Uh oh!

thaJeztah commented Jul 8, 2017

Uh oh!

lowenna commented Jul 10, 2017

Uh oh!

darstahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Jul 12, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!