Thanks Alex for writing in details.
On Tue, Jan 24, 2017 at 6:05 PM, Alex Xu <sou...@gmail.com> wrote: > Unfortunately the device tag support in the API was broken in the old > Microversion https://bugs.launchpad.net/nova/+bug/1658571, which thanks > to Kevin Zheng to find out that. > > Actually there are two bugs, just all of them are about device tag. The > first one [0] is a mistake in the initial introduce of device tag. The new > schema only available for the version 2.32, when the request version > > 2.32, the schema fallback to the old one. > > The second one [1] is that when we bump the API to 2.37, the network > device tag was removed accidentally which also added in 2.32 [2]. > > So the current API behavior is as below: > > 2.32: BDM tag and network device tag added. > 2.33 - 2.36: 'tag' in the BDM disappeared. The network device tag still > works. > 2.37: The network device tag disappeared also. > I am modifying the tempest tests in (feel free to modify it for more verification) and running all above cases combination with version, 2.32, 2.33 and >2.37 and we should be able to verify what we broke actually from tests results. But from looking at api-ref and db, i am little bit confused. API-ref says device tag as "An arbitrary tag." [6] and DB also have it as Column(String(255)) [7] But our tag validation we mistakenly removed is more strict actually as per [8] which seems good so we should fix/mention the same in DB and api-ref clearly. > > There are few questions we should think about: > > 1. Should we fix that by Microversion? > Thanks to Chris Dent point that out in the review. I also think we > need to bump Microversion, which follow the rule of Microversion. > Initially i thought it only validation part we loosen and feature still works but with additionalPropoerties=False, its all disallowed to add tag :(. > > 2. If we need Microversion, is that something we can do before release? > We are very close to the feature freeze. And in normal, we need spec > for microversion. Maybe we only can do that in Pike. For now we can update > the API-ref, and microversion history to notice that, maybe a reno also. > > 2. How can we prevent that happened again? > Both of those patches were reviewed multiple cycles. But we still miss > that. It is worth to think about how to prevent that happened again. > This is very nice point. Actually we planned a BP [9] to cover the (Top, Bottom & Changed) testing of each microversion, but my bad here as i did not get time to work on that. May be this BP can be helpful here and add to verify those tests in review guidelines. > > Talk with Sean. He suggests stop passing plain string version to the > schema extension point. We should always pass APIVersionRequest object > instead of plain string. Due to "version == APIVersionRequest('2.32')" is > always wrong, we should remove the '__eq__'. The developer should always > use the 'APIVersionRequest.matches' [3] method. > > That can prevent the first mistake we made. But nothing help for second > mistake. Currently we only run the test on the specific Microversion for > the specific interesting point. In the before, the new tests always > inherits from the previous microversion tests, just like [4]. That can test > the old API behavior won't be changed in the new microversion. But now, we > said that is waste, we didn't do that again just like [5]. Should we change > that back? > Another issue we have in functional tests for device tagging is we did not make "supports_device_tagging" as true in fake driver and it was something never got worked in https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1730 which seems failure on this - http://logs.openstack.org/52/423952/5/check/gate-nova-tox-db-functional-ubuntu-xenial/ff1c09a/ I will check and fix functional tests bits. > > Thanks > Alex > > [0] https://review.openstack.org/#/c/304510/64/nova/api/ > openstack/compute/block_device_mapping.py > [1] https://review.openstack.org/#/c/316398/37/nova/api/ > openstack/compute/schemas/servers.py@88 > [2] https://review.openstack.org/#/c/316398/37/nova/api/ > openstack/compute/schemas/servers.py@79 > [3] https://github.com/openstack/nova/blob/master/nova/api/openstack/api_ > version_request.py#L219 > [4] https://github.com/openstack/nova/blob/master/ > nova/tests/unit/api/openstack/compute/test_evacuate.py#L415 > [5] https://github.com/openstack/nova/blob/master/ > nova/tests/unit/api/openstack/compute/test_serversV21.py#L3584 > > [6] http://developer.openstack.org/api-ref/compute/?expanded=create-server-detail [7] https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L624 [8] http://developer.openstack.org/api-ref/compute/#server-tags-servers-tags [9] https://blueprints.launchpad.net/nova/+spec/nova-microversion-functional-tests > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
