-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REARCH] Container workers #15884
[REARCH] Container workers #15884
Changes from all commits
7c2818b
6506f61
0245bf1
bf8a63c
048fc2a
29cabd5
97f9fb8
1283509
990bbd2
d30dc2b
7f53a13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,16 @@ | ||
class ManageIQ::Providers::BaseManager::MetricsCollectorWorker < MiqQueueWorkerBase | ||
include MiqWorker::ReplicaPerWorker | ||
|
||
require_nested :Runner | ||
|
||
include PerEmsTypeWorkerMixin | ||
|
||
self.required_roles = ["ems_metrics_collector"] | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we could probably move this into the Alternatively I could just flip the default implementation in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think either is fine, and it is probably simpler to only have it in one spot. Though, if you do invert it in |
||
|
||
def self.normalized_type | ||
@normalized_type ||= "ems_metrics_collector_worker" | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
class MiqEventHandler < MiqQueueWorkerBase | ||
include MiqWorker::ReplicaPerWorker | ||
|
||
require_nested :Runner | ||
|
||
self.required_roles = ["event"] | ||
self.default_queue_name = "ems" | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
class MiqGenericWorker < MiqQueueWorkerBase | ||
include MiqWorker::ReplicaPerWorker | ||
|
||
require_nested :Runner | ||
|
||
self.default_queue_name = "generic" | ||
self.check_for_minimal_role = false | ||
self.workers = -> { MiqServer.minimal_env? ? 1 : worker_settings[:count] } | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,15 @@ | ||
class MiqPriorityWorker < MiqQueueWorkerBase | ||
include MiqWorker::ReplicaPerWorker | ||
|
||
require_nested :Runner | ||
|
||
self.default_queue_name = "generic" | ||
|
||
def self.queue_priority | ||
MiqQueue::HIGH_PRIORITY | ||
end | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
class MiqReportingWorker < MiqQueueWorkerBase | ||
include MiqWorker::ReplicaPerWorker | ||
|
||
require_nested :Runner | ||
|
||
self.required_roles = ["reporting"] | ||
self.default_queue_name = "reporting" | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,4 +20,17 @@ def friendly_name | |
end | ||
|
||
include MiqWebServerWorkerMixin | ||
include MiqWorker::ServiceWorker | ||
|
||
def self.supports_container? | ||
true | ||
end | ||
|
||
def container_port | ||
3001 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be 3000? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
end | ||
|
||
def container_image_name | ||
"manageiq/manageiq-ui-worker" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you be able to change the org name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we drop the "manageiq-" from the container name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @carbonin and I discussed moving this into Settings, but I wanted to get this in first, and do that as an incremental improvement. |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
class MiqUiWorker::Runner < MiqWorker::Runner | ||
include MiqWebServerRunnerMixin | ||
|
||
def prepare | ||
super | ||
MiqApache::Control.start if MiqEnvironment::Command.is_container? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was going away. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? We need it to serve static assets. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot about that 👍 |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
require 'io/wait' | ||
|
||
class MiqWorker < ApplicationRecord | ||
include_concern 'ContainerCommon' | ||
include UuidMixin | ||
|
||
before_destroy :log_destroy_of_worker_messages | ||
|
@@ -251,18 +252,22 @@ def self.log_status(level = :info) | |
find_current.each { |w| w.log_status(level) } | ||
end | ||
|
||
def self.create_worker_record(*params) | ||
def self.init_worker_object(*params) | ||
params = params.first | ||
params = {} unless params.kind_of?(Hash) | ||
params[:queue_name] = default_queue_name unless params.key?(:queue_name) || default_queue_name.nil? | ||
params[:status] = STATUS_CREATING | ||
params[:last_heartbeat] = Time.now.utc | ||
|
||
server_scope.create(params) | ||
server_scope.new(params) | ||
end | ||
|
||
def self.create_worker_record(*params) | ||
init_worker_object(*params).tap(&:save) | ||
end | ||
|
||
def self.start_worker(*params) | ||
w = create_worker_record(*params) | ||
w = containerized_worker? ? init_worker_object(*params) : create_worker_record(*params) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think I see what your doing here, and why, but curious what this means for the future:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Doh... I even wrote the code that does that and forgot... okay, guess that pretty much answers most of the questions then huh?
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct, the environment for the container is defined at the deployment config level so each replica will have the same set of environment variables. |
||
w.start | ||
w | ||
end | ||
|
@@ -360,14 +365,32 @@ def queue_name | |
end | ||
end | ||
|
||
def self.supports_container? | ||
false | ||
end | ||
|
||
def self.containerized_worker? | ||
MiqEnvironment::Command.is_container? && supports_container? | ||
end | ||
|
||
def containerized_worker? | ||
self.class.containerized_worker? | ||
end | ||
|
||
def start_runner | ||
if ENV['MIQ_SPAWN_WORKERS'] || !Process.respond_to?(:fork) | ||
start_runner_via_spawn | ||
elsif containerized_worker? | ||
start_runner_via_container | ||
else | ||
start_runner_via_fork | ||
end | ||
end | ||
|
||
def start_runner_via_container | ||
create_container_objects | ||
end | ||
|
||
def start_runner_via_fork | ||
self.class.before_fork | ||
pid = fork(:cow_friendly => true) do | ||
|
@@ -412,10 +435,10 @@ def start_runner_via_spawn | |
|
||
def start | ||
self.pid = start_runner | ||
save | ||
save unless containerized_worker? | ||
|
||
msg = "Worker started: ID [#{id}], PID [#{pid}], GUID [#{guid}]" | ||
MiqEvent.raise_evm_event_queue(miq_server, "evm_worker_start", :event_details => msg, :type => self.class.name) | ||
MiqEvent.raise_evm_event_queue(miq_server || MiqServer.my_server, "evm_worker_start", :event_details => msg, :type => self.class.name) | ||
|
||
_log.info(msg) | ||
self | ||
|
@@ -502,6 +525,8 @@ def log_destroy_of_worker_messages | |
end | ||
|
||
def status_update | ||
return if MiqEnvironment::Command.is_container? | ||
|
||
begin | ||
pinfo = MiqProcess.processInfo(pid) | ||
rescue Errno::ESRCH | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
require 'kubeclient' | ||
|
||
class MiqWorker | ||
module ContainerCommon | ||
extend ActiveSupport::Concern | ||
|
||
def configure_worker_deployment(definition, replicas = 0) | ||
definition[:spec][:replicas] = replicas | ||
definition[:spec][:template][:spec][:terminationGracePeriodSeconds] = self.class.worker_settings[:stopping_timeout].seconds | ||
|
||
container = definition[:spec][:template][:spec][:containers].first | ||
container[:image] = "#{container_image_name}:#{container_image_tag}" | ||
container[:env] << {:name => "WORKER_CLASS_NAME", :value => self.class.name} | ||
end | ||
|
||
def scale_deployment | ||
ContainerOrchestrator.new.scale(worker_deployment_name, self.class.workers_configured_count) | ||
delete_container_objects if self.class.workers_configured_count.zero? | ||
end | ||
|
||
def container_image_name | ||
"manageiq/manageiq-base-worker" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
end | ||
|
||
def container_image_tag | ||
"latest" | ||
end | ||
|
||
def worker_deployment_name | ||
@worker_deployment_name ||= begin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol |
||
deployment_name = abbreviated_class_name.dup.chomp("Worker").sub("Manager", "").sub(/^Miq/, "") | ||
deployment_name << "-#{Array(ems_id).map { |id| ApplicationRecord.split_id(id).last }.join("-")}" if respond_to?(:ems_id) | ||
deployment_name.underscore.dasherize.tr("/", "-") | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
class MiqWorker | ||
module DeploymentPerWorker | ||
extend ActiveSupport::Concern | ||
|
||
def create_container_objects | ||
ContainerOrchestrator.new.create_deployment_config(worker_deployment_name) do |definition| | ||
configure_worker_deployment(definition, 1) | ||
definition[:spec][:template][:spec][:containers].first[:env] << {:name => "EMS_IDS", :value => Array.wrap(self.class.ems_id_from_queue_name(queue_name)).join(",")} | ||
end | ||
end | ||
|
||
def delete_container_objects | ||
ContainerOrchestrator.new.delete_deployment_config(worker_deployment_name) | ||
end | ||
|
||
def stop_container | ||
delete_container_objects | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
class MiqWorker | ||
module ReplicaPerWorker | ||
extend ActiveSupport::Concern | ||
|
||
def create_container_objects | ||
ContainerOrchestrator.new.create_deployment_config(worker_deployment_name) do |definition| | ||
configure_worker_deployment(definition) | ||
end | ||
scale_deployment | ||
end | ||
|
||
def delete_container_objects | ||
ContainerOrchestrator.new.delete_deployment_config(worker_deployment_name) | ||
end | ||
|
||
def stop_container | ||
scale_deployment | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of this now?