New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LCOW: Dynamic sandbox management #34170

Merged
merged 3 commits into from Aug 3, 2017

Conversation

Projects
None yet
8 participants
@jhowardmsft
Contributor

jhowardmsft commented Jul 18, 2017

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?

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft
Contributor

jhowardmsft commented Jul 18, 2017

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jul 18, 2017

Contributor

And @gupta-ak FYI too.

Contributor

jhowardmsft commented Jul 18, 2017

And @gupta-ak FYI too.

// -- 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

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jul 19, 2017

Member

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

@AkihiroSuda

AkihiroSuda Jul 19, 2017

Member

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

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 19, 2017

Contributor

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

@jhowardmsft

jhowardmsft Jul 19, 2017

Contributor

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

@PatrickLang

This comment has been minimized.

Show comment
Hide comment
@PatrickLang

PatrickLang Jul 24, 2017

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

PatrickLang commented Jul 24, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jul 25, 2017

Contributor

@johnstep do you have time to look? Thx

Contributor

jhowardmsft commented Jul 25, 2017

@johnstep do you have time to look? Thx

@darstahl

This comment has been minimized.

Show comment
Hide comment
@darstahl

darstahl Jul 25, 2017

Contributor

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

Contributor

darstahl commented Jul 25, 2017

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.

Show outdated Hide outdated daemon/graphdriver/lcow/lcow.go Outdated
ci.refCount++
logrus.Debugf("incremented refcount on cache item %+v", ci)
}

This comment has been minimized.

@johnstep

johnstep Jul 28, 2017

Contributor

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
	}
@johnstep

johnstep Jul 28, 2017

Contributor

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
	}

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

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.

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

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.

This comment has been minimized.

@johnstep

johnstep Jul 31, 2017

Contributor

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

johnstep Jul 31, 2017

Contributor

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. :)

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

Done

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

Done

Show outdated Hide outdated daemon/graphdriver/lcow/lcow.go Outdated
Show outdated Hide outdated daemon/graphdriver/lcow/lcow.go Outdated
logrus.Debugf("%s (%s): creating an SVM scratch - locking serviceVM", title, context)
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 {

This comment has been minimized.

@johnstep

johnstep Jul 28, 2017

Contributor

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

@johnstep

johnstep Jul 28, 2017

Contributor

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

This comment has been minimized.

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

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?

@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

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?

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

@johnstep - Update pushed with comments addressed.

Contributor

jhowardmsft commented Jul 31, 2017

@johnstep - Update pushed with comments addressed.

@johnstep

LGTM

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jul 31, 2017

Contributor

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

Contributor

jhowardmsft commented Jul 31, 2017

@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.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 1, 2017

Contributor

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

Contributor

jhowardmsft commented Aug 1, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 2, 2017

Contributor

#34272 is merged. Vendoring updated to match.

Contributor

jhowardmsft commented Aug 2, 2017

#34272 is merged. Vendoring updated to match.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 3, 2017

Contributor

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

Contributor

jhowardmsft commented Aug 3, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 3, 2017

Contributor

@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

Contributor

jhowardmsft commented Aug 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Aug 3, 2017

Contributor

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

Contributor

mlaventure commented Aug 3, 2017

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

jhowardmsft added some commits Jul 19, 2017

Revendor microsoft/go-winio @ v0.4.4
Signed-off-by: John Howard <jhoward@microsoft.com>
LCOW: Graphdriver dynamic sandbox management
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)

Requires:
- go-winio: v0.4.3
- opengcs:  v0.0.12
- hcsshim:  v0.6.x
Revendor jhowardmsft/opengcs @ v0.0.12
Signed-off-by: John Howard <jhoward@microsoft.com>
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 3, 2017

Contributor

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

Contributor

jhowardmsft commented Aug 3, 2017

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

@mlaventure

LGTM

Anyone seeing this green on Windows can just merge :)

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 3, 2017

Contributor

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).

Contributor

jhowardmsft commented Aug 3, 2017

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).

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

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36069 has succeeded
Details
janky Jenkins build Docker-PRs 44686 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5069 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3658 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16076 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4797 has succeeded
Details

@jhowardmsft jhowardmsft deleted the Microsoft:jjh/sandbox branch Aug 3, 2017

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