Add container groups\images statistics for container projects #10470

zeari · 2016-08-15T12:57:44Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1487172
The container group calculation is relevant only for ems. This adjusts the calculation to be correct for projects too

@miq-bot add_label providers/containers
@yaacov Please review

zeari · 2016-08-15T12:59:30Z

app/models/metric/statistic.rb

@@ -4,7 +4,14 @@ def self.calculate_stat_columns(obj, timestamp)
    return {} unless obj.respond_to?(:container_images)


Do we also want to separate these two conditions? (ie. we might have just :container_groups or just :container_images)

zeari · 2016-08-18T08:12:19Z

cc @simon3z

simon3z · 2016-09-05T15:02:35Z

@zeari I have the feeling that we shouldn't check the object kind (e.g. obj.kind_of? ContainerProject). I have seen that @yaacov commented something around that. Is there a conclusion there?

@miq-bot assign zeari

simon3z · 2016-09-05T16:28:54Z

app/models/metric/statistic.rb

+    if obj.respond_to?(:all_container_groups)
+      container_groups = obj.all_container_groups # Get disconnected entities as well
+      hash[:stat_container_group_create_rate] =  container_groups.where(:ems_created_on => capture_interval).count
+      hash[:stat_container_group_delete_rate] =  container_groups.where(:deleted_on => capture_interval).count


Minor, style: two spaces instead of just one after the equal sign.

simon3z · 2016-09-05T16:29:31Z

@yaacov @cben can you review?

yaacov · 2016-09-05T16:37:58Z

app/models/metric/statistic.rb

+      hash[:stat_container_group_delete_rate] =  container_groups.where(:deleted_on => capture_interval).count
+    end
+
+    if obj.respond_to?(:all_container_groups)


if obj.respond_to?(:container_images)?

yaacov · 2016-09-05T16:41:13Z

app/models/metric/statistic.rb

+      hash[:stat_container_group_delete_rate] = container_groups.where(:deleted_on => capture_interval).count
+    end
+
+    if obj.respond_to?(:all_container_groups)


if obj.respond_to?(:container_images) ?

yaacov · 2016-09-05T16:46:08Z

@zeari do we need the all_container_groups in app/models/container_project.rb ?

zeari · 2016-09-05T16:59:02Z

@zeari do we need the all_container_groups in app/models/container_project.rb

yes, Its a different query for ContainerManager and ContainerProject and they both have disconnected ContainerGroups.

yaacov · 2016-09-05T17:50:23Z

yes, Its a different query for ContainerManager and ContainerProject and they both have disconnected ContainerGroups.

@zeari the question was why this function is missing from in this PR ?

zeari · 2016-09-05T17:53:13Z

@zeari the question was why this function is missing from in this PR ?

@yaacov It was added for chargeback in a different PR.

yaacov · 2016-09-05T17:55:00Z

LGTM 👍

simon3z · 2016-09-05T21:50:50Z

The container group calculation was relevant only for ems.
Now its relevant for all entities.

@zeari is this a part of some other larger work or is this standalone? (I understand what it does, but why do we need it?)

zeari · 2016-09-05T22:09:48Z

@simon3z I was fixing metrics for ContainerProject and noticed this code was odd.

Edit:
Previously the statistics were calculated wrong -
with ems_id == id for all entities that respond_to?(:container_groups, :container_images)

simon3z · 2016-09-06T09:18:00Z

@zeari should you label this as a bug? Should we care about fixing this in darga?

zeari · 2016-09-06T10:33:42Z

Not really, we dont currently use creation statistics for anything other than ems

@miq-bot add_label darga/no

zeari · 2016-09-06T10:33:56Z

@miq-bot add_label bug

moolitayer · 2017-08-23T08:46:12Z

app/models/container_project.rb

@@ -9,7 +9,6 @@ class ContainerProject < ApplicationRecord
  has_many :container_replicators
  has_many :container_services
  has_many :containers, :through => :container_groups
-  has_many :containers, :through => :container_groups


@zeari @moolitayer do we want to split this fix out and get it merged right away?

@zeari if you need review on that one, I will try to merge fast.

moolitayer · 2017-08-23T09:13:09Z

app/models/metric/statistic.rb

-    container_groups = ContainerGroup.where(:ems_id => obj.id).or(ContainerGroup.where(:old_ems_id => obj.id))
+    stats = {}
+
+    if obj.respond_to?(:all_container_groups)


looks like this fixes a bug (previously stat_container_group_delete_rate for ems were always 0)

moolitayer · 2017-08-23T09:23:50Z

app/models/metric/statistic.rb

+      stats[:stat_container_group_delete_rate] = container_groups.where(:deleted_on => capture_interval).count
+    end
+
+    if obj.respond_to?(:container_images)


I see this was mentioned before, but shouldn't we have something like all_container_images in container_project and use that here? otherwise we might miss images that were registered and then deleted?

Ah I read you wrote it's in another PR. So this should be updated after adding the method? Maybe it's worth a TODO so we don't forget?

ah, it wasnt relevant when i opened the PR but it is now. Adding.

simon3z · 2017-08-24T08:03:02Z

@moolitayer @zeari do we need a BZ for fine?

zeari · 2017-08-24T08:04:59Z

@moolitayer @zeari do we need a BZ for fine?

@simon3z i dont think so. it currently works correctly for ems and ManageIQ/manageiq-ui-classic#1527 is for next version.

simon3z · 2017-08-24T08:07:03Z

@simon3z i dont think so. it currently works correctly for ems and ManageIQ/manageiq-ui-classic#1527 is for next version.

@zeari @moolitayer mentioned a bug related to stat_container_group_delete_rate, isn't that affecting fine as well?

zeari · 2017-08-27T09:13:08Z

@moolitayer does this need to be in fine? I think we will need a fine specific PR since this is based on the archival change from :ems_id => nil to active => true

zeari · 2017-08-27T09:36:57Z

@moolitayer @simon3z @cben stat_container_image_registration_rate for projects could be how many images that were never in use before in this project, are first used in this hour. but we dont use this information anywhere for projects.

ContainerProject.find(18).all_container_groups.joins(:containers).joins(:container_images).group('container_images.id').select('container_images.id').having('min(container_groups.ems_created_on) > ? and min(container_groups.ems_created_on) < ?', 1.hour.ago, Time.now).collect(&:attributes).count

moolitayer · 2017-08-27T14:58:25Z

@moolitayer does this need to be in fine? I think we will need a fine specific PR since this is based on the archival change from :ems_id => nil to active => true

Is this based on a bz or is it for a new feature(e.g a dashboard showing this info)?
So in fine we had these states attached to container_projects but we weren't calculating them correctly?
(we are calculating them from the quite some time but I haven't checked if we always calculated it for projects too)

I think fixing fine is a good idea. We will need a bz to backport.

simon3z · 2017-08-28T07:17:05Z

I think fixing fine is a good idea. We will need a bz to backport.

@zeari @moolitayer please let's open one. If you're unsure you can have QE to try and reproduce the issue (so then they'll also know how to verify that it's fixed once delivered).

zeari · 2017-08-31T11:29:27Z

@miq-bot add_label fine/yes

zeari · 2017-08-31T12:00:58Z

This requires a fine-specific PR

miq-bot · 2017-08-31T12:02:58Z

Checked commit zeari@1613f75 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

zeari · 2017-09-05T10:17:26Z

ping @moolitayer @simon3z BZ is in place and comments addressed

moolitayer

LGTM 👍

zeari · 2017-10-15T11:03:41Z

ping @simon3z

simon3z

👍

simon3z · 2017-10-20T15:30:02Z

@miq-bot assign blomquisg

simaishi · 2017-11-13T21:41:27Z

Backported to Fine via #16345

zeari force-pushed the fix_group_creation_rollup branch from 0cd30de to cc9cda0 Compare August 15, 2016 12:58

miq-bot added the providers/containers label Aug 15, 2016

zeari reviewed Aug 15, 2016
View reviewed changes

chessbyte assigned simon3z Aug 15, 2016

miq-bot assigned zeari and unassigned simon3z Sep 5, 2016

zeari force-pushed the fix_group_creation_rollup branch from cc9cda0 to 7f23186 Compare September 5, 2016 16:05

simon3z reviewed Sep 5, 2016
View reviewed changes

zeari force-pushed the fix_group_creation_rollup branch from 7f23186 to 2f98e41 Compare September 5, 2016 16:37

yaacov reviewed Sep 5, 2016
View reviewed changes

zeari force-pushed the fix_group_creation_rollup branch from 2f98e41 to b523494 Compare September 5, 2016 16:39

yaacov reviewed Sep 5, 2016
View reviewed changes

zeari force-pushed the fix_group_creation_rollup branch from b523494 to 7476e2c Compare September 5, 2016 16:51

miq-bot added darga/no bug labels Sep 6, 2016

zeari closed this Feb 22, 2017

moolitayer reviewed Aug 23, 2017

View reviewed changes

Fix container groups\images statistics for all entities

1613f75

miq-bot added the fine/yes label Aug 31, 2017

zeari force-pushed the fix_group_creation_rollup branch from 5475b07 to 1613f75 Compare August 31, 2017 11:59

moolitayer approved these changes Sep 6, 2017

View reviewed changes

zeari mentioned this pull request Oct 3, 2017

Allow 'assign to containers provider' in chargeback storage assignments ManageIQ/manageiq-ui-classic#2276

Merged

simon3z approved these changes Oct 20, 2017

View reviewed changes

miq-bot assigned blomquisg and unassigned zeari Oct 20, 2017

blomquisg merged commit 35616a5 into ManageIQ:master Oct 23, 2017

blomquisg added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 23, 2017

zeari mentioned this pull request Oct 30, 2017

[FINE] Fix container groups\images statistics for all entities #16345

Merged

simaishi added fine/backported and removed fine/yes labels Nov 13, 2017

		@@ -4,7 +4,14 @@ def self.calculate_stat_columns(obj, timestamp)
		return {} unless obj.respond_to?(:container_images)

Add container groups\images statistics for container projects #10470

Add container groups\images statistics for container projects #10470

Conversation

zeari commented Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

zeari commented Aug 18, 2016

simon3z commented Sep 5, 2016

Choose a reason for hiding this comment

simon3z commented Sep 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Sep 5, 2016

zeari commented Sep 5, 2016

yaacov commented Sep 5, 2016

zeari commented Sep 5, 2016

yaacov commented Sep 5, 2016

simon3z commented Sep 5, 2016

zeari commented Sep 5, 2016 • edited Loading

simon3z commented Sep 6, 2016

zeari commented Sep 6, 2016

zeari commented Sep 6, 2016

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

simon3z commented Aug 24, 2017

zeari commented Aug 24, 2017 • edited Loading

simon3z commented Aug 24, 2017

zeari commented Aug 27, 2017

zeari commented Aug 27, 2017

moolitayer commented Aug 27, 2017

simon3z commented Aug 28, 2017

zeari commented Aug 31, 2017

zeari commented Aug 31, 2017

miq-bot commented Aug 31, 2017

zeari commented Sep 5, 2017

moolitayer left a comment

Choose a reason for hiding this comment

zeari commented Oct 15, 2017

simon3z left a comment

Choose a reason for hiding this comment

simon3z commented Oct 20, 2017

simaishi commented Nov 13, 2017

zeari commented Aug 15, 2016 •

edited

Loading

zeari commented Sep 5, 2016 •

edited

Loading

zeari commented Aug 24, 2017 •

edited

Loading