Merge lp:~axwalk/juju-core/instances-arch-preference into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Fix arch preference algorithm

Change the instance architecture preference
selection to widest word-size, then first
arch name by alphabetical order. For tie-
breakers, choose the instance type based on
provider-specific ordering.

In the future we may want to extend the
ordering to include cost. For now, we rely
on the providers ordering instance types by
their own preference (no change here).

https://codereview.appspot.com/94530045/

Fix arch preference algorithm

Change the instance architecture preference
selection to widest word-size, then first
arch name by alphabetical order. For tie-
breakers, choose the instance type based on
provider-specific ordering.

In the future we may want to extend the
ordering to include cost. For now, we rely
on the providers ordering instance types by
their own preference (no change here).

https://codereview.appspot.com/94530045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+219839_code.launchpad.net,

Message:
Please take a look.

Description:
Fix arch preference algorithm

Change the instance architecture preference
selection to widest word-size, then first
arch name by alphabetical order. For tie-
breakers, choose the instance type based on
provider-specific ordering.

In the future we may want to extend the
ordering to include cost. For now, we rely
on the providers ordering instance types by
their own preference (no change here).

https://code.launchpad.net/~axwalk/juju-core/instances-arch-preference/+merge/219839

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/94530045/

Affected files (+76, -58 lines):
   A [revision details]
   M environs/instances/image.go
   M environs/instances/image_test.go
   M juju/arch/arch.go
   M juju/arch/arch_test.go

[William Reade (fwereade)]

I feel like we're missing some tests here. Otherwise looking good.

https://codereview.appspot.com/94530045/diff/1/environs/instances/image_test.go
File environs/instances/image_test.go (right):

https://codereview.appspot.com/94530045/diff/1/environs/instances/image_test.go#newcode194
environs/instances/image_test.go:194: },
Could use a few more tests here, I think.

https://codereview.appspot.com/94530045/diff/1/juju/arch/arch.go
File juju/arch/arch.go (right):

https://codereview.appspot.com/94530045/diff/1/juju/arch/arch.go#newcode36
juju/arch/arch.go:36: PPC64: {64},
Nice, thanks for this.

https://codereview.appspot.com/94530045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/94530045/diff/1/environs/instances/image_test.go
File environs/instances/image_test.go (right):

https://codereview.appspot.com/94530045/diff/1/environs/instances/image_test.go#newcode194
environs/instances/image_test.go:194: },
On 2014/05/16 21:12:38, fwereade wrote:
> Could use a few more tests here, I think.

Done.

https://codereview.appspot.com/94530045/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/94530045/

=== modified file 'environs/instances/image.go'
--- environs/instances/image.go	2014-05-09 13:24:50 +0000
+++ environs/instances/image.go	2014-05-19 03:30:40 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"fmt"
+	"sort"
 
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/imagemetadata"
@@ -44,6 +45,8 @@
 type InstanceSpec struct {
 	InstanceType InstanceType
 	Image        Image
+	// order is used to sort InstanceSpec based on the input InstanceTypes.
+	order int
 }
 
 // FindInstanceSpec returns an InstanceSpec satisfying the supplied InstanceConstraint.
@@ -83,13 +86,17 @@
 	for _, itype := range matchingTypes {
 		for _, image := range possibleImages {
 			if image.match(itype) {
-				specs = append(specs, &InstanceSpec{itype, image})
+				specs = append(specs, &InstanceSpec{
+					InstanceType: itype,
+					Image:        image,
+					order:        len(specs),
+				})
 			}
 		}
 	}
-
-	if spec := preferredSpec(specs); spec != nil {
-		return spec, nil
+	if len(specs) > 0 {
+		sort.Sort(byArch(specs))
+		return specs[0], nil
 	}
 
 	names := make([]string, len(matchingTypes))
@@ -99,21 +106,39 @@
 	return nil, fmt.Errorf("no %q images in %s matching instance types %v", ic.Series, ic.Region, names)
 }
 
-// preferredSpec will if possible return a spec with arch matching that
-// of the host machine.
-func preferredSpec(specs []*InstanceSpec) *InstanceSpec {
-	if len(specs) > 1 {
-		hostArch := arch.HostArch()
-		for _, spec := range specs {
-			if spec.Image.Arch == hostArch {
-				return spec
-			}
-		}
-	}
-	if len(specs) > 0 {
-		return specs[0]
-	}
-	return nil
+// byArch sorts InstanceSpecs first by descending word-size, then
+// alphabetically by name, and choose the first spec in the sequence.
+type byArch []*InstanceSpec
+
+func (a byArch) Len() int {
+	return len(a)
+}
+
+func (a byArch) Less(i, j int) bool {
+	iArchName := a[i].Image.Arch
+	jArchName := a[j].Image.Arch
+	iArch := arch.Info[iArchName]
+	jArch := arch.Info[jArchName]
+	// Wider word-size first.
+	switch {
+	case iArch.WordSize > jArch.WordSize:
+		return true
+	case iArch.WordSize < jArch.WordSize:
+		return false
+	}
+	// Alphabetically by arch name.
+	switch {
+	case iArchName < jArchName:
+		return true
+	case iArchName > jArchName:
+		return false
+	}
+	// If word-size and name the same, keep stable.
+	return a[i].order < a[j].order
+}
+
+func (a byArch) Swap(i, j int) {
+	a[i], a[j] = a[j], a[i]
 }
 
 // Image holds the attributes that vary amongst relevant images for

=== modified file 'environs/instances/image_test.go'
--- environs/instances/image_test.go	2014-05-09 13:24:50 +0000
+++ environs/instances/image_test.go	2014-05-19 03:30:40 +0000
@@ -11,7 +11,6 @@
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/imagemetadata"
 	"launchpad.net/juju-core/environs/simplestreams"
-	"launchpad.net/juju-core/juju/arch"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/utils"
 )
@@ -91,10 +90,10 @@
        }
      }
    },
-   "com.ubuntu.cloud:server:12.04:arm": {
+   "com.ubuntu.cloud:server:12.04:armhf": {
      "release": "precise",
      "version": "12.04",
-     "arch": "arm",
+     "arch": "armhf",
      "versions": {
        "20121218": {
          "items": {
@@ -117,7 +116,57 @@
              "id": "ami-00000036"
            }
          },
-         "pubname": "ubuntu-precise-12.04-arm-server-20121218",
+         "pubname": "ubuntu-precise-12.04-armhf-server-20121218",
+         "label": "release"
+       }
+     }
+   },
+   "com.ubuntu.cloud:server:12.04:i386": {
+     "release": "precise",
+     "version": "12.04",
+     "arch": "i386",
+     "versions": {
+       "20121218": {
+         "items": {
+           "apne1pe": {
+             "root_store": "ebs",
+             "virt": "pv",
+             "region": "ap-northeast-1",
+             "id": "ami-b79b09b6"
+           },
+           "test1pe": {
+             "root_store": "ebs",
+             "virt": "pv",
+             "region": "test",
+             "id": "ami-b79b09b7"
+           }
+         },
+         "pubname": "ubuntu-precise-12.04-i386-server-20121218",
+         "label": "release"
+       }
+     }
+   },
+   "com.ubuntu.cloud:server:12.04:ppc64": {
+     "release": "precise",
+     "version": "12.04",
+     "arch": "ppc64",
+     "versions": {
+       "20121218": {
+         "items": {
+           "apne1pe": {
+             "root_store": "ebs",
+             "virt": "pv",
+             "region": "ap-northeast-1",
+             "id": "ami-b79b09b8"
+           },
+           "test1pe": {
+             "root_store": "ebs",
+             "virt": "pv",
+             "region": "test",
+             "id": "ami-b79b09b9"
+           }
+         },
+         "pubname": "ubuntu-precise-12.04-ppc64-server-20121218",
          "label": "release"
        }
      }
@@ -167,10 +216,10 @@
 
 func (p *instanceSpecTestParams) init() {
 	if p.arches == nil {
-		p.arches = []string{"amd64", "arm"}
+		p.arches = []string{"amd64", "armhf"}
 	}
 	if p.instanceTypes == nil {
-		p.instanceTypes = []InstanceType{{Id: "1", Name: "it-1", Arches: []string{"amd64", "arm"}}}
+		p.instanceTypes = []InstanceType{{Id: "1", Name: "it-1", Arches: []string{"amd64", "armhf"}}}
 		p.instanceTypeId = "1"
 		p.instanceTypeName = "it-1"
 	}
@@ -187,6 +236,42 @@
 		},
 	},
 	{
+		desc:    "prefer amd64 over i386",
+		region:  "test",
+		imageId: "ami-00000033",
+		arches:  []string{"amd64", "i386"},
+		instanceTypes: []InstanceType{
+			{Id: "1", Name: "it-1", Arches: []string{"i386", "amd64"}, VirtType: &pv, Mem: 512},
+		},
+	},
+	{
+		desc:    "prefer armhf over i386 (first alphabetical wins)",
+		region:  "test",
+		imageId: "ami-00000034",
+		arches:  []string{"armhf", "i386"},
+		instanceTypes: []InstanceType{
+			{Id: "1", Name: "it-1", Arches: []string{"armhf", "i386"}, VirtType: &pv, Mem: 512},
+		},
+	},
+	{
+		desc:    "prefer ppc64 over i386 (64-bit trumps 32-bit, regardless of alphabetical order)",
+		region:  "test",
+		imageId: "ami-b79b09b9",
+		arches:  []string{"ppc64", "i386"},
+		instanceTypes: []InstanceType{
+			{Id: "1", Name: "it-1", Arches: []string{"i386", "ppc64"}, VirtType: &pv, Mem: 512},
+		},
+	},
+	{
+		desc:    "prefer amd64 over arm64 (first 64-bit alphabetical wins)",
+		region:  "test",
+		imageId: "ami-00000033",
+		arches:  []string{"arm64", "amd64"},
+		instanceTypes: []InstanceType{
+			{Id: "1", Name: "it-1", Arches: []string{"arm64", "amd64"}, VirtType: &pv, Mem: 512},
+		},
+	},
+	{
 		desc:    "explicit release stream",
 		region:  "test",
 		stream:  "released",
@@ -243,7 +328,7 @@
 	{
 		desc:   "no image exists in metadata",
 		region: "invalid-region",
-		err:    `no "precise" images in invalid-region with arches \[amd64 arm\]`,
+		err:    `no "precise" images in invalid-region with arches \[amd64 armhf\]`,
 	},
 	{
 		desc:          "no valid instance types",
@@ -307,44 +392,6 @@
 	}
 }
 
-func (s *imageSuite) TestPreferredSpec(c *gc.C) {
-	type prefTest struct {
-		desc     string
-		specs    []*InstanceSpec
-		expected *InstanceSpec
-	}
-
-	s.PatchValue(&arch.HostArch, func() string { return arch.ARM64 })
-
-	amd64 := &InstanceSpec{Image: Image{Arch: arch.AMD64}}
-	i386 := &InstanceSpec{Image: Image{Arch: arch.I386}}
-	arm64 := &InstanceSpec{Image: Image{Arch: arch.ARM64}}
-
-	prefTests := []prefTest{
-		{
-			"choose hostarch (arm64) over other arches",
-			[]*InstanceSpec{i386, arm64, amd64},
-			arm64,
-		},
-		{
-			"choose first image if no arm64",
-			[]*InstanceSpec{i386, amd64},
-			i386,
-		},
-		{
-			"choose only image only one there",
-			[]*InstanceSpec{amd64},
-			amd64,
-		},
-	}
-
-	for n, test := range prefTests {
-		c.Logf("PreferredSpec test %d: %s", n, test.desc)
-		actual := preferredSpec(test.specs)
-		c.Assert(actual, gc.Equals, test.expected)
-	}
-}
-
 var imageMatchtests = []struct {
 	image Image
 	itype InstanceType
@@ -356,14 +403,14 @@
 		match: true,
 	}, {
 		image: Image{Arch: "amd64"},
-		itype: InstanceType{Arches: []string{"amd64", "arm"}},
+		itype: InstanceType{Arches: []string{"amd64", "armhf"}},
 		match: true,
 	}, {
 		image: Image{Arch: "amd64", VirtType: hvm},
 		itype: InstanceType{Arches: []string{"amd64"}, VirtType: &hvm},
 		match: true,
 	}, {
-		image: Image{Arch: "arm"},
+		image: Image{Arch: "armhf"},
 		itype: InstanceType{Arches: []string{"amd64"}},
 	}, {
 		image: Image{Arch: "amd64", VirtType: hvm},

=== modified file 'juju/arch/arch.go'
--- juju/arch/arch.go	2014-05-08 06:58:42 +0000
+++ juju/arch/arch.go	2014-05-19 03:30:40 +0000
@@ -27,6 +27,21 @@
 	PPC64,
 }
 
+// Info records the information regarding each architecture recognised by Juju.
+var Info = map[string]ArchInfo{
+	AMD64: {64},
+	I386:  {32},
+	ARM:   {32},
+	ARM64: {64},
+	PPC64: {64},
+}
+
+// ArchInfo is a struct containing information about a supported architecture.
+type ArchInfo struct {
+	// WordSize is the architecture's word size, in bits.
+	WordSize int
+}
+
 // archREs maps regular expressions for matching
 // `uname -m` to architectures recognised by Juju.
 var archREs = []struct {

=== modified file 'juju/arch/arch_test.go'
--- juju/arch/arch_test.go	2014-05-08 06:58:42 +0000
+++ juju/arch/arch_test.go	2014-05-19 03:30:40 +0000
@@ -52,3 +52,10 @@
 	}
 	c.Assert(arch.IsSupportedArch("invalid"), jc.IsFalse)
 }
+
+func (s *archSuite) TestArchInfo(c *gc.C) {
+	for _, a := range arch.AllSupportedArches {
+		_, ok := arch.Info[a]
+		c.Assert(ok, jc.IsTrue)
+	}
+}