Check metrics authentication validity before perf_capture_queue #493

agrare · 2023-06-12T18:34:14Z

Check that the EMS's metrics authentication is valid before queueing perf_capture calls, potentially flooding the queue with calls that are going to fail later.

requires:

fix test by giving metrics endpoint to metrics capture ems manageiq#22567

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb

jrafanie · 2023-06-12T18:52:32Z

Nice

Check that the EMS's metrics authentication is valid before queueing perf_capture

agrare · 2023-06-12T19:50:31Z

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb

+    end
+
+    def verify_metrics_connection!(ems)
+      t = target || ems


@kbrock do we have a general way of handling this? When called from perf_capture_timer as ems.perf_capture_object.perf_capture_all_queue target is nil and only ems is set

I really don't like how @target is set behind the scenes.
perf_capture_gap, perf_capture_all_queue, perf_capture_realtime_queue ( /via ui)

if you are running capture for a whole ems, target is null.
If you are running capture for a single object, target is single object.
If you are running capture for a batch/vmware, target/targets is an array

I think I'd just use ems

Note: you need to handle multiple targets case. (I would like everyone to have targets)

Does it make sense to ensure that prometheus exists in here? (aka capture_context)

That way verify does all the raises here.

I think I'd just use ems

Well when we use this below it is on a single object so this has to work with target or ems

Does it make sense to ensure that prometheus exists in here? (aka capture_context)

Yeah I was thinking that but we use the return of capture_context and returning something from a verify_* method seems weird.

Also building a prometheus_capture_context object seems like overkill when it isn't used in the perf_capture_timer case here

I probably could call this from capture_context and raise if that is nil in that method

Okay I pushed a second commit which moves some things around, LMK if you like it better or worse and it is easy to revert out

You know, it doesn't look like it needs to know about target at all beside building a couple of printf strings. Looks good for now.

TANGENT:

Yea, I guess the code is currently coded to reference a single target.

I have on my short list to convert this over to targets.
For some reason, I can't find the initialize method on PrometheusCaptureContext.
But from what I can tell, that code assumes a single target when building the labels.

Not sure how hard it would be to handle a list of targets over there.

agrare · 2023-06-12T19:51:08Z

spec/models/manageiq/providers/kubernetes/container_manager/metrics_capture_spec.rb

@@ -59,6 +59,33 @@
    end
  end

+  context "#perf_capture_all_queue" do
+    it "returns the objects" do
+      expect(@ems_kubernetes.perf_capture_object.perf_capture_all_queue).to include("Container" => [@container], "ContainerNode" => [@node])


TODO why only @container and @node and not @group

agrare · 2023-06-12T19:51:41Z

spec/models/manageiq/providers/kubernetes/container_manager/metrics_capture_spec.rb

+
+    context "with a missing metrics endpoint" do
+      before do
+        @ems_kubernetes.endpoints.find_by(:role => "prometheus").destroy


NOTE I don't love how this spec sets up these ivars since it makes it difficult to override in sub-contexts. I worked around it but I think I should refactor to use let

agrare · 2023-06-12T19:51:59Z

spec/models/manageiq/providers/kubernetes/container_manager/metrics_capture_spec.rb

+    context "with invalid authentication on the metrics endpoint" do
+      before do
+        @ems_kubernetes.authentications.find_by(:authtype => "prometheus").update!(:status => "Error")
+        @ems_kubernetes.reload


NOTE a good example of why the before {} setting up ivars isn't great

kbrock · 2023-06-13T03:48:01Z

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb

-        context = capture_context(ems, target, start_time, end_time)
-
-        raise TargetValidationWarning, "no metrics endpoint found for #{target_name}" if context.nil?
+        context = build_capture_context!(ems, target, start_time, end_time)


yea. this is nice

kbrock

LGTM

miq-bot · 2023-06-13T14:35:21Z

Checked commits agrare/manageiq-providers-kubernetes@be53bdc~...0c258a3 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 1 offense detected

spec/factories/ext_management_system.rb

❗ - Line 18, Col 27 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

agrare · 2023-06-13T16:23:54Z

@kbrock can you re-review? I refactored the specs to use let() and pulled out target_name into a method to keep the logic in one place.

jrafanie · 2023-06-13T18:20:44Z

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb

+      rescue TargetValidationError, TargetValidationWarning => e
+        _log.send(e.log_severity, e.message)
+        return []
+      end


Do all providers implement similar logic? Can we extract this so other providers will also bypass metrics capture with invalid auth?

This is a "future" PR question... I'm fine with fixing kubernetes in this way now.

It is a little tricky because not all providers have different endpoints and/or authentications so what you need to check varies.

What I've considered doing is consolidating this in the supports :metrics {} block in ext_management_system so that the logic could be limited to the provider but that is going to be more of a cross-provider change.

updating supports to handle all the actual logic totally got me in a few PRs. they are still outstanding :( ✨

jrafanie · 2023-06-13T18:21:40Z

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb

+      raise TargetValidationError, "no provider for #{target_name}" if ems.nil?
+
+      raise TargetValidationWarning, "no metrics endpoint found for #{target_name}" if metrics_connection(ems).nil?
+      raise TargetValidationWarning, "metrics authentication isn't valid for #{target_name}" unless metrics_connection_valid?(ems)


Do we need to check if the ems is paused?

I wonder if we should do that up in core perf_capture_timer since that is going to be the same for all providers, good call though. I'm a little wary of that just because it feels like every single queue method would have to implement this check but I don't know that there's a more central place to handle it and this is a real-world problem.

ManageIQ/manageiq#22564

kbrock · 2023-06-13T19:53:27Z

spec/models/manageiq/providers/kubernetes/container_manager/metrics_capture_spec.rb

+    FactoryBot.create(:kubernetes_node, :name => 'node', :ext_management_system => ems, :ems_ref => 'target').tap do |node|
+      node.computer_system.hardware = FactoryBot.create(:hardware, :cpu_total_cores => 2, :memory_mb => 2_048)
+    end


lets keep. But in the future if we merge ManageIQ/manageiq#22565 this could be:

Suggested change

FactoryBot.create(:kubernetes_node, :name => 'node', :ext_management_system => ems, :ems_ref => 'target').tap do |node|

node.computer_system.hardware = FactoryBot.create(:hardware, :cpu_total_cores => 2, :memory_mb => 2_048)

end

FactoryBot.create(:kubernetes_node, :name => 'node', :ext_management_system => ems, :ems_ref => 'target', :hardware => FactoryBot.create(:hardware, :cpu_total_cores => 2, :memory_mb => 2_048))

👍 yeah I wanted to keep this dependent-less for simpler backport but that's a much better way to do it

agreed. keep. reduce dependencies

kbrock

these updates look good.

introduced by: ManageIQ/manageiq-providers-kubernetes#493 This makes sure the ems is all good and tests pass

Fryguy · 2023-06-28T14:57:58Z

Backported to petrosian in commit 968fcea.

commit 968fcea19c17ceab63e9272a3df91a85461d58aa
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Tue Jun 13 18:21:24 2023 -0400

    Merge pull request #493 from agrare/check_metrics_connection_validity_before_queueing_perf_captures
    
    Check metrics authentication validity before perf_capture_queue
    
    (cherry picked from commit ba04739fc4ab3eee4625630ec4622dd71d576d67)

…_before_queueing_perf_captures Check metrics authentication validity before perf_capture_queue (cherry picked from commit ba04739)

introduced by: ManageIQ/manageiq-providers-kubernetes#493 This makes sure the ems is all good and tests pass

agrare added the bug label Jun 12, 2023

agrare requested review from kbrock and jrafanie June 12, 2023 18:34

agrare requested review from cben and Fryguy as code owners June 12, 2023 18:34

kbrock reviewed Jun 12, 2023

View reviewed changes

app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb Show resolved Hide resolved

agrare force-pushed the check_metrics_connection_validity_before_queueing_perf_captures branch 4 times, most recently from 5d3c22d to d70847b Compare June 12, 2023 19:45

Check metrics auth valid before perf_capture_queue

be53bdc

Check that the EMS's metrics authentication is valid before queueing perf_capture

agrare force-pushed the check_metrics_connection_validity_before_queueing_perf_captures branch from d70847b to be53bdc Compare June 12, 2023 19:49

agrare commented Jun 12, 2023

View reviewed changes

Refactor capture_context to check endpoints and auth

4df0a93

kbrock reviewed Jun 13, 2023

View reviewed changes

kbrock approved these changes Jun 13, 2023

View reviewed changes

Refactor metrics_capture_spec

b8a12a9

agrare force-pushed the check_metrics_connection_validity_before_queueing_perf_captures branch from 32e316c to b8a12a9 Compare June 13, 2023 14:10

agrare added 2 commits June 13, 2023 10:13

Extract target_name to a method

5e07948

Add test for invalid capture context

0c258a3

agrare force-pushed the check_metrics_connection_validity_before_queueing_perf_captures branch from 0aac8bf to 0c258a3 Compare June 13, 2023 14:34

agrare mentioned this pull request Jun 13, 2023

Improve performance capture ems targets #494

Open

1 task

jrafanie reviewed Jun 13, 2023

View reviewed changes

kbrock reviewed Jun 13, 2023

View reviewed changes

kbrock approved these changes Jun 13, 2023

View reviewed changes

kbrock added the metrics label Jun 13, 2023

kbrock self-assigned this Jun 13, 2023

kbrock merged commit ba04739 into ManageIQ:master Jun 13, 2023

agrare deleted the check_metrics_connection_validity_before_queueing_perf_captures branch June 13, 2023 22:21

kbrock added a commit to kbrock/manageiq that referenced this pull request Jun 14, 2023

fix test by giving metrics endpoint to metrics capture ems

dd9cc6b

introduced by: ManageIQ/manageiq-providers-kubernetes#493 This makes sure the ems is all good and tests pass

kbrock mentioned this pull request Jun 14, 2023

fix test by giving metrics endpoint to metrics capture ems ManageIQ/manageiq#22567

Merged

agrare added the petrosian/yes? label Jun 15, 2023

Fryguy added petrosian/yes and removed petrosian/yes? labels Jun 28, 2023

Fryguy pushed a commit that referenced this pull request Jun 28, 2023

Merge pull request #493 from agrare/check_metrics_connection_validity…

968fcea

…_before_queueing_perf_captures Check metrics authentication validity before perf_capture_queue (cherry picked from commit ba04739)

Fryguy added petrosian/backported and removed petrosian/yes labels Jun 28, 2023

agrare mentioned this pull request Jul 5, 2023

[PETROSIAN] Update kubernetes reference since core specs depend on it ManageIQ/manageiq#22601

Merged

GilbertCherrie pushed a commit to GilbertCherrie/manageiq that referenced this pull request Jul 7, 2023

fix test by giving metrics endpoint to metrics capture ems

f6a5767

introduced by: ManageIQ/manageiq-providers-kubernetes#493 This makes sure the ems is all good and tests pass

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check metrics authentication validity before perf_capture_queue #493

Check metrics authentication validity before perf_capture_queue #493

agrare commented Jun 12, 2023 •

edited by kbrock

Loading

jrafanie commented Jun 12, 2023

agrare Jun 12, 2023

kbrock Jun 12, 2023 •

edited

Loading

kbrock Jun 12, 2023

agrare Jun 12, 2023

agrare Jun 12, 2023

agrare Jun 12, 2023

agrare Jun 12, 2023

kbrock Jun 13, 2023

agrare Jun 12, 2023

agrare Jun 12, 2023

agrare Jun 12, 2023

kbrock Jun 13, 2023

kbrock left a comment

miq-bot commented Jun 13, 2023

agrare commented Jun 13, 2023

jrafanie Jun 13, 2023

jrafanie Jun 13, 2023

agrare Jun 13, 2023

kbrock Jun 13, 2023

jrafanie Jun 13, 2023

agrare Jun 13, 2023 •

edited

Loading

agrare Jun 13, 2023

kbrock Jun 13, 2023

agrare Jun 13, 2023

kbrock Jun 13, 2023

kbrock left a comment

Fryguy commented Jun 28, 2023

Check metrics authentication validity before perf_capture_queue #493

Check metrics authentication validity before perf_capture_queue #493

Conversation

agrare commented Jun 12, 2023 • edited by kbrock Loading

jrafanie commented Jun 12, 2023

Choose a reason for hiding this comment

kbrock Jun 12, 2023 • 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

kbrock left a comment

Choose a reason for hiding this comment

miq-bot commented Jun 13, 2023

agrare commented Jun 13, 2023

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

agrare Jun 13, 2023 • 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

kbrock left a comment

Choose a reason for hiding this comment

Fryguy commented Jun 28, 2023

agrare commented Jun 12, 2023 •

edited by kbrock

Loading

kbrock Jun 12, 2023 •

edited

Loading

agrare Jun 13, 2023 •

edited

Loading