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 committed Dec 2, 2024
1 parent 5175f08 commit 2e2a902
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 47 deletions.
3 changes: 2 additions & 1 deletion core/lease/interface.go
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
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)
}
14 changes: 7 additions & 7 deletions domain/services/testing/suite.go
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 Down Expand Up @@ -292,7 +292,7 @@ func (s *DomainServicesSuite) NoopObjectStore(c *gc.C) coreobjectstore.ObjectSto
}

// NoopLeaseManager returns a no-op implementation of the LeaseCheckerWaiter.
func (s *DomainServicesSuite) NoopLeaseManager(c *gc.C) lease.LeaseCheckerWaiter {
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 Down
32 changes: 2 additions & 30 deletions internal/worker/domainservices/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,42 +249,14 @@ type modelApplicationLeaseManager struct {
}

// GetLeaseManager returns a lease manager for the given model UUID.
func (s modelApplicationLeaseManager) GetLeaseManager() (lease.LeaseCheckerWaiter, error) {
func (s modelApplicationLeaseManager) GetLeaseManager() (lease.Checker, error) {
// TODO (stickupkid): These aren't cheap to make, so we should cache them
// and reuse them where possible. I'm not sure these should be workers, I'd
// be happy with a sync.Pool at minimum though.
claimer, err := s.manager.Claimer(lease.ApplicationLeadershipNamespace, s.modelUUID.String())
if err != nil {
return nil, internalerrors.Errorf("getting claim lease manager: %w", err)
}

checker, err := s.manager.Checker(lease.ApplicationLeadershipNamespace, s.modelUUID.String())
if err != nil {
return nil, internalerrors.Errorf("getting checker lease manager: %w", err)
}

return &leaseManager{
claimer: claimer,
checker: checker,
}, nil
}

type leaseManager struct {
claimer lease.Claimer
checker lease.Checker
}

// WaitUntilExpired returns nil when the named lease is no longer held. If
// it returns any error, no reasonable inferences may be made. The supplied
// context can be used to cancel the request; in this case, the method will
// return ErrWaitCancelled.
// The started channel when non-nil is closed when the wait begins.
func (m *leaseManager) WaitUntilExpired(ctx context.Context, leaseName string, started chan<- struct{}) error {
return m.claimer.WaitUntilExpired(ctx, leaseName, started)
}

// Token returns a Token that can be interrogated at any time to discover
// whether the supplied lease is currently held by the supplied holder.
func (m *leaseManager) Token(leaseName, holderName string) lease.Token {
return m.checker.Token(leaseName, holderName)
return checker, nil
}

0 comments on commit 2e2a902

Please sign in to comment.