VMAX should not tie volume extra specs exclusively to driver-wide config

Bug #1425641 reported by Carl Pecinovsky
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
Critical
Xing Yang

Bug Description

The change recently merged here, https://review.openstack.org/#/c/157679/2, makes significant changes to "extra_specs" handling in addition to adding support for 2 extra_specs configuration options to the EMC VMAX driver. Essentially, driver operations that call _initial_setup() to get the "extra_specs" can now hang the extra_specs returned on the EMCVMAXCommon object, rendering the information static or global to the driver instance.

This *breaks* PowerVC concurrency support because of the PowerVC requirement to support volume-based extra_specs from "volume types". For example, storagetype:stripecount (aka storagetype:membercount), is an extra_spec applicable on a volume-by-volume basis. In our product, a user may request volume1 to be created as concatenated, but then request that volume2 be created with a member count of 3. In the VMAX storage case, we achieve this by overriding EMCVMAXCommon.initial_setup() to override certain extra_spec values that are present in the volume type.

But with https://review.openstack.org/#/c/157679/2 changes, since the extra_specs get set on and accessed from EMCVMAXCommon class instance data rather than being passed down on each helper method call, the data can get clobbered and parallel operations may pick up the wrong extra specs. We would like the processing to revert to how it was handled before, at least for extra specs that are applicable to a volume creation (membercount, pool, array, fastpolicy, slo, workload). Extra specs like storagetype:interval and storagetype:retries recently added could be seen as global and not volume specific.

Ideally, the VMAX driver would add a facility like other drivers do, to obtain a set of extra_specs from the volume type, accessed via volume['volume_type_id'] if set there. EMC's own VNX driver does this. Compare to emc_vnx_cli.py-->create_volume().

UPDATE: I looked at the EMC code a little closer and it appears to be broken in general, not just because we override _initial_setup(). The code does get the volume type extra specs:
  extraSpecs = self.utils.get_volumetype_extraspecs(volume, volumeTypeId)

The issue is that these extraSpecs are overwritten in-memory by what is contained in the config xml file. With FAST policy for example, if it is not in the config file, the in-memory extra spec still gets overwritten with None. So the volume type extra spec does not get honored. And in the case of storagetype:stripecount, it does *try* to honor the value in the volume type, but concurrency is broken because of the EMCVMAXCommon self.extra_specs issue described earlier.

Tags: drivers emc vmax
Carl Pecinovsky (csky)
description: updated
Xing Yang (xing-yang)
tags: added: drivers emc vmax
Changed in cinder:
assignee: nobody → Xing Yang (xing-yang)
Xing Yang (xing-yang)
Changed in cinder:
milestone: none → kilo-3
status: New → In Progress
importance: Undecided → Critical
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/162658

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/162658
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=ffc867723297397235c6a4bb0d3a12c96f1aec18
Submitter: Jenkins
Branch: master

commit ffc867723297397235c6a4bb0d3a12c96f1aec18
Author: Xing Yang <email address hidden>
Date: Mon Mar 9 11:18:11 2015 -0400

    Don't override extra specs with config in VMAX

    A recent merge in https://review.openstack.org/#/c/157679
    brought in regression because it tied volume type extra specs to
    config file settings.

    This patch rolled back most of the changes and don't let extra specs
    be overwritten by the config file settings. Intervals and retries
    will be read from the config file.

    Partial-Bug: #1425641
    Change-Id: I7b7959d64f9cc5e954d03f56f6a37021c4c0e9e1

Mike Perez (thingee)
Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in cinder:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: kilo-3 → 2015.1.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.