Skip to content
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.

Sign up for GitHub

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

Only run kube_monitor threads on master MiqServer #21532

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Only run kube_monitor threads on master MiqServer #21532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91a9230
Only run kube_monitor threads on master MiqServer
agrare Nov 1, 2021
17ed688
Don't rely on is_master? on podified
agrare Nov 2, 2021
3773816
Ensure primary miq_server is first in servers_from_db
agrare Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion app/models/miq_server/worker_management/kubernetes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,15 @@ def sync_monitor
end

def sync_from_system
ensure_kube_monitors_started
# All miq_server instances have to reside on the same Kubernetes cluster, so
# we only have to sync the list of pods and deployments once
ensure_kube_monitors_started if my_server_is_primary?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change I think we now need the "default" to be first in the list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, as I'm looking at all of this closer, this is all really weird to me. We should really be starting this thread outside of the as_each_server { monitor } loop and also ensure it's still alive outside of the loop. This whole monitoring kube thing has nothing to do with individual servers. The only thing we really need individual servers for is to know if that server's workers are now orphaned or not. Even then we don't need to do it server by server, because we can just make it check all the workers in_my_region. I'm tempted to say let's just get rid of this whole "looping over the servers" thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point. I don't think we need anything to be done on each individual server. The code outside of the as_each_server block could easily check the deployments, settings, and pods/rows for the whole region.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we're definitely overloading the miq_server concept here, doesn't help that we're creating multiple miq_servers for a single "platform" instance (aka before 1 miq_server <=> 1 vm)

I think we need to decide what exactly an miq_server is, does is track 1:1 with a runtime platform (vm, k8s namespace) or is it just a logical grouping for zones and other resources.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, short term I've unshifted the "real" miq_server to the front of the line. Long term we definitely need to either extract this out of miq_server or not create "fake" miq_servers for zones.

Not sure if there is enough here to be considered short-term, maybe we just jump straight in to reworking this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I have a solid idea on my head on that - I'll rip out a quick WIP PR once this is merged to discuss.


# Before syncing the workers check for any orphaned worker rows that don't have
# a current pod and delete them
cleanup_orphaned_worker_rows
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this belongs in the cleanup_failed_workers section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to ensure that the master server goes first in the list...otherwise if a non-master server goes first it will try to compare to an empty "global" list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or we spawn off the kubernetes watches as a completely separate thing beforehand, and then piggy back on the master server only to ensure the thread is still alive ornot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate podified does this thing where it creates servers for zones. Anything that hits the miq_workers table is scoped for the server of the zone that it's currently processing, but anything that looks at current_pods is global and needs the check to make sure only the primary server does it.

It does make sense to to put this with delete_failed_deployments in cleanup_failed_workers as long as the timing works but the latter is global so it has the primary? check whereas this is for each server and doesn't. It's similar behavior at a different scope.


# Update worker deployments with updated settings such as cpu/memory limits
sync_deployment_settings

This comment was marked as outdated.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO this seems like something that sync_workers should handle when looping through the worker classes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes although, we'd be trading guard clauses:

# currently we loop through `podified_miq_workers` and skip duplicate deployment names we already processed:

next if checked_deployments.include?(worker.worker_deployment_name)

Instead, we'd have to skip worker classes that have no existing podified_miq_workers / deployment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, it's easier now whereas previously it was just messy to have sync_workers have podified? checks plus introspect the server's current_pods and current_deployments.

end

Expand Down Expand Up @@ -87,6 +94,12 @@ def constraints_changed?(current, desired)

private

# In podified there is only one "primary" miq_server whose zone is "default", the
# other miq_server instances are simply to allow for additional zones
def my_server_is_primary?
my_server.zone&.name == "default"
end

def cpu_value_eql?(current, desired)
# Convert to millicores if not already converted: "1" -> 1000; "1000m" -> 1000
current = current.to_s[-1] == "m" ? current.to_f : current.to_f * 1000
Expand Down Expand Up @@ -131,6 +144,8 @@ def ensure_kube_monitors_started
end

def delete_failed_deployments
return unless my_server_is_primary?

failed_deployments.each do |failed|
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failed_deployments has "global" scope (aka not scoped to a specific miq_server) so this should also be only run on the master server

orchestrator.delete_deployment(failed)
end
Expand Down
11 changes: 10 additions & 1 deletion lib/workers/evm_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@ def self.start(*args)
private

def servers_from_db
MiqEnvironment::Command.is_podified? ? MiqServer.in_my_region.to_a : [MiqServer.my_server(true)].compact
my_server = MiqServer.my_server(true)

if MiqEnvironment::Command.is_podified?
# Ensure that the "primary" miq_server is first in the list of servers.
# This ensures that any work that is only done on the primary is completed
# before processing the rest of the servers.
MiqServer.in_my_region.to_a.unshift(my_server).uniq.compact
else
[my_server].compact
end
end

def set_process_title
Expand Down
2 changes: 1 addition & 1 deletion spec/models/miq_server/worker_management/kubernetes_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'recursive-open-struct'

RSpec.describe MiqServer::WorkerManagement::Kubernetes do
let(:server) { EvmSpecHelper.create_guid_miq_server_zone.second }
let(:server) { FactoryBot.create(:miq_server_in_default_zone).tap { |s| EvmSpecHelper.stub_as_local_server(s) } }
let(:orchestrator) { double("ContainerOrchestrator") }
let(:deployment_name) { '1-generic-79bb8b8bb5-8ggbg' }
let(:pod_label) { '1-generic' }
Expand Down