LCOW: Dynamic sandbox management by lowenna · Pull Request #34170 · moby/moby

lowenna · 2017-07-18T21:49:21Z

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 #33969 (comment) from @thaJeztah

Requires:
- go-winio: v0.4.3 (v0.4.4 is vendored)
- opengcs: v0.0.12
- hcsshim: v0.6.x (merged as part of 34272 so not in this PR)

@johnstep PTAL. Note, you won't be able to use this on current builds you have, as to create the sandbox and scratch space blank VHDX's in the cache, it relies on HCS APIs which aren't present in your builds. You can temporarily work around this by keeping sandbox.vhdx and scratch.vhdx from \lcow\lcow\cache safe somewhere, and copying them manually so that they are in place.

@darrenstahlmsft Can you track/address feedback comments while I'm on vacation?

lowenna · 2017-07-18T21:49:31Z

@PatrickLang FYI.

lowenna · 2017-07-18T21:49:49Z

And @gupta-ak FYI too.

AkihiroSuda · 2017-07-19T02:57:25Z

daemon/graphdriver/lcow/lcow.go

+//        -- Possible values:      Any local path that is not a mapped drive
+//        -- Default if ommitted:  %ProgramFiles%\Linux Containers
+//
+//   * lcow.kernel - Specifies a custom kernel file located in the `lcow.kirdpath` path


nit: it maybe good to mention that a custom kernel needs to be embedded in UEFI boot program

That information is in the build instructions in the Microsoft/opengcs repo. I don't think it belongs here.

PatrickLang · 2017-07-24T22:20:00Z

Just to clarify - this isn't testable on Windows builds 16237 or 16241. Will be in a later build

lowenna · 2017-07-25T04:49:43Z

@johnstep do you have time to look? Thx

darstahl · 2017-07-25T22:13:24Z

This all LGTM, but I'm not a maintainer. I'll address any comments while John is on vacation.

johnstep

Looks pretty good, just a couple small things.

johnstep · 2017-07-28T17:19:17Z

daemon/graphdriver/lcow/lcow.go

+//
+//   * lcow.vhdx - Specifies a custom vhdx file to boot (instead of a kernel+initrd)
+//        -- Possible values:      Any valid filename
+//        -- Default if ommitted:  C:\Program Files\Linux Containers\uvm.vhdx


Nit: This default is really %ProgramFiles%\Linux Containers\uvm.vhdx, but, more precisely, it's uvm.vhdx under lcow.kirdpath.

Sure, that'll be fixed in the next push.

johnstep · 2017-07-28T17:36:33Z

daemon/graphdriver/lcow/lcow.go

+	ci.refCount++
+	logrus.Debugf("incremented refcount on cache item %+v", ci)
+}
+


How about a corresponding decrementRefCount for Put, replacing the following code:

// Are we just decrementing the reference count? logrus.Debugf("%s: locking cache item for possible decrement", title) entry.Lock() if entry.refCount > 1 { entry.refCount-- logrus.Debugf("%s: releasing cache item for decrement and early get-out as refCount is now %d", title, entry.refCount) entry.Unlock() logrus.Debugf("%s: refCount decremented to %d. Releasing cacheMutex", title, entry.refCount) d.cacheMutex.Unlock() return nil } logrus.Debugf("%s: releasing cache item", title) entry.Unlock()

...with something like this:

// Are we just decrementing the reference count? if entry.decrementRefCount() > 0 { d.cacheMutex.Unlock() return nil }

I didn't do that deliberately. It's currently a conditional decrement rather than absolute decrement - it just seemed clearer the way it currently is. IMO.

Fair enough, and slightly more efficient, but my proposal seems easier to read, providing symmetry to the increment function. It doesn't matter if the condition is checked before or after, and decrementing makes the debug log accurate about reaching zero. :)

johnstep · 2017-07-28T17:49:27Z

daemon/graphdriver/lcow/lcow.go

 		}
 		d.cache[id] = cacheEntry
-		logrus.Debugf("%s: added cache entry %+v", title, cacheEntry)
+		logrus.Debugf("%s: added cache item %+v", title, cacheEntry)


Are you not renaming variables from entry (and cacheEntry) to item (or ci) to simplify this change?

Yeah, I missed a few. Fixing them up :)

johnstep · 2017-07-28T17:57:04Z

daemon/graphdriver/lcow/lcow.go

+		if fileInfo, err = os.Stat(ld.filename); err != nil {
 			if os.IsNotExist(err) {
-				return "", 0, isSandbox, fmt.Errorf("could not find layer or sandbox in %s", folder)
+				return nil, fmt.Errorf("could not find layer or sandbox in %s", folder)


Nit: Minor, and not a regression, but if Stat fails for a layer, but the file exists, this message is technically inaccurate. However, that seems like an unlikely scenario. I'm not sure this existence check is worthwhile.

That's fair. Fixed.

johnstep · 2017-07-28T18:27:12Z

daemon/graphdriver/lcow/lcow.go

 		svm.Lock()
-		if err := svm.config.CreateSandbox(d.cachedScratchFile, client.DefaultSandboxSizeMB, d.cachedSandboxFile); err != nil {
-			logrus.Debugf("%s (%s): releasing serviceVM on error path", title, context)
+		if err := svm.config.CreateExt4Vhdx(scratchTargetFile, client.DefaultVhdxSizeGB, d.cachedScratchFile); err != nil {


Is the last parameter supposed to be d.cachedSandboxFile, like it was before? This is a little confusing to me.

Ah, no this is correct. First parameter is the target (which in this case the scratch we're trying to create). Last parameter is both the source and target of a cached file - if it exists, it's a straight CopyFileW from the cache to target file; if it doesn't then first the target file gets created, then it's cached to the cache location. Make sense?

lowenna · 2017-07-31T15:37:26Z

@johnstep - Update pushed with comments addressed.

johnstep

LGTM

lowenna · 2017-07-31T17:26:46Z

@thaJeztah PTAL. Addressed John's comments (and also fixed your decrement/increment comment from the previous PR, with helper functions).

johnstep

The update looks good.

lowenna · 2017-08-01T01:25:39Z

Note: If #34272 gets merged first, I'll need to update the vendoring on this to reflect versions with lowercase s for Sirupsen.

lowenna · 2017-08-02T02:15:21Z

#34272 is merged. Vendoring updated to match.

lowenna · 2017-08-03T00:58:36Z

Ugh. z is sooooooo flaky. Restarting. 4th time lucky 😭

lowenna · 2017-08-03T01:39:34Z

@mlaventure @thaJeztah @vdemeester Do you guys have time to look at this? There's a few other LCOW related PRs not yet submitted which are backed up behind this being merged. On the bright side, it only affects LCOW code, so the risk of regressing other functionality (it doesn't.....) is as close as it gets to zero. It's also already had some pretty thorough reviews from @johnstep & @darrenstahlmsft

mlaventure · 2017-08-03T14:45:44Z

@jhowardmsft LGTM, but could you switch to go-winio v0.4.4 ? It fixes a couple of possible deadlock/race conditions

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

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

lowenna · 2017-08-03T16:08:52Z

@mlaventure Yes, that's bumped to go-winio v0.4.4 now

mlaventure

LGTM

Anyone seeing this green on Windows can just merge :)

lowenna · 2017-08-03T21:36:06Z

Darn z. I've kicked it off for the 3rd attempt at https://jenkins.dockerproject.org/job/Docker-PRs-s390x/4797/console (not showing below for some reason).

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

lowenna mentioned this pull request Jul 18, 2017

Remove createSandbox utility microsoft/opengcs#41

Merged

lowenna force-pushed the jjh/sandbox branch 4 times, most recently from 64ef85f to 5b5b641 Compare July 18, 2017 23:47

lowenna added status/needs-vendoring and removed status/0-triage labels Jul 19, 2017

lowenna force-pushed the jjh/sandbox branch from 07da5cc to 02bd02c Compare July 19, 2017 02:32

lowenna added status/2-code-review and removed status/needs-vendoring labels Jul 19, 2017

lowenna requested review from johnstep and thaJeztah July 19, 2017 02:33

AkihiroSuda reviewed Jul 19, 2017

View reviewed changes

lowenna force-pushed the jjh/sandbox branch from 02bd02c to 58f5900 Compare July 19, 2017 03:44

johnstep suggested changes Jul 28, 2017

View reviewed changes

lowenna force-pushed the jjh/sandbox branch from 58f5900 to 78440ca Compare July 31, 2017 15:37

johnstep approved these changes Jul 31, 2017

View reviewed changes

lowenna force-pushed the jjh/sandbox branch from 78440ca to f9af62d Compare July 31, 2017 17:25

johnstep approved these changes Jul 31, 2017

View reviewed changes

GordonTheTurtle assigned stevvooe Aug 2, 2017

lowenna force-pushed the jjh/sandbox branch from f9af62d to 6829cff Compare August 2, 2017 02:14

lowenna unassigned stevvooe Aug 2, 2017

PatrickLang mentioned this pull request Aug 2, 2017

Epic: Linux Containers on Windows (LCOW) #33850

Closed

GordonTheTurtle assigned mlaventure Aug 3, 2017

John Howard added 3 commits August 3, 2017 09:06

Revendor microsoft/go-winio @ v0.4.4

45e2dd1

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

Revendor jhowardmsft/opengcs @ v0.0.12

0bd1cf2

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

lowenna force-pushed the jjh/sandbox branch from 6829cff to 8c279ef Compare August 3, 2017 16:07

mlaventure approved these changes Aug 3, 2017

View reviewed changes

johnstep merged commit a3ffc42 into moby:master Aug 3, 2017

lowenna deleted the jjh/sandbox branch August 3, 2017 23:08

gupta-ak mentioned this pull request Aug 4, 2017

LCOW: Support for docker cp, ADD/COPY on build #34252

Merged

thaJeztah mentioned this pull request Mar 15, 2018

Update vendored packages, containerd, docker moby/swarmkit#2560

Merged

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

Conversation

lowenna commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna commented Jul 18, 2017

Uh oh!

lowenna commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PatrickLang commented Jul 24, 2017

Uh oh!

lowenna commented Jul 25, 2017

Uh oh!

darstahl commented Jul 25, 2017

Uh oh!

johnstep 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!

johnstep Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lowenna commented Jul 31, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Jul 31, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Aug 1, 2017

Uh oh!

lowenna commented Aug 2, 2017

Uh oh!

lowenna commented Aug 3, 2017

Uh oh!

lowenna commented Aug 3, 2017

Uh oh!

mlaventure commented Aug 3, 2017

Uh oh!

lowenna commented Aug 3, 2017

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lowenna commented Jul 18, 2017 •

edited

Loading

johnstep Jul 31, 2017 •

edited

Loading

lowenna commented Aug 3, 2017 •

edited

Loading