Add function to remove a volume if its state is ready

[EmilienM]

Volumes can only be removed when in a certain state. This function can be used to check if a volume can be removed before actually removing it.

Note that this function will remove the volume if it's ready.

API doc: https://docs.openstack.org/api-ref/block-storage/v3/index.html?expanded=delete-a-volume-detail#delete-a-volume

[iamemilio]

aside from silly nitpick, /lgtm

[EmilienM]

@jtopjian for review. Thanks!

[jtopjian]

Is it possible to confirm whether the statuses listed in the documentation are still the most up-to-date statuses?

Here are all the possible statuses of a volume: https://github.com/openstack/cinder/blob/master/cinder/objects/fields.py#L168-L190

I spent about an hour browsing through the Cinder code, but couldn't find an obvious place where a volume's status is compared against a list valid deletable statuses. Maybe someone else would have better luck?

This isn't a blocker. The code could be amended in the future and acceptance criteria for gophercloud/utils is much more open than Gophercloud core. But if someone can confirm the known statues now, it would save some work in the future.

[EmilienM]

Is it possible to confirm whether the statuses listed in the documentation are still the most up-to-date statuses?

Here are all the possible statuses of a volume: https://github.com/openstack/cinder/blob/master/cinder/objects/fields.py#L168-L190

I spent about an hour browsing through the Cinder code, but couldn't find an obvious place where a volume's status is compared against a list valid deletable statuses. Maybe someone else would have better luck?

This isn't a blocker. The code could be amended in the future and acceptance criteria for gophercloud/utils is much more open than Gophercloud core. But if someone can confirm the known statues now, it would save some work in the future.

I'm actually thinking solving this problem in Cinder instead: https://review.opendev.org/c/openstack/cinder/+/805027

Let's hold this patch for now, so we avoid creating functions that wouldn't be that useful, if we can have the wanted logging into the right place (Cinder API).

[EmilienM]

And to answer your question @jtopjian , have a look here: https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L454-L455

[jtopjian]

have a look here:

ah-hah! That's exactly what I was trying to find - thank you!

In this case, should in-use be removed from one of the valid statuses?

Let's hold this patch for now

No problem. Just let me know when you want to proceed.

[EmilienM]

@jtopjian ready when you are :-)

[pierreprinetti]

I am worried by the fact that this convenience function limits the user’s action based on a set of rules that may fall out of sync with OpenStack.

Ultimately, the purpose of this convenience function is to clearly write why an operation failed, if it failed. But then, if its interesting logic only exists in a failed operation, we may avoid adding execution time to a successful operation.

My suggested alternative is to first try the deletion. If it fails, then we can modify the returned error to add information. This way, successful operations get no time penalty, and we do not risk applying rules that are no longer valid (if OpenStack changes the name or the logic connected to volume states).

WDYT?

[kayrus]

I also have doubts on merging this. From my point of view this function shouldn't be placed in this repo. The responsibility should be on the end app side and app developer can reuse https://pkg.go.dev/github.com/gophercloud/gophercloud?utm_source=godoc#WaitFor to meet the needs of the app logic.

[jtopjian]

I'm pretty flexible with what gets merged into this utils repo. If a function is useful for more than one person, it can be added here.

With that in mind, my experience mirrors @kayrus where the more event-driven WaitFor is commonly used for this. As an example: https://github.com/gophercloud/gophercloud/blob/513734676e6495f6fec60e7aaf1f86f1ce807428/openstack/blockstorage/v3/volumes/util.go (though I just realized that these still exist in Gophercloud "core" and should be moved to this repo).

I may be making an incorrect assumption that @EmilienM and @pierreprinetti are coworkers. If the assumption is true, I'll let you two discuss what this function may ultimately be used for. If it's agreed to keep this function, I'm happy to merge it.

[pierreprinetti]

@EmilienM To be clear, here's an example of what I propose:

type ErrVolumeStatus struct {
    gophercloud.ErrDefault400
    Status string
}

func (e ErrVolumeStatus) Unwrap() error {
    return e.ErrDefault400
}

// VolumeDeleteVerbose attempts volume deletion. In case the error is
// attribuable to the current volume status, this function stores the volume
// status in the returned error.
//
// Example usage:
//
//
//    if err := VolumeDeleteVerbose(computeClient, volID, volumes.DeleteOpts{}); err != nil {
//      var gerr ErrVolumeStatus
//      if errors.As(err, &gerr) {
//          log.Printf("%v Volume status: %q", err, gerr.Status)
//      } else {
//          log.Print(err)
//      }
//    }
func VolumeDeleteVerbose(client *gophercloud.ServiceClient, id string, opts volumes.DeleteOpts) error {
    err := volumes.Delete(client, id, opts).ExtractErr()

    // Add some information in case of failure
    var gerr gophercloud.ErrDefault400
    if err != nil && errors.As(err, &gerr) {
        vol, err1 := volumes.Get(client, id).Extract()

        // if something goes wrong, just return what the normal
        // call would have returned
        if err1 == nil {
            return ErrVolumeStatus{gerr, vol.Status}
        }
    }

    return err
}

Is that what you want to achieve?

[EmilienM]

closing for now, will re-open if it has more traction.