Introduce and use FindTools provisioner API

We introduce a new FindTools API in the provisioner facade, and move the existing client FindTools API implementation into the apiserver/common package. The provisioner worker is updated to use this API instead of going directly to provider storage (via environs/tools.FindTools). There are a couple of bugs already that require this to be done, but now that we're moving tools storage behind the API server it is imperative. Fixes https://bugs.launchpad.net/bugs/1347984 Fixes https://bugs.launchpad.net/bugs/1304151
zhatin · Aug 29, 2014 · e436ca5 · e436ca5
1 parent 7eb478b
commit e436ca5
Show file tree

Hide file tree

Showing 23 changed files with 232 additions and 222 deletions.
diff --git a/cmd/juju/synctools.go b/cmd/juju/synctools.go
@@ -86,7 +86,7 @@ func (c *SyncToolsCommand) Init(args []string) error {
 // syncToolsAPI provides an interface with a subset of the
 // state/api.Client API. This exists to enable mocking.
 type syncToolsAPI interface {
-	FindTools(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error)
+	FindTools(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error)
 	UploadTools(r io.Reader, v version.Binary) (*coretools.Tools, error)
 	Close() error
 }

diff --git a/cmd/juju/synctools_test.go b/cmd/juju/synctools_test.go
@@ -202,13 +202,13 @@ func (s *syncToolsSuite) TestAPIAdapterFindTools(c *gc.C) {
 	var called bool
 	result := coretools.List{&coretools.Tools{}}
 	fake := fakeSyncToolsAPI{
-		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error) {
+		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error) {
 			called = true
 			c.Assert(majorVersion, gc.Equals, 2)
 			c.Assert(minorVersion, gc.Equals, -1)
 			c.Assert(series, gc.Equals, "")
 			c.Assert(arch, gc.Equals, "")
-			return params.FindToolsResults{List: result}, nil
+			return params.FindToolsResult{List: result}, nil
 		},
 	}
 	a := syncToolsAPIAdapter{&fake}
@@ -220,9 +220,9 @@ func (s *syncToolsSuite) TestAPIAdapterFindTools(c *gc.C) {
 
 func (s *syncToolsSuite) TestAPIAdapterFindToolsNotFound(c *gc.C) {
 	fake := fakeSyncToolsAPI{
-		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error) {
+		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error) {
 			err := common.ServerError(errors.NotFoundf("tools"))
-			return params.FindToolsResults{Error: err}, nil
+			return params.FindToolsResult{Error: err}, nil
 		},
 	}
 	a := syncToolsAPIAdapter{&fake}
@@ -234,8 +234,8 @@ func (s *syncToolsSuite) TestAPIAdapterFindToolsNotFound(c *gc.C) {
 func (s *syncToolsSuite) TestAPIAdapterFindToolsAPIError(c *gc.C) {
 	findToolsErr := common.ServerError(errors.NotFoundf("tools"))
 	fake := fakeSyncToolsAPI{
-		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error) {
-			return params.FindToolsResults{Error: findToolsErr}, findToolsErr
+		findTools: func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error) {
+			return params.FindToolsResult{Error: findToolsErr}, findToolsErr
 		},
 	}
 	a := syncToolsAPIAdapter{&fake}
@@ -261,11 +261,11 @@ func (s *syncToolsSuite) TestAPIAdapterUploadTools(c *gc.C) {
 }
 
 type fakeSyncToolsAPI struct {
-	findTools   func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error)
+	findTools   func(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error)
 	uploadTools func(r io.Reader, v version.Binary) (*coretools.Tools, error)
 }
 
-func (f *fakeSyncToolsAPI) FindTools(majorVersion, minorVersion int, series, arch string) (params.FindToolsResults, error) {
+func (f *fakeSyncToolsAPI) FindTools(majorVersion, minorVersion int, series, arch string) (params.FindToolsResult, error) {
 	return f.findTools(majorVersion, minorVersion, series, arch)
 }
 

diff --git a/state/api/client.go b/state/api/client.go
@@ -570,9 +570,10 @@ func (c *Client) SetEnvironAgentVersion(version version.Number) error {
 }
 
 // FindTools returns a List containing all tools matching the specified parameters.
-func (c *Client) FindTools(majorVersion, minorVersion int,
-	series, arch string) (result params.FindToolsResults, err error) {
-
+func (c *Client) FindTools(
+	majorVersion, minorVersion int,
+	series, arch string,
+) (result params.FindToolsResult, err error) {
 	args := params.FindToolsParams{
 		MajorVersion: majorVersion,
 		MinorVersion: minorVersion,

diff --git a/state/api/params/internal.go b/state/api/params/internal.go
@@ -506,20 +506,6 @@ type ToolsResults struct {
 	Results []ToolsResult
 }
 
-// FindToolsParams defines parameters for the FindTools method.
-type FindToolsParams struct {
-	MajorVersion int
-	MinorVersion int
-	Arch         string
-	Series       string
-}
-
-// FindToolsResults holds a list of tools from FindTools and any error.
-type FindToolsResults struct {
-	List  tools.List
-	Error *Error
-}
-
 // Version holds a specific binary version.
 type Version struct {
 	Version version.Binary

diff --git a/state/api/params/params.go b/state/api/params/params.go
@@ -15,6 +15,7 @@ import (
 	"github.com/juju/juju/constraints"
 	"github.com/juju/juju/instance"
 	"github.com/juju/juju/network"
+	"github.com/juju/juju/tools"
 	"github.com/juju/juju/utils/ssh"
 	"github.com/juju/juju/version"
 )
@@ -777,3 +778,28 @@ type StateServersChanges struct {
 	Promoted   []string `json:promoted,omitempty`
 	Demoted    []string `json:demoted,omitempty`
 }
+
+// FindToolsParams defines parameters for the FindTools method.
+type FindToolsParams struct {
+	// Number will be used to match tools versions exactly if non-zero.
+	Number version.Number
+
+	// MajorVersion will be used to match the major version if non-zero.
+	MajorVersion int
+
+	// MinorVersion will be used to match the major version if greater
+	// than or equal to zero, and Number is zero.
+	MinorVersion int
+
+	// Arch will be used to match tools by architecture if non-empty.
+	Arch string
+
+	// Series will be used to match tools by series if non-empty.
+	Series string
+}
+
+// FindToolsResult holds a list of tools from FindTools and any error.
+type FindToolsResult struct {
+	List  tools.List
+	Error *Error
+}
diff --git a/state/api/provisioner/provisioner.go b/state/api/provisioner/provisioner.go
@@ -4,15 +4,14 @@
 package provisioner
 
 import (
-	"fmt"
-
 	"github.com/juju/names"
 
 	"github.com/juju/juju/state/api/base"
 	"github.com/juju/juju/state/api/common"
 	"github.com/juju/juju/state/api/params"
 	"github.com/juju/juju/state/api/watcher"
 	"github.com/juju/juju/tools"
+	"github.com/juju/juju/version"
 )
 
 // State provides access to the Machiner API facade.
@@ -91,28 +90,6 @@ func (st *State) StateAddresses() ([]string, error) {
 	return result.Result, nil
 }
 
-// Tools returns the agent tools for the given entity.
-func (st *State) Tools(tag names.MachineTag) (*tools.Tools, error) {
-	var results params.ToolsResults
-	args := params.Entities{
-		Entities: []params.Entity{{Tag: tag.String()}},
-	}
-	err := st.facade.FacadeCall("Tools", args, &results)
-	if err != nil {
-		// TODO: Not directly tested
-		return nil, err
-	}
-	if len(results.Results) != 1 {
-		// TODO: Not directly tested
-		return nil, fmt.Errorf("expected 1 result, got %d", len(results.Results))
-	}
-	result := results.Results[0]
-	if err := result.Error; err != nil {
-		return nil, err
-	}
-	return result.Tools, nil
-}
-
 // ContainerManagerConfig returns information from the environment config that is
 // needed for configuring the container manager.
 func (st *State) ContainerManagerConfig(args params.ContainerManagerConfigParams) (result params.ContainerManagerConfig, err error) {
@@ -148,3 +125,25 @@ func (st *State) MachinesWithTransientErrors() ([]*Machine, []params.StatusResul
 	}
 	return machines, results.Results, nil
 }
+
+// FindTools returns al ist of tools matching the specified version number and
+// series, and, if non-empty, arch.
+func (st *State) FindTools(v version.Number, series string, arch *string) (tools.List, error) {
+	args := params.FindToolsParams{
+		Number:       v,
+		Series:       series,
+		MajorVersion: -1,
+		MinorVersion: -1,
+	}
+	if arch != nil {
+		args.Arch = *arch
+	}
+	var result params.FindToolsResult
+	if err := st.facade.FacadeCall("FindTools", args, &result); err != nil {
+		return nil, err
+	}
+	if result.Error != nil {
+		return nil, result.Error
+	}
+	return result.List, nil
+}
diff --git a/state/api/provisioner/provisioner_test.go b/state/api/provisioner/provisioner_test.go
@@ -4,6 +4,7 @@
 package provisioner_test
 
 import (
+	"fmt"
 	stdtesting "testing"
 
 	"github.com/juju/errors"
@@ -14,6 +15,7 @@ import (
 
 	"github.com/juju/juju/constraints"
 	"github.com/juju/juju/container"
+	envtools "github.com/juju/juju/environs/tools"
 	"github.com/juju/juju/instance"
 	"github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/mongo"
@@ -25,7 +27,7 @@ import (
 	apitesting "github.com/juju/juju/state/api/testing"
 	statetesting "github.com/juju/juju/state/testing"
 	coretesting "github.com/juju/juju/testing"
-	"github.com/juju/juju/tools"
+	coretools "github.com/juju/juju/tools"
 	"github.com/juju/juju/version"
 )
 
@@ -617,25 +619,6 @@ func (s *provisionerSuite) TestContainerConfig(c *gc.C) {
 	c.Assert(result.PreferIPv6, jc.IsTrue)
 }
 
-func (s *provisionerSuite) TestToolsWrongMachine(c *gc.C) {
-	tools, err := s.provisioner.Tools(names.NewMachineTag("42"))
-	c.Assert(err, gc.ErrorMatches, "machine 42 not found")
-	c.Assert(tools, gc.IsNil)
-}
-
-func (s *provisionerSuite) TestTools(c *gc.C) {
-	cur := version.Current
-	curTools := &tools.Tools{Version: cur, URL: ""}
-	curTools.Version.Minor++
-	s.machine.SetAgentVersion(cur)
-	// Provisioner.Tools returns the *desired* set of tools, not the
-	// currently running set. We want to be upgraded to cur.Version
-	stateTools, err := s.provisioner.Tools(s.machine.Tag().(names.MachineTag))
-	c.Assert(err, gc.IsNil)
-	c.Assert(stateTools.Version, gc.Equals, cur)
-	c.Assert(stateTools.URL, gc.Not(gc.Equals), "")
-}
-
 func (s *provisionerSuite) TestSetSupportedContainers(c *gc.C) {
 	apiMachine, err := s.provisioner.Machine(s.machine.Tag().(names.MachineTag))
 	c.Assert(err, gc.IsNil)
@@ -661,3 +644,27 @@ func (s *provisionerSuite) TestSupportsNoContainers(c *gc.C) {
 	c.Assert(ok, jc.IsTrue)
 	c.Assert(containers, gc.DeepEquals, []instance.ContainerType{})
 }
+
+func (s *provisionerSuite) TestFindTools(c *gc.C) {
+	list, err := envtools.FindTools(s.Environ, -1, -1, coretools.Filter{
+		Number: version.Current.Number,
+		Series: version.Current.Series,
+		Arch:   version.Current.Arch,
+	}, false)
+	c.Assert(err, gc.IsNil)
+	apiList, err := s.provisioner.FindTools(version.Current.Number, version.Current.Series, &version.Current.Arch)
+	c.Assert(err, gc.IsNil)
+	c.Assert(apiList, gc.HasLen, len(list))
+
+	listStrings := make([]string, len(list))
+	for i, tools := range list {
+		listStrings[i] = fmt.Sprintf("%+v", tools)
+	}
+
+	apiListStrings := make([]string, len(apiList))
+	for i, tools := range apiList {
+		apiListStrings[i] = fmt.Sprintf("%+v", tools)
+	}
+
+	c.Assert(apiListStrings, jc.SameContents, listStrings)
+}
diff --git a/state/apiserver/client/client.go b/state/apiserver/client/client.go
@@ -18,15 +18,13 @@ import (
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/manual"
-	envtools "github.com/juju/juju/environs/tools"
 	"github.com/juju/juju/instance"
 	"github.com/juju/juju/juju"
 	"github.com/juju/juju/network"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/state/api"
 	"github.com/juju/juju/state/api/params"
 	"github.com/juju/juju/state/apiserver/common"
-	coretools "github.com/juju/juju/tools"
 	"github.com/juju/juju/version"
 )
 
@@ -906,24 +904,8 @@ func (c *Client) SetEnvironAgentVersion(args params.SetEnvironAgentVersion) erro
 }
 
 // FindTools returns a List containing all tools matching the given parameters.
-func (c *Client) FindTools(args params.FindToolsParams) (params.FindToolsResults, error) {
-	result := params.FindToolsResults{}
-	// Get the existing environment config from the state.
-	envConfig, err := c.api.state.EnvironConfig()
-	if err != nil {
-		return result, err
-	}
-	env, err := environs.New(envConfig)
-	if err != nil {
-		return result, err
-	}
-	filter := coretools.Filter{
-		Arch:   args.Arch,
-		Series: args.Series,
-	}
-	result.List, err = envtools.FindTools(env, args.MajorVersion, args.MinorVersion, filter, envtools.DoNotAllowRetry)
-	result.Error = common.ServerError(err)
-	return result, nil
+func (c *Client) FindTools(args params.FindToolsParams) (params.FindToolsResult, error) {
+	return common.FindTools(c.api.state, args)
 }
 
 func destroyErr(desc string, ids, errs []string) error {

diff --git a/state/apiserver/common/export_test.go b/state/apiserver/common/export_test.go
@@ -7,6 +7,7 @@ var (
 	ValidateNewFacade = validateNewFacade
 	WrapNewFacade     = wrapNewFacade
 	NilFacadeRecord   = facadeRecord{}
+	EnvtoolsFindTools = &envtoolsFindTools
 )
 
 type Patcher interface {

diff --git a/state/apiserver/common/tools.go b/state/apiserver/common/tools.go
@@ -17,8 +17,14 @@ import (
 	"github.com/juju/names"
 )
 
+var envtoolsFindTools = envtools.FindTools
+
 type EntityFinderEnvironConfigGetter interface {
 	state.EntityFinder
+	EnvironConfigGetter
+}
+
+type EnvironConfigGetter interface {
 	EnvironConfig() (*config.Config, error)
 }
 
@@ -162,3 +168,25 @@ func (t *ToolsSetter) setOneAgentVersion(tag names.Tag, vers version.Binary, can
 	}
 	return entity.SetAgentVersion(vers)
 }
+
+// FindTools returns a List containing all tools matching the given parameters.
+func FindTools(e EnvironConfigGetter, args params.FindToolsParams) (params.FindToolsResult, error) {
+	result := params.FindToolsResult{}
+	// Get the existing environment config from the state.
+	envConfig, err := e.EnvironConfig()
+	if err != nil {
+		return result, err
+	}
+	env, err := environs.New(envConfig)
+	if err != nil {
+		return result, err
+	}
+	filter := coretools.Filter{
+		Number: args.Number,
+		Arch:   args.Arch,
+		Series: args.Series,
+	}
+	result.List, err = envtoolsFindTools(env, args.MajorVersion, args.MinorVersion, filter, envtools.DoNotAllowRetry)
+	result.Error = ServerError(err)
+	return result, nil
+}