Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine #2298

skatsuta · 2015-11-14T15:50:50Z

close #1790

This PR will

check the version of the local ISO cache and the latest release tag, then upgrade the ISO if possible when creating a VirtualBox machine from boot2docker.iso.
add many unit tests for B2dUtils's methods

Signed-off-by: Soshi Katsuta soshi.katsuta@gmail.com

dmp42 · 2015-11-16T08:14:24Z

cc @docker/machine-maintainers

jeanlaurent · 2015-11-16T14:26:11Z

Could probably help #2248

nathanleclaire · 2015-11-16T18:03:52Z

Very nice. Will review this soon.

nathanleclaire · 2015-11-17T02:41:50Z

libmachine/mcnutils/b2d.go

+
+	// offset and length of ISO volume ID
+	// cf. http://serverfault.com/questions/361474/is-there-a-way-to-change-a-iso-files-volume-id-from-the-command-line
+	volumeIDOffset int64


Why not just make these package-level const instead? They are unlikely to change.

@nathanleclaire Actually I did at first, but I added these fields for testing.
0x8028 is too big and I want to set the offset as a smaller value in testing.
Any idea for cooler way? Maybe interface here too?

nathanleclaire · 2015-11-17T02:53:37Z

Instead of spinning up a whole server each time you need one, why not move the bits that you need to "mock" out into an embedded interface instead? I worry that spinning up fake API servers like that for each bit will become a big hairy mess to maintain, and also is more up the alley of "integration test" than "unit test".

nathanleclaire · 2015-11-17T02:57:20Z

libmachine/mcnutils/b2d.go

+}
+
+// pathExists reports whether a file or directory specified by path exists.
+func pathExists(path string) bool {


For instance, this could become a method on an interface and embedded into the B2DUtils struct:

type PathChecker interface { PathExists(string) bool } type StatPathChecker struct {} func (spc *StatPathChecker) PathExists(path string) bool { _, err := os.Stat(path) return os.IsNotExist(err) } type B2DUtils struct { StatPathChecker ... }

Likewise, GetLatestBoot2DockerReleaseURL and other methods that contact the GitHub API could be turned into interfaces and embedded:

type LatestReleaseGetter interface { GetLatestBoot2DockerReleaseURL(string) (string, error) } type GithubLatestReleaseGetter struct {} func (g GithubLatestReleaseGetter) GetLatestBoot2DockerReleaseURL(url string) (string, error) { .... } type B2DUtils struct { LatestReleaseGetter ... }

Then it could be mocked out in a struct for the tests:

type FakeLatestReleaseGetter struct {} func (g FakeLatestReleaseGetter) GetLatestBoot2DockerReleaseURL(url string) (string, error) { return "v1.9.0", nil // or however you wish to configure the behavior }

@nathanleclaire Useful sample code! Thanks!

Hmm... Current GetLatestBoot2DockerReleaseURL() heavily relies on B2dUtils.
If I delegate its implementation to GitHubLatestReleaseGetter, I have to rethink an implementation of LatestReleaseGetter interface...

Yep, sounds like you're on exactly the right track. You notice that GetLatestBoot2DockerReleaseURL relies on getReleaseTag. So, you will actually have to make that method as an interface of LatestReleaseGetter with getReleaseTag method instead. This will enable you to embed it in the next interface up and mock that out completely in the bits which rely on it. It will require some jiggery-pokery due to all the passing of API URLs and so on that we're doing here, but doesn't have to be perfect on the first iteration.

nathanleclaire · 2015-11-17T03:06:33Z

Have posted some feedback, mostly that I would prefer to use interfaces to test instead of spinning up whole web servers in the tests (we're long past due for implementing this). Looking pretty decent so far though 👍

skatsuta · 2015-11-17T07:54:29Z

@nathanleclaire Thank you for your kind review!
I should rewrite them to use interfaces and mocks more.

nathanleclaire · 2015-11-17T22:45:59Z

Good times. Make sure to ping us (@docker/machine-maintainers) when you've done some work in that direction.

nathanleclaire · 2015-11-23T20:38:50Z

Any update on this @skatsuta ?

skatsuta · 2015-11-29T06:29:29Z

@nathanleclaire Sorry for the late fix. Now I'm done. Please re-review it.

Changelog

define releaseGetter interface and B2dUtils holds b2dReleaseGetter that implements releaseGetter
define iso interface and B2dUtils holds b2dISO that implements iso
use mockReleaseGetter and mockISO in testing

nathanleclaire · 2015-11-30T21:59:15Z

test/integration/drivers/virtualbox/create.bats

@@ -0,0 +1,27 @@
+#!/usr/bin/env bats
+
+load ${BASE_TEST_DIR}/helpers.bash


Why not give this test file a more descriptive name? create.bats doesn't tell me much about what it's intended to test.

nathanleclaire · 2015-12-03T02:09:48Z

Reviewed this and everything looks solid. Even works without network access (defaults to the cached version). Great work @skatsuta.

LGTM

cc @docker/machine-maintainers

nathanleclaire · 2015-12-03T02:10:24Z

We can follow-up after merge to rename the test file to something more specific.

skatsuta · 2015-12-03T04:07:11Z

Very sorry I seemed to make a mistake to merge and rebase the upstream.
I'll fix soon. Please wait to merge.

skatsuta · 2015-12-03T04:38:08Z

OK fixed and renamed create.bats to create-with-upgrading.bats.
You can change it to whatever more suitable.

nathanleclaire · 2015-12-03T19:23:31Z

OK thanks @skatsuta. @dgageot @jeanlaurent please take a look at this when you get a chance.

dgageot · 2015-12-07T16:40:00Z

Hi @skatsuta Each time I upgrade a VBox VM that has docker 1.9.1 installed, b2d 1.9.1 is downloaded again even if it is already in my cache folder. Isn't it unexpected?

$ file ~/.docker/machine/cache/boot2docker.iso
/Users/dgageot/.docker/machine/cache/boot2docker.iso: ISO 9660 CD-ROM filesystem data 'Boot2Docker-v1.9.1             ' (bootable)

$ /bin/docker-machine upgrade default
Detecting the provisioner...
Upgrading docker...
Downloading latest boot2docker iso...
Latest release for github.com/boot2docker/boot2docker is v1.9.1
Downloading https://github.com/boot2docker/boot2docker/releases/download/v1.9.1/boot2docker.iso to /Users/dgageot/.docker/machine/cache/boot2docker.iso...
0%....10%....20%....30%....40%....50%....60%....70%....80%....90%....100%
Stopping machine to do the upgrade...
(default) Stopping VM...
Upgrading machine "default"...
Latest Boot2Docker ISO found locally, copying it to /Users/dgageot/.docker/machine/machines/default/boot2docker.iso...
Starting machine back up...
(default) Starting VM...
Restarting docker...

dgageot · 2015-12-07T16:42:21Z

Also, when the cache folder does not exist, the upgrade fails.

$ rm -Rf ~/.docker/machine/cache

$ ./bin/docker-machine upgrade default
Detecting the provisioner...
Upgrading docker...
Downloading latest boot2docker iso...
Latest release for github.com/boot2docker/boot2docker is v1.9.1
Downloading https://github.com/boot2docker/boot2docker/releases/download/v1.9.1/boot2docker.iso to /Users/dgageot/.docker/machine/cache/boot2docker.iso...

open /Users/dgageot/.docker/machine/cache/boot2docker.iso.tmp212374960: no such file or directory

nathanleclaire · 2015-12-10T23:08:07Z

Nice catches @dgageot . @skatsuta do you have time to work on this? Or maybe one of us could take a look at it?

… latest one when it is out-of-date Signed-off-by: Soshi Katsuta <soshi.katsuta@gmail.com>

skatsuta · 2015-12-11T14:16:20Z

@nathanleclaire @dgageot
Thank you for your reviews and comments!
To fix @dgageot's issues I removed unnecessary explicit downloading process in Boot2DockerProvisioner#upgradeIso(). This solved the both problems.

nathanleclaire · 2015-12-12T01:49:51Z

Thanks @skatsuta. This PR LGTM. @dgageot @jeanlaurent at your discretion whether you'd like to merge in for the 0.5.3 release. I'm in favor but don't feel like there's any hurry.

nathanleclaire · 2015-12-16T23:50:25Z

@dgageot @jeanlaurent PTAL at this when you get a chance.

dgageot · 2015-12-18T07:39:20Z

LGTM

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine

GordonTheTurtle added the status/0-triage label Nov 14, 2015

skatsuta force-pushed the create-update-iso branch 2 times, most recently from a4d1b80 to 711e1c4 Compare November 15, 2015 06:46

skatsuta changed the title ~~Upgrade an ISO cache if possible when creating a VirtualBox machine~~ Upgrade the ISO cache if possible when creating a VirtualBox machine Nov 16, 2015

skatsuta changed the title ~~Upgrade the ISO cache if possible when creating a VirtualBox machine~~ Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine Nov 16, 2015

dmp42 added status/2-code-review and removed status/0-triage labels Nov 16, 2015

jeanlaurent added the driver/virtualbox label Nov 16, 2015

skatsuta force-pushed the create-update-iso branch from 711e1c4 to 9cd51c2 Compare November 17, 2015 01:38

nathanleclaire added this to the 0.5.2 milestone Nov 17, 2015

nathanleclaire self-assigned this Nov 17, 2015

nathanleclaire reviewed Nov 17, 2015
View reviewed changes

nathanleclaire modified the milestones: 0.5.3, 0.5.2 Nov 25, 2015

skatsuta force-pushed the create-update-iso branch 2 times, most recently from e636400 to a361636 Compare November 29, 2015 06:09

skatsuta force-pushed the create-update-iso branch 2 times, most recently from e53b0d9 to 9e3af3c Compare November 29, 2015 06:46

nathanleclaire reviewed Nov 30, 2015
View reviewed changes

skatsuta force-pushed the create-update-iso branch 2 times, most recently from c068697 to bc15894 Compare December 2, 2015 02:25

skatsuta force-pushed the create-update-iso branch 2 times, most recently from 6f3a746 to fa69652 Compare December 3, 2015 04:33

skatsuta force-pushed the create-update-iso branch from fa69652 to 9c9a334 Compare December 3, 2015 05:05

libmachine/mcnutils: check the version of cached iso and download the…

bbe7a00

… latest one when it is out-of-date Signed-off-by: Soshi Katsuta <soshi.katsuta@gmail.com>

skatsuta force-pushed the create-update-iso branch from 9c9a334 to bbe7a00 Compare December 11, 2015 14:04

dgageot modified the milestones: 0.5.3, 0.6.0 Dec 14, 2015

dgageot added a commit that referenced this pull request Dec 18, 2015

Merge pull request #2298 from skatsuta/create-update-iso

0a74068

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine

dgageot merged commit 0a74068 into docker:master Dec 18, 2015

dgageot added area/b2d driver/hyperv labels Dec 30, 2015

dgageot mentioned this pull request Dec 30, 2015

Need a mechanism to update boot2docker.iso image #1285

Closed

skatsuta deleted the create-update-iso branch January 29, 2016 14:36

batsatt mentioned this pull request Feb 18, 2016

boot2docker.iso auto upgrade should be constrained to same docker major version #3084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine #2298

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine #2298

skatsuta commented Nov 14, 2015

dmp42 commented Nov 16, 2015

jeanlaurent commented Nov 16, 2015

nathanleclaire commented Nov 16, 2015

nathanleclaire Nov 17, 2015

skatsuta Nov 17, 2015

nathanleclaire commented Nov 17, 2015

nathanleclaire Nov 17, 2015

nathanleclaire Nov 17, 2015

skatsuta Nov 17, 2015

skatsuta Nov 17, 2015

nathanleclaire Nov 17, 2015

nathanleclaire commented Nov 17, 2015

skatsuta commented Nov 17, 2015

nathanleclaire commented Nov 17, 2015

nathanleclaire commented Nov 23, 2015

skatsuta commented Nov 29, 2015

nathanleclaire Nov 30, 2015

nathanleclaire commented Dec 3, 2015

nathanleclaire commented Dec 3, 2015

skatsuta commented Dec 3, 2015

skatsuta commented Dec 3, 2015

nathanleclaire commented Dec 3, 2015

dgageot commented Dec 7, 2015

dgageot commented Dec 7, 2015

nathanleclaire commented Dec 10, 2015

skatsuta commented Dec 11, 2015

nathanleclaire commented Dec 12, 2015

nathanleclaire commented Dec 16, 2015

dgageot commented Dec 18, 2015

		@@ -0,0 +1,27 @@
		#!/usr/bin/env bats

		load ${BASE_TEST_DIR}/helpers.bash

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine #2298

Upgrade the boot2docker.iso cache if possible when creating a VirtualBox machine #2298

Conversation

skatsuta commented Nov 14, 2015

dmp42 commented Nov 16, 2015

jeanlaurent commented Nov 16, 2015

nathanleclaire commented Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanleclaire commented Nov 17, 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

nathanleclaire commented Nov 17, 2015

skatsuta commented Nov 17, 2015

nathanleclaire commented Nov 17, 2015

nathanleclaire commented Nov 23, 2015

skatsuta commented Nov 29, 2015

Changelog

Choose a reason for hiding this comment

nathanleclaire commented Dec 3, 2015

nathanleclaire commented Dec 3, 2015

skatsuta commented Dec 3, 2015

skatsuta commented Dec 3, 2015

nathanleclaire commented Dec 3, 2015

dgageot commented Dec 7, 2015

dgageot commented Dec 7, 2015

nathanleclaire commented Dec 10, 2015

skatsuta commented Dec 11, 2015

nathanleclaire commented Dec 12, 2015

nathanleclaire commented Dec 16, 2015

dgageot commented Dec 18, 2015