Skip to content

Commit

Permalink
refactor: simplify model lease manager
Browse files Browse the repository at this point in the history 
Instead of getting the lease manager twice, just include Waiter in the
interface for checker and get the lease secretary once.

This is a reasonable capability includion into the checker interface and
saves us an unecessasry creation of a second secretary.
  • Loading branch information
@manadart
manadart committed Dec 2, 2024
1 parent 5175f08 commit 9f19f27
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 140 deletions.
3 changes: 2 additions & 1 deletion core/lease/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ type Pinner interface {
Pinned() (map[string][]string, error)
}

// Checker exposes facts about lease ownership.
// Checker exposes facts about lease ownership and expiry.
type Checker interface {
Waiter

// Token returns a Token that can be interrogated at any time to discover
// whether the supplied lease is currently held by the supplied holder.
Expand Down
11 changes: 2 additions & 9 deletions core/lease/lease.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,17 @@ package lease

import "github.com/juju/juju/core/model"

// LeaseCheckerWaiter is an interface that checks and waits if a lease is held
// by a holder.
type LeaseCheckerWaiter interface {
Waiter
Checker
}

// LeaseManagerGetter is an interface that provides a method to get a lease
// manager for a given lease using its UUID. The lease namespace could be a
// model or an application.
type LeaseManagerGetter interface {
// GetLeaseManager returns a lease manager for the given model UUID.
GetLeaseManager(model.UUID) (LeaseCheckerWaiter, error)
GetLeaseManager(model.UUID) (Checker, error)
}

// ModelLeaseManagerGetter is an interface that provides a method to
// get a lease manager in the scope of a model.
type ModelLeaseManagerGetter interface {
// GetLeaseManager returns a lease manager for the given model UUID.
GetLeaseManager() (LeaseCheckerWaiter, error)
GetLeaseManager() (Checker, error)
}
72 changes: 36 additions & 36 deletions domain/lease_mock_test.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 12 additions & 12 deletions domain/leaseservice_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import (
type leaseServiceSuite struct {
testing.IsolationSuite

modelLeaseManager *MockModelLeaseManagerGetter
leaseCheckerWaiter *MockLeaseCheckerWaiter
token *MockToken
modelLeaseManager *MockModelLeaseManagerGetter
leaseChecker *MockChecker
token *MockToken
}

var _ = gc.Suite(&leaseServiceSuite{})
Expand All @@ -33,7 +33,7 @@ func (s *leaseServiceSuite) TestWithLease(c *gc.C) {
done := make(chan struct{})

// Force the lease wait to be triggered.
s.leaseCheckerWaiter.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
close(start)

// Don't return until the lease function is done.
Expand All @@ -46,7 +46,7 @@ func (s *leaseServiceSuite) TestWithLease(c *gc.C) {
})

// Check we correctly hold the lease.
s.leaseCheckerWaiter.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.token.EXPECT().Check().Return(nil)

service := NewLeaseService(s.modelLeaseManager)
Expand All @@ -64,7 +64,7 @@ func (s *leaseServiceSuite) TestWithLease(c *gc.C) {
func (s *leaseServiceSuite) TestWithLeaseWaitReturnsError(c *gc.C) {
defer s.setupMocks(c).Finish()

s.leaseCheckerWaiter.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
return fmt.Errorf("not holding lease")
})

Expand All @@ -87,7 +87,7 @@ func (s *leaseServiceSuite) TestWithLeaseWaitHasLeaseChange(c *gc.C) {

// Cause the start to be triggered right away, but ensure that the
// lease has changed.
s.leaseCheckerWaiter.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
close(start)

select {
Expand All @@ -102,7 +102,7 @@ func (s *leaseServiceSuite) TestWithLeaseWaitHasLeaseChange(c *gc.C) {
})

// Check we correctly hold the lease.
s.leaseCheckerWaiter.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.token.EXPECT().Check().Return(nil)

service := NewLeaseService(s.modelLeaseManager)
Expand Down Expand Up @@ -145,7 +145,7 @@ func (s *leaseServiceSuite) TestWithLeaseFailsOnWaitCheck(c *gc.C) {

// Cause the start to be triggered right away, but ensure that the
// lease has changed.
s.leaseCheckerWaiter.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
close(start)

select {
Expand All @@ -157,7 +157,7 @@ func (s *leaseServiceSuite) TestWithLeaseFailsOnWaitCheck(c *gc.C) {
})

// Fail the lease check.
s.leaseCheckerWaiter.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)
s.token.EXPECT().Check().Return(errors.Errorf("not holding lease"))

service := NewLeaseService(s.modelLeaseManager)
Expand All @@ -177,10 +177,10 @@ func (s *leaseServiceSuite) setupMocks(c *gc.C) *gomock.Controller {
ctrl := gomock.NewController(c)

s.modelLeaseManager = NewMockModelLeaseManagerGetter(ctrl)
s.leaseCheckerWaiter = NewMockLeaseCheckerWaiter(ctrl)
s.leaseChecker = NewMockChecker(ctrl)
s.token = NewMockToken(ctrl)

s.modelLeaseManager.EXPECT().GetLeaseManager().Return(s.leaseCheckerWaiter, nil).AnyTimes()
s.modelLeaseManager.EXPECT().GetLeaseManager().Return(s.leaseChecker, nil).AnyTimes()

return ctrl
}
2 changes: 1 addition & 1 deletion domain/package_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

//go:generate go run go.uber.org/mock/mockgen -typed -package domain -destination changestream_mock_test.go github.com/juju/juju/core/changestream Subscription,EventSource
//go:generate go run go.uber.org/mock/mockgen -typed -package domain -destination lease_mock_test.go github.com/juju/juju/core/lease Token,LeaseCheckerWaiter,ModelLeaseManagerGetter
//go:generate go run go.uber.org/mock/mockgen -typed -package domain -destination lease_mock_test.go github.com/juju/juju/core/lease Token,Checker,ModelLeaseManagerGetter

func TestPackage(t *testing.T) {
defer goleak.VerifyNone(t)
Expand Down
18 changes: 9 additions & 9 deletions domain/services/testing/suite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func (s *DomainServicesSuite) SeedModelDatabases(c *gc.C) {

// DomainServicesGetter provides an implementation of the DomainServicesGetter
// interface to use in tests. This includes the dummy storage registry.
func (s *DomainServicesSuite) DomainServicesGetter(c *gc.C, objectStore coreobjectstore.ObjectStore, leaseManager lease.LeaseCheckerWaiter) DomainServicesGetterFunc {
func (s *DomainServicesSuite) DomainServicesGetter(c *gc.C, objectStore coreobjectstore.ObjectStore, leaseManager lease.Checker) DomainServicesGetterFunc {
return s.DomainServicesGetterWithStorageRegistry(c, objectStore, leaseManager, storage.ChainedProviderRegistry{
// Using the dummy storage provider for testing purposes isn't
// ideal. We should potentially use a mock storage provider
Expand All @@ -249,7 +249,7 @@ func (s *DomainServicesSuite) DomainServicesGetter(c *gc.C, objectStore coreobje
// DomainServicesGetterWithStorageRegistry provides an implementation of the
// DomainServicesGetterWithStorageRegistry interface to use in tests with the
// additional storage provider.
func (s *DomainServicesSuite) DomainServicesGetterWithStorageRegistry(c *gc.C, objectStore coreobjectstore.ObjectStore, leaseManager lease.LeaseCheckerWaiter, storageRegistry storage.ProviderRegistry) DomainServicesGetterFunc {
func (s *DomainServicesSuite) DomainServicesGetterWithStorageRegistry(c *gc.C, objectStore coreobjectstore.ObjectStore, leaseManager lease.Checker, storageRegistry storage.ProviderRegistry) DomainServicesGetterFunc {
return func(modelUUID coremodel.UUID) services.DomainServices {
return domainservicefactory.NewDomainServices(
databasetesting.ConstFactory(s.TxnRunner()),
Expand All @@ -264,7 +264,7 @@ func (s *DomainServicesSuite) DomainServicesGetterWithStorageRegistry(c *gc.C, o
return storageRegistry, nil
}),
sshimporter.NewImporter(&http.Client{}),
modelApplicationLeaseManagerGetter(func() lease.LeaseCheckerWaiter {
modelApplicationLeaseManagerGetter(func() lease.Checker {
return leaseManager
}),
clock.WallClock,
Expand All @@ -291,8 +291,8 @@ func (s *DomainServicesSuite) NoopObjectStore(c *gc.C) coreobjectstore.ObjectSto
return TestingObjectStore{}
}

// NoopLeaseManager returns a no-op implementation of the LeaseCheckerWaiter.
func (s *DomainServicesSuite) NoopLeaseManager(c *gc.C) lease.LeaseCheckerWaiter {
// NoopLeaseManager returns a no-op implementation of lease.Checker.
func (s *DomainServicesSuite) NoopLeaseManager(c *gc.C) lease.Checker {
return TestingLeaseManager{}
}

Expand Down Expand Up @@ -345,9 +345,9 @@ func (s modelStorageRegistryGetter) GetStorageRegistry(ctx context.Context) (sto
return s(ctx)
}

type modelApplicationLeaseManagerGetter func() lease.LeaseCheckerWaiter
type modelApplicationLeaseManagerGetter func() lease.Checker

func (s modelApplicationLeaseManagerGetter) GetLeaseManager() (lease.LeaseCheckerWaiter, error) {
func (s modelApplicationLeaseManagerGetter) GetLeaseManager() (lease.Checker, error) {
return s(), nil
}

Expand All @@ -365,7 +365,7 @@ func (TestingObjectStore) Put(ctx context.Context, path string, r io.Reader, siz
return "", nil
}

// Put stores data from reader at path, namespaced to the model.
// PutAndCheckHash stores data from reader at path, namespaced to the model.
// It also ensures the stored data has the correct hash.
func (TestingObjectStore) PutAndCheckHash(ctx context.Context, path string, r io.Reader, size int64, hash string) (objectstore.UUID, error) {
return "", nil
Expand All @@ -376,7 +376,7 @@ func (TestingObjectStore) Remove(ctx context.Context, path string) error {
return nil
}

// TestingLeaseManager is a testing implementation of the LeaseCheckerWaiter
// TestingLeaseManager is a testing implementation of the lease.Checker
// interface. It returns canned responses for the methods.
type TestingLeaseManager struct{}

Expand Down
Loading

0 comments on commit 9f19f27

Please sign in to comment.