pod-spec-set delay commit to hook success

- Cached SetPodSpec on HookContext, commit on hook success. - Exposed ParsePodSpec from k8s.
sdg0919 · Aug 2, 2019 · aaa99a5 · aaa99a5
1 parent 21ef8a3
commit aaa99a5
Show file tree

Hide file tree

Showing 16 changed files with 180 additions and 115 deletions.
diff --git a/apiserver/facades/agent/caasoperator/operator.go b/apiserver/facades/agent/caasoperator/operator.go
@@ -10,9 +10,8 @@ import (
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/apiserver/params"
-	"github.com/juju/juju/caas"
+	k8sprovider "github.com/juju/juju/caas/kubernetes/provider"
 	"github.com/juju/juju/core/status"
-	"github.com/juju/juju/environs"
 	"github.com/juju/juju/state/watcher"
 )
 
@@ -176,19 +175,6 @@ func (f *Facade) SetPodSpec(args params.SetPodSpecParams) (params.ErrorResults,
 		Results: make([]params.ErrorResult, len(args.Specs)),
 	}
 
-	cfg, err := f.model.ModelConfig()
-	if err != nil {
-		return params.ErrorResults{}, errors.Trace(err)
-	}
-	provider, err := environs.Provider(cfg.Type())
-	if err != nil {
-		return params.ErrorResults{}, errors.Trace(err)
-	}
-	caasProvider, ok := provider.(caas.ContainerEnvironProvider)
-	if !ok {
-		return params.ErrorResults{}, errors.NotValidf("container environ provider %T", provider)
-	}
-
 	for i, arg := range args.Specs {
 		tag, err := names.ParseApplicationTag(arg.Tag)
 		if err != nil {
@@ -199,7 +185,7 @@ func (f *Facade) SetPodSpec(args params.SetPodSpecParams) (params.ErrorResults,
 			results.Results[i].Error = common.ServerError(common.ErrPerm)
 			continue
 		}
-		if _, err := caasProvider.ParsePodSpec(arg.Value); err != nil {
+		if _, err := k8sprovider.ParsePodSpec(arg.Value); err != nil {
 			results.Results[i].Error = common.ServerError(errors.New("invalid pod spec"))
 			continue
 		}

diff --git a/apiserver/facades/agent/uniter/uniter.go b/apiserver/facades/agent/uniter/uniter.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/juju/collections/set"
 	"github.com/juju/errors"
-	"github.com/juju/juju/environs"
 	"github.com/juju/loggo"
 	"gopkg.in/juju/charm.v6"
 	"gopkg.in/juju/names.v2"
@@ -24,7 +23,7 @@ import (
 	"github.com/juju/juju/apiserver/facades/agent/meterstatus"
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/caas"
-	"github.com/juju/juju/caas/kubernetes/provider"
+	k8sprovider "github.com/juju/juju/caas/kubernetes/provider"
 	"github.com/juju/juju/core/cache"
 	"github.com/juju/juju/core/leadership"
 	corenetwork "github.com/juju/juju/core/network"
@@ -2063,7 +2062,7 @@ func (u *UniterAPI) NetworkInfo(args params.NetworkInfoParams) (params.NetworkIn
 			if err != nil {
 				return params.NetworkInfoResults{}, err
 			}
-			svcType := cfg.GetString(provider.ServiceTypeConfigKey, "")
+			svcType := cfg.GetString(k8sprovider.ServiceTypeConfigKey, "")
 			switch k8score.ServiceType(svcType) {
 			case k8score.ServiceTypeLoadBalancer, k8score.ServiceTypeExternalName:
 				pollPublic = true
@@ -2488,19 +2487,6 @@ func (u *UniterAPI) SetPodSpec(args params.SetPodSpecParams) (params.ErrorResult
 		return false
 	}
 
-	cfg, err := u.m.ModelConfig()
-	if err != nil {
-		return params.ErrorResults{}, errors.Trace(err)
-	}
-	provider, err := environs.Provider(cfg.Type())
-	if err != nil {
-		return params.ErrorResults{}, errors.Trace(err)
-	}
-	cassProvider, ok := provider.(caas.ContainerEnvironProvider)
-	if !ok {
-		return params.ErrorResults{}, errors.NotValidf("container environ provider %T", provider)
-	}
-
 	for i, arg := range args.Specs {
 		tag, err := names.ParseApplicationTag(arg.Tag)
 		if err != nil {
@@ -2511,7 +2497,7 @@ func (u *UniterAPI) SetPodSpec(args params.SetPodSpecParams) (params.ErrorResult
 			results.Results[i].Error = common.ServerError(common.ErrPerm)
 			continue
 		}
-		if _, err := cassProvider.ParsePodSpec(arg.Value); err != nil {
+		if _, err := k8sprovider.ParsePodSpec(arg.Value); err != nil {
 			results.Results[i].Error = common.ServerError(errors.Annotate(err, "invalid pod spec"))
 			continue
 		}

diff --git a/apiserver/facades/client/client/status.go b/apiserver/facades/client/client/status.go
@@ -16,13 +16,12 @@ import (
 
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/params"
-	"github.com/juju/juju/caas"
+	k8sprovider "github.com/juju/juju/caas/kubernetes/provider"
 	"github.com/juju/juju/core/cache"
 	"github.com/juju/juju/core/crossmodel"
 	"github.com/juju/juju/core/lxdprofile"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/status"
-	"github.com/juju/juju/environs"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/state/multiwatcher"
 )
@@ -1182,16 +1181,7 @@ func (context *statusContext) processApplication(application *state.Application)
 			return params.ApplicationStatus{Err: common.ServerError(err)}
 		}
 		if specStr != "" {
-			provider, err := environs.Provider(context.providerType)
-			if err != nil {
-				return params.ApplicationStatus{Err: common.ServerError(err)}
-			}
-			caasProvider, ok := provider.(caas.ContainerEnvironProvider)
-			if !ok {
-				err := errors.NotValidf("container environ provider %T", provider)
-				return params.ApplicationStatus{Err: common.ServerError(err)}
-			}
-			spec, err := caasProvider.ParsePodSpec(specStr)
+			spec, err := k8sprovider.ParsePodSpec(specStr)
 			if err != nil {
 				return params.ApplicationStatus{Err: common.ServerError(err)}
 			}

diff --git a/caas/broker.go b/caas/broker.go
@@ -31,9 +31,6 @@ type ContainerEnvironProvider interface {
 	// Open should not perform any expensive operations, such as querying
 	// the cloud API, as it will be called frequently.
 	Open(args environs.OpenParams) (Broker, error)
-
-	// ParsePodSpec unmarshalls the given YAML pod spec.
-	ParsePodSpec(in string) (*PodSpec, error)
 }
 
 // RegisterContainerProvider is used for providers that we want to use for managing 'instances',

diff --git a/caas/kubernetes/provider/export_test.go b/caas/kubernetes/provider/export_test.go
@@ -21,7 +21,6 @@ import (
 
 var (
 	MakeUnitSpec             = makeUnitSpec
-	ParseK8sPodSpec          = parseK8sPodSpec
 	OperatorPod              = operatorPod
 	ExtractRegistryURL       = extractRegistryURL
 	CreateDockerConfigJSON   = createDockerConfigJSON

diff --git a/caas/kubernetes/provider/k8stypes.go b/caas/kubernetes/provider/k8stypes.go
@@ -88,10 +88,10 @@ func (*K8sPodSpec) Validate() error {
 var boolValues = set.NewStrings(
 	strings.Split("y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF", "|")...)
 
-// parseK8sPodSpec parses a YAML file which defines how to
+// ParsePodSpec parses a YAML file which defines how to
 // configure a CAAS pod. We allow for generic container
 // set up plus k8s select specific features.
-func parseK8sPodSpec(in string) (*caas.PodSpec, error) {
+func ParsePodSpec(in string) (*caas.PodSpec, error) {
 	// Do the common fields.
 	var spec caas.PodSpec
 	if err := yaml.Unmarshal([]byte(in), &spec); err != nil {
@@ -143,7 +143,8 @@ either use ServiceAccountName to reference existing service account or define Se
 		spec.InitContainers[i] = containerFromK8sSpec(c)
 	}
 	spec.CustomResourceDefinitions = containers.CustomResourceDefinitions
-	return &spec, nil
+
+	return &spec, spec.Validate()
 }
 
 func quoteBoolStrings(containers []k8sContainer) {

diff --git a/caas/kubernetes/provider/k8stypes_test.go b/caas/kubernetes/provider/k8stypes_test.go
@@ -413,7 +413,7 @@ serviceAccount:
 		},
 	} {
 		c.Logf("%v: %s", i, tc.title)
-		spec, err := provider.ParseK8sPodSpec(tc.podSpecStr)
+		spec, err := provider.ParsePodSpec(tc.podSpecStr)
 		c.Assert(err, jc.ErrorIsNil)
 		c.Assert(spec, jc.DeepEquals, tc.podSpec)
 	}
@@ -441,7 +441,7 @@ func (s *ContainersSuite) TestValidateMissingContainers(c *gc.C) {
 containers:
 `[1:]
 
-	_, err := provider.ParseK8sPodSpec(specStr)
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, "require at least one container spec")
 }
 
@@ -452,9 +452,7 @@ containers:
   - image: gitlab/latest
 `[1:]
 
-	spec, err := provider.ParseK8sPodSpec(specStr)
-	c.Assert(err, jc.ErrorIsNil)
-	err = spec.Validate()
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, "spec name is missing")
 }
 
@@ -465,9 +463,7 @@ containers:
   - name: gitlab
 `[1:]
 
-	spec, err := provider.ParseK8sPodSpec(specStr)
-	c.Assert(err, jc.ErrorIsNil)
-	err = spec.Validate()
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, "spec image details is missing")
 }
 
@@ -484,9 +480,7 @@ containers:
             foo: bar
 `[1:]
 
-	spec, err := provider.ParseK8sPodSpec(specStr)
-	c.Assert(err, jc.ErrorIsNil)
-	err = spec.Validate()
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, `file set name is missing`)
 }
 
@@ -504,9 +498,7 @@ containers:
             foo: bar
 `[1:]
 
-	spec, err := provider.ParseK8sPodSpec(specStr)
-	c.Assert(err, jc.ErrorIsNil)
-	err = spec.Validate()
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, `mount path is missing for file set "configuration"`)
 }
 
@@ -528,7 +520,7 @@ serviceAccount:
         resources: ["pods"]
         verbs: ["get", "watch", "list"]
 `[1:]
-	_, err := provider.ParseK8sPodSpec(specStr)
+	_, err := provider.ParsePodSpec(specStr)
 	c.Assert(err, gc.ErrorMatches, "either use ServiceAccountName to reference existing service account or define ServiceAccount spec to create a new one")
 
 }
@@ -622,9 +614,7 @@ containers:
 
 	for i, tc := range serviceAccountValidationTestCases {
 		c.Logf("%v: %s", i, tc.Title)
-		spec, err := provider.ParseK8sPodSpec(containerSpec + tc.Spec)
-		c.Check(err, jc.ErrorIsNil)
-		err = spec.Validate()
+		_, err := provider.ParsePodSpec(containerSpec + tc.Spec)
 		c.Check(err, gc.ErrorMatches, tc.Err)
 	}
 }
diff --git a/caas/kubernetes/provider/provider.go b/caas/kubernetes/provider/provider.go
@@ -114,15 +114,6 @@ func (p kubernetesEnvironProvider) Open(args environs.OpenParams) (caas.Broker,
 	return controllerCorelation(broker)
 }
 
-// ParsePodSpec is part of the ContainerEnvironProvider interface.
-func (kubernetesEnvironProvider) ParsePodSpec(in string) (*caas.PodSpec, error) {
-	spec, err := parseK8sPodSpec(in)
-	if err != nil {
-		return nil, errors.Trace(err)
-	}
-	return spec, spec.Validate()
-}
-
 // CloudSchema returns the schema for adding new clouds of this type.
 func (p kubernetesEnvironProvider) CloudSchema() *jsonschema.Schema {
 	return nil

diff --git a/worker/caasunitprovisioner/deployment_worker.go b/worker/caasunitprovisioner/deployment_worker.go
@@ -11,7 +11,7 @@ import (
 
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/caas"
-	"github.com/juju/juju/caas/kubernetes/provider"
+	k8sprovider "github.com/juju/juju/caas/kubernetes/provider"
 	"github.com/juju/juju/core/watcher"
 )
 
@@ -146,7 +146,7 @@ func (w *deploymentWorker) loop() error {
 		if err != nil {
 			return errors.Trace(err)
 		}
-		spec, err := w.broker.Provider().ParsePodSpec(specStr)
+		spec, err := k8sprovider.ParsePodSpec(specStr)
 		if err != nil {
 			return errors.Annotate(err, "cannot parse pod spec")
 		}
@@ -171,7 +171,7 @@ func (w *deploymentWorker) loop() error {
 		err = w.broker.EnsureService(w.application, w.provisioningStatusSetter.SetOperatorStatus, serviceParams, desiredScale, appConfig)
 		if err != nil {
 			// Some errors we don't want to exit the worker.
-			if provider.MaskError(err) {
+			if k8sprovider.MaskError(err) {
 				logger.Errorf(err.Error())
 				continue
 			}

diff --git a/worker/caasunitprovisioner/mock_test.go b/worker/caasunitprovisioner/mock_test.go
@@ -41,7 +41,6 @@ type mockServiceBroker struct {
 	caas.ContainerEnvironProvider
 	ensured        chan<- struct{}
 	deleted        chan<- struct{}
-	podSpec        *caas.PodSpec
 	serviceStatus  status.StatusInfo
 	serviceWatcher *watchertest.MockNotifyWatcher
 }
@@ -50,10 +49,6 @@ func (m *mockServiceBroker) Provider() caas.ContainerEnvironProvider {
 	return m
 }
 
-func (m *mockServiceBroker) ParsePodSpec(in string) (*caas.PodSpec, error) {
-	return m.podSpec, nil
-}
-
 func (m *mockServiceBroker) EnsureService(appName string, statusCallback caas.StatusCallbackFunc, params *caas.ServiceParams, numUnits int, config application.ConfigAttributes) error {
 	m.MethodCall(m, "EnsureService", appName, params, numUnits, config)
 	statusCallback(appName, status.Waiting, "ensuring", map[string]interface{}{"foo": "bar"})
@@ -105,10 +100,6 @@ func (m *mockContainerBroker) Provider() caas.ContainerEnvironProvider {
 	return m
 }
 
-func (m *mockContainerBroker) ParsePodSpec(in string) (*caas.PodSpec, error) {
-	return m.podSpec, nil
-}
-
 func (m *mockContainerBroker) WatchUnits(appName string) (watcher.NotifyWatcher, error) {
 	m.MethodCall(m, "WatchUnits", appName)
 	return m.unitsWatcher, m.NextErr()

diff --git a/worker/caasunitprovisioner/worker_test.go b/worker/caasunitprovisioner/worker_test.go
@@ -150,7 +150,6 @@ func (s *WorkerSuite) SetUpTest(c *gc.C) {
 	s.serviceBroker = mockServiceBroker{
 		ensured:        s.serviceEnsured,
 		deleted:        s.serviceDeleted,
-		podSpec:        &parsedSpec,
 		serviceWatcher: watchertest.NewMockNotifyWatcher(s.caasServiceChanges),
 	}
 	s.statusSetter = mockProvisioningStatusSetter{}
@@ -426,7 +425,7 @@ func (s *WorkerSuite) TestNewPodSpecChange(c *gc.C) {
 		anotherSpec = `
 containers:
   - name: gitlab
-    image-name: gitlab/latest
+    image: gitlab/latest
 `[1:]
 
 		anotherParsedSpec = caas.PodSpec{
@@ -436,8 +435,6 @@ containers:
 			}}}
 	)
 
-	s.serviceBroker.podSpec = &anotherParsedSpec
-
 	s.podSpecGetter.setProvisioningInfo(apicaasunitprovisioner.ProvisioningInfo{
 		PodSpec: anotherSpec,
 		Tags:    map[string]string{"foo": "bar"},
@@ -498,6 +495,9 @@ customResourceDefinitions:
                   replicas:
                     type: integer
                     minimum: 1
+containers:
+  - name: gitlab
+    image: gitlab/latest
 `[1:]
 
 		anotherParsedSpec = caas.PodSpec{
@@ -531,11 +531,13 @@ customResourceDefinitions:
 					},
 				},
 			},
+			Containers: []caas.ContainerSpec{{
+				Name:  "gitlab",
+				Image: "gitlab/latest",
+			}},
 		}
 	)
 
-	s.serviceBroker.podSpec = &anotherParsedSpec
-
 	s.podSpecGetter.setProvisioningInfo(apicaasunitprovisioner.ProvisioningInfo{
 		PodSpec: anotherSpec,
 		Tags:    map[string]string{"foo": "bar"},