Embedded Ansible role worker #13551

jrafanie · 2017-01-17T22:38:43Z

This PR runs a "monitoring" worker thread in the server process that monitors another process on the appliance. It implements the following:

a new role embedded_ansible limited to 1 per region
a new "worker" using our existing worker mechanics but living in a Thread running on the server process

What works:

the worker and thread do not run unless you enable the role, which is not enabled by default
when the role is enabled, we instantiate a worker and run the monitoring thread using the existing worker mechanics but without fork/spawn process creation since we can do the work in a thread.

What needs to be done:

configure nginx
enable supervisord service
unconfiguring(if applicable) and disabling/stopping supervisord when the role is removed
check supervisord's status
restart supervisord if it's not in a good state
handle stop/restart/kill in the normal worker way
add tests

@carbonin @Fryguy @gtanzillo Please review/throw 🍅

jrafanie · 2017-01-17T22:41:27Z

app/models/manageiq/providers/ansible_tower/embedded_ansible_worker/runner.rb

+
+  def supervisord_ok?
+    # fake method, we'll think it's not ok 5% of the time
+    rand(20) > 0


This is just so I can run it locally and sometimes have it return false.

jrafanie · 2017-01-17T22:41:58Z

app/models/miq_worker.rb

@@ -336,7 +336,7 @@ def self.close_pg_sockets_inherited_from_parent
    end
  end

-  def start
+  def start_runner


i extracted the fork/process creation to a method so the subclass can override and NOT create a fork/process

jrafanie · 2017-01-17T22:42:27Z

config/settings.yml

@@ -1188,6 +1188,8 @@
      :poll_method: :normal
      :restart_interval: 0.hours
      :starting_timeout: 10.minutes
+    :embedded_ansible_worker:
+      :poll: 30.seconds


since the do_work is really only monitoring supervisord, I have it doing it every 30 seconds

gtanzillo · 2017-01-17T22:45:31Z

db/fixtures/server_roles.csv

@@ -2,6 +2,7 @@ name,description,max_concurrent,external_failover,role_scope
 automate,Automation Engine,0,false,region
 database_operations,Database Operations,0,false,region
 database_owner,Database Owner,1,false,database
+embedded_ansible,Embedded Ansible,1,false,zone


Shouldn't the role scope be region initially?

I thought we were thinking of having an ansible zone in the region as the first step. Limiting it to the region might box us into a corner when we want to deal with ansible clustering and multiple independent ansibles. Maybe?

🤔 💭 Is the plural of ansible, ansibles?

Seriously, this is what you ask about... 😒

What color is the 🚲 ?

I also thought we can only support 1 per region, initially.

Correct @chessbyte we can only support 1 active ansible at a time in a region. So this needs to be scoped to region

Yeah, the only downside I can see to using an explicit region role is that it's different than all of the other provider type roles. So, while we're trying to treat it as a provider, it's not quite one. I can't think of any other problems with that though, migrating to zonal role should be straight forward.

I think that keeping this zone will introduce more tech debt (in the form of preventing the role from starting in multiple zones) than the trouble of possibly having the role fail over into a different zone.

Sounds good 👍

carbonin · 2017-01-18T19:54:15Z

app/models/manageiq/providers/ansible_tower/embedded_ansible_worker.rb

@@ -0,0 +1,20 @@
+class ManageIQ::Providers::AnsibleTower::EmbeddedAnsibleWorker < MiqWorker


I'm not sure about namespacing this under the provider.

Maybe directly under the model would be better?

Technically it should live with the "new" provider right?

@Fryguy I was thinking it would be more of a separate thing because it's related to running the service on the appliance rather than with talking to the provider's API.

haha, I can see how convinced all of us are to where it belongs. This is because namespacing it is part of naming it. And we know how easy naming is...

Ok, I'll change this to:

class ManageIQ::Providers::EmbeddedAnsible::MonitoringWorker < MiqWorker

in manageiq/providers/embedded_ansible/monitoring_worker.rb or something.

Until, we decide to move it again... The important part is it will be in the namespace of a separate provider specific to the embedded ansible.

I would resort to ManageIQ::Providers::EmbeddedAnsible only as a last option, because it adds a new Provider.

Why not make this a ManageIQ::Providers::AnsibleTower::TowerWorker which monitors a tower? First is embedded, next could be external?

@durandom Running ansible locally on the server (embedded) and talking to it through an endpoint are fundamentally different and, in my opinion, not actually related at all. They will share no common functionality.

I think what we need to do is come up with a new template for what we call a thing that actually is responsible for running a service locally (because to me, that's not a provider at all).

durandom · 2017-01-19T15:13:35Z

app/models/manageiq/providers/ansible_tower/provider.rb

@@ -60,6 +62,18 @@ def url=(new_url)
    default_endpoint.url = uri.to_s
  end

+  def self.seed


how about seeding via the rest api? as a post install script. Or maybe something with the appliance console? This could be re-used to seed other providers too

@durandom sorry, didn't see this comment. Maybe? This is just a placeholder for now. I'll be integrating with NickC's work and we'll be addressing how we find/seed the default tower per zone.

carbonin

I think we will also need to do something in here: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_server/role_management.rb#L4

carbonin · 2017-01-25T21:07:17Z

app/models/manageiq/providers/ansible_tower/provider.rb

@@ -60,6 +62,18 @@ def url=(new_url)
    default_endpoint.url = uri.to_s
  end

+  def self.seed
+    embedded_in_zone ||
+      create_with(:url => "https://localhost:8443", :zone => Zone.default_zone)


This will need a reliable way to determine how all the other appliances will be able to access this one.
Maybe something like MiqRegion#remote_ws_url will be required?

So I think this zone should by MiqServer.my_server.my_zone. That way the provider's work will follow the server that is running the role.

jrafanie · 2017-01-27T16:14:24Z

app/models/manageiq/providers/ansible_tower/provider.rb

+        .find_or_create_by(:name=> DEFAULT_EMBEDDED_NAME)
+        # do we need to create an endpoint/authentication here?
+        # We probably need to tie this provider to a miq_servers row
+  end


I'll remove this seeding and do this when the worker starts.

jrafanie · 2017-01-27T21:38:04Z

app/models/embedded_ansible_worker/runner.rb

+
+  def do_work
+    if EmbeddedAnsible.running?
+      _log.info("#{log_prefix} supervisord is ok!")


NOTE, drop this line before merging... it's helpful during testing...

https://www.pivotaltracker.com/story/show/135450841

Fryguy · 2017-01-30T19:23:30Z

app/models/embedded_ansible_worker/runner.rb

+  # we need to intercept SystemExit and exit our thread,
+  # not the main server thread!
+  def do_exit(*args)
+    # ensure this doesn't fail or that we can still get to the super call


does this work?

ensure super end

jrafanie · 2017-01-30T20:00:03Z

app/models/embedded_ansible_worker.rb

+
+  def stop
+    super
+    # stop supervisord service


let's drop this method since do_exit handles this for us

jrafanie · 2017-01-30T20:00:48Z

app/models/embedded_ansible_worker.rb

+  end
+
+  def kill
+    # Does the base class's kill -9 work on the supervisord process as we want?


This and setting the right worker pid is not handled in this PR. For now, the server's pid is stored in the worker row but we have disabled monitoring of this pid's memory.

Right now, this is a worker that doesn't have a separate process. Instead, it's just a thread running in the server that monitors the supervisord process and handles restarting it when it goes down. https://www.pivotaltracker.com/story/show/135450841

miq-bot · 2017-01-30T22:46:38Z

Some comments on commits jrafanie/manageiq@1773711~...23af660

spec/models/embedded_ansible_worker/runner_spec.rb

⚠️ - 12 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-01-30T22:47:33Z

gtanzillo

👍

jrafanie added core/workers enhancement providers/ansible_tower wip labels Jan 17, 2017

jrafanie commented Jan 17, 2017

View reviewed changes

gtanzillo reviewed Jan 17, 2017

View reviewed changes

carbonin reviewed Jan 18, 2017

View reviewed changes

durandom reviewed Jan 19, 2017

View reviewed changes

carbonin mentioned this pull request Jan 19, 2017

Add methods for configuring and starting ansible inside #13584

Merged

carbonin reviewed Jan 25, 2017

View reviewed changes

jrafanie force-pushed the ansible_role_worker branch from bb51944 to 74f2707 Compare January 26, 2017 21:47

jrafanie mentioned this pull request Jan 27, 2017

Run a cockpit-ws as a worker #12506

Merged

jrafanie commented Jan 27, 2017

View reviewed changes

jrafanie force-pushed the ansible_role_worker branch 2 times, most recently from 25d62f9 to 91995f5 Compare January 27, 2017 21:37

jrafanie commented Jan 27, 2017

View reviewed changes

jrafanie force-pushed the ansible_role_worker branch from 91995f5 to 7a9ee52 Compare January 30, 2017 18:26

jrafanie added 2 commits January 30, 2017 13:34

Add embedded ansible role

1773711

https://www.pivotaltracker.com/story/show/135450841

Extract forking/process creation to a method subclasses can override.

d7a9ff3

https://www.pivotaltracker.com/story/show/135450841

jrafanie force-pushed the ansible_role_worker branch 2 times, most recently from ad19008 to 40c90bb Compare January 30, 2017 19:21

Fryguy reviewed Jan 30, 2017

View reviewed changes

jrafanie commented Jan 30, 2017

View reviewed changes

jrafanie force-pushed the ansible_role_worker branch 2 times, most recently from f18ba22 to f670204 Compare January 30, 2017 20:34

Add embedded ansible worker.

23af660

Right now, this is a worker that doesn't have a separate process. Instead, it's just a thread running in the server that monitors the supervisord process and handles restarting it when it goes down. https://www.pivotaltracker.com/story/show/135450841

jrafanie force-pushed the ansible_role_worker branch from f670204 to 23af660 Compare January 30, 2017 22:44

jrafanie changed the title ~~[WIP] Embedded Ansible role worker~~ Embedded Ansible role worker Jan 31, 2017

gtanzillo added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 31, 2017

gtanzillo self-assigned this Jan 31, 2017

gtanzillo approved these changes Jan 31, 2017

View reviewed changes

jrafanie removed the wip label Jan 31, 2017

gtanzillo merged commit 51ea8b3 into ManageIQ:master Jan 31, 2017

jrafanie deleted the ansible_role_worker branch February 6, 2017 18:47

jrafanie mentioned this pull request Jun 15, 2017

Make spawn an option again #15379

Merged

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embedded Ansible role worker #13551

Embedded Ansible role worker #13551

jrafanie commented Jan 17, 2017 •

edited

Loading

jrafanie Jan 17, 2017

jrafanie Jan 17, 2017

jrafanie Jan 17, 2017

gtanzillo Jan 17, 2017

jrafanie Jan 18, 2017

Fryguy Jan 18, 2017 •

edited

Loading

jrafanie Jan 18, 2017

jrafanie Jan 18, 2017

chessbyte Jan 26, 2017

gtanzillo Jan 27, 2017

jrafanie Jan 27, 2017

carbonin Jan 27, 2017

jrafanie Jan 27, 2017

carbonin Jan 18, 2017

Fryguy Jan 18, 2017

carbonin Jan 18, 2017

jrafanie Jan 18, 2017

jrafanie Jan 18, 2017

durandom Jan 19, 2017

carbonin Jan 19, 2017

durandom Jan 19, 2017

jrafanie Jan 26, 2017

carbonin left a comment

carbonin Jan 25, 2017

carbonin Jan 27, 2017

jrafanie Jan 27, 2017

jrafanie Jan 27, 2017

Fryguy Jan 30, 2017

jrafanie Jan 30, 2017

jrafanie Jan 30, 2017

miq-bot commented Jan 30, 2017

miq-bot commented Jan 30, 2017

gtanzillo left a comment

		@@ -0,0 +1,20 @@
		class ManageIQ::Providers::AnsibleTower::EmbeddedAnsibleWorker < MiqWorker

Embedded Ansible role worker #13551

Embedded Ansible role worker #13551

Conversation

jrafanie commented Jan 17, 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

Choose a reason for hiding this comment

Fryguy Jan 18, 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

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

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

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

Choose a reason for hiding this comment

carbonin left a comment

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

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

miq-bot commented Jan 30, 2017

miq-bot commented Jan 30, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

jrafanie commented Jan 17, 2017 •

edited

Loading

Fryguy Jan 18, 2017 •

edited

Loading