Add support for os-retype volumeaction#2113
Conversation
nikParasyr
left a comment
There was a problem hiding this comment.
@jtopjian I've added some comments/questions I have. Also the acceptance tests cover only blockstorage V3. The action is valid for V2 as well. let me know if you want me to add acceptance tests for V2 as well.
| // no extra specs. This is required to bypass cinder-scheduler filters and be able | ||
| // to create a volume with this volumeType. An error will be returned if the volume | ||
| // type was unable to be created. | ||
| func CreateVolumeTypeNoExtraSpecs(t *testing.T, client *gophercloud.ServiceClient) (*volumetypes.VolumeType, error) { |
There was a problem hiding this comment.
VolumeType needs to have empty extra specs in order to be able to create a volume with this type. Otherwise the cinder-scheduler and filterOpts get in the middle and the volume cannot be created. Currently on CreateVolumeType the extra specs are not tested due to some limitations, so it could be incorporated but i'm not sure if its the right move as ideally extra specs should be tested
There was a problem hiding this comment.
I think the limitations have to do with the OpenLab testing environment? If so it's OK to pass on doing that kind of test. As long as functionality was tested some other way - such as a local devstack environment.
There was a problem hiding this comment.
In my understanding, if you set extra_specs then the cinder-scheduler tries to map the volumeType to a back-end that matches these extra_specs. That behavior can be different per cloud depending on how the cinder-scheduler has been configured (which filters to use etc). I'd still say that the acceptance test is useful and i expect without extra_specs to avoid all the cloud-specific limitations of how the backens and cinder-scheduler is set-up.
I tested this on our test cluster and the acceptance passed (volume created properly, and type changed without an issue etc). I've also run a retype, basically the example on docs.go, between 2 valid volume_types that also went fine.
I've tried also on "in-use" volumes, the api call works, but the underlying retype operation fails. Same thing when i use openstack cli or horizon. This has to do with how the backends are set-up, the volume types etc so i consider it out of scope for gophercloud. The api docs have some info about restrictions.
|
Build failed.
|
| // MigrationPolicy specifies if the volume should be migrated when it is | ||
| // re-typed. Possible values are "on-demand" or "never". If not specified, | ||
| // the default is "never". | ||
| MigrationPolicy string `json:"migration_policy,omitempty"` |
There was a problem hiding this comment.
It looks like there are only two possible values for MigrationPolicy: https://github.com/openstack/cinder/blob/876ac4e79fe14887ee3902ef833be5e2273d800b/cinder/api/schemas/volume_actions.py#L94-L96
Using a generic string is fine, but it's also possible to create a specific type for this like how it's done here:
- https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/extensions/lbaas_v2/listeners/requests.go#L8-L17
- https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/extensions/lbaas_v2/listeners/requests.go#L85
If the migration policy choices rarely change, creating a type is usually a good method for more specific data modeling.
d3b8749 to
724b3de
Compare
724b3de to
c2f789d
Compare
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
@nikParasyr Is this ready for review/merge? |
|
@jtopjian Yes |
For #2110
Add support for the "os-retype" volumeaction that changes the volume_type of a volume.
The lowe-level implementation of the os-retype is driver specific on cinder. The links below are blobs on the api level.