Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renamed CinderVolume properties to be compatible with block storage API v2, fixed broken VolumeTests #1046

Merged
merged 3 commits into from
Jun 27, 2017

Conversation

cbirajdar
Copy link
Contributor

Fixes #1019. For v2/v3, getName() and getDescription() should return the correct values. For v1, it will return null as API fields since v2 are updated to name and description.

@@ -63,8 +63,6 @@ public void getVolumeV1() throws Exception {
assertTrue(getRequest.getPath().matches("/v[12]/\\p{XDigit}*/volumes/8a9287b7-4f4d-4213-8d75-63470f19f27c"));

assertEquals(volume.getId(), "8a9287b7-4f4d-4213-8d75-63470f19f27c");
assertEquals(volume.getName(), "vol-test");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason you have to remove these, the methods are the same i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vinodborole!

As mentioned above - for block storage API v1, volume.getName() and volume.getDescription() now return null values (due to json property change display_name->name and display_description->description) and these assertions would fail.

Similar change was applied to CinderVolumeSnapshot.java in #1031. Nevertheless, json property change fixes the response for API v2/v3. Removed the skip from getVolumeV2() which does assert for name and description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@auhlig What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While thinking about it: How about keeping both properties display_name and name? Maybe we should have done the same in the other class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Fixed both classes, added display_name and display_description to maintain backwards compatibility with deprecated cinder v1 API.

@cbirajdar
Copy link
Contributor Author

@auhlig When you get a chance, can you review and merge this? Thanks!

@auhlig
Copy link
Member

auhlig commented Jun 23, 2017

Thanks @cbirajdar. LGTM. Happy with this @vinodborole?

@auhlig auhlig added this to the 3.1.0 Release milestone Jun 23, 2017
@vinodborole
Copy link
Contributor

LGTM

@auhlig auhlig merged commit c0a7f2b into ContainX:master Jun 27, 2017
@pdube
Copy link
Contributor

pdube commented Jul 6, 2017

How do you configure your the client to use V2/V3? This fix actually broke V1 volume creation. The "name" field is no longer serialized to "display_name" so all created volumes have an empty name field

@cbirajdar
Copy link
Contributor Author

@pdube For V2/V3 before this fix, getName() and getDescription() was returning null due to the breaking changes introduced from V1 -> V2. Openstack4j relies on the service catalog to apply default service endpoints. You can implement the EndpointURLResolver interface and use different version instead as mentioned here.

public class CustomEndpointUrlResolver implements EndpointURLResolver {

    @Override
    public String findURLV2(URLResolverParams params) {
        // Identity V2
        return params.token.getEndpoint();
    }

    @Override
    public String findURLV3(URLResolverParams params) {
        // Identity V3
        if (params.type.equals(ServiceType.BLOCK_STORAGE)) {
            return BLOCK_STORAGE_V2_OR_V3_URL;
        }
}

V1 is deprecated as per openstack docs but in order to fix the volume creation for V1, just need to update the VolumeBuilder and set these deprecated fields to serialize into correct json attributes.

@pdube
Copy link
Contributor

pdube commented Jul 7, 2017

Thanks for the quick response, I will test with the EndpointURLResolver. Since V1 is deprecated, does it make sense to change the default resolver to use cinder V3?

@cbirajdar
Copy link
Contributor Author

Default endpoints and versions are resolved based on the openstack release version and configured endpoints in the service catalog. Can you check the service catalog for your OS environment and make sure if it points to the correct cinder version (or the latest available)?

Also, I fixed the cinder V1 volume creation so that name and description will persist and won't be empty. Pull request #1058.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants