CA-287863: xe vm-shutdown complete the task too early #3556

krizex · 2018-04-12T07:42:52Z

When master forward a task to slave, slave will mark the task as completed once
finish processing it, however when master finishing the forwarding, it will also
mark the task as completed. Furthermore, when slave finished the task, that does
not mean the task is done, there may be some stuff that master has to handle, so
we have to ensure the task only complete once the master finish its work.

Signed-off-by: Yang Qian yang.qian@citrix.com

coveralls · 2018-04-12T08:02:28Z

Coverage increased (+0.001%) to 20.363% when pulling 7c31dd1 on krizex:CA-287863 into 7cea812 on xapi-project:master.

lindig · 2018-04-12T09:50:35Z

ocaml/xapi/server_helpers.ml

          forward ~local_fn:f ~__context
    in
+    (* For forwarded task, we should not complete it here, the server which forward the task will complete it *)
+    if not (Context.forwarded_task __context) then


This code has become dependent on a condition but indentation has not changed to reflect this. Please check the indentation of the code.

robhoes

Brilliant! This also finally gets rid of the stupid "the status of xxx is: success; cannot set it to success" log lines :)

robhoes · 2018-04-12T13:14:14Z

Don't merge yet though, because some BST tests seem to be failing. Please check those out first.

krizex · 2018-04-12T13:16:34Z

ocaml/xapi/server_helpers.ml

+    if not (Context.forwarded_task __context) then begin
+      match marshaller with
      | None    -> TaskHelper.complete ~__context None
      | Some fn -> TaskHelper.complete ~__context (Some (fn result))


@robhoes I have some concern about this. After this change, the result in the slave won't be updated to the task result, but instead will update with the execution result in the master, will it bring some other issues?

I should hope that the result of the forwarded call is already always returned (unaltered) by the master to the original caller, especially for synchronous calls. So I think this is fine.

robhoes · 2018-04-12T13:26:41Z

The failing test seems to be an import that times out. We should check how the task completion is handled for such calls, which don't come as RPCs through server.ml, but have their own HTTP handlers.

krizex · 2018-04-12T13:29:04Z

Seems the installation of the build is failed, so all BST & BVT failed. I have updated with the latest specs of team/ring3/master and has started rerun the test again.
Privately I have tested this fix by uploading xapi to my hosts and the error messages gone.

robhoes · 2018-04-12T13:30:29Z

Well, the installation was OK, but the 62-create-guest-templates firstboot script failed:

Load /usr/share/xapi/vm-templates/sles-11-sp4-64bit.json
Load /usr/share/xapi/vm-templates/ubuntu-12.04-64bit.json
Load /usr/share/xapi/vm-templates/base-windows-8.json
Insert 11fd3dc9-96cc-49af-b091-a2ca7e94c589
Traceback (most recent call last):
  File "/usr/bin/create-guest-templates", line 17, in <module>
    loader.insert_templates()
  File "/usr/lib/python2.7/site-packages/guesttemplates/loader.py", line 181, in insert_templates
    self._insert_template(i)
  File "/usr/lib/python2.7/site-packages/guesttemplates/loader.py", line 165, in _insert_template
    raise RuntimeError("Template import timeout")
RuntimeError: Template import timeout

This does a VM metadata import.

krizex · 2018-04-12T13:35:31Z

Hmm, I will take a look at that tomorrow.

Move `exec` into `exec_with_context` to simplify the code. Signed-off-by: Yang Qian <yang.qian@citrix.com>

When master forward a task to slave, slave will mark the task as completed once finish processing it, however when master finishing the forwarding, it will also mark the task as completed. Furthermore, when slave finished the task, that does not mean the task is done, there may be some stuff that master has to handle, so we have to ensure the task only complete once the master finish its work. For the task forwarded from client, which called with `exec_with_forwarded_task`, we also have to complete it once it finished. Signed-off-by: Yang Qian <yang.qian@citrix.com>

krizex · 2018-04-13T08:20:17Z

BST & BVT passed, please have a look again @robhoes , thanks.

robhoes · 2018-04-13T09:57:28Z

Hmm... I wonder why exec_with_forwarded_task is used in Xapi_http.with_context. That looks odd to me. In this patch, I don't quite like the new ~has_task param. I'll look a bit further...

krizex · 2018-04-13T10:03:49Z

@robhoes The name of has_task comes from the original comment https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/server_helpers.ml#L59
I meant to introduce a new param ~need_complete ;-)

robhoes · 2018-04-13T10:33:17Z

Ah, that was another mysterious comment indeed :)

robhoes · 2018-04-13T10:50:44Z

I think that it is declared a forwarded task, to make sure that the task is not destroyed when the operation completes; the task is created by the client, and should be destroyed by the client.

I also notice that the "forwarded task destroyed" log lines that we often see actually mean that the forwarded task was not destroyed...

robhoes · 2018-04-13T13:21:46Z

We are on the right track here, but before we continue, I am going to make an overview about how tasks and contexts are used in xapi right now, because it is too confusing.

krizex · 2018-04-16T09:21:10Z

@robhoes Yes, it takes more than half a day to understand how context/task works.
Besides this change, I think the Server_helpers.exec_with_forwarded_task is a bad name, for every reference, it is not with a forwarded task, instead it's a task created by client side. So how about rename it to exec_with_existed_task to make it clearer?

robhoes · 2018-04-16T10:42:31Z

Well that is indeed the problem and one of the sources of confusion: the word forwarded is used to different things, or at least the function is "abused" for different things. I think that we should reserve the term "forwarding" for the action of passing a call from the master to a slave (as in "message_forwarding").

I have some ideas now to simplify some of this area, and will do that in follow-up PRs. I'll accept this PR now, because it already fixes a real issue.

krizex requested review from jonludlam and robhoes April 12, 2018 07:42

lindig reviewed Apr 12, 2018

View reviewed changes

krizex force-pushed the CA-287863 branch 2 times, most recently from 42620dc to 69783da Compare April 12, 2018 10:34

robhoes approved these changes Apr 12, 2018

View reviewed changes

krizex commented Apr 12, 2018

View reviewed changes

krizex added the don't merge label Apr 12, 2018

krizex force-pushed the CA-287863 branch from 69783da to 68f8976 Compare April 13, 2018 06:31

krizex added 2 commits April 13, 2018 14:38

CA-287863: Reorgnize the code

3f9e67c

Move `exec` into `exec_with_context` to simplify the code. Signed-off-by: Yang Qian <yang.qian@citrix.com>

krizex force-pushed the CA-287863 branch from 68f8976 to 7c31dd1 Compare April 13, 2018 06:38

krizex removed the don't merge label Apr 13, 2018

robhoes merged commit 40fce3e into xapi-project:master Apr 16, 2018

CA-287863: xe vm-shutdown complete the task too early #3556

CA-287863: xe vm-shutdown complete the task too early #3556

Uh oh!

Conversation

krizex commented Apr 12, 2018

Uh oh!

coveralls commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lindig Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

robhoes left a comment

Choose a reason for hiding this comment

Uh oh!

robhoes commented Apr 12, 2018

Uh oh!

krizex Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

robhoes Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

robhoes commented Apr 12, 2018

Uh oh!

krizex commented Apr 12, 2018

Uh oh!

robhoes commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krizex commented Apr 12, 2018

Uh oh!

krizex commented Apr 13, 2018

Uh oh!

robhoes commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krizex commented Apr 13, 2018

Uh oh!

robhoes commented Apr 13, 2018

Uh oh!

robhoes commented Apr 13, 2018

Uh oh!

robhoes commented Apr 13, 2018

Uh oh!

krizex commented Apr 16, 2018

Uh oh!

robhoes commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Apr 12, 2018 •

edited

Loading

robhoes commented Apr 12, 2018 •

edited

Loading

robhoes commented Apr 13, 2018 •

edited

Loading