Merge pull request juju#18068 from wallyworld/wireup-unit-life

juju#18068

Application and unit life is written to dqlite and so can be read from there. The facades which used to get the life value from mongo now use the domain service. There were domain state methods to get life but service methods were needed to expose these to the facades.

As with the EnsureDead and Remove apiserver plugins which relied on a generic mongo FindEntity, the LifeGetter plugin is removed from facades and bespoke code used instead. The code calls the relevant GetUnitLife or GetApplicationLife. These common API plugins were meant to stop the code duplication on the facade structs. However, we ran have run into issues around versioning etc. One option we discussed at the time was to have an instance of the common struct instead of embedding it, and then thunk to it via a top level method on the facade.
However, I wanted just to do it this way to start with - yes a bit of duplication etc. But it's minimal and is essentially just iterating over some api args, parsing a tag, doing an api call, returning result. Let's keep everything simple and flat to start with, and once all the facades have the common plugins refactored out and sittling on top of the new services, then we can look at optimisation, rather than doing it now (early) and maybe missing something.

Not all places could be converted. Specifically, there's the life flag worker and similar which will need to be done separately. Also, there's places where more than just unit life is needed and that's not yet in dqlite.

Another temporary compromise is to add a method to allow the state cleanup job to set an app life to dead in dqlite. This is because cleanup still happens in mongo and will be needed for a while until that get sorted out.

The second commit reverts the use of domain errors outside the apiserver layer. The new work in this PR only uses generic errors and some existing code needed to be converted so that everything could work together.

## QA steps

1. test migration
while units are still coming after after deployment, check the pre checks work, eg
```
$ juju migrate foo c2
ERROR source prechecks failed: unit ubuntu/1 not idle or executing (allocating)
```
after things quiesce, test migration works

2. test k8s scale down
bootstrap to k8s
juju deploy snappass-test -n 3
juju scale-application snappass-test 2
juju destroy-model foo

## Links

**Jira card:** [JUJU-6591](https://warthogs.atlassian.net/browse/JUJU-6591)



[JUJU-6591]: https://warthogs.atlassian.net/browse/JUJU-6591?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

api/common/charms/common.go

@@ -53,7 +53,7 @@ func (c *ApplicationCharmInfoClient) ApplicationCharmInfo(ctx context.Context, a
 	args := params.Entity{Tag: names.NewApplicationTag(appName).String()}
 	var info params.Charm
 	if err := c.facade.FacadeCall(ctx, "ApplicationCharmInfo", args, &info); err != nil {
-		return nil, errors.Trace(err)
+		return nil, params.TranslateWellKnownError(err)
 	}
 	return convertCharm(&info)
 }

api/controller/caasfirewaller/client.go

@@ -159,7 +159,7 @@ func (c *Client) Life(ctx context.Context, appName string) (life.Value, error) {
 		return "", errors.Errorf("expected 1 result, got %d", n)
 	}
 	if err := results.Results[0].Error; err != nil {
-		return "", maybeNotFound(err)
+		return "", params.TranslateWellKnownError(err)
 	}
 	return results.Results[0].Life, nil
 }
@@ -197,16 +197,7 @@ func (c *Client) IsExposed(ctx context.Context, appName string) (bool, error) {
 		return false, errors.Errorf("expected 1 result, got %d", n)
 	}
 	if err := results.Results[0].Error; err != nil {
-		return false, maybeNotFound(err)
+		return false, params.TranslateWellKnownError(err)
 	}
 	return results.Results[0].Result, nil
 }
-
-// maybeNotFound returns an error satisfying errors.IsNotFound
-// if the supplied error has a CodeNotFound error.
-func maybeNotFound(err *params.Error) error {
-	if err == nil || !params.IsCodeNotFound(err) {
-		return err
-	}
-	return errors.NewNotFound(err, "")
-}

api/package_test.go

@@ -51,7 +51,6 @@ func (*ImportSuite) TestImports(c *gc.C) {
 		"core/user",
 		"core/version",
 		"core/watcher",
-		"domain/application/errors",
 		"domain/model/errors",
 		"domain/secret/errors",
 		"domain/secretbackend/errors",

apiserver/common/life.go

@@ -30,7 +30,8 @@ func NewLifeGetter(st state.EntityFinder, getCanRead GetAuthFunc) *LifeGetter {
 	}
 }
 
-func (lg *LifeGetter) oneLife(tag names.Tag) (life.Value, error) {
+// OneLife returns the life of the specified entity.
+func (lg *LifeGetter) OneLife(tag names.Tag) (life.Value, error) {
 	entity0, err := lg.st.FindEntity(tag)
 	if err != nil {
 		return "", err
@@ -62,7 +63,7 @@ func (lg *LifeGetter) Life(ctx context.Context, args params.Entities) (params.Li
 		}
 		err = apiservererrors.ErrPerm
 		if canRead(tag) {
-			result.Results[i].Life, err = lg.oneLife(tag)
+			result.Results[i].Life, err = lg.OneLife(tag)
 		}
 		result.Results[i].Error = apiservererrors.ServerError(err)
 	}

apiserver/errors/errors.go

@@ -15,7 +15,6 @@ import (
 	"github.com/juju/juju/core/lease"
 	corelogger "github.com/juju/juju/core/logger"
 	"github.com/juju/juju/core/upgrade"
-	applicationerrors "github.com/juju/juju/domain/application/errors"
 	modelerrors "github.com/juju/juju/domain/model/errors"
 	secreterrors "github.com/juju/juju/domain/secret/errors"
 	secretbackenderrors "github.com/juju/juju/domain/secretbackend/errors"
@@ -120,11 +119,9 @@ func ServerErrorAndStatus(err error) (*params.Error, int) {
 		params.CodeSecretNotFound,
 		params.CodeSecretRevisionNotFound,
 		params.CodeSecretConsumerNotFound,
-		params.CodeSecretBackendNotFound,
-		params.CodeApplicationNotFound:
+		params.CodeSecretBackendNotFound:
 		status = http.StatusNotFound
-	case params.CodeBadRequest,
-		params.CodeScalingStateInconsistent:
+	case params.CodeBadRequest:
 		status = http.StatusBadRequest
 	case params.CodeMethodNotAllowed:
 		status = http.StatusMethodNotAllowed
@@ -233,10 +230,6 @@ func ServerError(err error) *params.Error {
 		code = params.CodeSecretBackendNotSupported
 	case errors.Is(err, errors.BadRequest):
 		code = params.CodeBadRequest
-	case errors.Is(err, applicationerrors.ApplicationNotFound):
-		code = params.CodeApplicationNotFound
-	case errors.Is(err, applicationerrors.ScalingStateInconsistent):
-		code = params.CodeScalingStateInconsistent
 	case errors.Is(err, errors.MethodNotAllowed):
 		code = params.CodeMethodNotAllowed
 	case errors.Is(err, errors.NotImplemented):

apiserver/errors/errors_test.go

@@ -20,7 +20,6 @@ import (
 	"github.com/juju/juju/core/leadership"
 	"github.com/juju/juju/core/lease"
 	"github.com/juju/juju/core/network"
-	applicationerrors "github.com/juju/juju/domain/application/errors"
 	secreterrors "github.com/juju/juju/domain/secret/errors"
 	secretbackenderrors "github.com/juju/juju/domain/secretbackend/errors"
 	"github.com/juju/juju/internal/testing"
@@ -70,16 +69,6 @@ var errorTransformTests = []struct {
 	code:       params.CodeSecretBackendNotFound,
 	status:     http.StatusNotFound,
 	helperFunc: params.IsCodeSecretBackendNotFound,
-}, {
-	err:        applicationerrors.ApplicationNotFound,
-	code:       params.CodeApplicationNotFound,
-	status:     http.StatusNotFound,
-	helperFunc: params.IsCodeApplicationNotFound,
-}, {
-	err:        applicationerrors.ScalingStateInconsistent,
-	code:       params.CodeScalingStateInconsistent,
-	status:     http.StatusBadRequest,
-	helperFunc: params.IsCodeScalingStateInconsistent,
 }, {
 	err:        secretbackenderrors.Forbidden,
 	code:       params.CodeSecretBackendForbidden,

apiserver/facades/agent/agent/agent.go

@@ -20,6 +20,7 @@ import (
 	"github.com/juju/juju/core/credential"
 	"github.com/juju/juju/core/life"
 	"github.com/juju/juju/core/model"
+	applicationerrors "github.com/juju/juju/domain/application/errors"
 	"github.com/juju/juju/internal/mongo"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -40,6 +41,7 @@ type AgentAPI struct {
 
 	credentialService       CredentialService
 	controllerConfigService ControllerConfigService
+	applicationService      ApplicationService
 	st                      *state.State
 	auth                    facade.Authorizer
 	resources               facade.Resources
@@ -56,6 +58,7 @@ func NewAgentAPI(
 	credentialService common.CredentialService,
 	rebootMachineService common.MachineRebootService,
 	modelConfigService common.ModelConfigService,
+	applicationService ApplicationService,
 	watcherRegistry facade.WatcherRegistry,
 ) (*AgentAPI, error) {
 	getCanChange := func() (common.AuthFunc, error) {
@@ -85,6 +88,7 @@ func NewAgentAPI(
 		),
 		credentialService:       credentialService,
 		controllerConfigService: controllerConfigService,
+		applicationService:      applicationService,
 		st:                      st,
 		auth:                    auth,
 		resources:               resources,
@@ -101,6 +105,24 @@ func (api *AgentAPI) GetEntities(ctx context.Context, args params.Entities) para
 			results.Entities[i].Error = apiservererrors.ServerError(err)
 			continue
 		}
+		// Allow only for the owner agent.
+		// Note: having a bulk API call for this is utter madness, given that
+		// this check means we can only ever return a single object.
+		if !api.auth.AuthOwner(tag) {
+			results.Entities[i].Error = apiservererrors.ServerError(apiservererrors.ErrPerm)
+			continue
+		}
+		// Handle units using the domain service.
+		// Eventually all entities will be supported via dqlite.
+		if tag.Kind() == names.UnitTagKind {
+			lifeValue, err := api.applicationService.GetUnitLife(ctx, tag.Id())
+			if errors.Is(err, applicationerrors.UnitNotFound) {
+				err = errors.NotFoundf("unit %s", tag.Id())
+			}
+			results.Entities[i].Life = lifeValue
+			results.Entities[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
 		result, err := api.getEntity(tag)
 		result.Error = apiservererrors.ServerError(err)
 		results.Entities[i] = result
@@ -109,13 +131,6 @@ func (api *AgentAPI) GetEntities(ctx context.Context, args params.Entities) para
 }
 
 func (api *AgentAPI) getEntity(tag names.Tag) (result params.AgentGetEntitiesResult, err error) {
-	// Allow only for the owner agent.
-	// Note: having a bulk API call for this is utter madness, given that
-	// this check means we can only ever return a single object.
-	if !api.auth.AuthOwner(tag) {
-		err = apiservererrors.ErrPerm
-		return
-	}
 	entity0, err := api.st.FindEntity(tag)
 	if err != nil {
 		return

apiserver/facades/agent/agent/agent_test.go

@@ -89,6 +89,7 @@ func (s *agentSuite) agentAPI(c *gc.C, auth facade.Authorizer, credentialService
 		nil,
 		nil,
 		nil,
+		nil,
 	)
 }
 

apiserver/facades/agent/agent/register.go

@@ -10,7 +10,10 @@ import (
 	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/core/credential"
+	"github.com/juju/juju/core/life"
 	"github.com/juju/juju/core/watcher"
+	"github.com/juju/juju/domain/application/service"
+	"github.com/juju/juju/internal/storage"
 )
 
 // Register is called to expose a package of facades onto a given registry.
@@ -24,6 +27,11 @@ type CredentialService interface {
 	WatchCredential(ctx context.Context, key credential.Key) (watcher.NotifyWatcher, error)
 }
 
+// ApplicationService provides access to the application service.
+type ApplicationService interface {
+	GetUnitLife(ctx context.Context, name string) (life.Value, error)
+}
+
 // NewAgentAPIV3 returns an object implementing version 3 of the Agent API
 // with the given authorizer representing the currently logged in client.
 func NewAgentAPIV3(ctx facade.ModelContext) (*AgentAPI, error) {
@@ -42,6 +50,10 @@ func NewAgentAPIV3(ctx facade.ModelContext) (*AgentAPI, error) {
 		ctx.ServiceFactory().Credential(),
 		ctx.ServiceFactory().Machine(),
 		ctx.ServiceFactory().Config(),
+		ctx.ServiceFactory().Application(service.ApplicationServiceParams{
+			StorageRegistry: storage.NotImplementedProviderRegistry{},
+			Secrets:         service.NotImplementedSecretService{},
+		}),
 		ctx.WatcherRegistry(),
 	)
 }

apiserver/facades/agent/caasapplication/application.go

@@ -19,9 +19,11 @@ import (
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/caas"
 	"github.com/juju/juju/controller"
+	"github.com/juju/juju/core/life"
 	"github.com/juju/juju/core/logger"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/paths"
+	applicationerrors "github.com/juju/juju/domain/application/errors"
 	applicationservice "github.com/juju/juju/domain/application/service"
 	"github.com/juju/juju/internal/password"
 	"github.com/juju/juju/rpc/params"
@@ -37,6 +39,8 @@ type ControllerConfigService interface {
 type ApplicationService interface {
 	RegisterCAASUnit(ctx context.Context, appName string, unit applicationservice.RegisterCAASUnitParams) error
 	CAASUnitTerminating(ctx context.Context, appName string, unitNum int, broker applicationservice.Broker) (bool, error)
+	GetApplicationLife(ctx context.Context, appName string) (life.Value, error)
+	GetUnitLife(ctx context.Context, unitName string) (life.Value, error)
 }
 
 // Facade defines the API methods on the CAASApplication facade.
@@ -93,8 +97,16 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 		return params.CAASUnitIntroductionResult{}, apiservererrors.ErrPerm
 	}
 
+	var unitName string
 	errResp := func(err error) (params.CAASUnitIntroductionResult, error) {
 		f.logger.Warningf("error introducing k8s pod %q: %v", args.PodName, err)
+		if errors.Is(err, applicationerrors.ApplicationNotFound) {
+			err = errors.NotFoundf("appliction %s", tag.Name)
+		} else if errors.Is(err, applicationerrors.UnitAlreadyExists) {
+			err = errors.AlreadyExistsf("unit %s", unitName)
+		} else if errors.Is(err, applicationerrors.UnitNotAssigned) {
+			err = errors.NotAssignedf("unit %s", unitName)
+		}
 		return params.CAASUnitIntroductionResult{Error: apiservererrors.ServerError(err)}, nil
 	}
 
@@ -107,12 +119,13 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 
 	f.logger.Debugf("introducing pod %q (%q)", args.PodName, args.PodUUID)
 
-	application, err := f.state.Application(tag.Name)
+	appName := tag.Name
+	appLife, err := f.applicationService.GetApplicationLife(ctx, appName)
 	if err != nil {
 		return errResp(err)
 	}
 
-	if application.Life() != state.Alive {
+	if appLife != life.Alive {
 		return errResp(errors.NotProvisionedf("application"))
 	}
 
@@ -133,16 +146,16 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 		if err != nil {
 			return errResp(err)
 		}
-		n := fmt.Sprintf("%s/%d", application.Name(), ord)
-		upsert.UnitName = &n
+		unitName = fmt.Sprintf("%s/%d", appName, ord)
+		upsert.UnitName = &unitName
 		upsert.OrderedId = ord
 		upsert.OrderedScale = true
 	default:
 		return errResp(errors.NotSupportedf("unknown deployment type"))
 	}
 
 	// Find the pod/unit in the provider.
-	caasApp := f.broker.Application(application.Name(), caas.DeploymentStateful)
+	caasApp := f.broker.Application(appName, caas.DeploymentStateful)
 	pods, err := caasApp.Units()
 	if err != nil {
 		return errResp(err)
@@ -176,13 +189,7 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 	passwordHash := password.AgentPasswordHash(pass)
 	upsert.PasswordHash = &passwordHash
 
-	// TODO(units) - remove dual write to state
-	_, err = application.UpsertCAASUnit(upsert)
-	if err != nil {
-		return errResp(err)
-	}
-
-	if err := f.applicationService.RegisterCAASUnit(ctx, application.Name(), applicationservice.RegisterCAASUnitParams{
+	if err := f.applicationService.RegisterCAASUnit(ctx, appName, applicationservice.RegisterCAASUnitParams{
 		UnitName:     *upsert.UnitName,
 		ProviderId:   upsert.ProviderId,
 		Address:      upsert.Address,
@@ -194,6 +201,16 @@ func (f *Facade) UnitIntroduction(ctx context.Context, args params.CAASUnitIntro
 		return errResp(err)
 	}
 
+	// TODO(units) - remove dual write to state
+	application, err := f.state.Application(tag.Name)
+	if err != nil {
+		return errResp(err)
+	}
+	_, err = application.UpsertCAASUnit(upsert)
+	if err != nil {
+		return errResp(err)
+	}
+
 	controllerConfig, err := f.controllerConfigService.ControllerConfig(ctx)
 	if err != nil {
 		return errResp(err)
@@ -258,6 +275,9 @@ func (f *Facade) UnitTerminating(ctx context.Context, args params.Entity) (param
 	}
 
 	errResp := func(err error) (params.CAASUnitTerminationResult, error) {
+		if errors.Is(err, applicationerrors.ApplicationNotFound) {
+			err = errors.NotFoundf("application %s", tag.Id())
+		}
 		return params.CAASUnitTerminationResult{Error: apiservererrors.ServerError(err)}, nil
 	}
 
@@ -269,12 +289,11 @@ func (f *Facade) UnitTerminating(ctx context.Context, args params.Entity) (param
 		return params.CAASUnitTerminationResult{}, apiservererrors.ErrPerm
 	}
 
-	// TODO(units): should be in service but we don't keep life up to date yet
-	unit, err := f.state.Unit(unitTag.Id())
+	unitLife, err := f.applicationService.GetUnitLife(ctx, unitTag.Id())
 	if err != nil {
 		return errResp(err)
 	}
-	if unit.Life() != state.Alive {
+	if unitLife != life.Alive {
 		return params.CAASUnitTerminationResult{WillRestart: false}, nil
 	}