Collect metrics for archived containers #13686

zeari · 2017-01-29T12:07:13Z

Sometimes a container 'dies' but we still have uncollected metrics for it on the hawkular side which we would like to retrieve.

Has to do with https://bugzilla.redhat.com/show_bug.cgi?id=1408968 but does not solve it.

EDIT: This is the BZ fixed by this PR: https://bugzilla.redhat.com/show_bug.cgi?id=1420721

@moolitayer @yaacov @simon3z Please review

yaacov · 2017-01-29T12:21:27Z

LGTM 👍 (except for the extra selfs)

zeari · 2017-01-29T12:55:20Z

@miq-bot add_label providers/containers, metrics

simon3z · 2017-01-30T09:31:27Z

@kbrock it seems that this is related to a change introduced in PR #9448, can you review?

simon3z · 2017-01-31T11:15:35Z

@zeari can you add tests?

simon3z · 2017-02-02T13:00:29Z

LGTM 👍

@kbrock can you review? (This is touching an area that you've touched "recently")
cc @Fryguy

simon3z · 2017-02-02T13:01:33Z

app/models/container.rb

@@ -18,9 +19,6 @@ class Container < ApplicationRecord
  has_many   :metric_rollups, :as => :resource
  has_many   :vim_performance_states, :as => :resource

-  # Needed for metrics
-  delegate   :my_zone, :to => :ext_management_system


@zeari are you sure this is not used anywhere else in the metrics capture execution?

@simon3z why? I added my_zone as a method further down in this class.
You mean if it should be a virtual_column or cached somehow?

zeari · 2017-02-02T13:39:30Z

@simon3z Do we want similar changes for other archivable container entities?
Im a little unsure the other entities need this.

gtanzillo

👍 Looks good to me

simon3z · 2017-02-02T15:28:27Z

@simon3z Do we want similar changes for other archivable container entities?
Im a little unsure the other entities need this.

@zeari yes please let's keep them here in this PR.

zeari · 2017-02-02T16:10:37Z

@simon3z Added tests and made changes to other archivable container entities

simon3z · 2017-02-02T16:13:08Z

app/models/container.rb

@@ -51,4 +49,13 @@ def disconnect_inv
    self.ems_id = nil
    save
  end
+
+  # Needed for metrics
+  def my_zone


@zeari this seems duplicated code (on all the other entities) that may deserve a mixin or something. Anyway it seems to belong to a single place.

zeari · 2017-02-02T17:02:15Z

@zeari this seems duplicated code (on all the other entities) that may deserve a mixin or something.

@simon3z Moved to a mixin

simon3z · 2017-02-02T22:56:35Z

@simon3z Moved to a mixin

Thanks @zeari !
LGTM 👍

@Fryguy assigning this to you for now (final review/merge), still waiting for feedback from @kbrock

@miq-bot add_label bug, euwe/yes
@miq-bot assign Fryguy

simon3z · 2017-02-02T22:58:34Z

app/models/container.rb


  has_one    :container_group, :through => :container_definition
  belongs_to :ext_management_system, :foreign_key => :ems_id
+  belongs_to :old_ext_management_system, :foreign_key => :old_ems_id, :class_name => 'ExtManagementSystem'


@zeari @Fryguy should this belong to the mixin as well? (Is it even possible? Does it make sense? ...I am asking because I see that the mixing depends on it and it's repeated in all the classes).

Feels like it, yes

kbrock · 2017-02-17T19:09:14Z

Do other models have the same zone issue for archived vms?
A little curious how other vendors work find when an ems => nil

zeari · 2017-02-22T10:29:33Z

Do other models have the same zone issue for archived vms?
A little curious how other vendors work find when an ems => nil

@kbrock
from https://github.com/ManageIQ/manageiq/blob/master/app/models/vm_or_template.rb#L882 :

 def my_zone
    ems = ext_management_system
    ems ? ems.my_zone : MiqServer.my_zone
  end

I think getting the zone from the old ems is more correct

kbrock · 2017-02-22T15:19:09Z

@zeari yes, getting the zone from the old ems is probably more correct.
But not sure why containers should work differently than other providers.

@Fryguy Maybe this should be the same for all providers?

Fryguy · 2017-02-22T16:03:15Z

Maybe this should be the same for all providers?

Yes, I was thinking the same thing. I don't like that this is container specific, in particular the name of the mixin implies that it can only be used for containers, which isn't necessarily true.

Fryguy · 2017-02-22T16:04:49Z

app/models/container_group.rb

@@ -13,7 +14,6 @@ class ContainerGroup < ApplicationRecord
           :through => :container_definitions
  has_many :container_definitions, :dependent => :destroy
  has_many :container_images, -> { distinct }, :through => :container_definitions
-  belongs_to  :ext_management_system, :foreign_key => "ems_id"


Why is this being removed?

@Fryguy because im adding it to ContainerArchivedMixin (also in this PR)

Ah sorry, i thought this was old_ext_management_system

Fryguy · 2017-02-22T16:09:48Z

app/models/metric/ci_mixin/capture.rb

            ext_management_system
+          elsif respond_to?(:old_ext_management_system) && old_ext_management_system.present?
+            old_ext_management_system


This concerns me as queue_name_for_metrics_collection targets which worker will run the collection. If old_ext_management_system points to a no longer enabled EMS, what will happen? Will we just put stuff on the queue that will never get executed?

i dont think we archive those(@simon3z right?) so from what i understand old_ext_management_system would either be nil and get caught by the exception below or be a live ems and fetch metrics correctly

miq-bot · 2017-02-22T21:07:45Z

Checked commit zeari@4ad0054 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 0 offenses detected
Everything looks good. 🍰

zeari · 2017-02-22T21:14:37Z

Yes, I was thinking the same thing. I don't like that this is container specific, in particular the name of the mixin implies that it can only be used for containers, which isn't necessarily true.

@Fryguy Changed to ArchivedMixin.
Changing vm_or_template to work with the mixin needs a little more consideration. Can we open an issue for it and deal with it separately?

@Fryguy

Because @Fryguy did the bot thing again

https://bugzilla.redhat.com/show_bug.cgi?id=1420721 Collect metrics for archived containers (cherry picked from commit 5c98cf7)

jrafanie · 2017-02-24T15:22:37Z

Euwe backport is #14065

simaishi · 2017-02-24T15:56:06Z

Backported to Euwe via #14065

zeari force-pushed the cpature_dead_containers branch from cb698b3 to 4a718c1 Compare January 29, 2017 12:08

miq-bot added providers/metrics providers/containers labels Jan 29, 2017

chessbyte assigned simon3z Jan 30, 2017

zeari force-pushed the cpature_dead_containers branch from 4a718c1 to c06e7ad Compare February 2, 2017 12:52

simon3z reviewed Feb 2, 2017

View reviewed changes

zeari force-pushed the cpature_dead_containers branch 2 times, most recently from 96624ef to 9679e36 Compare February 2, 2017 13:36

gtanzillo reviewed Feb 2, 2017

View reviewed changes

zeari force-pushed the cpature_dead_containers branch 2 times, most recently from 3731e18 to 2d1ca6d Compare February 2, 2017 16:09

simon3z reviewed Feb 2, 2017

View reviewed changes

zeari force-pushed the cpature_dead_containers branch from 2d1ca6d to 4516e90 Compare February 2, 2017 17:01

zeari force-pushed the cpature_dead_containers branch 2 times, most recently from 8cde61c to 5995829 Compare February 2, 2017 17:21

miq-bot added bug euwe/yes labels Feb 2, 2017

miq-bot assigned Fryguy and unassigned simon3z Feb 2, 2017

simon3z reviewed Feb 2, 2017

View reviewed changes

zeari force-pushed the cpature_dead_containers branch from b7c8880 to 08bb967 Compare February 22, 2017 09:54

zeari force-pushed the cpature_dead_containers branch 3 times, most recently from ca75e81 to 8d2ef23 Compare February 22, 2017 15:30

Fryguy requested changes Feb 22, 2017

View reviewed changes

zeari force-pushed the cpature_dead_containers branch from 8d2ef23 to 4ad0054 Compare February 22, 2017 21:06

Collect metrics for archived containers

4ad0054

zeari closed this Feb 23, 2017

zeari reopened this Feb 23, 2017

isimluk mentioned this pull request Feb 24, 2017

Cannot route ContainerProject#perf_rollup after it was #disconnect_inv #14062

Closed

gtanzillo closed this Feb 24, 2017

gtanzillo reopened this Feb 24, 2017

miq-bot previously approved these changes Feb 24, 2017

View reviewed changes

Fryguy approved these changes Feb 24, 2017

View reviewed changes

Fryguy merged commit 5c98cf7 into ManageIQ:master Feb 24, 2017

Fryguy added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 24, 2017

jrafanie mentioned this pull request Feb 24, 2017

[EUWE] Collect metrics for archived containers #14065

Merged

simaishi added euwe/backported and removed euwe/yes labels Feb 24, 2017

zeari mentioned this pull request Apr 2, 2017

Add purge timer for archived entities #14322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect metrics for archived containers #13686

Collect metrics for archived containers #13686

zeari commented Jan 29, 2017 •

edited by jrafanie

Loading

yaacov commented Jan 29, 2017

zeari commented Jan 29, 2017

simon3z commented Jan 30, 2017

simon3z commented Jan 31, 2017

simon3z commented Feb 2, 2017

simon3z Feb 2, 2017

zeari Feb 2, 2017

zeari commented Feb 2, 2017

gtanzillo left a comment

simon3z commented Feb 2, 2017

zeari commented Feb 2, 2017

simon3z Feb 2, 2017 •

edited

Loading

zeari commented Feb 2, 2017

simon3z commented Feb 2, 2017

simon3z Feb 2, 2017

Fryguy Feb 20, 2017

kbrock commented Feb 17, 2017

zeari commented Feb 22, 2017

kbrock commented Feb 22, 2017 •

edited

Loading

Fryguy commented Feb 22, 2017

Fryguy Feb 22, 2017

zeari Feb 22, 2017

zeari Feb 22, 2017

Fryguy Feb 22, 2017

zeari Feb 22, 2017 •

edited

Loading

miq-bot commented Feb 22, 2017

zeari commented Feb 22, 2017 •

edited

Loading

jrafanie commented Feb 24, 2017

simaishi commented Feb 24, 2017

Collect metrics for archived containers #13686

Collect metrics for archived containers #13686

Conversation

zeari commented Jan 29, 2017 • edited by jrafanie Loading

yaacov commented Jan 29, 2017

zeari commented Jan 29, 2017

simon3z commented Jan 30, 2017

simon3z commented Jan 31, 2017

simon3z commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Feb 2, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

simon3z commented Feb 2, 2017

zeari commented Feb 2, 2017

simon3z Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

zeari commented Feb 2, 2017

simon3z commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 17, 2017

zeari commented Feb 22, 2017

kbrock commented Feb 22, 2017 • edited Loading

Fryguy commented Feb 22, 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

zeari Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Feb 22, 2017

zeari commented Feb 22, 2017 • edited Loading

jrafanie commented Feb 24, 2017

simaishi commented Feb 24, 2017

zeari commented Jan 29, 2017 •

edited by jrafanie

Loading

simon3z Feb 2, 2017 •

edited

Loading

kbrock commented Feb 22, 2017 •

edited

Loading

zeari Feb 22, 2017 •

edited

Loading

zeari commented Feb 22, 2017 •

edited

Loading