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: clean up machine-bound storage properly #7052
Conversation
| @@ -541,6 +541,14 @@ func cleanupDyingMachineResources(m *Machine) error { | ||
| return errors.Annotate(err, "getting machine volume attachments") | ||
| } | ||
| for _, va := range volumeAttachments { | ||
| + detachable, err := isDetachableVolumeTag(m.st, va.Volume()) |
mjs
Mar 1, 2017
Contributor
if detachable isn't needed beyond here then I'd tend to roll the call into the if so that detachable doesn't escape, and you save a few lines.
| @@ -556,6 +564,14 @@ func cleanupDyingMachineResources(m *Machine) error { | ||
| return errors.Annotate(err, "getting machine filesystem attachments") | ||
| } | ||
| for _, fsa := range filesystemAttachments { | ||
| + detachable, err := isDetachableFilesystemTag(m.st, fsa.Filesystem()) |
| + // to be Dying, then Dead, and finally removed. Only once the attachment | ||
| + // is removed will the filesystem transition to Dead and then be removed. | ||
| + // Therefore, there may be filesystems that are bound, but not attached, | ||
| + // to the machine. |
| @@ -936,7 +947,7 @@ func (s *FilesystemStateSuite) testFilesystemAttachmentParamsConcurrent(c *gc.C, | ||
| func (s *FilesystemStateSuite) TestFilesystemAttachmentParamsConcurrentRemove(c *gc.C) { | ||
| // this creates a filesystem mounted at "/srv". | ||
| - filesystem, machine := s.setupFilesystemAttachment(c, "rootfs") | ||
| + filesystem, machine := s.setupFilesystemAttachment(c, "environscoped") |
| @@ -1507,6 +1507,13 @@ func (i *importer) addVolume(volume description.Volume) error { | ||
| Info: info, | ||
| AttachmentCount: len(attachments), | ||
| } | ||
| + detachable, err := isDetachableVolumePool(i.st, volume.Pool()) |
| @@ -1603,6 +1610,13 @@ func (i *importer) addFilesystem(filesystem description.Filesystem) error { | ||
| Info: info, | ||
| AttachmentCount: len(attachments), | ||
| } | ||
| + detachable, err := isDetachableFilesystemPool(i.st, filesystem.Pool()) |
| + if err != nil { | ||
| + return errors.Trace(err) | ||
| + } | ||
| + if len(attachments) != 1 { |
mjs
Mar 1, 2017
Contributor
maybe add a comment about this along the lines of multiple attachments meaning it must be detachable
| + if err != nil { | ||
| + return errors.Trace(err) | ||
| + } | ||
| + if len(attachments) != 1 { |
| - // e.g. NFS shares, then we need to check the filesystem info | ||
| - // to decide whether or not to remove. | ||
| +// isDetachableFilesystemPool reports whether or not the given | ||
| +// storage pool will create a filesystem that is not inherently |
wallyworld
Mar 1, 2017
Owner
"is not inherently a machine" seems clunky
"is not inherently bound to a machine" perhaps?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
axw commentedMar 1, 2017
Description of change
We were previously relying on a combination of kludges to
get non-detachable machine storage to be removed along
with a machine. Due to recent changes to expose more of
storage (i.e.g for removal and detachment, etc.), machine
storage stopped getting removed along with the machine.
For "non-detachable" storage, the one-and-only attachment
may still be destroyed and removed independently of the
volume/filesystem. For example, if you destroy a unit on
a machine, and the unit has a "loop" volume, then the
volume will be marked Dying when the unit is destroyed.
That will cascade and mark the volume attachment as
Dying; the volume will be detached, at which point the
volume attachment is removed from the model. At this
point there is no association remaining between the machine
and the volume.
To fix this, we now record the ID of the machine to which
a volume or filesystem is inherently bound (if it is at all)
on the volume/filesystem document. We can then immediately
identify all volumes/filesystems that are inherently bound to
a machine when the machine is to be removed from state,
and remove them as well. We also use this to identify whether
the storage is detachable in various parts.
QA steps
SCENARIO 1
(wait for unit to become idle)
(wait for unit to be removed)
SCENARIO 2
As above, but migrate the model to another controller
between steps 2 and 3.
SCENARIO 3
Run the "assess_storage.py" test from juju-ci-tools.
Documentation changes
None.
Bug reference
https://bugs.launchpad.net/bugs/1667883