Skip to content

Commit 5ea5f39

Browse files
jujubotjujubot
authored
Merge pull request juju#16743 from SimonRichardson/objectstore-lock
juju#16743 Adds a locking mechanism around the persistence of a file to both the disk and the database to prevent any logical races when cleaning up. A clean-up worker is expected to run at a given interval to remove orphaned files from failed put actions or no longer used files. As we write the file first, before storing it in the database, there is a potential race for the worker to clean up the file we're attempting to persist in the database. The locking mechanism leans on the lease mechanics. The holder of the lease will be the objectstore vs the cleanup worker. The granularity of the lock is per file, so we shouldn't be blocking the objectstore when the cleanup worker is running. The lock only wraps the persistence of the tmp file to disk and then writes to the database. Not the writing to the tmp file and the checksum verification. We should be safe to do this, as the cleanup worker isn't concerned with tmp file location. If the file is removed from the tmp location for any reason, we verify its existence before renaming it to the correct location whilst in the lock. The claimer has basic semantics, `Claim`, `Extend`, and `Release`. We claim based on the underlying hash and the claimer is namespace to the objectstore worker. The default duration of an extension is 1 minute, and if we don't release or extend then the lease will be revoked for someone else to claim. The extension attempts to claim every 30 seconds (half the extended duration, for another minute). I don't envisage this being a problem for file-backed storage, but when we move to s3 storage, this will become critical. As the s3 service might not be fast when pruning files. The work is divided into two camps, the `internal/objectstore` contains the claimer interface, which is a simplified version of the lease interfaces and the `internal/worker/objectstore` contains the implementation of the claim. It hides a lot of the complexity away, giving a nice and tidy interface for the locking mechanism. The secretary finder is lifted out of the worker and pushed into the internal/lease package. Thus allowing us to define leases outside of the worker package. Tests have been added to various parts to ensure that it works correctly. ## 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 ## QA steps ```sh $ juju bootstrap lxd test --build-agent $ juju enable-ha $ juju deploy ubuntu -n 3 ``` Tests pass for now. There will be a full round of integration testing once PR juju#16715 has landed. ## Links **Jira card:** JUJU-5266
2 parents 673adb1 + f581c1c commit 5ea5f39

40 files changed

+1546
-233
lines changed

‎cmd/jujud/agent/machine/manifolds.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
coretrace "github.com/juju/juju/core/trace"
3636
containerbroker "github.com/juju/juju/internal/container/broker"
3737
"github.com/juju/juju/internal/container/lxd"
38+
internallease "github.com/juju/juju/internal/lease"
3839
internalobjectstore "github.com/juju/juju/internal/objectstore"
3940
proxyconfig "github.com/juju/juju/internal/proxy/config"
4041
"github.com/juju/juju/internal/servicefactory"
@@ -775,6 +776,7 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds {
775776
PrometheusRegisterer: config.PrometheusRegisterer,
776777
NewWorker: leasemanager.NewWorker,
777778
NewStore: leasemanager.NewStore,
779+
NewSecretaryFinder: internallease.NewSecretaryFinder,
778780
})),
779781

780782
// The proxy config updater is a leaf worker that sets http/https/apt/etc
@@ -822,6 +824,7 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds {
822824
StateName: stateName,
823825
TraceName: traceName,
824826
ServiceFactoryName: serviceFactoryName,
827+
LeaseManagerName: leaseManagerName,
825828
Clock: config.Clock,
826829
Logger: loggo.GetLogger("juju.worker.objectstore"),
827830
NewObjectStoreWorker: internalobjectstore.ObjectStoreFactory,

‎cmd/jujud/agent/machine/manifolds_test.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -547,15 +547,17 @@ var expectedMachineManifoldsWithDependenciesIAAS = map[string][]string{
547547
"agent",
548548
"change-stream",
549549
"charmhub-http-client",
550+
"clock",
550551
"db-accessor",
551552
"file-notify-watcher",
552553
"is-bootstrap-gate",
553554
"is-controller-flag",
555+
"lease-manager",
554556
"object-store",
555557
"query-logger",
556558
"service-factory",
557-
"state",
558559
"state-config-watcher",
560+
"state",
559561
"trace",
560562
"upgrade-database-flag",
561563
"upgrade-database-gate",
@@ -995,13 +997,15 @@ var expectedMachineManifoldsWithDependenciesIAAS = map[string][]string{
995997
"object-store": {
996998
"agent",
997999
"change-stream",
1000+
"clock",
9981001
"db-accessor",
9991002
"file-notify-watcher",
10001003
"is-controller-flag",
1004+
"lease-manager",
10011005
"query-logger",
10021006
"service-factory",
1003-
"state",
10041007
"state-config-watcher",
1008+
"state",
10051009
"trace",
10061010
},
10071011

@@ -1265,15 +1269,17 @@ var expectedMachineManifoldsWithDependenciesCAAS = map[string][]string{
12651269
"agent",
12661270
"change-stream",
12671271
"charmhub-http-client",
1272+
"clock",
12681273
"db-accessor",
12691274
"file-notify-watcher",
12701275
"is-bootstrap-gate",
12711276
"is-controller-flag",
1277+
"lease-manager",
12721278
"object-store",
12731279
"query-logger",
12741280
"service-factory",
1275-
"state",
12761281
"state-config-watcher",
1282+
"state",
12771283
"trace",
12781284
"upgrade-database-flag",
12791285
"upgrade-database-gate",
@@ -1562,13 +1568,15 @@ var expectedMachineManifoldsWithDependenciesCAAS = map[string][]string{
15621568
"object-store": {
15631569
"agent",
15641570
"change-stream",
1571+
"clock",
15651572
"db-accessor",
15661573
"file-notify-watcher",
15671574
"is-controller-flag",
1575+
"lease-manager",
15681576
"query-logger",
15691577
"service-factory",
1570-
"state",
15711578
"state-config-watcher",
1579+
"state",
15721580
"trace",
15731581
},
15741582

‎core/lease/interface.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ const (
1515
// SingularControllerNamespace is the namespace used to manage
1616
// controller leases.
1717
SingularControllerNamespace = "singular-controller"
18+
19+
// ObjectStoreNamespace is the namespace used to manage
20+
// object store files.
21+
ObjectStoreNamespace = "object-store"
1822
)
1923

2024
// Claimer exposes lease acquisition and expiry notification capabilities.

‎core/lease/secertary.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright 2024 Canonical Ltd.
2+
// Licensed under the AGPLv3, see LICENCE file for details.
3+
4+
package lease
5+
6+
import (
7+
"time"
8+
)
9+
10+
// Secretary is responsible for validating the sanity of lease and holder names
11+
// before bothering the manager with them.
12+
type Secretary interface {
13+
14+
// CheckLease returns an error if the supplied lease name is not valid.
15+
CheckLease(key Key) error
16+
17+
// CheckHolder returns an error if the supplied holder name is not valid.
18+
CheckHolder(name string) error
19+
20+
// CheckDuration returns an error if the supplied duration is not valid.
21+
CheckDuration(duration time.Duration) error
22+
}
23+
24+
// SecretaryFinder is responsible for returning the correct Secretary for a
25+
// given namespace.
26+
type SecretaryFinder interface {
27+
// SecretaryFor returns the Secretary for the given namespace.
28+
// Returns an error if the namespace is not valid.
29+
SecretaryFor(namespace string) (Secretary, error)
30+
}

‎core/objectstore/lease.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright 2024 Canonical Ltd.
2+
// Licensed under the AGPLv3, see LICENCE file for details.
3+
4+
package objectstore
5+
6+
import "github.com/juju/errors"
7+
8+
const (
9+
// ObjectStoreLeaseHolderName is the name of the lease holder for the
10+
// object store.
11+
ObjectStoreLeaseHolderName = "objectstore"
12+
)
13+
14+
// ParseLeaseHolderName returns true if the supplied name is a valid lease
15+
// holder.
16+
// This is used to ensure that the lease manager does not attempt to acquire
17+
// leases for invalid names.
18+
func ParseLeaseHolderName(name string) error {
19+
if name == ObjectStoreLeaseHolderName {
20+
return nil
21+
}
22+
return errors.NotValidf("lease holder name %q", name)
23+
}

‎core/objectstore/lease_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright 2024 Canonical Ltd.
2+
// Licensed under the AGPLv3, see LICENCE file for details.
3+
4+
package objectstore
5+
6+
import (
7+
"github.com/juju/errors"
8+
"github.com/juju/testing"
9+
jc "github.com/juju/testing/checkers"
10+
gc "gopkg.in/check.v1"
11+
)
12+
13+
type LeaseSuite struct {
14+
testing.IsolationSuite
15+
}
16+
17+
var _ = gc.Suite(&LeaseSuite{})
18+
19+
func (s *LeaseSuite) TestParseLeaseHolderName(c *gc.C) {
20+
tests := []struct {
21+
name string
22+
expected error
23+
}{{
24+
name: "objectstore",
25+
expected: nil,
26+
}, {
27+
name: "foo",
28+
expected: errors.NotValid,
29+
}}
30+
31+
for i, test := range tests {
32+
c.Logf("test %d: %s", i, test.name)
33+
c.Assert(ParseLeaseHolderName(test.name), jc.ErrorIs, test.expected)
34+
}
35+
}

‎go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ require (
108108
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.21.0
109109
go.opentelemetry.io/otel/sdk v1.21.0
110110
go.opentelemetry.io/otel/trace v1.21.0
111+
go.uber.org/goleak v1.3.0
111112
go.uber.org/mock v0.4.0
112113
golang.org/x/crypto v0.17.0
113114
golang.org/x/net v0.19.0

‎internal/lease/lease.go

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// Copyright 2024 Canonical Ltd.
2+
// Licensed under the AGPLv3, see LICENCE file for details.
3+
4+
package lease
5+
6+
import (
7+
"time"
8+
9+
"github.com/juju/errors"
10+
"github.com/juju/names/v5"
11+
12+
coreagent "github.com/juju/juju/core/agent"
13+
"github.com/juju/juju/core/lease"
14+
"github.com/juju/juju/core/objectstore"
15+
)
16+
17+
// SecretaryFinder is responsible for returning the correct Secretary for a
18+
// given namespace.
19+
type SecretaryFinder struct {
20+
secretaries map[string]lease.Secretary
21+
}
22+
23+
// Register adds a Secretary to the Cabinet.
24+
func (c SecretaryFinder) Register(namespace string, secretary lease.Secretary) {
25+
c.secretaries[namespace] = secretary
26+
}
27+
28+
// SecretaryFor returns the Secretary for the given namespace.
29+
// Returns an error if the namespace is not valid.
30+
func (c SecretaryFinder) SecretaryFor(namespace string) (lease.Secretary, error) {
31+
result, found := c.secretaries[namespace]
32+
if !found {
33+
return nil, errors.NotValidf("namespace %q", namespace)
34+
}
35+
return result, nil
36+
}
37+
38+
// NewCabinet returns a SecretaryFinder with default set of secretaries
39+
// registered with it.
40+
// Note: a cabinet is a group of secretaries.
41+
func NewSecretaryFinder(controllerUUID string) lease.SecretaryFinder {
42+
finder := SecretaryFinder{
43+
secretaries: map[string]lease.Secretary{
44+
lease.SingularControllerNamespace: SingularSecretary{
45+
ControllerUUID: controllerUUID,
46+
},
47+
lease.ApplicationLeadershipNamespace: LeadershipSecretary{},
48+
lease.ObjectStoreNamespace: ObjectStoreSecretary{},
49+
},
50+
}
51+
return finder
52+
}
53+
54+
type baseSecretary struct{}
55+
56+
// CheckDuration is part of the lease.Secretary interface.
57+
func (baseSecretary) CheckDuration(duration time.Duration) error {
58+
if duration <= 0 {
59+
return errors.NewNotValid(nil, "non-positive")
60+
}
61+
return nil
62+
}
63+
64+
// SingularSecretary implements Secretary to restrict claims to either
65+
// a lease for the controller or the specific model it's asking for,
66+
// holdable only by machine-tag strings.
67+
type SingularSecretary struct {
68+
baseSecretary
69+
ControllerUUID string
70+
}
71+
72+
// CheckLease is part of the lease.Secretary interface.
73+
func (s SingularSecretary) CheckLease(key lease.Key) error {
74+
if key.Lease != s.ControllerUUID && key.Lease != key.ModelUUID {
75+
return errors.New("expected controller or model UUID")
76+
}
77+
return nil
78+
}
79+
80+
// CheckHolder is part of the lease.Secretary interface.
81+
func (s SingularSecretary) CheckHolder(name string) error {
82+
tag, err := names.ParseTag(name)
83+
if err != nil {
84+
return errors.Annotate(err, "expected a valid tag")
85+
}
86+
if !coreagent.IsAllowedControllerTag(tag.Kind()) {
87+
return errors.New("expected machine or controller tag")
88+
}
89+
return nil
90+
}
91+
92+
// LeadershipSecretary implements Secretary; it checks that leases are
93+
// application names, and holders are unit names.
94+
type LeadershipSecretary struct {
95+
baseSecretary
96+
}
97+
98+
// CheckLease is part of the lease.Secretary interface.
99+
func (LeadershipSecretary) CheckLease(key lease.Key) error {
100+
if !names.IsValidApplication(key.Lease) {
101+
return errors.NewNotValid(nil, "not an application name")
102+
}
103+
return nil
104+
}
105+
106+
// CheckHolder is part of the lease.Secretary interface.
107+
func (LeadershipSecretary) CheckHolder(name string) error {
108+
if !names.IsValidUnit(name) {
109+
return errors.NewNotValid(nil, "not a unit name")
110+
}
111+
return nil
112+
}
113+
114+
// ObjectStoreSecretary implements Secretary; it checks that leases are
115+
// application names, and holders are unit names.
116+
type ObjectStoreSecretary struct {
117+
baseSecretary
118+
}
119+
120+
// CheckLease is part of the lease.Secretary interface.
121+
func (ObjectStoreSecretary) CheckLease(key lease.Key) error {
122+
if key.Lease == "" {
123+
return errors.NewNotValid(nil, "empty lease name")
124+
}
125+
return nil
126+
}
127+
128+
// CheckHolder is part of the lease.Secretary interface.
129+
func (ObjectStoreSecretary) CheckHolder(name string) error {
130+
return objectstore.ParseLeaseHolderName(name)
131+
}

0 commit comments

Comments
 (0)