[REARCH] Container workers #15884

carbonin · 2017-08-23T20:30:23Z

Run workers in OpenShift containers.

This change allows the server to start workers by talking to OpenShift and creating deployments and scaling them when needed.

This also stubs out some existing functionality which is process specific and doesn't apply to workers running in containers.

carbonin · 2017-08-24T13:37:48Z

cc @Fryguy @bdunne @gtanzillo @jrafanie @NickLaMuro

Just a look at the work so far for Openshift based worker containers

miq-bot · 2017-08-29T13:37:16Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-09-06T17:59:50Z

app/models/manageiq/providers/base_manager/metrics_collector_worker.rb

  require_nested :Runner

  include PerEmsTypeWorkerMixin

  self.required_roles = ["ems_metrics_collector"]

+  def self.supports_container?
+    true
+  end


Can we get rid of this now?

Fryguy · 2017-09-06T18:02:35Z

app/models/mixins/ansible_authentications.rb

@@ -25,6 +25,7 @@ def ansible_secret_key
  def ansible_secret_key=(key)
    auth = authentication_for_type(ANSIBLE_SECRET_KEY_TYPE, "Ansible Secret Key")
    update_ansible_authentication(auth, :auth_key => key)
+    key


This is not necessary as any setter method (ending in =) automatically returns the incoming param

$ irb [1] pry(main)> def blah=(x) [1] pry(main)* "HI" [1] pry(main)* end => :blah= [2] pry(main)> blah = "NOT HI" => "NOT HI"

Fryguy · 2017-09-06T18:03:38Z

lib/container_orchestrator.rb

@@ -1,28 +1,84 @@
+require 'kubeclient'
+
 class ContainerOrchestrator


We may want to namespace this under ManageIQ, so as not to confuse any code from the container providers

Fryguy · 2017-09-06T18:05:33Z

lib/container_orchestrator.rb

+    yield(definition) if block_given?
+    connection.create_deployment_config(definition)
+  rescue KubeException => e
+    raise unless e.message =~ /already exists/


I wonder if we can open this as an RFE on KubeClient where they deal with the "already exists" message and raise a different exception. Also, their exceptions should be namespaced (a second separate issue).

def create_deployment_config(name) ... rescue KubeClient::AlreadyExistsException # suppress end

miq-bot · 2017-09-07T20:42:21Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-10-03T17:57:52Z

This pull request is not mergeable. Please rebase and repush.

bdunne · 2018-02-16T13:07:53Z

app/models/miq_ui_worker.rb

+  end
+
+  def container_port
+    3001


Should this be 3000?

Nope, 3000 is exposed from the container and since the UI needs to serve assets as well as run the worker httpd listens on 3000 then points to 3001 for the worker.

bdunne · 2018-02-16T13:08:38Z

app/models/miq_ui_worker.rb

+  end
+
+  def container_image_name
+    "manageiq/manageiq-ui-worker"


Should you be able to change the org name?

Can we drop the "manageiq-" from the container name?

I want to put all of this in settings like we did for the awx stuff, but I think I'm going to change that in a follow-up.

As for the name, I don't really have a preference. Thoughts @Fryguy ?

@carbonin and I discussed moving this into Settings, but I wanted to get this in first, and do that as an incremental improvement.

bdunne · 2018-02-16T13:10:16Z

app/models/miq_ui_worker/runner.rb

+
+  def prepare
+    super
+    MiqApache::Control.start if MiqEnvironment::Command.is_container?


I thought this was going away.

Why?

We need it to serve static assets.

Forgot about that 👍

bdunne · 2018-02-16T13:14:07Z

app/models/miq_worker.rb

+  end
+
+  def self.create_worker_record(*params)
+    w = init_worker_object(*params)


This could simplify to init_worker_object(*params).tap(&:save)

bdunne · 2018-02-16T13:17:00Z

app/models/miq_worker/container_common.rb

+    end
+
+    def container_image_name
+      "manageiq/manageiq-base-worker"


bdunne · 2018-02-16T13:18:16Z

app/models/miq_worker/container_common.rb

+    end
+
+    def worker_deployment_name
+      @worker_deployment_name ||= begin


bdunne · 2018-02-16T13:21:43Z

app/models/miq_worker/service_worker.rb

+
+    # Can be overriden by including classes
+    def container_image_name
+      "manageiq/manageiq-webserver-worker"


These types of things will probably not be needed when we use containers exclusively, but try to support both for now.

The idea is that each leaf worker class will include one of these concerns which will determine how it gets deployed. ReplicaPerWorker workers are more like cattle. If we need a new one we just add a new one; if we need one less we just delete one. We never need to know their identity. DeploymentPerWorker workers are more like pets, but instead of using StatefulSets, we are using separate deployments so that we can provide a particular environment to each of them. This will be used to give provider workers a provider specific queue name. ServiceWorker workers are like ReplicaPerWorker workers, but fronted by a service. As such they have readiness checks and must define the service's port.

OpenShift does not allow names over 63 characters

In this container, httpd will listen on port 3000 and will serve assets and some error documents as well as the UI

This will allow the worker (in the container) to create its own GUID. This is necessary for worker types with multiple replicas because we cannot tell an individual replica what its GUID is supposed to be. Also, we need the queue_name parameter for the deployment per worker model. In this case we want to be able to reference the queue name on the worker instance, but still don't want to save that instance to the database. We can't use create_worker_record as is, because run_single_worker.rb also needs a way to save the worker record when it's running in a container

Before this was failing because the worker instance didn't have any of the required values set. Things like the pid, server, and guid will be set by the worker when it comes up in the separate container and adds the worker row.

The container entrypoint will provide the contents of the EMS_IDS environment variable to run_single_worker.rb if EMS_IDS is set

NickLaMuro

Just a few comments. Mostly questions and refactoring suggestions, but nothing seemed incorrect from my un-educated perspective.

NickLaMuro · 2018-02-15T22:03:41Z

app/models/manageiq/providers/base_manager/metrics_collector_worker.rb

  require_nested :Runner

  include PerEmsTypeWorkerMixin

  self.required_roles = ["ems_metrics_collector"]

+  def self.supports_container?
+    true
+  end


@Fryguy 's comment here is a bit old, but this is on the same lines:

Is it possible that we can put this method into the MiqWorker::ReplicaPerWorker modules? Or as a shared module that the new modules you have created here inherit from?

Yeah, we could probably move this into the MiqWorker modules in some way rather than having it implemented explicitly in each worker.

Alternatively I could just flip the default implementation in miq_worker.rb and override where we still don't want to start a worker as a container which might provide more useful information at this point. What do you think?

I think either is fine, and it is probably simpler to only have it in one spot. Though, if you do invert it in miq_worker.rb, I think maybe a disclaimer comment above the method might be worth while, just so those passing through know it is disabled on certain workers. With the other approach, since you would be adding it to things like MiqWorker::ReplicaPerWorker and including it in the worker class, I think that would be more easily assumed.

NickLaMuro · 2018-02-16T16:51:22Z

app/models/miq_worker.rb

  end

  def self.start_worker(*params)
-    w = create_worker_record(*params)
+    w = containerized_worker? ? init_worker_object(*params) : create_worker_record(*params)


Okay, I think I see what your doing here, and why, but curious what this means for the future:

To clarify, we are doing creating worker records because it is too hard to manage our DB records with the containers that are currently running on OpenShift?

Does this mean scripts/tools like evm:status would have to be completely re-tooled to work when running in a container env?

Is the plan just to eventually drop these tables?

Does this mess any other portions of the MiqServer#monitor_loop, now that there aren't DB records? (looks like you covered stop_worker, but curious if there are other pieces I am missing)

To be clear, we're still going to have worker records, it's just that they need to be created by the worker container rather than by the server.

This is really because of the worker guid. We can't generate the guid here because we can't tell a replica what guid it should be using. The solution is to have the worker itself create the guid, and by extension the entire record.

it's just that they need to be created by the worker container rather than by the server.

Doh... I even wrote the code that does that and forgot... okay, guess that pretty much answers most of the questions then huh?

We can't generate the guid here because we can't tell a replica what guid it should be using.

Forgive my lacking of openshift knowledge, but I assume we can't even pass a replica a ENV var if we were to make a change to support that in the run_single_worker.rb script as well? I assume no, but just thought I would check and make sure.

I assume we can't even pass a replica a ENV var if we were to make a change to support that in the run_single_worker.rb script as well?

Correct, the environment for the container is defined at the deployment config level so each replica will have the same set of environment variables.

miq-bot · 2018-02-16T16:53:35Z

Checked commits carbonin/manageiq@7c2818b~...7f53a13 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
23 files checked, 0 offenses detected
Everything looks fine. 🍪

Fryguy

As discussed, let's do separate PRs for

moving the hardcoded images into Settings
Getting rid of self.supports_container, or defaulting differently.

Fryguy · 2018-02-16T20:55:17Z

This will allow us to use the G-release architecture for the H-release container images without undoing all of the work in ManageIQ#15884 https://www.pivotaltracker.com/story/show/157774394

carbonin added enhancement wip labels Aug 23, 2017

carbonin force-pushed the container_workers branch from d978618 to 176d515 Compare August 25, 2017 14:10

carbonin mentioned this pull request Aug 25, 2017

[REARCH] Container orchestrator ManageIQ/manageiq-pods#207

Merged

miq-bot added the unmergeable label Aug 29, 2017

carbonin force-pushed the container_workers branch 2 times, most recently from 93b8ff8 to e7ad45a Compare August 29, 2017 20:54

miq-bot removed the unmergeable label Aug 29, 2017

carbonin force-pushed the container_workers branch 2 times, most recently from 088f836 to 55d2d92 Compare August 31, 2017 22:04

Fryguy reviewed Sep 6, 2017

View reviewed changes

carbonin force-pushed the container_workers branch 2 times, most recently from 85b9f70 to b72c986 Compare September 7, 2017 19:07

miq-bot added the unmergeable label Sep 7, 2017

carbonin force-pushed the container_workers branch from dae0f95 to fd35242 Compare September 11, 2017 20:45

miq-bot removed the unmergeable label Sep 11, 2017

chessbyte added the core/workers label Sep 19, 2017

chessbyte assigned gtanzillo Sep 19, 2017

miq-bot added the unmergeable label Oct 3, 2017

carbonin force-pushed the container_workers branch from fd35242 to ce8125c Compare December 8, 2017 14:54

miq-bot removed the unmergeable label Dec 8, 2017

carbonin force-pushed the container_workers branch 2 times, most recently from 29166d4 to 5052f7b Compare December 8, 2017 21:52

bdunne requested changes Feb 16, 2018

View reviewed changes

carbonin added 11 commits February 16, 2018 11:22

Stub some worker management calls for containerized workers

7c2818b

These types of things will probably not be needed when we use containers exclusively, but try to support both for now.

Shorten deployment name method for workers

0245bf1

OpenShift does not allow names over 63 characters

Include the appropriate concern in each of the workers

bf8a63c

Start httpd in the UI worker container

048fc2a

In this container, httpd will listen on port 3000 and will serve assets and some error documents as well as the UI

Fix MiqWorker#start for containerized workers

97f9fb8

Before this was failing because the worker instance didn't have any of the required values set. Things like the pid, server, and guid will be set by the worker when it comes up in the separate container and adds the worker row.

Use #stop_container to stop containerized workers

1283509

Provide the ems id(s) to the worker as a comma separated list

990bbd2

The container entrypoint will provide the contents of the EMS_IDS environment variable to run_single_worker.rb if EMS_IDS is set

Add some specs

d30dc2b

Use manageiq docker hub account

7f53a13

carbonin force-pushed the container_workers branch from aa2abc3 to 7f53a13 Compare February 16, 2018 16:37

NickLaMuro reviewed Feb 16, 2018

View reviewed changes

bdunne approved these changes Feb 16, 2018

View reviewed changes

Fryguy approved these changes Feb 16, 2018

View reviewed changes

carbonin changed the title ~~[WIP] [REARCH] Container workers~~ [REARCH] Container workers Feb 16, 2018

carbonin added gaprindashvili/no and removed wip dependencies labels Feb 16, 2018

Fryguy merged commit 38ede59 into ManageIQ:master Feb 16, 2018

Fryguy added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 16, 2018

carbonin mentioned this pull request May 29, 2018

Remove changes to enable workers to be started in containers #17493

Merged

carbonin deleted the container_workers branch August 16, 2019 17:28

agrare mentioned this pull request Aug 29, 2020

MiqSmartProxyWorker not running in Podified #20497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REARCH] Container workers #15884

[REARCH] Container workers #15884

carbonin commented Aug 23, 2017

carbonin commented Aug 24, 2017

miq-bot commented Aug 29, 2017

Fryguy Sep 6, 2017

Fryguy Sep 6, 2017 •

edited

Loading

Fryguy Sep 6, 2017

Fryguy Sep 6, 2017

miq-bot commented Sep 7, 2017

miq-bot commented Oct 3, 2017

bdunne Feb 16, 2018

carbonin Feb 16, 2018

bdunne Feb 16, 2018

bdunne Feb 16, 2018

carbonin Feb 16, 2018

Fryguy Feb 16, 2018

bdunne Feb 16, 2018

carbonin Feb 16, 2018

bdunne Feb 16, 2018

bdunne Feb 16, 2018

bdunne Feb 16, 2018

bdunne Feb 16, 2018

bdunne Feb 16, 2018

NickLaMuro left a comment

NickLaMuro Feb 15, 2018

carbonin Feb 16, 2018

NickLaMuro Feb 16, 2018

NickLaMuro Feb 16, 2018

carbonin Feb 16, 2018

NickLaMuro Feb 16, 2018

carbonin Feb 16, 2018

miq-bot commented Feb 16, 2018

Fryguy left a comment

Fryguy commented Feb 16, 2018

		@@ -1,28 +1,84 @@
		require 'kubeclient'

		class ContainerOrchestrator

[REARCH] Container workers #15884

[REARCH] Container workers #15884

Conversation

carbonin commented Aug 23, 2017

carbonin commented Aug 24, 2017

miq-bot commented Aug 29, 2017

Choose a reason for hiding this comment

Fryguy Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 7, 2017

miq-bot commented Oct 3, 2017

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

NickLaMuro 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 Feb 16, 2018

Fryguy left a comment

Choose a reason for hiding this comment

Fryguy commented Feb 16, 2018

Fryguy Sep 6, 2017 •

edited

Loading