/juju

Release storage client worker provider #7649

Merged
merged 3 commits into from Jul 19, 2017

Conversation

Reviewers

@babbageclunk babbageclunk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@axw @jujubot @babbageclunk
@axw
Member

axw commented Jul 18, 2017 •
Edited 1 time

Description of change

Add api, cmd, worker/storageprovisioner, and storage provider changes required to support non-destructively removing storage from a model.

The juju remove-storage command grows a --no-destroy flag, which instructs Juju to remove the storage without destroying the associated cloud storage resources. This can be used to release the storage from Juju's control. Once the storage is removed from the model, destroying the model or controller will not destroy the storage.

The VolumeSource and FilesystemSource interfaces grow methods for releasing storage, rather than destroying it. The storageprovisioner worker uses these methods for storage that is marked for releasing.

Requires #7648

QA steps

  1. juju bootstrap localhost
  2. juju deploy postgresql --storage pgdata=lxd
  3. juju remove-storage --force --no-destroy pgdata/0
  4. juju destroy-controller localhost
  5. lxc storage volume show juju juju--filesystem-0 # use "list" to get the name
    (volume should remain, but should show no juju-controller-uuid or juju-model-uuid tags)

As above, but with EBS. In the portal, you should see the juju-controller-uuid/juju-model-uuid tags are empty after the storage is released/removed.

Documentation changes

Yes, we'll need to document juju remove-storage --no-destroy.

Bug reference

None.
@babbageclunk

babbageclunk suggested changes Jul 19, 2017
View changes

I think the code's fine but the names are really confusing. I can see that it's potentially a lot of work to change them now - do you think it's worth it?

api/storage/client.go
@@ -180,7 +180,7 @@ func (c *Client) Attach(unitId string, storageIds []string) ([]params.ErrorResul
}
// Destroy destroys specified storage entities.
-func (c *Client) Destroy(storageIds []string, destroyAttached bool) ([]params.ErrorResult, error) {
+func (c *Client) Destroy(storageIds []string, destroyAttachments, releaseStorage bool) ([]params.ErrorResult, error) {
@babbageclunk

babbageclunk Jul 19, 2017

Member

Maybe this should also be Remove? It feels like less of a problem here though, I guess because here there's a distinction between the storage in the model and the underlying storage in the cloud.

@axw

axw Jul 19, 2017

Member

I was all ready to change this to Remove, but I'm hesitating now because it makes it different to all the other places we use the name Destroy in the client APIs. Probably better to keep consistency, and put "underlying" in the arg name somewhere.

@axw

axw Jul 19, 2017

Member

OTOH, the API structure is different enough that it can't be directly compared anyway. I'll call it Remove here.

api/storageprovisioner/provisioner.go
@@ -243,6 +243,26 @@ func (st *State) VolumeParams(tags []names.VolumeTag) ([]params.VolumeParamsResu
return results.Results, nil
}
+// DestroyVolumeParams returns the parameters for destroying the volumes
+// with the specified tags.
+func (st *State) DestroyVolumeParams(tags []names.VolumeTag) ([]params.DestroyVolumeParamsResult, error) {
@babbageclunk

babbageclunk Jul 19, 2017

Member

Similarly, the struct that comes back shouldn't be a DestroyVolumeParams (or -Filesystem-) if it only sometimes indicates that the volume should be destroyed. I think ReleaseVolumeParams would be better here too.

@axw

axw Jul 19, 2017

Member

Renamed to RemoveVolumeParams (same for Filesystem), and changed the field from Release to Destroy.

cmd/juju/storage/remove.go
@@ -85,7 +95,7 @@ func (c *removeStorageCommand) Run(ctx *cmd.Context) error {
}
defer destroyer.Close()
- results, err := destroyer.Destroy(c.storageIds, c.force)
+ results, err := destroyer.Destroy(c.storageIds, c.force, c.noDestroy)
@babbageclunk

babbageclunk Jul 19, 2017

Member

What about if you called the flag noDestroyUnderlying? Then you keep fairly close to the option name (which is fine since the command is remove), but make this line less oxymoronic.

I know it's minor but I find things like this really throw me off when I'm reading code.

@axw

axw Jul 19, 2017

Member

Done.

provider/ec2/ebs.go
+ return destroyVolumes(v.env.ec2, volIds, true), nil
+}
+
+func destroyVolumes(client *ec2.EC2, volIds []string, release bool) []error {
@babbageclunk

babbageclunk Jul 19, 2017

Member

The name's pretty misleading now - destroyVolumes but with a don't-actually-destroy-them flag. The other sense would be less surprising: releaseVolumes with an also-destroy-them flag.

@axw

axw Jul 19, 2017

Member

I've looked at this a bit more closely, and there's enough differences (which I missed) that these should just be separate methods. e.g. release should error if delete-on-termination is set, or if there are attachments in the attaching/attached state.

worker/storageprovisioner/filesystem_ops.go
- filesystem, ok := ctx.filesystems[filesystemParams.Tag]
- if !ok {
- return errors.NotFoundf("filesystem %s", filesystemParams.Tag.Id())
+ destroyFilesystemTags := make([]names.FilesystemTag, 0, len(destroyFilesystemParams))
@babbageclunk

babbageclunk Jul 19, 2017

Member

This method's become really long and is shuffling things around in several different data structures. Maybe split this part (partitioning into release and destroy groups, and then executing the groups) into a separate method?

@axw

axw Jul 19, 2017

Member

I've split out the code that partitions the tags/IDs, but I think it's going to be messy to split out any more than that. I've created a function literal to avoid repeating the code that deals with the errors (rescheduling/adding to the set of storage to remove).

worker/storageprovisioner/volume_ops.go
- volume, ok := ctx.volumes[volumeParams.Tag]
- if !ok {
- return errors.NotFoundf("volume %s", volumeParams.Tag.Id())
+ destroyVolumeTags := make([]names.VolumeTag, 0, len(destroyVolumeParams))
@babbageclunk

babbageclunk Jul 19, 2017

Member

Same here.

@babbageclunk

babbageclunk approved these changes Jul 19, 2017
View changes

Thanks! I find these names much clearer. I feel like you could change the noDestroyUnderlying flag back to noDestroy now that the API method is Remove though. (Sorry! But not essential either.)

cmd/juju/storage/remove.go
+ newStorageRemoverCloser NewStorageRemoverCloserFunc
+ storageIds []string
+ force bool
+ noDestroyUnderlying bool
@babbageclunk

babbageclunk Jul 19, 2017

Member

Sorry to do this, but now that the method's Remove, I think this would be better as just noDestroy. I only wanted it to be noDestroyUnderlying so that it wouldn't read as Destroy(noDestroy). Not a big deal though.

@axw

axw Jul 19, 2017

Member

all good, I've changed it back

provider/ec2/ebs.go
@@ -445,22 +445,22 @@ func (v *ebsVolumeSource) DescribeVolumes(volIds []string) ([]storage.DescribeVo
// DestroyVolumes is specified on the storage.VolumeSource interface.
func (v *ebsVolumeSource) DestroyVolumes(volIds []string) ([]error, error) {
- return destroyVolumes(v.env.ec2, volIds, false), nil
+ return foreachVolume(v.env.ec2, volIds, destroyVolume), nil
@babbageclunk

babbageclunk Jul 19, 2017

Member

Ooh, nice!

worker/storageprovisioner/filesystem_ops.go
- destroyFilesystemParams := make([]params.RemoveFilesystemParams, len(filesystemParams))
- for i, args := range filesystemParams {
- destroyFilesystemParams[i] = destroyFilesystemParamsByTag[args.Tag]
+ removeFilesystems := func(tags []names.FilesystemTag, ids []string, f func([]string) ([]error, error)) error {
@babbageclunk

babbageclunk Jul 19, 2017

Member

Thanks, I definitely find this a bit easier to follow.

axw added some commits Jul 18, 2017

@axw
storage: add Release{Volumes,Filesystems} 
Add ReleaseVolumes and ReleaseFilesystems to
VolumeSource and FilesystemSource respectively.
Add implementations for LXD and EBS, and stubs
for all other providers to be implemented later.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
14e32b1
@axw
remove-storage: add --no-destroy flag 
Update the remove-storage command with a --no-destroy
flag, which instructs Juju to non-descructively remove
(AKA "release") the storage. This means the cloud storage
will no longer be under the control of Juju, but it is
not destroyed, and will not be destroyed by destroy-model
or destroy-controller.

The Storage API client is updated with an option to
non-destructively remove (aka "release") storage to
support this. Drive-by renaming of DestroyAttached to
DestroyAttachments (no need to rev, the new facade version
hasn't been released yet).
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
1b11608
@axw
worker/storageprovisioner: release storage 
Update the storageprovisioner worker to use the
new DestroyVolumeParams and DestroyFilesystemParams
methods to acquire destruction parameters; and use
the ReleaseVolumes/ReleaseFilesystems calls according
to whether volumes/filesystems are being released
rather than destroyed.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
80cfc5a
@axw
Member

axw commented Jul 19, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jul 19, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 76ce8a1 into juju:develop Jul 19, 2017
1 check passed

1 check passed

github-check-merge-juju Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment