Fix tests

sdg0919 · May 24, 2019 · 04e06e9 · 04e06e9
1 parent ceadcca
commit 04e06e9
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 38 deletions.
diff --git a/caas/kubernetes/provider/bootstrap.go b/caas/kubernetes/provider/bootstrap.go
@@ -348,10 +348,8 @@ func (c *controllerStack) getControllerSvcSpec(cloudType string) (*controllerSer
 
 func (c *controllerStack) createControllerService() error {
 	svcName := c.resourceNameService
-	cloudType, _, err := cloud.SplitHostCloudRegion(c.pcfg.Bootstrap.ControllerCloud.HostCloudRegion)
-	if err != nil {
-		return errors.Trace(err)
-	}
+
+	cloudType, _, _ := cloud.SplitHostCloudRegion(c.pcfg.Bootstrap.ControllerCloud.HostCloudRegion)
 	controllerSvcSpec, err := c.getControllerSvcSpec(cloudType)
 	if err != nil {
 		return errors.Trace(err)

diff --git a/caas/kubernetes/provider/cloud.go b/caas/kubernetes/provider/cloud.go
@@ -161,7 +161,7 @@ func UpdateKubeCloudWithStorage(k8sCloud *cloud.Cloud, storageParams KubeCloudSt
 
 	if err != nil {
 		// Region is optional, but cloudType is required for next step.
-		return "", errors.Trace(err)
+		return "", ClusterQueryError{}
 	}
 
 	// check Juju's opinionated defaults if cloudType is usable.
@@ -208,12 +208,12 @@ func UpdateKubeCloudWithStorage(k8sCloud *cloud.Cloud, storageParams KubeCloudSt
 				storageMsg = fmt.Sprintf("%s provisioned\nby the existing %q storage class", storageMsg, storageParams.WorkloadStorage)
 			}
 		} else {
-			clusterMetadata.NominatedStorageClass = sp
-			clusterMetadata.OperatorStorageClass = sp
 			storageMsg = fmt.Sprintf(" with storage provisioned\nby the existing %q storage class", storageParams.WorkloadStorage)
 		}
+		clusterMetadata.NominatedStorageClass = sp
+		clusterMetadata.OperatorStorageClass = sp
 	}
-	return
+	return storageMsg, nil
 }
 
 // BaseKubeCloudOpenParams provides a basic OpenParams for a cluster

diff --git a/caas/kubernetes/provider/metadata.go b/caas/kubernetes/provider/metadata.go
@@ -167,22 +167,19 @@ func (k *kubernetesClient) GetClusterMetadata(storageClass string) (*caas.Cluste
 	}
 
 	if result.OperatorStorageClass == nil && len(possibleOperatorStorage) > 0 {
-		sc := possibleOperatorStorage[0]
-		result.OperatorStorageClass = sc
-		logger.Debugf("Use %q for operator storage class", sc.Name)
+		result.OperatorStorageClass = possibleOperatorStorage[0]
+		logger.Debugf("Use %q for operator storage class", possibleOperatorStorage[0].Name)
 	}
 	// Even if no storage class was marked as default for the cluster, if there's only
 	// one of them, use it for workload storage.
-	if result.NominatedStorageClass == nil && len(possibleWorkloadStorage) > 0 {
-		sc := possibleWorkloadStorage[0]
-		result.NominatedStorageClass = sc
-		logger.Debugf("Use %q for nominated storage class", sc.Name)
+	if result.NominatedStorageClass == nil && len(possibleWorkloadStorage) == 1 {
+		result.NominatedStorageClass = possibleWorkloadStorage[0]
+		logger.Debugf("Use %q for nominated storage class", possibleWorkloadStorage[0].Name)
 	}
-	if result.OperatorStorageClass == nil {
+	if result.OperatorStorageClass == nil && result.NominatedStorageClass != nil {
 		// use workload storage class if no operator storage class preference found.
-		sc := result.NominatedStorageClass
-		result.OperatorStorageClass = sc
-		logger.Debugf("Use nominated storage class %q for operator storage class", sc.Name)
+		result.OperatorStorageClass = result.NominatedStorageClass
+		logger.Debugf("Use nominated storage class %q for operator storage class", result.NominatedStorageClass.Name)
 	}
 	return &result, nil
 }

diff --git a/caas/kubernetes/provider/metadata_test.go b/caas/kubernetes/provider/metadata_test.go
@@ -97,21 +97,21 @@ var hostRegionsTestCases = []hostRegionTestcase{
 		expectedCloud:   "gce",
 		expectedRegions: set.NewStrings(""),
 		nodes: newNodeList(map[string]string{
-			"juju.io/cloud": "gce",
+			"juju-io-cloud": "gce",
 		}),
 	},
 	{
 		expectedCloud:   "ec2",
 		expectedRegions: set.NewStrings(""),
 		nodes: newNodeList(map[string]string{
-			"juju.io/cloud": "ec2",
+			"juju-io-cloud": "ec2",
 		}),
 	},
 	{
 		expectedCloud:   "azure",
 		expectedRegions: set.NewStrings(""),
 		nodes: newNodeList(map[string]string{
-			"juju.io/cloud": "azure",
+			"juju-io-cloud": "azure",
 		}),
 	},
 	{

diff --git a/cloud/clouds.go b/cloud/clouds.go
@@ -170,10 +170,14 @@ type Cloud struct {
 // SplitHostCloudRegion splits host cloud region to cloudType and region.
 func SplitHostCloudRegion(hostCloudRegion string) (string, string, error) {
 	fields := strings.SplitN(hostCloudRegion, "/", 2)
-	if len(fields) != 2 || fields[0] == "" || fields[1] == "" {
-		return "", "", errors.NotValidf("cloud region %q", hostCloudRegion)
+	if len(fields) == 0 || fields[0] == "" {
+		return "", "", errors.NotValidf("host cloud region %q", hostCloudRegion)
 	}
-	return fields[0], fields[1], nil
+	region := ""
+	if len(fields) > 1 {
+		region = fields[1]
+	}
+	return fields[0], region, nil
 }
 
 // BuildHostCloudRegion combines cloudType with region to host cloud region.

diff --git a/cmd/juju/caas/add.go b/cmd/juju/caas/add.go
@@ -379,7 +379,6 @@ func (c *AddCAASCommand) Run(ctx *cmd.Context) error {
 	storageParams := provider.KubeCloudStorageParams{
 		WorkloadStorage:        c.workloadStorage,
 		HostCloudRegion:        c.hostCloudRegion,
-		IsBootstrap:			c.Local,
 		MetadataChecker:        broker,
 		GetClusterMetadataFunc: c.getClusterMetadataFunc(ctx),
 	}
@@ -503,7 +502,7 @@ func (c *AddCAASCommand) validateCloudRegion(cloudRegion string) (_ string, err
 }
 
 func (c *AddCAASCommand) getClusterMetadataFunc(ctx *cmd.Context) provider.GetClusterMetadataFunc {
-	return func(broker caas.ClusterMetadataChecker) (*caas.ClusterMetadata, error) {
+	return func(storageParams provider.KubeCloudStorageParams) (*caas.ClusterMetadata, error) {
 		interrupted := make(chan os.Signal, 1)
 		defer close(interrupted)
 		ctx.InterruptNotify(interrupted)
@@ -512,7 +511,7 @@ func (c *AddCAASCommand) getClusterMetadataFunc(ctx *cmd.Context) provider.GetCl
 		result := make(chan *caas.ClusterMetadata, 1)
 		errChan := make(chan error, 1)
 		go func() {
-			clusterMetadata, err := broker.GetClusterMetadata(c.workloadStorage)
+			clusterMetadata, err := storageParams.MetadataChecker.GetClusterMetadata(storageParams.WorkloadStorage)
 			if err != nil {
 				errChan <- err
 			} else {

diff --git a/cmd/juju/caas/add_test.go b/cmd/juju/caas/add_test.go
@@ -385,11 +385,18 @@ type regionTestCase struct {
 }
 
 func (s *addCAASSuite) TestRegionFlag(c *gc.C) {
+	errMsg := `
+	Juju needs to query the k8s cluster to ensure that the recommended
+	storage defaults are available and to detect the cluster's cloud/region.
+	This was not possible in this case so run add-k8s again, using
+	--storage=<name> to specify the storage class to use and
+	--region=<cloudType>/<region> to specify the cloud/region.
+`[1:]
 	for _, ts := range []regionTestCase{
 		{
 			title:          "missing cloud",
 			regionStr:      "/region",
-			expectedErrStr: `validating cloud region "/region": cloud region "/region" not valid`,
+			expectedErrStr: errMsg,
 		},
 		{
 			title:          "missing region",
@@ -473,7 +480,7 @@ func (s *addCAASSuite) TestGatherClusterRegionMetaRegionNoMatchesThenIgnored(c *
 	)
 }
 
-func (s *addCAASSuite) assertAddCloudResult(c *gc.C, cloudRegion, workloadStorage string, localOnly bool) {
+func (s *addCAASSuite) assertAddCloudResult(c *gc.C, cloudRegion, workloadStorage, operatorStorage string, localOnly bool) {
 	s.fakeK8sClusterMetadataChecker.CheckCall(c, 0, "GetClusterMetadata")
 	if localOnly {
 		s.fakeCloudAPI.CheckNoCalls(c)
@@ -489,7 +496,7 @@ func (s *addCAASSuite) assertAddCloudResult(c *gc.C, cloudRegion, workloadStorag
 				IdentityEndpoint: "",
 				StorageEndpoint:  "",
 				Regions:          []cloud.Region{{Name: "us-east1"}},
-				Config:           map[string]interface{}{"operator-storage": "operator-sc", "workload-storage": workloadStorage},
+				Config:           map[string]interface{}{"operator-storage": operatorStorage, "workload-storage": workloadStorage},
 				RegionConfig:     cloud.RegionConfig(nil),
 				CACertificates:   []string{"fakecadata2"},
 			},
@@ -531,7 +538,7 @@ func (s *addCAASSuite) assertAddCloudResult(c *gc.C, cloudRegion, workloadStorag
 				IdentityEndpoint: "",
 				StorageEndpoint:  "",
 				Regions:          []cloud.Region{{Name: "us-east1"}},
-				Config:           map[string]interface{}{"operator-storage": "operator-sc", "workload-storage": workloadStorage},
+				Config:           map[string]interface{}{"operator-storage": operatorStorage, "workload-storage": workloadStorage},
 				RegionConfig:     cloud.RegionConfig(nil),
 				CACertificates:   []string{"fakecadata2"},
 			},
@@ -547,7 +554,7 @@ func (s *addCAASSuite) TestGatherClusterRegionMetaRegionMatchesAndPassThrough(c
 	ctx, err := s.runCommand(c, nil, cmd, "myk8s", "-c", "foo", "--cluster-name", "mrcloud2")
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(strings.Trim(cmdtesting.Stdout(ctx), "\n"), gc.Equals, `k8s substrate "mrcloud2" added as cloud "myk8s".`)
-	s.assertAddCloudResult(c, cloudRegion, "", false)
+	s.assertAddCloudResult(c, cloudRegion, "", "operator-sc", false)
 }
 
 func (s *addCAASSuite) TestGatherClusterMetadataError(c *gc.C) {
@@ -635,7 +642,7 @@ func (s *addCAASSuite) TestUnknownClusterExistingStorageClass(c *gc.C) {
 	result := strings.Trim(cmdtesting.Stdout(ctx), "\n")
 	result = strings.Replace(result, "\n", " ", -1)
 	c.Assert(result, gc.Equals, `k8s substrate "mrcloud2" added as cloud "myk8s" with storage provisioned by the existing "mystorage" storage class.`)
-	s.assertAddCloudResult(c, cloudRegion, "mystorage", false)
+	s.assertAddCloudResult(c, cloudRegion, "mystorage", "mystorage", false)
 }
 
 func (s *addCAASSuite) TestCreateDefaultStorageProvisioner(c *gc.C) {
@@ -661,7 +668,7 @@ func (s *addCAASSuite) TestCreateDefaultStorageProvisioner(c *gc.C) {
 	result := strings.Trim(cmdtesting.Stdout(ctx), "\n")
 	result = strings.Replace(result, "\n", " ", -1)
 	c.Assert(result, gc.Equals, `k8s substrate "mrcloud2" added as cloud "myk8s" with gce disk default storage provisioned by the existing "mystorage" storage class.`)
-	s.assertAddCloudResult(c, cloudRegion, "mystorage", false)
+	s.assertAddCloudResult(c, cloudRegion, "mystorage", "mystorage", false)
 }
 
 func (s *addCAASSuite) TestCreateCustomStorageProvisioner(c *gc.C) {
@@ -671,7 +678,7 @@ func (s *addCAASSuite) TestCreateCustomStorageProvisioner(c *gc.C) {
 	s.fakeK8sClusterMetadataChecker.Call("CheckDefaultWorkloadStorage").Returns(
 		&jujucaas.NonPreferredStorageError{PreferredStorage: jujucaas.PreferredStorage{Name: "gce disk"}})
 	storageProvisioner := &jujucaas.StorageProvisioner{
-		Name:        "my disk",
+		Name:        "mystorage",
 		Provisioner: "my disk provisioner",
 	}
 	s.fakeK8sClusterMetadataChecker.Call("EnsureStorageProvisioner", jujucaas.StorageProvisioner{
@@ -684,7 +691,7 @@ func (s *addCAASSuite) TestCreateCustomStorageProvisioner(c *gc.C) {
 	result := strings.Trim(cmdtesting.Stdout(ctx), "\n")
 	result = strings.Replace(result, "\n", " ", -1)
 	c.Assert(result, gc.Equals, `k8s substrate "mrcloud2" added as cloud "myk8s" with storage provisioned by the existing "mystorage" storage class.`)
-	s.assertAddCloudResult(c, cloudRegion, "mystorage", false)
+	s.assertAddCloudResult(c, cloudRegion,"mystorage" ,"mystorage", false)
 }
 
 func (s *addCAASSuite) TestLocalOnly(c *gc.C) {
@@ -696,7 +703,7 @@ func (s *addCAASSuite) TestLocalOnly(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	expected := `k8s substrate "mrcloud2" added as cloud "myk8s".You can now bootstrap to this cloud by running 'juju bootstrap myk8s'.`
 	c.Assert(strings.Replace(cmdtesting.Stdout(ctx), "\n", "", -1), gc.Equals, expected)
-	s.assertAddCloudResult(c, cloudRegion, "", true)
+	s.assertAddCloudResult(c, cloudRegion, "", "operator-sc", true)
 }
 
 func mockStdinPipe(content string) (*os.File, error) {