Re: [openstack-dev] [Ironic] What to do with reservation check in node update API?
Hi, So, it looks like the only reason we check the reservation field here is because we want to return a 409 for node is locked rather than a 400, right? do_node_deploy and such will raise a NodeLocked, which should do the same as this check. It's unclear to me why we can't just remove this check and let the conductor deal with it. Looking at the code this assumption seems to be correct. If the RPC methods raise NodeLocked we will return 409 as expected. Cheers, Lucas __ 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] [Ironic] What to do with reservation check in node update API?
Hi, Another question folks: while the problem above is valid and should be solved, I was actually keeping in mind another one: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L1052-L1057 This is also not retried, and it prevents updating during power operations, which I'm not sure is a correct thing to do. WDYT about dropping it? Not retried by in the API side you mean? __ 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] [Ironic] What to do with reservation check in node update API?
On 07/21/2015 03:26 PM, Lucas Alvares Gomes wrote: Hi, So, it looks like the only reason we check the reservation field here is because we want to return a 409 for node is locked rather than a 400, right? do_node_deploy and such will raise a NodeLocked, which should do the same as this check. It's unclear to me why we can't just remove this check and let the conductor deal with it. Looking at the code this assumption seems to be correct. If the RPC methods raise NodeLocked we will return 409 as expected. It's a bit more tricky, please see my patch: https://review.openstack.org/#/c/204081/ Cheers, Lucas __ 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] [Ironic] What to do with reservation check in node update API?
On 07/21/2015 03:32 PM, Lucas Alvares Gomes wrote: Hi, Another question folks: while the problem above is valid and should be solved, I was actually keeping in mind another one: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L1052-L1057 This is also not retried, and it prevents updating during power operations, which I'm not sure is a correct thing to do. WDYT about dropping it? Not retried by in the API side you mean? Yep, we have to rely on client retry here. __ 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] [Ironic] What to do with reservation check in node update API?
On 07/21/2015 02:24 PM, Dmitry Tantsur wrote: Hi folks! If you're not aware already, I'm working on solving node is locked problems breaking users (and tracking it at https://etherpad.openstack.org/p/ironic-locking-reform). We have retries in place in client, but we all agree that it's not the eventual solution. One of the things we've figured out is that we actually have server-side retries - in task_manager.acquire. They're nice and configurable. Alas, we have one place that checks reservations without task_manager: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L401-L403 (note that this check is actually racy) I'd like to ask your opinions on how to solve it? I have 3 ideas: 1. Just implement retries on API level (possibly split away a common function from task_manager). 2. Move update to conductor instead of doing it directly in API. 3. Don't check reservation when updating node. At all. Another question folks: while the problem above is valid and should be solved, I was actually keeping in mind another one: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L1052-L1057 This is also not retried, and it prevents updating during power operations, which I'm not sure is a correct thing to do. WDYT about dropping it? And with check on provision state, the options are the same 1,2,3 as above: move to conductor, reimplement retries, just ignore. Ideas? __ 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] [Ironic] What to do with reservation check in node update API?
I'd choose Option #2 Cheers, Serge Kovaleff http://www.mirantis.com cell: +38 (063) 83-155-70 On Tue, Jul 21, 2015 at 3:24 PM, Dmitry Tantsur dtant...@redhat.com wrote: Hi folks! If you're not aware already, I'm working on solving node is locked problems breaking users (and tracking it at https://etherpad.openstack.org/p/ironic-locking-reform). We have retries in place in client, but we all agree that it's not the eventual solution. One of the things we've figured out is that we actually have server-side retries - in task_manager.acquire. They're nice and configurable. Alas, we have one place that checks reservations without task_manager: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L401-L403 (note that this check is actually racy) I'd like to ask your opinions on how to solve it? I have 3 ideas: 1. Just implement retries on API level (possibly split away a common function from task_manager). 2. Move update to conductor instead of doing it directly in API. 3. Don't check reservation when updating node. At all. Ideas? __ 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] [Ironic] What to do with reservation check in node update API?
On Tue, Jul 21, 2015 at 02:24:14PM +0200, Dmitry Tantsur wrote: Hi folks! If you're not aware already, I'm working on solving node is locked problems breaking users (and tracking it at https://etherpad.openstack.org/p/ironic-locking-reform). We have retries in place in client, but we all agree that it's not the eventual solution. One of the things we've figured out is that we actually have server-side retries - in task_manager.acquire. They're nice and configurable. Alas, we have one place that checks reservations without task_manager: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L401-L403 (note that this check is actually racy) I'd like to ask your opinions on how to solve it? I have 3 ideas: 1. Just implement retries on API level (possibly split away a common function from task_manager). 2. Move update to conductor instead of doing it directly in API. 3. Don't check reservation when updating node. At all. Ideas? So, it looks like the only reason we check the reservation field here is because we want to return a 409 for node is locked rather than a 400, right? do_node_deploy and such will raise a NodeLocked, which should do the same as this check. It's unclear to me why we can't just remove this check and let the conductor deal with it. // jim __ 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] [Ironic] What to do with reservation check in node update API?
On 07/21/2015 03:01 PM, Jim Rollenhagen wrote: On Tue, Jul 21, 2015 at 02:24:14PM +0200, Dmitry Tantsur wrote: Hi folks! If you're not aware already, I'm working on solving node is locked problems breaking users (and tracking it at https://etherpad.openstack.org/p/ironic-locking-reform). We have retries in place in client, but we all agree that it's not the eventual solution. One of the things we've figured out is that we actually have server-side retries - in task_manager.acquire. They're nice and configurable. Alas, we have one place that checks reservations without task_manager: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L401-L403 (note that this check is actually racy) I'd like to ask your opinions on how to solve it? I have 3 ideas: 1. Just implement retries on API level (possibly split away a common function from task_manager). 2. Move update to conductor instead of doing it directly in API. 3. Don't check reservation when updating node. At all. Ideas? So, it looks like the only reason we check the reservation field here is because we want to return a 409 for node is locked rather than a 400, right? do_node_deploy and such will raise a NodeLocked, which should do the same as this check. It's unclear to me why we can't just remove this check and let the conductor deal with it. I must admit I've confused 2 problems here. I've sent a separate mail about node-update. And I agree, I think this check can be just dropped without any effect on user experience. I'll check it. // jim __ 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
