Merge pull request #17652 from Aflynn50/use-sqlair-in-upgrade-domain

#17652

This commit changes the upgrade domain to use SQLair, but some other issues were also found with it while changing.

For one, database errors were escaping from state and being caught at the service layer. It makes a lot more sense to catch these errors next to the queries that caused them, especially when the function has multiple queries in it. I have moved all of the catching of these errors onto the actual state.

There were also some comments that didn't match the error that was given out. I have reworked the errors to match what I believe to be the intended behaviour and fixed the callers of the function.

The generic `NotFound` error was also used here. I have changed that to a specific `upgradeerorros.NotFound`.

There were also some tests for `SetControllerDone` that appeared incomplete. I have fixed these and made behaviour expectations consistent across the comments.

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [x] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing
- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps
### Unit tests
```
go test -v -tags=libsqlite3,dqlite ./domain/upgrade/... -check.v -check.f Suite
go test -v -tags=libsqlite3,dqlite ./internal/worker/upgradedatabase/... -check.v -check.f Suite
go test -v -tags=libsqlite3,dqlite ./internal/worker/upgradesteps/... -check.v -check.f Suite
```
### Integration tests
First apply this diff to allow the upgrade:
```diff
diff --git a/environs/sync/sync.go b/environs/sync/sync.go
index fa63d84426..f536f5e434 100644
--- a/environs/sync/sync.go
+++ b/environs/sync/sync.go
@@ -316,12 +316,12 @@ func buildAgentTarball(
 builtVersion.Build = 0
 clientVersion := jujuversion.Current
 clientVersion.Build = 0
- if builtVersion.Number.Compare(clientVersion) != 0 {
- return nil, errors.Errorf(
- "agent binary %v not compatible with bootstrap client %v",
- toolsVersion.Number, jujuversion.Current,
- )
- }
+ // if builtVersion.Number.Compare(clientVersion) != 0 {
+ // return nil, errors.Errorf(
+ // "agent binary %v not compatible with bootstrap client %v",
+ // toolsVersion.Number, jujuversion.Current,
+ // )
+ // }
 fileInfo, err := f.Stat()
 if err != nil {
 return nil, errors.Errorf("cannot stat newly made agent binary archive: %v", err)
```

Then update the patch version in `version/version.go`

```diff
diff --git a/version/version.go b/version/version.go
index 33b01db1af..3f599876ed 100644
--- a/version/version.go
+++ b/version/version.go
@@ -18,7 +18,7 @@ import (
 // The presence and format of this constant is very important.
 // The debian/rules build recipe uses this value for the version
 // number of the release package.
-const version = "4.0-beta4"
+const version = "4.0-beta5"

 // UserAgentVersion defines a user agent version used for communication for
 // outside resources.
```

```sh
$ juju bootstrap lxd test --build-agent
$ juju add-model default
$ juju upgrade-controller --build-agent
```



<!-- Describe steps to verify that the change works. -->

## Documentation changes

<!-- How it affects user workflow (CLI or API). -->

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

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



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

domain/upgrade/errors/errors.go

@@ -8,7 +8,13 @@ import (
 )
 
 const (
-	// ErrUpgradeAlreadyStarted states that the upgrade could not be started.
+	// AlreadyStarted states that the upgrade could not be started.
 	// This error occurs when the upgrade is already in progress.
-	ErrUpgradeAlreadyStarted = errors.ConstError("upgrade already started")
+	AlreadyStarted = errors.ConstError("upgrade already started")
+	// AlreadyExists states that an upgrade operation has already been created.
+	// This error can occur when an upgrade is created.
+	AlreadyExists = errors.ConstError("upgrade already exists")
+	// NotFound states that an upgrade operation cannot be found where one is
+	// expected.
+	NotFound = errors.ConstError("upgrade not found")
 )

domain/upgrade/service/service.go

@@ -5,33 +5,31 @@ package service
 
 import (
 	"context"
-	"database/sql"
 
 	"github.com/juju/errors"
 	"github.com/juju/version/v2"
 
 	"github.com/juju/juju/core/changestream"
 	coredatabase "github.com/juju/juju/core/database"
-	"github.com/juju/juju/core/upgrade"
+	coreupgrade "github.com/juju/juju/core/upgrade"
 	"github.com/juju/juju/core/watcher"
 	"github.com/juju/juju/core/watcher/eventsource"
 	"github.com/juju/juju/domain"
-	domainupgrade "github.com/juju/juju/domain/upgrade"
+	"github.com/juju/juju/domain/upgrade"
 	upgradeerrors "github.com/juju/juju/domain/upgrade/errors"
-	"github.com/juju/juju/internal/database"
 )
 
 // State describes retrieval and persistence methods for upgrade info.
 type State interface {
-	CreateUpgrade(context.Context, version.Number, version.Number) (domainupgrade.UUID, error)
-	SetControllerReady(context.Context, domainupgrade.UUID, string) error
-	AllProvisionedControllersReady(context.Context, domainupgrade.UUID) (bool, error)
-	StartUpgrade(context.Context, domainupgrade.UUID) error
-	SetControllerDone(context.Context, domainupgrade.UUID, string) error
-	ActiveUpgrade(context.Context) (domainupgrade.UUID, error)
-	SetDBUpgradeCompleted(context.Context, domainupgrade.UUID) error
-	SetDBUpgradeFailed(context.Context, domainupgrade.UUID) error
-	UpgradeInfo(context.Context, domainupgrade.UUID) (upgrade.Info, error)
+	CreateUpgrade(context.Context, version.Number, version.Number) (upgrade.UUID, error)
+	SetControllerReady(context.Context, upgrade.UUID, string) error
+	AllProvisionedControllersReady(context.Context, upgrade.UUID) (bool, error)
+	StartUpgrade(context.Context, upgrade.UUID) error
+	SetControllerDone(context.Context, upgrade.UUID, string) error
+	ActiveUpgrade(context.Context) (upgrade.UUID, error)
+	SetDBUpgradeCompleted(context.Context, upgrade.UUID) error
+	SetDBUpgradeFailed(context.Context, upgrade.UUID) error
+	UpgradeInfo(context.Context, upgrade.UUID) (coreupgrade.Info, error)
 }
 
 // WatcherFactory describes methods for creating watchers.
@@ -53,92 +51,71 @@ func NewService(st State, wf WatcherFactory) *Service {
 
 // CreateUpgrade creates an upgrade to and from specified versions
 // If an upgrade is already running/pending, return an AlreadyExists err
-func (s *Service) CreateUpgrade(ctx context.Context, previousVersion, targetVersion version.Number) (domainupgrade.UUID, error) {
+func (s *Service) CreateUpgrade(ctx context.Context, previousVersion, targetVersion version.Number) (upgrade.UUID, error) {
 	if previousVersion.Compare(targetVersion) >= 0 {
 		return "", errors.NotValidf("target version %q must be greater than current version %q", targetVersion, previousVersion)
 	}
-	upgradeUUID, err := s.st.CreateUpgrade(ctx, previousVersion, targetVersion)
-	if database.IsErrConstraintUnique(err) {
-		return "", upgradeerrors.ErrUpgradeAlreadyStarted
-	}
-	return upgradeUUID, domain.CoerceError(err)
+	return s.st.CreateUpgrade(ctx, previousVersion, targetVersion)
 }
 
 // SetControllerReady marks the supplied controllerID as being ready
 // to start its upgrade. All provisioned controllers need to be ready
 // before an upgrade can start
-func (s *Service) SetControllerReady(ctx context.Context, upgradeUUID domainupgrade.UUID, controllerID string) error {
+func (s *Service) SetControllerReady(ctx context.Context, upgradeUUID upgrade.UUID, controllerID string) error {
 	if err := upgradeUUID.Validate(); err != nil {
 		return errors.Trace(err)
 	}
 	err := s.st.SetControllerReady(ctx, upgradeUUID, controllerID)
-	if database.IsErrConstraintForeignKey(err) {
-		return errors.NotFoundf("upgrade %q", upgradeUUID)
-	}
 	return domain.CoerceError(err)
 }
 
 // StartUpgrade starts the current upgrade if it exists
-func (s *Service) StartUpgrade(ctx context.Context, upgradeUUID domainupgrade.UUID) error {
+func (s *Service) StartUpgrade(ctx context.Context, upgradeUUID upgrade.UUID) error {
 	if err := upgradeUUID.Validate(); err != nil {
 		return errors.Trace(err)
 	}
-	err := s.st.StartUpgrade(ctx, upgradeUUID)
-	if database.IsErrNotFound(err) {
-		return errors.NotFoundf("upgrade %q", upgradeUUID)
-	}
-	return domain.CoerceError(err)
+	return s.st.StartUpgrade(ctx, upgradeUUID)
 }
 
 // SetControllerDone marks the supplied controllerID as having
 // completed its upgrades. When SetControllerDone is called by the
 // last provisioned controller, the upgrade will be archived.
-func (s *Service) SetControllerDone(ctx context.Context, upgradeUUID domainupgrade.UUID, controllerID string) error {
+func (s *Service) SetControllerDone(ctx context.Context, upgradeUUID upgrade.UUID, controllerID string) error {
 	if err := upgradeUUID.Validate(); err != nil {
 		return errors.Trace(err)
 	}
-	return domain.CoerceError(s.st.SetControllerDone(ctx, upgradeUUID, controllerID))
+	return s.st.SetControllerDone(ctx, upgradeUUID, controllerID)
 }
 
 // SetDBUpgradeCompleted marks the upgrade as completed in the database
-func (s *Service) SetDBUpgradeCompleted(ctx context.Context, upgradeUUID domainupgrade.UUID) error {
+func (s *Service) SetDBUpgradeCompleted(ctx context.Context, upgradeUUID upgrade.UUID) error {
 	if err := upgradeUUID.Validate(); err != nil {
 		return errors.Trace(err)
 	}
-	err := s.st.SetDBUpgradeCompleted(ctx, upgradeUUID)
-	return domain.CoerceError(err)
+	return s.st.SetDBUpgradeCompleted(ctx, upgradeUUID)
 }
 
 // SetDBUpgradeFailed marks the upgrade as failed in the database.
 // Manual intervention will be required if this has been invoked.
-func (s *Service) SetDBUpgradeFailed(ctx context.Context, upgradeUUID domainupgrade.UUID) error {
+func (s *Service) SetDBUpgradeFailed(ctx context.Context, upgradeUUID upgrade.UUID) error {
 	if err := upgradeUUID.Validate(); err != nil {
 		return errors.Trace(err)
 	}
-	err := s.st.SetDBUpgradeFailed(ctx, upgradeUUID)
-	return domain.CoerceError(err)
+	return s.st.SetDBUpgradeFailed(ctx, upgradeUUID)
 }
 
 // ActiveUpgrade returns the uuid of the current active upgrade.
 // If there are no active upgrades, return a NotFound error
-func (s *Service) ActiveUpgrade(ctx context.Context) (domainupgrade.UUID, error) {
-	activeUpgrade, err := s.st.ActiveUpgrade(ctx)
-	if errors.Is(err, sql.ErrNoRows) {
-		return "", errors.NotFoundf("active upgrade")
-	}
-	return activeUpgrade, domain.CoerceError(err)
+func (s *Service) ActiveUpgrade(ctx context.Context) (upgrade.UUID, error) {
+	return s.st.ActiveUpgrade(ctx)
 }
 
 // UpgradeInfo returns the upgrade info for the supplied upgradeUUID
-func (s *Service) UpgradeInfo(ctx context.Context, upgradeUUID domainupgrade.UUID) (upgrade.Info, error) {
+func (s *Service) UpgradeInfo(ctx context.Context, upgradeUUID upgrade.UUID) (coreupgrade.Info, error) {
 	if err := upgradeUUID.Validate(); err != nil {
-		return upgrade.Info{}, errors.Trace(err)
-	}
-	upgradeInfo, err := s.st.UpgradeInfo(ctx, upgradeUUID)
-	if errors.Is(err, sql.ErrNoRows) {
-		return upgrade.Info{}, errors.NotFoundf("upgrade %q", upgradeUUID)
+		return coreupgrade.Info{}, errors.Trace(err)
 	}
-	return upgradeInfo, domain.CoerceError(err)
+	return s.st.UpgradeInfo(ctx, upgradeUUID)
 }
 
 // IsUpgrading returns true if there is an upgrade in progress.
@@ -149,7 +126,7 @@ func (s *Service) IsUpgrading(ctx context.Context) (bool, error) {
 	if err == nil {
 		return true, nil
 	}
-	if errors.Is(err, errors.NotFound) {
+	if errors.Is(err, upgradeerrors.NotFound) {
 		return false, nil
 	}
 
@@ -174,7 +151,7 @@ func NewWatchableService(st State, wf WatcherFactory) *WatchableService {
 
 // WatchForUpgradeReady creates a watcher which notifies when all controller
 // nodes have been registered, meaning the upgrade is ready to start.
-func (s *WatchableService) WatchForUpgradeReady(ctx context.Context, upgradeUUID domainupgrade.UUID) (watcher.NotifyWatcher, error) {
+func (s *WatchableService) WatchForUpgradeReady(ctx context.Context, upgradeUUID upgrade.UUID) (watcher.NotifyWatcher, error) {
 	if err := upgradeUUID.Validate(); err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -196,7 +173,7 @@ func (s *WatchableService) WatchForUpgradeReady(ctx context.Context, upgradeUUID
 
 // WatchForUpgradeState creates a watcher which notifies when the upgrade
 // has reached the given state.
-func (s *WatchableService) WatchForUpgradeState(ctx context.Context, upgradeUUID domainupgrade.UUID, state upgrade.State) (watcher.NotifyWatcher, error) {
+func (s *WatchableService) WatchForUpgradeState(ctx context.Context, upgradeUUID upgrade.UUID, state coreupgrade.State) (watcher.NotifyWatcher, error) {
 	if err := upgradeUUID.Validate(); err != nil {
 		return nil, errors.Trace(err)
 	}

domain/upgrade/service/service_test.go

@@ -5,12 +5,10 @@ package service
 
 import (
 	"context"
-	"database/sql"
 
 	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	"github.com/juju/version/v2"
-	"github.com/mattn/go-sqlite3"
 	"go.uber.org/mock/gomock"
 	gc "gopkg.in/check.v1"
 
@@ -53,11 +51,10 @@ func (s *serviceSuite) TestCreateUpgrade(c *gc.C) {
 func (s *serviceSuite) TestCreateUpgradeAlreadyExists(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	ucErr := sqlite3.Error{ExtendedCode: sqlite3.ErrConstraintUnique}
-	s.state.EXPECT().CreateUpgrade(gomock.Any(), version.MustParse("3.0.0"), version.MustParse("3.0.1")).Return(s.upgradeUUID, ucErr)
+	s.state.EXPECT().CreateUpgrade(gomock.Any(), version.MustParse("3.0.0"), version.MustParse("3.0.1")).Return(s.upgradeUUID, upgradeerrors.AlreadyExists)
 
 	_, err := s.service.CreateUpgrade(context.Background(), version.MustParse("3.0.0"), version.MustParse("3.0.1"))
-	c.Assert(err, jc.ErrorIs, upgradeerrors.ErrUpgradeAlreadyStarted)
+	c.Assert(err, jc.ErrorIs, upgradeerrors.AlreadyExists)
 }
 
 func (s *serviceSuite) TestCreateUpgradeInvalidVersions(c *gc.C) {
@@ -80,12 +77,11 @@ func (s *serviceSuite) TestSetControllerReady(c *gc.C) {
 func (s *serviceSuite) TestSetControllerReadyForeignKey(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	fkErr := sqlite3.Error{ExtendedCode: sqlite3.ErrConstraintForeignKey}
-	s.state.EXPECT().SetControllerReady(gomock.Any(), s.upgradeUUID, s.controllerUUID).Return(fkErr)
+	s.state.EXPECT().SetControllerReady(gomock.Any(), s.upgradeUUID, s.controllerUUID).Return(upgradeerrors.NotFound)
 
 	err := s.service.SetControllerReady(context.Background(), s.upgradeUUID, s.controllerUUID)
 	c.Log(err)
-	c.Assert(err, jc.ErrorIs, errors.NotFound)
+	c.Assert(err, jc.ErrorIs, upgradeerrors.NotFound)
 }
 
 func (s *serviceSuite) TestStartUpgrade(c *gc.C) {
@@ -100,10 +96,10 @@ func (s *serviceSuite) TestStartUpgrade(c *gc.C) {
 func (s *serviceSuite) TestStartUpgradeBeforeCreated(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	s.state.EXPECT().StartUpgrade(gomock.Any(), s.upgradeUUID).Return(sql.ErrNoRows)
+	s.state.EXPECT().StartUpgrade(gomock.Any(), s.upgradeUUID).Return(upgradeerrors.NotFound)
 
 	err := s.service.StartUpgrade(context.Background(), s.upgradeUUID)
-	c.Assert(err, jc.ErrorIs, errors.NotFound)
+	c.Assert(err, jc.ErrorIs, upgradeerrors.NotFound)
 }
 
 func (s *serviceSuite) TestActiveUpgrade(c *gc.C) {
@@ -119,10 +115,10 @@ func (s *serviceSuite) TestActiveUpgrade(c *gc.C) {
 func (s *serviceSuite) TestActiveUpgradeNoUpgrade(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	s.state.EXPECT().ActiveUpgrade(gomock.Any()).Return(s.upgradeUUID, errors.Trace(sql.ErrNoRows))
+	s.state.EXPECT().ActiveUpgrade(gomock.Any()).Return(s.upgradeUUID, errors.Trace(upgradeerrors.NotFound))
 
 	_, err := s.service.ActiveUpgrade(context.Background())
-	c.Assert(err, jc.ErrorIs, errors.NotFound)
+	c.Assert(err, jc.ErrorIs, upgradeerrors.NotFound)
 }
 
 func (s *serviceSuite) TestSetDBUpgradeCompleted(c *gc.C) {
@@ -189,7 +185,7 @@ func (s *serviceSuite) TestIsUpgrade(c *gc.C) {
 func (s *serviceSuite) TestIsUpgradeNoUpgrade(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	s.state.EXPECT().ActiveUpgrade(gomock.Any()).Return(s.upgradeUUID, errors.Trace(sql.ErrNoRows))
+	s.state.EXPECT().ActiveUpgrade(gomock.Any()).Return(s.upgradeUUID, errors.Trace(upgradeerrors.NotFound))
 
 	upgrading, err := s.service.IsUpgrading(context.Background())
 	c.Assert(err, jc.ErrorIsNil)