Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
------=_Part_22230002_1751095465.1384256418493 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Forwarding this email to engine-devel so that backend maintainers are aware of this issue. Looking at the code: - MultipleActionsRunner#Execute first creates and "validates" all commands: A. the "for" block which iterates over getParameters() 1-> validate correlation ID, if OK create and add command, otherwise add returnValue B the "if" block which tests getCommands().size() 1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above C. the "if" block which tests canRunActions 1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent. Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read. Changes done by Alex (patch attached): X1. returnValues changed to Collections.synchronizedList -> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in "synchronized (list)" block, so this: for (VdcReturnValueBase value : returnValues) ... should be this: synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... } X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable) -> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated Guys, what do you think? Vojtech ----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote:
Hi Alex,
recently I noticed problems with invoking console for multiple VMs (select more VMs in webadmin and then hit the console btn). I was sure it worked in past so i git-bisected the master branch and I discovered that this problem is apparently caused by patch [1]. For single vm console invocation it works fine, but for multiple VMs it doesn't.
I did some closer investigation with a debugger and it seems that getSucceeded() on the return val of SetVmTicket command returns false in case of multiple execution despite the fact SetVmTicketCommand sets this value to true. I suppose there is some problem with propagation of command return value from BE to FE. The same goes for the encapsulated returnValue attribute (it contains value of the generated ticket). When invoking multiple consoles it is null (although it has been filled on backend). Weird.
Tomas told me you did some optimizations for multiple command executions, do you know it might cause it? Please let me know if you have any idea...
Cheers, F.
------=_Part_22230002_1751095465.1384256418493 Content-Type: text/x-patch; name=thread_wait.patch Content-Disposition: attachment; filename=thread_wait.patch Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL2JhY2tlbmQvbWFuYWdlci9tb2R1bGVzL2JsbC9zcmMvbWFpbi9qYXZhL29y Zy9vdmlydC9lbmdpbmUvY29yZS9ibGwvTXVsdGlwbGVBY3Rpb25zUnVubmVyLmphdmEgYi9iYWNr ZW5kL21hbmFnZXIvbW9kdWxlcy9ibGwvc3JjL21haW4vamF2YS9vcmcvb3ZpcnQvZW5naW5lL2Nv cmUvYmxsL011bHRpcGxlQWN0aW9uc1J1bm5lci5qYXZhDQppbmRleCBhNzJmYmQ5Li4xYjRmNGRh IDEwMDY0NA0KLS0tIGEvYmFja2VuZC9tYW5hZ2VyL21vZHVsZXMvYmxsL3NyYy9tYWluL2phdmEv b3JnL292aXJ0L2VuZ2luZS9jb3JlL2JsbC9NdWx0aXBsZUFjdGlvbnNSdW5uZXIuamF2YQ0KKysr IGIvYmFja2VuZC9tYW5hZ2VyL21vZHVsZXMvYmxsL3NyYy9tYWluL2phdmEvb3JnL292aXJ0L2Vu Z2luZS9jb3JlL2JsbC9NdWx0aXBsZUFjdGlvbnNSdW5uZXIuamF2YQ0KQEAgLTEsNiArMSw3IEBA DQogcGFja2FnZSBvcmcub3ZpcnQuZW5naW5lLmNvcmUuYmxsOw0KIA0KIGltcG9ydCBqYXZhLnV0 aWwuQXJyYXlMaXN0Ow0KK2ltcG9ydCBqYXZhLnV0aWwuQ29sbGVjdGlvbnM7DQogaW1wb3J0IGph dmEudXRpbC5MaXN0Ow0KIGltcG9ydCBqYXZhLnV0aWwuY29uY3VycmVudC5DYWxsYWJsZTsNCiAN CkBAIC0yMCw3ICsyMSw3IEBAIHB1YmxpYyBjbGFzcyBNdWx0aXBsZUFjdGlvbnNSdW5uZXIgew0K ICAgICBwcml2YXRlIGZpbmFsIHN0YXRpYyBpbnQgQ09OQ1VSUkVOVF9BQ1RJT05TID0gMTA7DQog DQogICAgIHByaXZhdGUgVmRjQWN0aW9uVHlwZSBfYWN0aW9uVHlwZSA9IFZkY0FjdGlvblR5cGUu VW5rbm93bjsNCi0gICAgcHJpdmF0ZSBMaXN0PFZkY0FjdGlvblBhcmFtZXRlcnNCYXNlPiBfcGFy YW1ldGVyczsNCisgICAgcHJpdmF0ZSBmaW5hbCBMaXN0PFZkY0FjdGlvblBhcmFtZXRlcnNCYXNl PiBfcGFyYW1ldGVyczsNCiAgICAgcHJpdmF0ZSBmaW5hbCBBcnJheUxpc3Q8Q29tbWFuZEJhc2U8 Pz4+IF9jb21tYW5kcyA9IG5ldyBBcnJheUxpc3Q8Q29tbWFuZEJhc2U8Pz4+KCk7DQogICAgIHBy b3RlY3RlZCBib29sZWFuIGlzSW50ZXJuYWw7DQogDQpAQCAtNTUsNyArNTYsNyBAQCBwdWJsaWMg Y2xhc3MgTXVsdGlwbGVBY3Rpb25zUnVubmVyIHsNCiAgICAgICAgICAgICByZXR1cm4gbmV3IEFy cmF5TGlzdDxWZGNSZXR1cm5WYWx1ZUJhc2U+KCk7DQogICAgICAgICB9DQogDQotICAgICAgICBB cnJheUxpc3Q8VmRjUmV0dXJuVmFsdWVCYXNlPiByZXR1cm5WYWx1ZXMgPSBuZXcgQXJyYXlMaXN0 PFZkY1JldHVyblZhbHVlQmFzZT4oKTsNCisgICAgICAgIEFycmF5TGlzdDxWZGNSZXR1cm5WYWx1 ZUJhc2U+IHJldHVyblZhbHVlcyA9IChBcnJheUxpc3Q8VmRjUmV0dXJuVmFsdWVCYXNlPikgQ29s bGVjdGlvbnMuc3luY2hyb25pemVkTGlzdChuZXcgQXJyYXlMaXN0PFZkY1JldHVyblZhbHVlQmFz ZT4oKSk7DQogICAgICAgICB0cnkgew0KICAgICAgICAgICAgIFZkY1JldHVyblZhbHVlQmFzZSBy ZXR1cm5WYWx1ZTsNCiAgICAgICAgICAgICBmb3IgKFZkY0FjdGlvblBhcmFtZXRlcnNCYXNlIHBh cmFtZXRlciA6IGdldFBhcmFtZXRlcnMoKSkgew0KQEAgLTg4LDEyICs4OSwxMiBAQCBwdWJsaWMg Y2xhc3MgTXVsdGlwbGVBY3Rpb25zUnVubmVyIHsNCiAgICAgICAgICAgICB9DQogDQogICAgICAg ICAgICAgaWYgKGNhblJ1bkFjdGlvbnMpIHsNCi0gICAgICAgICAgICAgICAgVGhyZWFkUG9vbFV0 aWwuZXhlY3V0ZShuZXcgUnVubmFibGUoKSB7DQotICAgICAgICAgICAgICAgICAgICBAT3ZlcnJp ZGUNCi0gICAgICAgICAgICAgICAgICAgIHB1YmxpYyB2b2lkIHJ1bigpIHsNCi0gICAgICAgICAg ICAgICAgICAgICAgICBSdW5Db21tYW5kcygpOw0KLSAgICAgICAgICAgICAgICAgICAgfQ0KLSAg ICAgICAgICAgICAgICB9KTsNCisvLyAgICAgICAgICAgICAgICBUaHJlYWRQb29sVXRpbC5leGVj dXRlKG5ldyBSdW5uYWJsZSgpIHsNCisvLyAgICAgICAgICAgICAgICAgICAgQE92ZXJyaWRlDQor Ly8gICAgICAgICAgICAgICAgICAgIHB1YmxpYyB2b2lkIHJ1bigpIHsNCisgICAgICAgICAgICAg ICAgICAgICAgICBSdW5Db21tYW5kcyhyZXR1cm5WYWx1ZXMpOw0KKy8vICAgICAgICAgICAgICAg ICAgICB9DQorLy8gICAgICAgICAgICAgICAgfSk7DQogICAgICAgICAgICAgfQ0KICAgICAgICAg fSBjYXRjaCAoUnVudGltZUV4Y2VwdGlvbiBlKSB7DQogICAgICAgICAgICAgbG9nLmVycm9yKCJG YWlsZWQgdG8gZXhlY3V0ZSBtdWx0aXBsZSBhY3Rpb25zIG9mIHR5cGU6ICIgKyBfYWN0aW9uVHlw ZSwgZSk7DQpAQCAtMTMyLDYgKzEzMywxNiBAQCBwdWJsaWMgY2xhc3MgTXVsdGlwbGVBY3Rpb25z UnVubmVyIHsNCiAgICAgICAgIH07DQogICAgIH0NCiANCisgICAgcHJvdGVjdGVkIENhbGxhYmxl PFZkY1JldHVyblZhbHVlQmFzZT4gYnVpbGRBY3Rpb25Bc3luYyhmaW5hbCBDb21tYW5kQmFzZTw/ PiBjb21tYW5kKSB7DQorICAgICAgICByZXR1cm4gbmV3IENhbGxhYmxlPFZkY1JldHVyblZhbHVl QmFzZT4oKSB7DQorDQorICAgICAgICAgICAgQE92ZXJyaWRlDQorICAgICAgICAgICAgcHVibGlj IFZkY1JldHVyblZhbHVlQmFzZSBjYWxsKCkgew0KKyAgICAgICAgICAgICAgICByZXR1cm4gZXhl Y3V0ZVZhbGlkYXRlZENvbW1hbmQyKGNvbW1hbmQpOw0KKyAgICAgICAgICAgIH0NCisgICAgICAg IH07DQorICAgIH0NCisNCiAgICAgcHJvdGVjdGVkIFZkY1JldHVyblZhbHVlQmFzZSBydW5DYW5E b0FjdGlvbk9ubHkoZmluYWwgaW50IGN1cnJlbnRDYW5Eb0FjdGlvbklkLCBmaW5hbCBpbnQgdG90 YWxTaXplKSB7DQogICAgICAgICBDb21tYW5kQmFzZTw/PiBjb21tYW5kID0gZ2V0Q29tbWFuZHMo KS5nZXQoY3VycmVudENhbkRvQWN0aW9uSWQpOw0KICAgICAgICAgU3RyaW5nIGFjdGlvblR5cGUg PSBjb21tYW5kLmdldEFjdGlvblR5cGUoKS50b1N0cmluZygpOw0KQEAgLTE1OCw2ICsxNjksMTYg QEAgcHVibGljIGNsYXNzIE11bHRpcGxlQWN0aW9uc1J1bm5lciB7DQogICAgICAgICB9DQogICAg IH0NCiANCisgICAgcHJvdGVjdGVkIHZvaWQgUnVuQ29tbWFuZHMoQXJyYXlMaXN0PFZkY1JldHVy blZhbHVlQmFzZT4gcmV0dXJuVmFsdWVzKSB7DQorICAgICAgICBMaXN0PENhbGxhYmxlPFZkY1Jl dHVyblZhbHVlQmFzZT4+IGFjdGlvblRhc2tzID0gbmV3IEFycmF5TGlzdDxDYWxsYWJsZTxWZGNS ZXR1cm5WYWx1ZUJhc2U+PigpOw0KKyAgICAgICAgZm9yIChDb21tYW5kQmFzZTw/PiBjb21tYW5k IDogZ2V0Q29tbWFuZHMoKSkgew0KKyAgICAgICAgICAgIGlmIChjb21tYW5kLmdldFJldHVyblZh bHVlKCkuZ2V0Q2FuRG9BY3Rpb24oKSkgew0KKyAgICAgICAgICAgICAgICBhY3Rpb25UYXNrcy5h ZGQoYnVpbGRBY3Rpb25Bc3luYyhjb21tYW5kKSk7DQorICAgICAgICAgICAgfQ0KKyAgICAgICAg fQ0KKyAgICAgICAgcmV0dXJuVmFsdWVzLmFkZEFsbChUaHJlYWRQb29sVXRpbC5pbnZva2VBbGwo YWN0aW9uVGFza3MpKTsNCisgICAgfQ0KKw0KICAgICAvKioNCiAgICAgICogRXhlY3V0ZXMgY29t bWFuZHMgd2hpY2ggcGFzc2VkIHZhbGlkYXRpb24gYW5kIGNyZWF0ZXMgbW9uaXRvcmluZyBvYmpl Y3RzLg0KICAgICAgKg0KQEAgLTE3NSw2ICsxOTYsMTcgQEAgcHVibGljIGNsYXNzIE11bHRpcGxl QWN0aW9uc1J1bm5lciB7DQogICAgICAgICBjb21tYW5kLmV4ZWN1dGVBY3Rpb24oKTsNCiAgICAg fQ0KIA0KKyAgICBwcm90ZWN0ZWQgVmRjUmV0dXJuVmFsdWVCYXNlIGV4ZWN1dGVWYWxpZGF0ZWRD b21tYW5kMihDb21tYW5kQmFzZTw/PiBjb21tYW5kKSB7DQorICAgICAgICBpZiAoZXhlY3V0aW9u Q29udGV4dCA9PSBudWxsIHx8IGV4ZWN1dGlvbkNvbnRleHQuaXNNb25pdG9yZWQoKSkgew0KKyAg ICAgICAgICAgIEV4ZWN1dGlvbkhhbmRsZXIucHJlcGFyZUNvbW1hbmRGb3JNb25pdG9yaW5nKGNv bW1hbmQsDQorICAgICAgICAgICAgICAgICAgICBjb21tYW5kLmdldEFjdGlvblR5cGUoKSwNCisg ICAgICAgICAgICAgICAgICAgIGNvbW1hbmQuaXNJbnRlcm5hbEV4ZWN1dGlvbigpKTsNCisgICAg ICAgIH0NCisgICAgICAgIFRocmVhZExvY2FsUGFyYW1zQ29udGFpbmVyLnNldENvcnJlbGF0aW9u SWQoY29tbWFuZC5nZXRDb3JyZWxhdGlvbklkKCkpOw0KKyAgICAgICAgY29tbWFuZC5pbnNlcnRB c3luY1Rhc2tQbGFjZUhvbGRlcnMoKTsNCisgICAgICAgIHJldHVybiBjb21tYW5kLmV4ZWN1dGVB Y3Rpb24oKTsNCisgICAgfQ0KKw0KICAgICBwdWJsaWMgdm9pZCBzZXRFeGVjdXRpb25Db250ZXh0 KEV4ZWN1dGlvbkNvbnRleHQgZXhlY3V0aW9uQ29udGV4dCkgew0KICAgICAgICAgdGhpcy5leGVj dXRpb25Db250ZXh0ID0gZXhlY3V0aW9uQ29udGV4dDsNCiAgICAgfQ0K ------=_Part_22230002_1751095465.1384256418493--
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)? ---- Thanks, Einav ----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all commands:
A. the "for" block which iterates over getParameters() 1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size() 1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions 1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList -> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in "synchronized (list)" block, so this:
for (VdcReturnValueBase value : returnValues) ...
should be this:
synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable) -> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote:
Hi Alex,
recently I noticed problems with invoking console for multiple VMs (select more VMs in webadmin and then hit the console btn). I was sure it worked in past so i git-bisected the master branch and I discovered that this problem is apparently caused by patch [1]. For single vm console invocation it works fine, but for multiple VMs it doesn't.
I did some closer investigation with a debugger and it seems that getSucceeded() on the return val of SetVmTicket command returns false in case of multiple execution despite the fact SetVmTicketCommand sets this value to true. I suppose there is some problem with propagation of command return value from BE to FE. The same goes for the encapsulated returnValue attribute (it contains value of the generated ticket). When invoking multiple consoles it is null (although it has been filled on backend). Weird.
Tomas told me you did some optimizations for multiple command executions, do you know it might cause it? Please let me know if you have any idea...
Cheers, F.
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: awels@redhat.com, "engine-devel" <engine-devel@ovirt.org>, "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 18, 2013 10:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
+1 (or in other words, the purpose behind MultipleActionsRunner line 91 -> ThreadPoolUtil.execute(...RunCommands...) -> actions invoked in separate thread and MultipleActionsRunner.Execute immediately returns)
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all commands:
A. the "for" block which iterates over getParameters() 1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size() 1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions 1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList -> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in "synchronized (list)" block, so this:
for (VdcReturnValueBase value : returnValues) ...
should be this:
synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable) -> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote:
Hi Alex,
recently I noticed problems with invoking console for multiple VMs (select more VMs in webadmin and then hit the console btn). I was sure it worked in past so i git-bisected the master branch and I discovered that this problem is apparently caused by patch [1]. For single vm console invocation it works fine, but for multiple VMs it doesn't.
I did some closer investigation with a debugger and it seems that getSucceeded() on the return val of SetVmTicket command returns false in case of multiple execution despite the fact SetVmTicketCommand sets this value to true. I suppose there is some problem with propagation of command return value from BE to FE. The same goes for the encapsulated returnValue attribute (it contains value of the generated ticket). When invoking multiple consoles it is null (although it has been filled on backend). Weird.
Tomas told me you did some optimizations for multiple command executions, do you know it might cause it? Please let me know if you have any idea...
Cheers, F.
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Monday, November 18, 2013 11:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
There's a couple of years old patch exactly for that [1]. It should allow both synchronous and asynchronous multiple actions according to a flag. Question is when do we want to use the synchronous flavor? The reasoning behind the current implementation is to return a quick response to the client, as executing multiple vdsm commands can potentially require a long wait. Do we need to introduce a sync version just for VM console connection flow? IIRC, the flow actually uses multiple calls to the singular form (RunAction) rather than RunMultipleAction? [1] http://gerrit.ovirt.org/#/c/3210/
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all commands:
A. the "for" block which iterates over getParameters() 1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size() 1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions 1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList -> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in "synchronized (list)" block, so this:
for (VdcReturnValueBase value : returnValues) ...
should be this:
synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable) -> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote:
Hi Alex,
recently I noticed problems with invoking console for multiple VMs (select more VMs in webadmin and then hit the console btn). I was sure it worked in past so i git-bisected the master branch and I discovered that this problem is apparently caused by patch [1]. For single vm console invocation it works fine, but for multiple VMs it doesn't.
I did some closer investigation with a debugger and it seems that getSucceeded() on the return val of SetVmTicket command returns false in case of multiple execution despite the fact SetVmTicketCommand sets this value to true. I suppose there is some problem with propagation of command return value from BE to FE. The same goes for the encapsulated returnValue attribute (it contains value of the generated ticket). When invoking multiple consoles it is null (although it has been filled on backend). Weird.
Tomas told me you did some optimizations for multiple command executions, do you know it might cause it? Please let me know if you have any idea...
Cheers, F.
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote:
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Monday, November 18, 2013 11:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
There's a couple of years old patch exactly for that [1]. It should allow both synchronous and asynchronous multiple actions according to a flag. Question is when do we want to use the synchronous flavor? The reasoning behind the current implementation is to return a quick response to the client, as executing multiple vdsm commands can potentially require a long wait. Do we need to introduce a sync version just for VM console connection flow? IIRC, the flow actually uses multiple calls to the singular form (RunAction) rather than RunMultipleAction?
We recently introduced a patch [1] that on the front-end is smart enough to consolidate multiple query/actions into a single runMultiple(Query/Action) call to reduce the number http requests to the back-end. In the SetVMTicket case it combined two actions into one runMultipleAction call and was expected that to return once all the actions where complete, which didn't happen causing some undesired behavior on the client side. This sparked the investigation that ended in this email. So yes I would definitely like a 'synchronous' version of runMultipleAction so the frontend code can set that flag when combining multiple actions into a single call. We can only safely do this if we are combining several runActions as all of those are 'synchronous'. [1] http://gerrit.ovirt.org/#/c/17356/
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all commands: A. the "for" block which iterates over getParameters()
1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size()
1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions
1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList
-> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in
"synchronized (list)" block, so this: for (VdcReturnValueBase value : returnValues) ...
should be this: synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable)
-> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote:
Hi Alex,
recently I noticed problems with invoking console for multiple VMs (select more VMs in webadmin and then hit the console btn). I was sure it worked in past so i git-bisected the master branch and I discovered that this problem is apparently caused by patch [1]. For single vm console invocation it works fine, but for multiple VMs it doesn't.
I did some closer investigation with a debugger and it seems that getSucceeded() on the return val of SetVmTicket command returns false in case of multiple execution despite the fact SetVmTicketCommand sets this value to true. I suppose there is some problem with propagation of command return value from BE to FE. The same goes for the encapsulated returnValue attribute (it contains value of the generated ticket). When invoking multiple consoles it is null (although it has been filled on backend). Weird.
Tomas told me you did some optimizations for multiple command executions, do you know it might cause it? Please let me know if you have any idea...
Cheers, F.
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 11/21/2013 03:28 PM, Alexander Wels wrote:
On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote:
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Monday, November 18, 2013 11:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
There's a couple of years old patch exactly for that [1]. It should allow both synchronous and asynchronous multiple actions according to a flag. Question is when do we want to use the synchronous flavor? The reasoning behind the current implementation is to return a quick response to the client, as executing multiple vdsm commands can potentially require a long wait. Do we need to introduce a sync version just for VM console connection flow? IIRC, the flow actually uses multiple calls to the singular form (RunAction) rather than RunMultipleAction?
We recently introduced a patch [1] that on the front-end is smart enough to consolidate multiple query/actions into a single runMultiple(Query/Action) call to reduce the number http requests to the back-end. In the SetVMTicket case it combined two actions into one runMultipleAction call and was expected that to return once all the actions where complete, which didn't happen causing some undesired behavior on the client side. This sparked the investigation that ended in this email.
So yes I would definitely like a 'synchronous' version of runMultipleAction so the frontend code can set that flag when combining multiple actions into a single call. We can only safely do this if we are combining several runActions as all of those are 'synchronous'.
btw, how is this going to work with the REST API?
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all commands: A. the "for" block which iterates over getParameters()
1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size()
1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions
1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList
-> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in
"synchronized (list)" block, so this: for (VdcReturnValueBase value : returnValues) ...
should be this: synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable)
-> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote:
Ok, thank you very much!
----- Original Message ----- From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Sent: Monday, November 11, 2013 2:15:43 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Frantisek,
I had seen this before, let me test and fix it for you, it is very likely my patch broke that.
Alexander
On Monday, November 11, 2013 03:43:19 AM you wrote: > Hi Alex, > > recently I noticed problems with invoking console for multiple VMs > (select > more VMs in webadmin and then hit the console btn). I was sure it > worked > in > past so i git-bisected the master branch and I discovered that > this > problem > is apparently caused by patch [1]. For single vm console > invocation > it > works fine, but for multiple VMs it doesn't. > > I did some closer investigation with a debugger and it seems that > getSucceeded() on the return val of SetVmTicket command returns > false > in > case of multiple execution despite the fact SetVmTicketCommand > sets > this > value to true. I suppose there is some problem with propagation of > command > return value from BE to FE. The same goes for the encapsulated > returnValue > attribute (it contains value of the generated ticket). When > invoking > multiple consoles it is null (although it has been filled on > backend). > Weird. > > Tomas told me you did some optimizations for multiple command > executions, > do you know it might cause it? Please let me know if you have any > idea... > > Cheers, > F. > > [1]: http://gerrit.ovirt.org/#/c/17356/
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On Thursday, November 21, 2013 04:01:34 PM Itamar Heim wrote:
On 11/21/2013 03:28 PM, Alexander Wels wrote:
On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote:
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Monday, November 18, 2013 11:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
There's a couple of years old patch exactly for that [1]. It should allow both synchronous and asynchronous multiple actions according to a flag. Question is when do we want to use the synchronous flavor? The reasoning behind the current implementation is to return a quick response to the client, as executing multiple vdsm commands can potentially require a long wait. Do we need to introduce a sync version just for VM console connection flow? IIRC, the flow actually uses multiple calls to the singular form (RunAction) rather than RunMultipleAction?
We recently introduced a patch [1] that on the front-end is smart enough to consolidate multiple query/actions into a single runMultiple(Query/Action) call to reduce the number http requests to the back-end. In the SetVMTicket case it combined two actions into one runMultipleAction call and was expected that to return once all the actions where complete, which didn't happen causing some undesired behavior on the client side. This sparked the investigation that ended in this email.
So yes I would definitely like a 'synchronous' version of runMultipleAction so the frontend code can set that flag when combining multiple actions into a single call. We can only safely do this if we are combining several runActions as all of those are 'synchronous'.
btw, how is this going to work with the REST API?
I believe that is one of the outstanding issues with regards to the REST API transition. Before this optimization we already had code calling runMultiple(Query/Action) and we will have to figure something out for the REST API there as well. I think at the point we solve that issue we can decide if we want to keep the optimization or not.
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all
commands:
A. the "for" block which iterates over getParameters()
1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size()
1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions
1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList
-> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in
"synchronized (list)" block, so this: for (VdcReturnValueBase value : returnValues) ...
should be this: synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable)
-> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Frantisek Kobzik" <fkobzik@redhat.com> Cc: "Vojtech Szocs" <vszocs@redhat.com> Sent: Monday, November 11, 2013 9:19:08 PM Subject: Re: Weird behavior of multiple SetVmTicket query
Hi,
I did some debugging, and the problem is a race condition in the MultipleActions class. The whole class seems to have a lot of multi-threading issues but if I modify the code to wait for the results of the actions to return before returning the return value, everything works fine.
I am attaching a patch that solves the issue at hand, but should not be considered a real patch. It is just to show the issue is in the back-end not the front-end code. The front-end code is just exposing an issue in the back- end
Alexander
On Monday, November 11, 2013 09:53:22 AM you wrote: > Ok, thank you very much! > > ----- Original Message ----- > From: "Alexander Wels" <awels@redhat.com> > To: "Frantisek Kobzik" <fkobzik@redhat.com> > Sent: Monday, November 11, 2013 2:15:43 PM > Subject: Re: Weird behavior of multiple SetVmTicket query > > Frantisek, > > I had seen this before, let me test and fix it for you, it is very > likely > my > patch broke that. > > Alexander > > On Monday, November 11, 2013 03:43:19 AM you wrote: >> Hi Alex, >> >> recently I noticed problems with invoking console for multiple VMs >> (select >> more VMs in webadmin and then hit the console btn). I was sure it >> worked >> in >> past so i git-bisected the master branch and I discovered that >> this >> problem >> is apparently caused by patch [1]. For single vm console >> invocation >> it >> works fine, but for multiple VMs it doesn't. >> >> I did some closer investigation with a debugger and it seems that >> getSucceeded() on the return val of SetVmTicket command returns >> false >> in >> case of multiple execution despite the fact SetVmTicketCommand >> sets >> this >> value to true. I suppose there is some problem with propagation of >> command >> return value from BE to FE. The same goes for the encapsulated >> returnValue >> attribute (it contains value of the generated ticket). When >> invoking >> multiple consoles it is null (although it has been filled on >> backend). >> Weird. >> >> Tomas told me you did some optimizations for multiple command >> executions, >> do you know it might cause it? Please let me know if you have any >> idea... >> >> Cheers, >> F. >> >> [1]: http://gerrit.ovirt.org/#/c/17356/
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Alexander Wels" <awels@redhat.com> To: "Itamar Heim" <iheim@redhat.com> Cc: "Oved Ourfalli" <ovedo@redhat.com>, engine-devel@ovirt.org Sent: Thursday, November 21, 2013 3:15:40 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
On Thursday, November 21, 2013 04:01:34 PM Itamar Heim wrote:
On 11/21/2013 03:28 PM, Alexander Wels wrote:
On Thursday, November 21, 2013 07:18:42 AM Daniel Erez wrote:
----- Original Message -----
From: "Einav Cohen" <ecohen@redhat.com> To: "Livnat Peer" <lpeer@redhat.com>, "Eli Mesika" <emesika@redhat.com>, "Omer Frenkel" <ofrenkel@redhat.com>, "Doron Fediuck" <dfediuck@redhat.com>, "Oved Ourfalli" <ovedo@redhat.com> Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Monday, November 18, 2013 11:19:23 PM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
engine-core maintainers - this one is mainly for you - see below. the most important question (I think) is: Is there any reason to not introduced a "sync" flavor of RunMultipleActions (i.e. one that will return from the backend only when all actions have been completed, similarly to RunAction)?
There's a couple of years old patch exactly for that [1]. It should allow both synchronous and asynchronous multiple actions according to a flag. Question is when do we want to use the synchronous flavor? The reasoning behind the current implementation is to return a quick response to the client, as executing multiple vdsm commands can potentially require a long wait. Do we need to introduce a sync version just for VM console connection flow? IIRC, the flow actually uses multiple calls to the singular form (RunAction) rather than RunMultipleAction?
We recently introduced a patch [1] that on the front-end is smart enough to consolidate multiple query/actions into a single runMultiple(Query/Action) call to reduce the number http requests to the back-end. In the SetVMTicket case it combined two actions into one runMultipleAction call and was expected that to return once all the actions where complete, which didn't happen causing some undesired behavior on the client side. This sparked the investigation that ended in this email.
So yes I would definitely like a 'synchronous' version of runMultipleAction so the frontend code can set that flag when combining multiple actions into a single call. We can only safely do this if we are combining several runActions as all of those are 'synchronous'.
btw, how is this going to work with the REST API?
I believe that is one of the outstanding issues with regards to the REST API transition. Before this optimization we already had code calling runMultiple(Query/Action) and we will have to figure something out for the REST API there as well. I think at the point we solve that issue we can decide if we want to keep the optimization or not.
Yes, UI code currently reflects (and uses) what internal Backend API offers. This is why I mentioned we will eventually have to move away from query/action concept (RPC-method-centric) to REST resource/operation concept (resource-centric). GenericApiGWTService server-side implementation (aka GWT RPC endpoint) more or less delegates to BackendLocal which exposes methods such as RunMultipleActions by itself. To emulate RunMultipleActions when using REST API, assuming there is no such support provided by REST backend, we'll have to do that via oVirt.js library.
---- Thanks, Einav
----- Original Message -----
From: "Vojtech Szocs" <vszocs@redhat.com> To: awels@redhat.com Cc: "engine-devel" <engine-devel@ovirt.org> Sent: Tuesday, November 12, 2013 6:40:18 AM Subject: Re: [Engine-devel] Weird behavior of multiple SetVmTicket query
Forwarding this email to engine-devel so that backend maintainers are aware of this issue.
Looking at the code:
- MultipleActionsRunner#Execute first creates and "validates" all
commands:
A. the "for" block which iterates over getParameters()
1-> validate correlation ID, if OK create and add command, otherwise add returnValue
B the "if" block which tests getCommands().size()
1-> if single command to execute, add its "canDoActionOnly" returnValue, which is returnValue with canDoAction but without actual result object 2-> if multi commands to execute, execute chunks of max. 10 threads (sequentially, ThreadPoolUtil.invokeAll returns after all threads complete), same returnValue as above
C. the "if" block which tests canRunActions
1-> all commands are executed within SINGLE THREAD due to ThreadPoolUtil.execute(Runnable), which is kind of weird comapred to how returnValues are prepared (see B2) 2-> when executing command, code DOES NOT CARE about its returnValue, i.e. returnValue was already prepared (see B) and command execution should just update it
The problem (I think) is that C1 starts a different thread (to execute all commands) and immediately returns, i.e. code doesn't wait for thread to complete. This is why returnValues are observed on frontend as inconsistent.
Additionally, we're also mixing of two different things: canDoAction processing and returnValues processing. IMHO this should be refactored to make code easier to read.
Changes done by Alex (patch attached):
X1. returnValues changed to Collections.synchronizedList
-> this means all access to returnValues is now serial -> iteration over synchronizedList should also be enclosed in
"synchronized (list)" block, so this: for (VdcReturnValueBase value : returnValues) ...
should be this: synchronized (returnValues) { for (VdcReturnValueBase value : returnValues) ... }
X2. commented-out original command execution via ThreadPoolUtil.execute(Runnable)
-> new RunCommands method invokes all commands each in separate thread via ThreadPoolUtil.invokeAll -> returnValues list is explicitly updated
Guys, what do you think?
Vojtech
----- Original Message -----
> From: "Alexander Wels" <awels@redhat.com> > To: "Frantisek Kobzik" <fkobzik@redhat.com> > Cc: "Vojtech Szocs" <vszocs@redhat.com> > Sent: Monday, November 11, 2013 9:19:08 PM > Subject: Re: Weird behavior of multiple SetVmTicket query > > Hi, > > I did some debugging, and the problem is a race condition in the > MultipleActions class. The whole class seems to have a lot of > multi-threading > issues but if I modify the code to wait for the results of the > actions > to > return before returning the return value, everything works fine. > > I am attaching a patch that solves the issue at hand, but should not > be > considered a real patch. It is just to show the issue is in the > back-end > not > the front-end code. The front-end code is just exposing an issue in > the > back- > end > > Alexander > > On Monday, November 11, 2013 09:53:22 AM you wrote: >> Ok, thank you very much! >> >> ----- Original Message ----- >> From: "Alexander Wels" <awels@redhat.com> >> To: "Frantisek Kobzik" <fkobzik@redhat.com> >> Sent: Monday, November 11, 2013 2:15:43 PM >> Subject: Re: Weird behavior of multiple SetVmTicket query >> >> Frantisek, >> >> I had seen this before, let me test and fix it for you, it is very >> likely >> my >> patch broke that. >> >> Alexander >> >> On Monday, November 11, 2013 03:43:19 AM you wrote: >>> Hi Alex, >>> >>> recently I noticed problems with invoking console for multiple VMs >>> (select >>> more VMs in webadmin and then hit the console btn). I was sure it >>> worked >>> in >>> past so i git-bisected the master branch and I discovered that >>> this >>> problem >>> is apparently caused by patch [1]. For single vm console >>> invocation >>> it >>> works fine, but for multiple VMs it doesn't. >>> >>> I did some closer investigation with a debugger and it seems that >>> getSucceeded() on the return val of SetVmTicket command returns >>> false >>> in >>> case of multiple execution despite the fact SetVmTicketCommand >>> sets >>> this >>> value to true. I suppose there is some problem with propagation of >>> command >>> return value from BE to FE. The same goes for the encapsulated >>> returnValue >>> attribute (it contains value of the generated ticket). When >>> invoking >>> multiple consoles it is null (although it has been filled on >>> backend). >>> Weird. >>> >>> Tomas told me you did some optimizations for multiple command >>> executions, >>> do you know it might cause it? Please let me know if you have any >>> idea... >>> >>> Cheers, >>> F. >>> >>> [1]: http://gerrit.ovirt.org/#/c/17356/
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
participants (5)
-
Alexander Wels -
Daniel Erez -
Einav Cohen -
Itamar Heim -
Vojtech Szocs