Open Stack

On 05/09/2014 11:29 AM, Matthew Treinish wrote:
> On Thu, May 08, 2014 at 09:50:03AM -0400, David Kranz wrote:
>> On 05/07/2014 10:48 AM, Ken'ichi Ohmichi wrote:
>>> Hi Sean,
>>>
>>> 2014-05-07 23:28 GMT+09:00 Sean Dague <sean at dague.net>:
>>>> On 05/07/2014 10:23 AM, Ken'ichi Ohmichi wrote:
>>>>> Hi David,
>>>>>
>>>>> 2014-05-07 22:53 GMT+09:00 David Kranz <dkranz at redhat.com>:
>>>>>> I just looked at a patch https://review.openstack.org/#/c/90310/3 which was
>>>>>> given a -1 due to not checking that every call to list_hosts returns 200. I
>>>>>> realized that we don't have a shared understanding or policy about this. We
>>>>>> need to make sure that each api is tested to return the right response, but
>>>>>> many tests need to call multiple apis in support of the one they are
>>>>>> actually testing. It seems silly to have the caller check the response of
>>>>>> every api call. Currently there are many, if not the majority of, cases
>>>>>> where api calls are made without checking the response code. I see a few
>>>>>> possibilities:
>>>>>>
>>>>>> 1. Move all response code checking to the tempest clients. They are already
>>>>>> checking for failure codes and are now doing validation of json response and
>>>>>> headers as well. Callers would only do an explicit check if there were
>>>>>> multiple success codes possible.
>>>>>>
>>>>>> 2. Have a clear policy of when callers should check response codes and apply
>>>>>> it.
>>>>>>
>>>>>> I think the first approach has a lot of advantages. Thoughts?
>>>>> Thanks for proposing this, I also prefer the first approach.
>>>>> We will be able to remove a lot of status code checks if going on
>>>>> this direction.
>>>>> It is necessary for bp/nova-api-test-inheritance tasks also.
>>>>> Current https://review.openstack.org/#/c/92536/ removes status code checks
>>>>> because some Nova v2/v3 APIs return different codes and the codes are already
>>>>> checked in client side.
>>>>>
>>>>> but it is necessary to create a lot of patch for covering all API tests.
>>>>> So for now, I feel it is OK to skip status code checks in API tests
>>>>> only if client side checks are already implemented.
>>>>> After implementing all client validations, we can remove them of API
>>>>> tests.
>>>> Do we still have instances where we want to make a call that we know
>>>> will fail and not through the exception?
>>>>
>>>> I agree there is a certain clarity in putting this down in the rest
>>>> client. I just haven't figured out if it's going to break some behavior
>>>> that we currently expect.
>>> If a server returns unexpected status code, Tempest fails with client
>>> validations
>>> like the following sample:
>>>
>>> Traceback (most recent call last):
>>>    File "/opt/stack/tempest/tempest/api/compute/servers/test_servers.py",
>>> line 36, in test_create_server_with_admin_password
>>>      resp, server = self.create_test_server(adminPass='testpassword')
>>>    File "/opt/stack/tempest/tempest/api/compute/base.py", line 211, in
>>> create_test_server
>>>      name, image_id, flavor, **kwargs)
>>>    File "/opt/stack/tempest/tempest/services/compute/json/servers_client.py",
>>> line 95, in create_server
>>>      self.validate_response(schema.create_server, resp, body)
>>>    File "/opt/stack/tempest/tempest/common/rest_client.py", line 596,
>>> in validate_response
>>>      raise exceptions.InvalidHttpSuccessCode(msg)
>>> InvalidHttpSuccessCode: The success code is different than the expected one
>>> Details: The status code(202) is different than the expected one([200])
>>>
>>>
>>> Thanks
>>> Ken'ichi Ohmichi
>>>
>>> _______________________________________________
>>> OpenStack-dev mailing list
>>> OpenStack-dev at lists.openstack.org
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> Note that there are currently two different methods on RestClient
>> that do this sort of thing. Your stacktrace shows
>> "validate_response" which expects to be passed a schema. The other
>> is "expected_success" which takes the expected response code and is
>> only used by the image clients.
>> Both of these will need to stay around since not all APIs have
>> defined schemas but the expected_success method should probably be
>> changed to accept a list of valid success responses rather than just
>> one as it does at present.
> So expected_success() is just a better way of doing something like:
>
> assert.Equals(resp.status, 200)
>
> There isn't anything specific about the images clients with it.
> validate_response() should just call expected_success(), which I pushed out
> here:
> https://review.openstack.org/93035
Right, I was just observing that it was only used by the image clients 
at present.
>
>
>> I hope we can get agreement to move response checking to the client.
>> There was no opposition when we started doing this in nova to check
>> schema. Does any one see a reason to not do this? It would both
>> simplify the code and make sure responses are checked in all cases.
>>
>> Sean, do you have a concrete example of what you are concerned about
>> here? Moving the check from the value returned by a client call to
>> inside the client code should not have any visible effect unless the
>> value was actually wrong but not checked by the caller. But this
>> would be a bug that was just found if a test started failing.
>>
> Please draft a spec/bp for doing this, we can sort out the implementation
> details in the spec review. There is definitely some overlap with the jsonschema
> work though so we need to think about how to best integrate the 2 efforts. For
> example, for projects that don't use jsonschema yet does it make sense to start
> using jsonschema files like we do for nova tests to veriy the status codes. Just
> so we can have a common path for doing this. I think there may be value in doing
> it that way. We can discuss it more during the jsonschema summit session.
>
>
> -Matt Treinish
I pushed a spec https://review.openstack.org/#/c/93037/  but it does not 
propose routing all calls through the schema checker. I think if there 
is a schema we should use it but am not sure we should demand schemas 
for all api calls in a client in order to make progress on checking. But 
we can discuss this next week.

  -David
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev