Skip to content

Commit 74d4075

Browse files
wallyworldwallyworld
committed
Ensure storage constraints are removed when an application is deleted
1 parent 0cb801b commit 74d4075

File tree

5 files changed

+53
-27
lines changed

5 files changed

+53
-27
lines changed

‎state/application.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -275,17 +275,19 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) {
275275
},
276276
}
277277
// Note that appCharmDecRefOps might not catch the final decref
278-
// when run in a transaction that decrefs more than once. In
279-
// this case, luckily, we can be sure that we unconditionally
280-
// need finalAppCharmRemoveOps; and we trust that it's written
281-
// such that it's safe to run multiple times.
278+
// when run in a transaction that decrefs more than once. So we
279+
// avoid attempting to do the final cleanup in the ref dec ops and
280+
// do it explicitly below.
282281
name := a.doc.Name
283282
curl := a.doc.CharmURL
284-
charmOps, err := appCharmDecRefOps(a.st, name, curl)
283+
charmOps, err := appCharmDecRefOps(a.st, name, curl, false)
285284
if err != nil {
286285
return nil, errors.Trace(err)
287286
}
288287
ops = append(ops, charmOps...)
288+
// By the time we get to here, all units and charm refs have been removed,
289+
// so it's safe to do this additonal cleanup.
290+
ops = append(ops, finalAppCharmRemoveOps(name, curl)...)
289291

290292
globalKey := a.globalKey()
291293
ops = append(ops,
@@ -675,7 +677,7 @@ func (a *Application) changeCharmOps(
675677
// Drop the references to the old settings, storage constraints,
676678
// and charm docs (if the refs actually exist yet).
677679
if oldSettings != nil {
678-
decOps, err = appCharmDecRefOps(a.st, a.doc.Name, a.doc.CharmURL) // current charm
680+
decOps, err = appCharmDecRefOps(a.st, a.doc.Name, a.doc.CharmURL, true) // current charm
679681
if err != nil {
680682
return nil, errors.Trace(err)
681683
}
@@ -1256,7 +1258,10 @@ func (a *Application) removeUnitOps(u *Unit, asserts bson.D) ([]txn.Op, error) {
12561258
ops = append(ops, portsOps...)
12571259
ops = append(ops, storageInstanceOps...)
12581260
if u.doc.CharmURL != nil {
1259-
decOps, err := appCharmDecRefOps(a.st, a.doc.Name, u.doc.CharmURL)
1261+
// If the unit has a different URL to the application, allow any final
1262+
// cleanup to happen; otherwise we just do it when the app itself is removed.
1263+
maybeDoFinal := u.doc.CharmURL != a.doc.CharmURL
1264+
decOps, err := appCharmDecRefOps(a.st, a.doc.Name, u.doc.CharmURL, maybeDoFinal)
12601265
if errors.IsNotFound(err) {
12611266
return nil, errRefresh
12621267
} else if err != nil {

‎state/application_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/juju/juju/status"
2626
coretesting "github.com/juju/juju/testing"
2727
"github.com/juju/juju/testing/factory"
28+
"gopkg.in/juju/names.v2"
2829
)
2930

3031
type ApplicationSuite struct {
@@ -1728,6 +1729,37 @@ func (s *ApplicationSuite) TestRemoveServiceMachine(c *gc.C) {
17281729
assertLife(c, machine, state.Dying)
17291730
}
17301731

1732+
func (s *ApplicationSuite) TestApplicationCleanupRemovesStorageConstraints(c *gc.C) {
1733+
ch := s.AddTestingCharm(c, "storage-block")
1734+
storage := map[string]state.StorageConstraints{
1735+
"data": makeStorageCons("loop", 1024, 1),
1736+
}
1737+
app := s.AddTestingServiceWithStorage(c, "storage-block", ch, storage)
1738+
u, err := app.AddUnit()
1739+
c.Assert(err, jc.ErrorIsNil)
1740+
err = u.SetCharmURL(ch.URL())
1741+
c.Assert(err, jc.ErrorIsNil)
1742+
1743+
c.Assert(app.Destroy(), gc.IsNil)
1744+
assertLife(c, app, state.Dying)
1745+
assertCleanupCount(c, s.State, 2)
1746+
1747+
// These next API calls are normally done by the uniter.
1748+
err = s.State.RemoveStorageAttachment(names.NewStorageTag("data/0"), u.UnitTag())
1749+
c.Assert(err, jc.ErrorIsNil)
1750+
err = u.EnsureDead()
1751+
c.Assert(err, jc.ErrorIsNil)
1752+
err = u.Remove()
1753+
c.Assert(err, jc.ErrorIsNil)
1754+
1755+
// Ensure storage constraints and settings are now gone.
1756+
_, err = state.AppStorageConstraints(app)
1757+
c.Assert(err, jc.Satisfies, errors.IsNotFound)
1758+
settings := state.GetApplicationSettings(s.State, app)
1759+
err = settings.Read()
1760+
c.Assert(err, jc.Satisfies, errors.IsNotFound)
1761+
}
1762+
17311763
func (s *ApplicationSuite) TestRemoveQueuesLocalCharmCleanup(c *gc.C) {
17321764
s.assertNoCleanup(c)
17331765

‎state/charmref.go

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,13 @@ func appCharmIncRefOps(st modelBackend, appName string, curl *charm.URL, canCrea
6161

6262
// appCharmDecRefOps returns the operations necessary to delete a
6363
// reference to a charm and its per-application settings and storage
64-
// constraints document. If no references to a given (app, charm) pair
64+
// constraints document.
65+
// If maybeDoFinal is true, and no references to a given (app, charm) pair
6566
// remain, the operations returned will also remove the settings and
6667
// storage constraints documents for that pair, and schedule a cleanup
6768
// to see if the charm itself is now unreferenced and can be tidied
6869
// away itself.
69-
func appCharmDecRefOps(st modelBackend, appName string, curl *charm.URL) ([]txn.Op, error) {
70+
func appCharmDecRefOps(st modelBackend, appName string, curl *charm.URL, maybeDoFinal bool) ([]txn.Op, error) {
7071
refcounts, closer := st.db().GetCollection(refcountsC)
7172
defer closer()
7273

@@ -89,7 +90,7 @@ func appCharmDecRefOps(st modelBackend, appName string, curl *charm.URL) ([]txn.
8990
}
9091

9192
ops := []txn.Op{settingsOp, storageConstraintsOp, charmOp}
92-
if isFinal {
93+
if isFinal && maybeDoFinal {
9394
// XXX(fwereade): this construction, in common with ~all
9495
// our refcount logic, is safe in parallel but not in
9596
// serial. If this logic is used twice while composing a
@@ -142,19 +143,3 @@ func charmDestroyOps(st modelBackend, curl *charm.URL) ([]txn.Op, error) {
142143

143144
return []txn.Op{charmOp, refcountOp}, nil
144145
}
145-
146-
// charmRemoveOps implements the logic of charm.Remove.
147-
func charmRemoveOps(st modelBackend, curl *charm.URL) ([]txn.Op, error) {
148-
charms, closer := st.db().GetCollection(charmsC)
149-
defer closer()
150-
151-
charmKey := curl.String()
152-
153-
// Remove the charm document as long as the charm is dying.
154-
charmOp, err := nsLife.dyingOp(charms, charmKey)
155-
if err != nil {
156-
return nil, errors.Annotate(err, "charm")
157-
}
158-
charmOp.Remove = true
159-
return []txn.Op{charmOp}, nil
160-
}

‎state/export_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -684,3 +684,7 @@ func GetControllerSettings(st *State) *Settings {
684684
func NewSLALevel(level string) (slaLevel, error) {
685685
return newSLALevel(level)
686686
}
687+
688+
func AppStorageConstraints(app *Application) (map[string]StorageConstraints, error) {
689+
return readStorageConstraints(app.st, app.storageConstraintsKey())
690+
}

‎state/unit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,7 @@ func (u *Unit) SetCharmURL(curl *charm.URL) error {
11351135
})
11361136
if u.doc.CharmURL != nil {
11371137
// Drop the reference to the old charm.
1138-
decOps, err := appCharmDecRefOps(u.st, u.doc.Application, u.doc.CharmURL)
1138+
decOps, err := appCharmDecRefOps(u.st, u.doc.Application, u.doc.CharmURL, true)
11391139
if err != nil {
11401140
return nil, errors.Trace(err)
11411141
}

0 commit comments

Comments
 (0)