Re: [openstack-dev] [nova] Should PUT /os-services be idempotent?

[Blair Bethwaite]

Please don't make these 400s - it should not be a client error to be
unaware of the service status ahead of time.

On 12 July 2017 at 11:18, Matt Riedemann  wrote:
> I'm looking for some broader input on something being discussed in this
> change:
>
> https://review.openstack.org/#/c/464280/21/nova/api/openstack/compute/services.py
>
> This is collapsing the following APIs into a single API:
>
> Old:
>
> * PUT /os-services/enable
> * PUT /os-services/disable
> * PUT /os-services/disable-log-reason
> * PUT /os-services/force-down
>
> New:
>
> * PUT /os-services
>
> With the old APIs, if you tried to enable and already enabled service, it
> was not an error. The same is you tried to disable an already disabled
> service. It doesn't change anything, but it's not an error.
>
> The question is coming up in the new API if trying to enable an enabled
> service should be a 400, or trying to disable a disabled service. The way I
> wrote the new API, those are no 400 conditions. They don't do anything, like
> before, but they aren't errors.
>
> Looking at [1] it seems this should not be an error condition if you're
> trying to update the state of a resource and it's already at that state.
>
> I don't have a PhD in REST though so would like broader discussion on this.
>
> [1] http://www.restapitutorial.com/lessons/idempotency.html
>
> --
>
> Thanks,
>
> Matt
>
> __
> 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



-- 
Cheers,
~Blairo

__
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

Re: [openstack-dev] [nova] Should PUT /os-services be idempotent?

[Ghanshyam Mann]

On Wed, Jul 12, 2017 at 11:29 AM, Alex Xu  wrote:
>
>
> 2017-07-12 9:18 GMT+08:00 Matt Riedemann :
>>
>> I'm looking for some broader input on something being discussed in this
>> change:
>>
>>
>> https://review.openstack.org/#/c/464280/21/nova/api/openstack/compute/services.py
>>
>> This is collapsing the following APIs into a single API:
>>
>> Old:
>>
>> * PUT /os-services/enable
>> * PUT /os-services/disable
>> * PUT /os-services/disable-log-reason
>> * PUT /os-services/force-down
>>
>> New:
>>
>> * PUT /os-services
>>
>> With the old APIs, if you tried to enable and already enabled service, it
>> was not an error. The same is you tried to disable an already disabled
>> service. It doesn't change anything, but it's not an error.
>>
>> The question is coming up in the new API if trying to enable an enabled
>> service should be a 400, or trying to disable a disabled service. The way I
>> wrote the new API, those are no 400 conditions. They don't do anything, like
>> before, but they aren't errors.
>
>
> Sorry, I didn't describe clearly in the comment.
>
> Some of those comments about save a DB call with more conditions checks. It
> means if enable a enabled service, we needn't a db call, we can just return
> to the user 200 directly.
>
> One of those comments is about when the API user specified 'status=enabled'
> and 'disabled_reason' in the request body, then we just ignore the
> 'disabled_reason' and didn't save it into the db also. That sounds not
> right. We should return 400 to the API user, you can't specified the
> 'status=enabled' and 'disabled_reason'.

+1 for 400 in this case.


>
>>
>>
>> Looking at [1] it seems this should not be an error condition if you're
>> trying to update the state of a resource and it's already at that state.
>>
>> I don't have a PhD in REST though so would like broader discussion on
>> this.
>>
>> [1] http://www.restapitutorial.com/lessons/idempotency.html
>>
>> --
>>
>> Thanks,
>>
>> Matt
>>
>> __
>> 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
>

__
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

Re: [openstack-dev] [nova] Should PUT /os-services be idempotent?

[Ghanshyam Mann]

On Wed, Jul 12, 2017 at 11:20 AM, Ed Leafe  wrote:
> On Jul 11, 2017, at 8:18 PM, Matt Riedemann  wrote:
>
>> With the old APIs, if you tried to enable and already enabled service, it 
>> was not an error. The same is you tried to disable an already disabled 
>> service. It doesn't change anything, but it's not an error.
>>
>> The question is coming up in the new API if trying to enable an enabled 
>> service should be a 400, or trying to disable a disabled service. The way I 
>> wrote the new API, those are no 400 conditions. They don't do anything, like 
>> before, but they aren't errors.
>
> These should not be errors. You are calling the API to set a particular 
> condition. The result of that call is that the service is in that condition. 
> That should be a 2xx, most likely a 204. So yeah, it should be idempotent.

My vote also to make(keep) it idempotent but with status code. if it
return 200 then on second call on same status should be 200 with same
response body otherwise API become difficult to use.


-gmann

>
> -- Ed Leafe
>
>
>
>
>
>
> __
> 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

Re: [openstack-dev] [nova] Should PUT /os-services be idempotent?

[Alex Xu]

2017-07-12 9:18 GMT+08:00 Matt Riedemann :

> I'm looking for some broader input on something being discussed in this
> change:
>
> https://review.openstack.org/#/c/464280/21/nova/api/openstac
> k/compute/services.py
>
> This is collapsing the following APIs into a single API:
>
> Old:
>
> * PUT /os-services/enable
> * PUT /os-services/disable
> * PUT /os-services/disable-log-reason
> * PUT /os-services/force-down
>
> New:
>
> * PUT /os-services
>
> With the old APIs, if you tried to enable and already enabled service, it
> was not an error. The same is you tried to disable an already disabled
> service. It doesn't change anything, but it's not an error.
>
> The question is coming up in the new API if trying to enable an enabled
> service should be a 400, or trying to disable a disabled service. The way I
> wrote the new API, those are no 400 conditions. They don't do anything,
> like before, but they aren't errors.
>

Sorry, I didn't describe clearly in the comment.

Some of those comments about save a DB call with more conditions checks. It
means if enable a enabled service, we needn't a db call, we can just return
to the user 200 directly.

One of those comments is about when the API user specified 'status=enabled'
and 'disabled_reason' in the request body, then we just ignore the
'disabled_reason' and didn't save it into the db also. That sounds not
right. We should return 400 to the API user, you can't specified the
'status=enabled' and 'disabled_reason'.


>
> Looking at [1] it seems this should not be an error condition if you're
> trying to update the state of a resource and it's already at that state.
>
> I don't have a PhD in REST though so would like broader discussion on this.
>
> [1] http://www.restapitutorial.com/lessons/idempotency.html
>
> --
>
> Thanks,
>
> Matt
>
> __
> 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

Re: [openstack-dev] [nova] Should PUT /os-services be idempotent?

[Ed Leafe]

On Jul 11, 2017, at 8:18 PM, Matt Riedemann  wrote:

> With the old APIs, if you tried to enable and already enabled service, it was 
> not an error. The same is you tried to disable an already disabled service. 
> It doesn't change anything, but it's not an error.
> 
> The question is coming up in the new API if trying to enable an enabled 
> service should be a 400, or trying to disable a disabled service. The way I 
> wrote the new API, those are no 400 conditions. They don't do anything, like 
> before, but they aren't errors.

These should not be errors. You are calling the API to set a particular 
condition. The result of that call is that the service is in that condition. 
That should be a 2xx, most likely a 204. So yeah, it should be idempotent.

-- Ed Leafe







signature.asc
Description: Message signed with OpenPGP
__
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