Skip to content

Commit 997b0c0

Browse files
anastasiamacanastasiamac
committed
Watch model entities references instead of individual collections to avoid racing.
1 parent 7b6c96a commit 997b0c0

File tree

11 files changed

+56
-121
lines changed

11 files changed

+56
-121
lines changed

‎apiserver/common/watch.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func NewMultiNotifyWatcher(w ...state.NotifyWatcher) *MultiNotifyWatcher {
128128
// sending.
129129
func (w *MultiNotifyWatcher) loop(in <-chan struct{}) {
130130
defer close(w.changes)
131-
// out is initialised to m.changes to send the inital event.
131+
// out is initialised to m.changes to send the initial event.
132132
out := w.changes
133133
var timer <-chan time.Time
134134
for {

‎apiserver/undertaker/mock_test.go

Lines changed: 9 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -21,24 +21,13 @@ type mockState struct {
2121
env *mockModel
2222
removed bool
2323
isSystem bool
24-
machines []undertaker.Machine
25-
services []undertaker.Service
24+
25+
watcher state.NotifyWatcher
2626
}
2727

2828
var _ undertaker.State = (*mockState)(nil)
2929

3030
func newMockState(envOwner names.UserTag, envName string, isSystem bool) *mockState {
31-
machine := &mockMachine{
32-
watcher: &mockWatcher{
33-
changes: make(chan struct{}, 1),
34-
},
35-
}
36-
service := &mockService{
37-
watcher: &mockWatcher{
38-
changes: make(chan struct{}, 1),
39-
},
40-
}
41-
4231
env := mockModel{
4332
owner: envOwner,
4433
name: envName,
@@ -49,8 +38,9 @@ func newMockState(envOwner names.UserTag, envName string, isSystem bool) *mockSt
4938
m := &mockState{
5039
env: &env,
5140
isSystem: isSystem,
52-
machines: []undertaker.Machine{machine},
53-
services: []undertaker.Service{service},
41+
watcher: &mockWatcher{
42+
changes: make(chan struct{}, 1),
43+
},
5444
}
5545
return m
5646
}
@@ -78,14 +68,6 @@ func (m *mockState) ProcessDyingModel() error {
7868
return nil
7969
}
8070

81-
func (m *mockState) AllMachines() ([]undertaker.Machine, error) {
82-
return m.machines, nil
83-
}
84-
85-
func (m *mockState) AllApplications() ([]undertaker.Service, error) {
86-
return m.services, nil
87-
}
88-
8971
func (m *mockState) IsController() bool {
9072
return m.isSystem
9173
}
@@ -105,6 +87,10 @@ func (m *mockState) FindEntity(tag names.Tag) (state.Entity, error) {
10587
return nil, errors.NotFoundf("entity with tag %q", tag.String())
10688
}
10789

90+
func (m *mockState) WatchModelEntitiesReferences(mUUID string) state.NotifyWatcher {
91+
return m.watcher
92+
}
93+
10894
// mockModel implements Model interface and allows inspection of called
10995
// methods.
11096
type mockModel struct {
@@ -153,24 +139,6 @@ func (m *mockModel) SetStatus(sInfo status.StatusInfo) error {
153139
return nil
154140
}
155141

156-
type mockMachine struct {
157-
watcher state.NotifyWatcher
158-
err error
159-
}
160-
161-
func (m *mockMachine) Watch() state.NotifyWatcher {
162-
return m.watcher
163-
}
164-
165-
type mockService struct {
166-
watcher state.NotifyWatcher
167-
err error
168-
}
169-
170-
func (s *mockService) Watch() state.NotifyWatcher {
171-
return s.watcher
172-
}
173-
174142
type mockWatcher struct {
175143
state.NotifyWatcher
176144
changes chan struct{}

‎apiserver/undertaker/state.go

Lines changed: 4 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
package undertaker
55

66
import (
7-
"github.com/juju/errors"
87
"gopkg.in/juju/names.v2"
98

109
"github.com/juju/juju/environs/config"
@@ -31,62 +30,18 @@ type State interface {
3130
// collections.
3231
RemoveAllModelDocs() error
3332

34-
// AllMachines returns all machines in the model ordered by id.
35-
AllMachines() ([]Machine, error)
36-
37-
// AllApplications returns all deployed services in the model.
38-
AllApplications() ([]Service, error)
39-
4033
// ModelConfig retrieves the model configuration.
4134
ModelConfig() (*config.Config, error)
35+
36+
// WatchModelEntitiesReferences gets a watcher capable of monitoring
37+
// model entities references changes.
38+
WatchModelEntitiesReferences(mUUID string) state.NotifyWatcher
4239
}
4340

4441
type stateShim struct {
4542
*state.State
4643
}
4744

48-
func (s *stateShim) AllMachines() ([]Machine, error) {
49-
stateMachines, err := s.State.AllMachines()
50-
if err != nil {
51-
return nil, errors.Trace(err)
52-
}
53-
54-
machines := make([]Machine, len(stateMachines))
55-
for i := range stateMachines {
56-
machines[i] = stateMachines[i]
57-
}
58-
59-
return machines, nil
60-
}
61-
62-
// Machine defines the needed methods of state.Machine for
63-
// the work of the undertaker API.
64-
type Machine interface {
65-
// Watch returns a watcher for observing changes to a machine.
66-
Watch() state.NotifyWatcher
67-
}
68-
69-
func (s *stateShim) AllApplications() ([]Service, error) {
70-
stateServices, err := s.State.AllApplications()
71-
if err != nil {
72-
return nil, errors.Trace(err)
73-
}
74-
75-
services := make([]Service, len(stateServices))
76-
for i := range stateServices {
77-
services[i] = stateServices[i]
78-
}
79-
80-
return services, nil
81-
}
82-
83-
// Service defines the needed methods of state.Service for
84-
// the work of the undertaker API.
85-
type Service interface {
86-
// Watch returns a watcher for observing changes to a service.
87-
Watch() state.NotifyWatcher
88-
}
89-
9045
func (s *stateShim) Model() (Model, error) {
9146
return s.State.Model()
9247
}

‎apiserver/undertaker/undertaker.go

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -85,27 +85,18 @@ func (u *UndertakerAPI) RemoveModel() error {
8585
return u.st.RemoveAllModelDocs()
8686
}
8787

88-
func (u *UndertakerAPI) environResourceWatcher() params.NotifyWatchResult {
88+
func (u *UndertakerAPI) modelEntitiesWatcher() params.NotifyWatchResult {
8989
var nothing params.NotifyWatchResult
90-
machines, err := u.st.AllMachines()
91-
if err != nil {
92-
nothing.Error = common.ServerError(err)
93-
return nothing
94-
}
95-
services, err := u.st.AllApplications()
90+
91+
m, err := u.st.Model()
9692
if err != nil {
9793
nothing.Error = common.ServerError(err)
9894
return nothing
9995
}
100-
var watchers []state.NotifyWatcher
101-
for _, machine := range machines {
102-
watchers = append(watchers, machine.Watch())
103-
}
104-
for _, service := range services {
105-
watchers = append(watchers, service.Watch())
106-
}
10796

108-
watch := common.NewMultiNotifyWatcher(watchers...)
97+
// (anastasiamac 2017-04-10) I do not know what watcher is best suited here.
98+
// Keeping multi-notify watcher for historical reasons for now.
99+
watch := common.NewMultiNotifyWatcher(u.st.WatchModelEntitiesReferences(m.UUID()))
109100

110101
if _, ok := <-watch.Changes(); ok {
111102
return params.NotifyWatchResult{
@@ -121,7 +112,7 @@ func (u *UndertakerAPI) environResourceWatcher() params.NotifyWatchResult {
121112
func (u *UndertakerAPI) WatchModelResources() params.NotifyWatchResults {
122113
return params.NotifyWatchResults{
123114
Results: []params.NotifyWatchResult{
124-
u.environResourceWatcher(),
115+
u.modelEntitiesWatcher(),
125116
},
126117
}
127118
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ func Manifolds(config ManifoldsConfig) dependency.Manifolds {
197197

198198
// The leadership tracker attempts to secure and retain leadership of
199199
// the unit's service, and is consulted on such matters by the
200-
// uniter. As it stannds today, we'll need one per unit in a
200+
// uniter. As it stands today, we'll need one per unit in a
201201
// consolidated agent.
202202
leadershipTrackerName: ifNotMigrating(leadership.Manifold(leadership.ManifoldConfig{
203203
AgentName: agentName,

‎state/application.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -272,10 +272,6 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) {
272272
Id: a.doc.DocID,
273273
Assert: asserts,
274274
Remove: true,
275-
}, {
276-
C: settingsC,
277-
Id: a.settingsKey(),
278-
Remove: true,
279275
},
280276
}
281277
// Note that appCharmDecRefOps might not catch the final decref
@@ -290,7 +286,6 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) {
290286
return nil, errors.Trace(err)
291287
}
292288
ops = append(ops, charmOps...)
293-
ops = append(ops, finalAppCharmRemoveOps(name, curl)...)
294289

295290
globalKey := a.globalKey()
296291
ops = append(ops,

‎state/charm_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -256,12 +256,10 @@ func (s *CharmSuite) TestDestroyFinalUnitReference(c *gc.C) {
256256
app := s.Factory.MakeApplication(c, &factory.ApplicationParams{
257257
Charm: s.charm,
258258
})
259-
unit := s.Factory.MakeUnit(c, &factory.UnitParams{
260-
Application: app,
261-
SetCharmURL: true,
262-
})
259+
unit, err := app.AddUnit()
260+
c.Assert(err, jc.ErrorIsNil)
263261

264-
err := app.Destroy()
262+
err = app.Destroy()
265263
c.Assert(err, jc.ErrorIsNil)
266264
removeUnit(c, unit)
267265

‎state/model.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,10 @@ type modelMeterStatusdoc struct {
143143

144144
// modelEntityRefsDoc records references to the top-level entities
145145
// in the model.
146+
// (anastasiamac 2017-04-10) This is also used to determine if a model can be destroyed.
147+
// Consequently, any changes, especially additions of entities, here,
148+
// would need to be reflected, at least, in Model.checkEmpty(...) as well as
149+
// Model.destroyOps(...)
146150
type modelEntityRefsDoc struct {
147151
UUID string `bson:"_id"`
148152

@@ -910,6 +914,8 @@ func (m *Model) destroyOps(ensureNoHostedModels, ensureEmpty bool) ([]txn.Op, er
910914
Assert: bson.D{
911915
{"machines", bson.D{{"$size", 0}}},
912916
{"applications", bson.D{{"$size", 0}}},
917+
{"volumes", bson.D{{"$size", 0}}},
918+
{"filesystems", bson.D{{"$size", 0}}},
913919
},
914920
}}
915921
if !m.isControllerModel() {
@@ -1040,16 +1046,22 @@ func (m *Model) checkEmpty() error {
10401046
}
10411047
return errors.Annotatef(err, "getting entity references for model %s", m.UUID())
10421048
}
1049+
// These errors could be potentially swallowed as we re-try to destroy model.
1050+
// Let's, at least, log them for observations.
10431051
if n := len(doc.Machines); n > 0 {
1052+
logger.Infof("model is still not empty, has machines: %v", doc.Machines)
10441053
return errors.Errorf("model not empty, found %d machine(s)", n)
10451054
}
10461055
if n := len(doc.Applications); n > 0 {
1056+
logger.Infof("model is still not empty, has applications: %v", doc.Applications)
10471057
return errors.Errorf("model not empty, found %d application(s)", n)
10481058
}
10491059
if n := len(doc.Volumes); n > 0 {
1060+
logger.Infof("model is still not empty, has volumes: %v", doc.Volumes)
10501061
return errors.Errorf("model not empty, found %d volume(s)", n)
10511062
}
10521063
if n := len(doc.Filesystems); n > 0 {
1064+
logger.Infof("model is still not empty, has file systems: %v", doc.Filesystems)
10531065
return errors.Errorf("model not empty, found %d filesystem(s)", n)
10541066
}
10551067
return nil

‎state/watcher.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1343,7 +1343,7 @@ func (u *Unit) Watch() NotifyWatcher {
13431343
return newEntityWatcher(u.st, unitsC, u.doc.DocID)
13441344
}
13451345

1346-
// Watch returns a watcher for observing changes to an model.
1346+
// Watch returns a watcher for observing changes to a model.
13471347
func (e *Model) Watch() NotifyWatcher {
13481348
return newEntityWatcher(e.st, modelsC, e.doc.UUID)
13491349
}
@@ -1366,6 +1366,12 @@ func (st *State) WatchForModelConfigChanges() NotifyWatcher {
13661366
return newEntityWatcher(st, settingsC, st.docID(modelGlobalKey))
13671367
}
13681368

1369+
// WatchModelEntitiesReferences returns a NotifyWatcher waiting for the Model
1370+
// Entities references to change for specified model.
1371+
func (st *State) WatchModelEntitiesReferences(mUUID string) NotifyWatcher {
1372+
return newEntityWatcher(st, modelEntityRefsC, mUUID)
1373+
}
1374+
13691375
// WatchForUnitAssignment watches for new services that request units to be
13701376
// assigned to machines.
13711377
func (st *State) WatchForUnitAssignment() StringsWatcher {

‎worker/undertaker/undertaker.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
package undertaker
55

66
import (
7+
"fmt"
8+
79
"github.com/juju/errors"
810

911
"github.com/juju/juju/apiserver/params"
@@ -154,6 +156,7 @@ func (u *Undertaker) processDyingModel() error {
154156
}
155157
defer watcher.Kill() // The watcher is not needed once this func returns.
156158

159+
attempt := 1
157160
for {
158161
select {
159162
case <-u.catacomb.Dying():
@@ -172,7 +175,10 @@ func (u *Undertaker) processDyingModel() error {
172175
// destroy any remaining environ resources.
173176
return nil
174177
}
175-
// Yes, we ignore the error. See comment above.
178+
// Yes, we ignore the error. See comment above. But let's at least
179+
// surface it in status.
180+
u.setStatus(status.Destroying, fmt.Sprintf("%d attempt to destroy model: %v", attempt, err))
176181
}
182+
attempt++
177183
}
178184
}

‎worker/undertaker/undertaker_test.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,10 @@ func (s *UndertakerSuite) TestProcessDyingModelErrorRetried(c *gc.C) {
119119
nil, // ModelInfo
120120
nil, // SetStatus
121121
nil, // WatchModelResources,
122-
errors.New("meh, will retry"), // ProcessDyingModel,
122+
errors.New("meh, will retry"), // ProcessDyingModel,
123+
nil, // SetStatus
123124
errors.New("will retry again"), // ProcessDyingModel,
125+
nil, // SetStatus
124126
nil, // ProcessDyingModel,
125127
nil, // SetStatus
126128
nil, // Destroy,
@@ -134,7 +136,9 @@ func (s *UndertakerSuite) TestProcessDyingModelErrorRetried(c *gc.C) {
134136
"SetStatus",
135137
"WatchModelResources",
136138
"ProcessDyingModel",
139+
"SetStatus",
137140
"ProcessDyingModel",
141+
"SetStatus",
138142
"ProcessDyingModel",
139143
"SetStatus",
140144
"Destroy",

0 commit comments

Comments
 (0)