Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
state: make storage persistent by default #6795
Conversation
|
QA (combined with #6673)
|
|
What are doing when we are removing units/application with persistent storage? Do we need to ask user confirmation whether persistent storage needs to be removed? Do we default to removing it unless there is an explicit "get rid of persistent storage" option set? |
When this change goes ahead, we'll only remove the storage when (a) the user explicitly says so (runs a remove- command), or (b) when the model is destroyed (or machine, for inherently machine-bound storage, such as loop, tmpfs, etc.). We may want to extend the remove-application/remove-unit/remove-machine commands later to give the user a chance to make a choice up front; that can be layered on top of this change. |
| + for _, v := range volumes { | ||
| + err := st.DestroyVolume(v.VolumeTag()) | ||
| + if err != nil && !IsContainsFilesystem(err) { | ||
| + return errors.Trace(err) |
wallyworld
Jan 17, 2017
Owner
I wonder if this error should be typed so that we can avoid log noise. Wouldn't it be the case that the filesystem cleanup would take care of the underlying volume? So a dying model should see both filesystems and volumes destroyed as expected.
axw
Jan 18, 2017
Member
I don't follow. DestroyVolume returns a typed error if the volume contains a filesystem. If you destroy the filesystem, and the volume is bound to the filesystem, the volume will be destroyed when the filesystem is removed from the model.
| + return nil | ||
| +} | ||
| + | ||
| +// cleanupApplicationsForDyingModel sets all services to Dying, if they are |
| buildTxn := func(attempt int) ([]txn.Op, error) { | ||
| filesystem, err := st.filesystemByTag(tag) | ||
| if errors.IsNotFound(err) { | ||
| return nil, jujutxn.ErrNoOperations | ||
| } else if err != nil { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| + if filesystem.doc.StorageId != "" { | ||
| + return nil, errors.Errorf( | ||
| + "filesystem is assigned to %s", |
wallyworld
Jan 17, 2017
Owner
it would be useful to include the machine/unit in the error message as that's how users tend think of the scope of storage
axw
Jan 18, 2017
Member
A filesystem is not necessarily attached to a machine, and is never directly associated with a unit. The associated storage could potentially be attached to multiple units. I'm going to leave this for now, unless you have strong feelings about it.
wallyworld
Jan 18, 2017
Owner
Let's release as is and get feedback. Maybe just add an extra line to the error message telling the user to juju show-storage to get more details on what in the model is using the storage. An average user will have no idea what "assigned to " means.
| - Count: 1, | ||
| +func (st *State) filesystemParamsWithDefaults(params FilesystemParams, machineId string) (FilesystemParams, error) { | ||
| + if params.Pool == "" { | ||
| + envConfig, err := st.ModelConfig() |
| + filesystem := s.storageInstanceFilesystem(c, storageTag) | ||
| + | ||
| + err = s.State.DestroyFilesystem(filesystem.FilesystemTag()) | ||
| + c.Assert(err, gc.ErrorMatches, "destroying filesystem 0/0: filesystem is assigned to storage data/0") |
wallyworld
Jan 17, 2017
Owner
As per earlier comment, including machine/unit would be helpful here for the user
| + c.Assert(err, gc.ErrorMatches, "destroying filesystem 0/0: filesystem is assigned to storage data/0") | ||
| + | ||
| + removeStorageInstance(c, s.State, storageTag) | ||
| + err = s.State.DestroyFilesystem(filesystem.FilesystemTag()) |
wallyworld
Jan 17, 2017
Owner
can we replace these 2 lines with assertDestroy() to do a proper check
wallyworld
Jan 18, 2017
Owner
Ah, hard to tell from the diff. There's a local func defined on line 526. This destroys and checks life. Perhaps that func could be extracted to live on the test struct and used in multiple tests.
axw
Jan 18, 2017
Member
I see. That one exists for testing concurrency. Probably not worth extracting, but I'll look at doing the same thing here.
axw
Jan 18, 2017
Member
Hmm actually no, doesn't really make sense here, since you can't yet assign a filesystem to a storage instance post-creation. If you were able to do that, we could do a concurrent DestroyFilesystem and assign-to-storage.
I've extracted the DestroyFilesystem & life check, and used it throughout the suite.
| @@ -95,6 +95,12 @@ type modelEntityRefsDoc struct { | ||
| // Applicatons contains the names of the applications in the model. | ||
| Applications []string `bson:"applications"` | ||
| + | ||
| + // Volumes contains the IDs of the volumes in the model. |
wallyworld
Jan 17, 2017
Owner
perhaps s/in the model/bound to the model
to be consistent with terminology used elsewhere?
axw
Jan 18, 2017
Member
They mean different things. A volume "in" the model means that it is tracked by the model. If it is "bound to" the model, it means that it dies when the model dies.
| @@ -661,6 +661,81 @@ func (s *ModelSuite) TestProcessDyingEnvironWithMachinesAndServicesNoOp(c *gc.C) | ||
| c.Assert(env.Destroy(), jc.ErrorIsNil) | ||
| } | ||
| +func (s *ModelSuite) TestProcessDyingEnvironWithVolumeBackedFilesystems(c *gc.C) { |
| + c.Assert(err, jc.ErrorIsNil) | ||
| + c.Assert(machine.EnsureDead(), jc.ErrorIsNil) | ||
| + c.Assert(machine.Remove(), jc.ErrorIsNil) | ||
| + err = st.ProcessDyingModel() |
wallyworld
Jan 17, 2017
Owner
can we add a comment similar to the previous test about volume being persistent
axw
Jan 18, 2017
Member
The only way you can have non-persistent storage is by having it bound to some non-model entity. There are lifecycle binding tests elsewhere.
| @@ -655,6 +667,12 @@ func (st *State) DestroyVolume(tag names.VolumeTag) (err error) { | ||
| } else if err != nil { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| + if volume.doc.StorageId != "" { | ||
| + return nil, errors.Errorf( | ||
| + "volume is assigned to %s", |
| - Count: 1, | ||
| +func (st *State) volumeParamsWithDefaults(params VolumeParams, machineId string) (VolumeParams, error) { | ||
| + if params.Pool == "" { | ||
| + envConfig, err := st.ModelConfig() |
| + c.Assert(err, gc.ErrorMatches, "destroying volume 0/0: volume is assigned to storage data/0") | ||
| + | ||
| + removeStorageInstance(c, s.State, storageTag) | ||
| + err = s.State.DestroyVolume(volume.VolumeTag()) |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit 666c913
into
juju:feature-persistent-storage
Jan 18, 2017
1 check failed
added a commit
that referenced
this pull request
Feb 23, 2017
axw commentedJan 13, 2017
Volumes and filesystems will not default to
being bound to the model, or machine, depending
on whether or not the storage can be persistent.
This is the minimal change. Volume-backed
filesystems are currently always considered
machine-scoped, which leads to undesirable
behaviour: the filesystem cannot be detached
from the machine it is initially attached to
(or doing so will cause the volume to be
destroyed). We will need to make further
changes so that volume-backed filesystems
are not scoped or bound to a single machine.