Add #raw_stdout_via_worker method #16441

bzwei · 2017-11-10T15:14:06Z

Add Job#raw_stdout_via_worker method mainly used by UI to get and display a standard output from an Ansible job. The implementation detects whether the embedded role is enabled and gets the output through the embedded ansible worker.

An error message is returned when it fails to get the stdout, so the caller does not need to rescue an error.

Solves ManageIQ/manageiq-ui-classic#2470

ManageIQ/manageiq-ui-classic#2470

bzwei · 2017-11-10T15:18:31Z

@miq-bot add_label enhancement
@miq-bot assign @gmcculloug
@miq-bot add_label gaprindashvili/yes
@mkanoor @himdel please review

mkanoor · 2017-11-10T15:47:32Z

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

+                     :priority    => MiqQueue::HIGH_PRIORITY,
+                     :role        => 'embedded_ansible'}
+    taskid = MiqTask.generic_action_with_callback(options, queue_options)
+    MiqTask.wait_for_taskid(taskid)


@bzwei @himdel
This is going to block the UI?, are there other ways to fetch the data later given a task.
If we can return a Task and the UI knows to check for it periodically and display the results once available.

Yes, this would block the UI.

We have an initiate_wait_for_task in ApplicationController... which causes the browser to poll a the task and call a specific action when done.

But you probably can't use it either because that would block the whole summary screen from appearing.

As far as I can tell, the only safe way of doing that would be to implement the bit in JS - that is:

render the summary screen, with the stdout field empty, and initiate that task

replace the html of that field with a component which knows (as a parameter) which task it's waiting for

that JS component would keep polling the API for the task to complete (needs Wait for task in JS manageiq-ui-classic#2218 - API.wait_for_task)

once done, it would display the result

That way, this can error nicely if the server is not available, but won't block all the other stuff from appearing.
(But note that caching the info on the server, same as everything else, would be much much cleaner from the perspective of UI changes involved.)

I can help with the component :) (or the UI bits in general :)).

(So whereas all the other fields come from the server as..

<tr class="no-hover" onclick="" title=""> <td class="label"> MAC Address </td> <td> 00:50:56:8c:8b:00 </td> </tr>

this would come more as...

<tr class="no-hover" onclick="" title=""> <td class="label"> Raw Stdout</td> <td> <raw-stdout task-id="12345"></raw-stdout> </td> </tr>

)

@himdel What I have is a replacement of current method. I agree a non-blocking method is better. Will a MiqTask sufficient for your proposed work in JS? Can UI simply call MiqTask.generic_action_with_callback? If yes then this PR is not needed at all.
I don't understand the part of caching info on the server. Can you explain with more details?

Lastly, the output in general can be large. I think we want to destroy the MiqTask after UI gets the text.

Aah, OK, I'll update the PR to do that, thanks :)

(Just to be sure .. you do mean any job, not any ansible job, right? Because .. in the UI, this is a job associated with the service.. so.. really potentially any job at all.)

The context is embedded ansible playbook. The job is ansible tower job, not Miq Job.

The point is .. this is a job belonging to a service.

Looking at a random service, I can't know the job will be an embedded ansible playbook job. All I know is, it will be a job.

But if you think using the supports mixin here is wrong, I can add an explicit @record.type == 'ServiceAnsiblePlaybook'... iff you're sure that:

any service of type ServiceAnsiblePlaybook will return a job which has the method whenever you call .job, regardless of the other argument.

no other type of service should show anything like that

This applies to ServiceAnsiblePlaybook only, as already in https://github.com/ManageIQ/manageiq-ui-classic/pull/2708/files#diff-ef007b9d34027eeec429c666e2186d8eL37. A random service will not provide a job object.

Well, both ServiceAnsiblePlaybook and ServiceAnsibleTower provide a job method.

Disregardign the latter (because you're right, we do check that type already), ServiceAnsiblePlaybook's job method looks for any resource of the kind OrchestrationStack.

Since OrchestrationStack#raw_stdout_via_worker definitely fails, leaving only that type check seems very brittle (sounds like a "this can't happen, but we'll it hit immediately after the next release" :)).

himdel · 2017-11-13T13:43:30Z

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

+
+  # Intend to be called by UI to display stdout. Therefore the error message directly returned
+  # instead of raising an exception.
+  def raw_stdout_via_worker(userid = User.current_user, format = 'txt')


Maybe the argument order could be reversed here - the UI will never need to provide a different userid, but does need to provide a format.

himdel · 2017-11-13T14:42:43Z

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

+      return "Cannot get standard output of this playbook because the embedded Ansible role is not enabled"
+    end
+
+    options = {:userid => userid, :action => 'ansible_stdout'}


Could be something wrong here.. the resulting task (accessed via the API) gives me...

userid: "#<User:0x00007fdd65fc6c38>"

himdel · 2017-11-13T14:44:59Z

@bzwei I created ManageIQ/manageiq-ui-classic#2708 depending on this PR (with some changes)..

Looks like it gets as far as trying to query the task :).

(I don't currently have an ansible working.. any chance you know how a sucessfully done task will look in the API output? (Currently the component in that PR just assumes it gets the right data, directly as a string, which is possibly wrong.))

bzwei · 2017-11-13T16:00:21Z

@himdel If the UI takes only the task_id, what's the plan to destroy the task?

himdel · 2017-11-13T16:56:08Z

@himdel If the UI takes only the task_id, what's the plan to destroy the task?

@bzwei I'm hoping you know :).

From what I can see, other UI code which needs to clean up tasks after itself does...

  def store_for_import(file_contents)
    import_file_upload = ImportFileUpload.create
    import_file_upload.store_binary_data_as_yml(file_contents, "Automation import")
    import_file_upload
  ensure
    queue_deletion(import_file_upload.id)
  end

  def queue_deletion(import_file_upload_id)
    MiqQueue.put(
      :class_name  => "ImportFileUpload",
      :instance_id => import_file_upload_id,
      :deliver_on  => 1.day.from_now,
      :method_name => "destroy"
    )
  end

... so, it immediately queues a removal, to happen a day from then.

I'm not sure that's the best we could do though :).

Another option would be to make the component explicitly delete that task once it finishes retrieving the data - but we would need API support for that.

himdel · 2017-11-13T17:02:43Z

BTW another consideration is...

if you run service.job("Retirement") and service.job("Provision") on the same service.. is this really 2 different jobs?

I'm asking because you seemed concerned about the task size - so possibly creating 2 such tasks whenever anyone views a service detail screen is not perfect.

(But that brings us back to caching, so not sure if you want to go there.)

bzwei · 2017-11-13T17:17:37Z

The main consideration is the output size. All output are already stored in embedded tower, and they can be very big. It makes no sense to cache them another copy in VMDB. Embedded tower is local or within the zone, so the latency is expected to be minimal.
For the same reason we want to destroy the task as soon as possible, not to occupy too much space. Because we do live fetch every time, UI can delete the task immediately, not to schedule it to be deleted after 1 day. UI will not reuse the task to get the output, will it?
The other proposal I would make is the UI does not get the output by default. Instead it provides a button for the user to click to demand the output. In this case the request may be synchronous. This has to be approved by PM though.

himdel · 2017-11-13T17:22:09Z

UI will not reuse the task to get the output, will it?

We would need to cache the service_id => task_id mapping, and invalidate the cache at some points, so no, we will not reuse the task.

So yeah, I agree we should be removing the task immediately.. which means we'll need API support for that (already pinged the api room, and created ManageIQ/manageiq-api#203).

The other proposal I would make is the UI does not get the output by default. Instead it provides a button for the user to click to demand the output. In this case the request may be synchronous. This has to be approved by PM though.

Agreed, if that becomes relevant, we can just make the component be able to trigger the task creation .. and this PR would be needed either way (+ one to trigger the task from the API)

bzwei · 2017-11-13T20:22:49Z

@himdel I pushed an update. I leave the argument as #raw_stdout_via_worker(userid, format = 'txt'), where userid is mandatory but can be nil. This is the same for many other similar use cases.

himdel · 2017-11-14T12:10:51Z

Thanks.. that should be all we need :).

Can you add an example of the error and success task outputs?

bzwei · 2017-11-14T17:12:39Z

When a stdout exists, it can be retrieved through Task#task_results, the sample the output looks like

PLAY RECAP *****************************************************************************************************************************
localhost                  : ok=3    changed=0    unreachable=0    failed=0

When failed to get stdout, the error message can be retrieved through Task#message, for example

Cannot get standard output of this playbook because the embedded Ansible role is not enabled

The caller thus can check Task#status be Error or Ok first, or blindly try #task_results first and then #message

himdel · 2017-11-14T17:15:24Z

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

+  # Since the task_results may contain a large block of data, it is desired to remove the task upon receiving the data
+  def raw_stdout_via_worker(userid, format = 'txt')
+    unless MiqRegion.my_region.role_active?("embedded_ansible")
+      msg = "Cannot get standard output of this playbook because the embedded Ansible role is not enabled"


missing i18n? This needs to be at least N_, if you think _( would be wrong.

miq-bot · 2017-11-14T17:18:18Z

Checked commits bzwei/manageiq@be61127~...8946174 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

himdel · 2017-11-14T17:18:30Z

@bzwei sorry, I didn't express myself clearly :).

I'm not asking how to retrieve the data, I am asking how the data actually looks when it is there (is task_results an object, or is it a string? If it's a hash, which field contains the string..)

For example, when the role is not enabled, what I will see in the API is:

http://localhost:3000/api/tasks/10000000054710?attributes=task_results

{
"href": "http://localhost:3000/api/tasks/10000000054710",
"id": "10000000054710",
"name": "ansible_stdout",
"state": "Finished",
"status": "Error",
"message": "Cannot get standard output of this playbook because the embedded Ansible role is not enabled",
"userid": "system",
"created_on": "2017-11-14T17:14:48Z",
"updated_on": "2017-11-14T17:14:48Z"
}

What I'm asking is the same thing for a sucessfully run task.
(Why I'm asking is because I can't get embedded ansible working for anything response other than Invalid pk "2" - object does not exist., sorry :).)

bzwei · 2017-11-14T18:51:37Z

The result is always a string. If you request html, it is a string in html format.

himdel · 2017-11-14T19:04:24Z

...

What I'm asking is the same thing for a sucessfully run task.

Is this really that much to ask? I'm assuming you tested this, so it should be trivial for you to look at the task and provide the info I'm asking for, shouldn't it?

If you're not willing to do that, please tell me, I'll take the time to get ansible working instead.

But... just to be really clear... what I'm asking for is:
Please run your function to get a successful result, and give me the API output for that task's details, same as I did for the error task in #16441 (comment).

bzwei · 2017-11-14T19:35:05Z

@himdel I did not realize you wanted a sample of API call. Here is what I got. The results are like what has been described above

{
"href":"http://localhost:3000/api/tasks/44",
"id":"44",
"name":"ansible_stdout",
"state":"Finished",
"status":"Ok",
"message":"Task completed successfully",
"userid":"system",
"created_on":"2017-11-14T19:22:01Z",
"updated_on":"2017-11-14T19:22:06Z",
"miq_server_id":"1",
"task_results":"\u003c!DOCTYPE HTML\u003e\n\u003chtml\u003e\n  \u003chead\u003e\n    \u003cmeta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"\u003e\n    \u003ctitle\u003eJob Stdout\u003c/title\u003e\n\u003cstyle type=\"text/css\"\u003e\n.ansi_fore { color: #000000; }\n.ansi_back { background-color: #F5F5F5; }\n.ansi_fore.ansi_dark { color: #AAAAAA; }\n.ansi_back.ansi_dark { background-color: #000000; }\n.ansi1 { font-weight: bold; }\n.ansi3 { font-weight: italic; }\n.ansi4 { text-decoration: underline; }\n.ansi9 { text-decoration: line-through; }\n.ansi30 { color: #161b1f; }\n.ansi31 { color: #d9534f; }\n.ansi32 { color: #5cb85c; }\n.ansi33 { color: #f0ad4e; }\n.ansi34 { color: #337ab7; }\n.ansi35 { color: #e1539e; }\n.ansi36 { color: #2dbaba; }\n.ansi37 { color: #ffffff; }\n.ansi40 { background-color: #161b1f; }\n.ansi41 { background-color: #d9534f; }\n.ansi42 { background-color: #5cb85c; }\n.ansi43 { background-color: #f0ad4e; }\n.ansi44 { background-color: #337ab7; }\n.ansi45 { background-color: #e1539e; }\n.ansi46 { background-color: #2dbaba; }\n.ansi47 { background-color: #ffffff; }\nbody.ansi_back pre {\n  font-family: Monaco, Menlo, Consolas, \"Courier New\", monospace;\n  font-size: 12px;\n}\ndiv.ansi_back.ansi_dark {\n  padding: 0 8px;\n  -webkit-border-radius: 3px;\n  -moz-border-radius: 3px;\n  border-radius: 3px;\n}\n\u003c/style\u003e\n  \u003c/head\u003e\n  \u003cbody class=\"ansi_fore ansi_back ansi_dark\"\u003e\n    \u003cpre\u003estdout capture is missing\u003c/pre\u003e\n  \u003c/body\u003e\n\u003c/html\u003e"}

himdel · 2017-11-15T13:00:52Z

@bzwei Thanks, that really helps :) Updating the PR to match this..

Add #raw_stdout_via_worker method (cherry picked from commit 1c98131)

simaishi · 2017-11-17T13:45:45Z

Gaprindashvili backport details:

$ git log -1
commit e080e996baa3b2236440b3eecabec469b3269617
Author: Greg McCullough <[email protected]>
Date:   Thu Nov 16 08:55:38 2017 -0500

    Merge pull request #16441 from bzwei/stdout_queued
    
    Add #raw_stdout_via_worker method
    (cherry picked from commit 1c98131006df5b30023780b1273feae365707875)

hstastna · 2018-06-15T13:44:51Z

@miq-bot add_label fine/yes

hstastna · 2018-06-15T13:46:11Z

@miq-bot remove_label gaprindashvili/yes

hstastna · 2018-06-19T09:29:25Z

@miq-bot remove_label fine/yes

Add #raw_stdout_via_worker method

be61127

ManageIQ/manageiq-ui-classic#2470

miq-bot added the enhancement label Nov 10, 2017

miq-bot assigned gmcculloug Nov 10, 2017

miq-bot added the gaprindashvili/yes label Nov 10, 2017

mkanoor reviewed Nov 10, 2017

View reviewed changes

himdel reviewed Nov 13, 2017

View reviewed changes

himdel mentioned this pull request Nov 13, 2017

Service summary - asynchronous raw stdout for Ansible ManageIQ/manageiq-ui-classic#2708

Merged

himdel reviewed Nov 13, 2017

View reviewed changes

himdel mentioned this pull request Nov 13, 2017

Tasks - missing delete support ManageIQ/manageiq-api#203

Closed

bzwei force-pushed the stdout_queued branch from 8fe1724 to 711a226 Compare November 13, 2017 20:19

Don't wait on task, returns task_id only

8946174

bzwei force-pushed the stdout_queued branch from 711a226 to 8946174 Compare November 14, 2017 17:13

himdel reviewed Nov 14, 2017

View reviewed changes

gmcculloug merged commit 1c98131 into ManageIQ:master Nov 16, 2017

gmcculloug added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 16, 2017

bzwei deleted the stdout_queued branch November 16, 2017 14:15

simaishi pushed a commit that referenced this pull request Nov 17, 2017

Merge pull request #16441 from bzwei/stdout_queued

e080e99

Add #raw_stdout_via_worker method (cherry picked from commit 1c98131)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 17, 2017

bzwei mentioned this pull request Mar 5, 2018

[Embedded Ansible] Standard output missing after AnsiblePlaybook service ordered #15892

Closed

This was referenced Jun 14, 2018

[WIP] [FINE] Change raw_stdout method to fix displaying details of active ansible services #17584

Closed

[FINE] Fix displaying Child Services of Active Ansible Services ManageIQ/manageiq-ui-classic#4150

Merged

miq-bot added gaprindashvili/yes labels Jun 15, 2018

miq-bot removed the gaprindashvili/yes label Jun 15, 2018

miq-bot removed the fine/yes label Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #raw_stdout_via_worker method #16441

Add #raw_stdout_via_worker method #16441

bzwei commented Nov 10, 2017 •

edited

Loading

bzwei commented Nov 10, 2017

mkanoor Nov 10, 2017

himdel Nov 10, 2017 •

edited

Loading

himdel Nov 10, 2017 •

edited

Loading

himdel Nov 10, 2017 •

edited

Loading

bzwei Nov 10, 2017

himdel Nov 13, 2017

bzwei Nov 13, 2017

himdel Nov 13, 2017 •

edited

Loading

bzwei Nov 13, 2017

himdel Nov 13, 2017 •

edited

Loading

himdel Nov 13, 2017

himdel Nov 13, 2017

himdel commented Nov 13, 2017

bzwei commented Nov 13, 2017

himdel commented Nov 13, 2017 •

edited

Loading

himdel commented Nov 13, 2017

bzwei commented Nov 13, 2017

himdel commented Nov 13, 2017 •

edited

Loading

bzwei commented Nov 13, 2017

himdel commented Nov 14, 2017

bzwei commented Nov 14, 2017

himdel Nov 14, 2017

miq-bot commented Nov 14, 2017

himdel commented Nov 14, 2017 •

edited

Loading

bzwei commented Nov 14, 2017

himdel commented Nov 14, 2017 •

edited

Loading

bzwei commented Nov 14, 2017

himdel commented Nov 15, 2017

simaishi commented Nov 17, 2017

hstastna commented Jun 15, 2018

hstastna commented Jun 15, 2018

hstastna commented Jun 19, 2018

Add #raw_stdout_via_worker method #16441

Add #raw_stdout_via_worker method #16441

Conversation

bzwei commented Nov 10, 2017 • edited Loading

bzwei commented Nov 10, 2017

Choose a reason for hiding this comment

himdel Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Nov 13, 2017

bzwei commented Nov 13, 2017

himdel commented Nov 13, 2017 • edited Loading

himdel commented Nov 13, 2017

bzwei commented Nov 13, 2017

himdel commented Nov 13, 2017 • edited Loading

bzwei commented Nov 13, 2017

himdel commented Nov 14, 2017

bzwei commented Nov 14, 2017

Choose a reason for hiding this comment

miq-bot commented Nov 14, 2017

himdel commented Nov 14, 2017 • edited Loading

bzwei commented Nov 14, 2017

himdel commented Nov 14, 2017 • edited Loading

bzwei commented Nov 14, 2017

himdel commented Nov 15, 2017

simaishi commented Nov 17, 2017

hstastna commented Jun 15, 2018

hstastna commented Jun 15, 2018

hstastna commented Jun 19, 2018

bzwei commented Nov 10, 2017 •

edited

Loading

himdel Nov 10, 2017 •

edited

Loading

himdel Nov 10, 2017 •

edited

Loading

himdel Nov 10, 2017 •

edited

Loading

himdel Nov 13, 2017 •

edited

Loading

himdel Nov 13, 2017 •

edited

Loading

himdel commented Nov 13, 2017 •

edited

Loading

himdel commented Nov 13, 2017 •

edited

Loading

himdel commented Nov 14, 2017 •

edited

Loading

himdel commented Nov 14, 2017 •

edited

Loading