Fix non existent container showing in report

[nimrodshn]

Currently, we output 'archived' containers to the user in the report 'Pods per Ready Status'.
In this PR we fix this behavior to only show the user the current containers.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1435958

Screenshots:

-Before:

-After:

Notice the list shrunk from 14 to 9.
cc: @simon3z @moolitayer @kbrock

[nimrodshn]

@miq-bot add_label containers/providers bug wip

[nimrodshn]

@simon3z we should probably review the other container related reports and make sure this behavior is the same for them also.. what do you think?

[miq-bot]

@nimrodshn Cannot apply the following label because they are not recognized: containers/providers bug wip

[miq-bot]

@nimrodshn Cannot apply the following label because they are not recognized: containers/providers bug wip

[nimrodshn]

@miq-bot add_labels containers/providers, bug, wip

[miq-bot]

@nimrodshn Cannot apply the following label because they are not recognized: containers/providers

[nimrodshn]

@kbrock ended up using you're solution 👍

[simon3z]

conditions: !ruby/object:MiqExpression
  exp:
    IS NOT NULL:
      field: ContainerGroup.ext_management_system-name

This heavily relies on the underlying implementation 😢

What if the deletion will be handled with an extra deleted flag as @Ladas is working on?

I feel we should rely on an explicit attribute of the objects... I think there is the deleted_on field that we can leverage (instead of the implicit underlying implementation of the disconnection).

[nimrodshn]

@simon3z I think leveraging the deleted can work - just need for @Ladas PR (#15250) to get merged 😢

Currently, conditionds is implemented to support only conditions on MiqExpression meaning class fields and not Active Record Associations. From what I gathered deleted flag is a class field so it should work.

[simon3z]

@simon3z I think leveraging the deleted can work - just need for @Ladas PR (#15250) to get merged

@nimrodshn that's why I said deleted_on

[nimrodshn]

@simon3z I'll give it a try 👍

[Ladas]

@nimrodshn @simon3z yeah, deleted_on should always work, I think I will just make it the only source of truth, for the sake of simplicity

[nimrodshn]

@simon3z move to deleted_on and also checked the related widgets are working fine. 👍

[kbrock]

Thanks @nimrodshn
looking good

[nimrodshn]

@simon3z PTAL.

[simon3z]

@kbrock ready for merge 👍

[nimrodshn]

@gtanzillo @lpichler please review/merge, this report is ready. 😇

[lpichler]

this is working?
the syntax is defined in MiqExpression::Field::REGEX (just fyi it is not needed to read the regex :) )
for this case it should be ContainerGroup-deleted_on, the "." is for relations(ContainerGroup.ext_management_system-name).
MiqExpression::Field.parse('ContainerGroup-deleted_on') => true

MiqExpression::Field.parse('ContainerGroup.deleted_on') => nil

so please use 'ContainerGroup-deleted_on' (and also in other places)

[nimrodshn]

@lpichler fixed 👍

[miq-bot]

Checked commit nimrodshn@61a54e7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

[kbrock]

@Ladas are there other tables/reports that need to add a deleted_on field?

[Ladas]

@kbrock I think we have total 5 tables with deleted on for containers, those having active scope in https://github.com/Ladas/manageiq/blob/e7f155589c2342b10780accab2aab4539ae14b48/app/models/manageiq/providers/container_manager.rb#L11)

@cben we miss active scope for container nodes? Do we have a PR?

[cben]

cc @zakiva
Node archiving migration landed but #15351 not yet <-- waiting for core committers hint hint ;-)
That would also apply the active scope. I'm not fully sure what happens currently until it lands.

If I understand this report correctly, it queries ContainerGroup.all, not ems.container_groups? If so it always included archived ones.

Is it good for reports to depend on deleted_on? (1) undiscoverable to users that this is the magic incantation (2) if we change implementation of archiving, reports would break.
Could we expose scopes (of other official way to express "active/archived") to reports?

[kbrock]

@cben - darn. good point.
Wish we didn't even have to explicitly use active too, but that is the way it is.

Is someone adding archive scope to the containers?

[moolitayer]

@kbrock ArchivedMixin?
BTW Do we need to add archived? as a virtual_column to be able include it in reports?

If I understand this report correctly, it queries ContainerGroup.all, not ems.container_groups? If so it always included archived ones

Probably didn't understand what you meant there @cben. Isn't this why we are adding the condition (maybe you were responding to something)

Is it good for reports to depend on deleted_on? (1) undiscoverable to users that this is the magic incantation (2) if we change implementation of archiving, reports would break.
Could we expose scopes (of other official way to express "active/archived") to reports?

We would definitely be better off by relying on ArchivedMixin's methods. I don't know if that makes sense in this context - looks like reports are designed to go straight to the DB. Can we use a default scope for that goal?

2.3.0 :010 > class User < ActiveRecord::Base
2.3.0 :011?>   default_scope { where(:name => 'admin')}
2.3.0 :012?>   end
 => [#<Proc:0x00000008378ea0@(irb):11>] 
2.3.0 :013 > User.all
  User Load (84.9ms)  SELECT "users".* FROM "users" WHERE "users"."name" = $1  [["name", "admin"]]
  User Inst Including Associations (0.0ms - 0rows)
 => #<ActiveRecord::Relation []> 
2.3.0 :014 > 

[simon3z]

@miq-bot add_label fine/yes

[simon3z]

@miq-bot add_label fine/yes

@nimrodshn @Ladas @cben keep me honest we can backport this to fine, the deleted_on field was already present there, no dependency on any schema change, etc.

[Ladas]

@simon3z yes if there is the deleted_on column, then it should work.

btw. we don't have an index on deleted_on column in Fine, but it should not hurt the perf, since ContainerGroup table is pretty small (PG will probably do sequential scan instead of using the index anyway)

[kbrock]

@moolitayer no to default_scope, but adding in the active scope does sound like a good plan.

[simaishi]

Fine backport details:

$ git log -1
commit fc661191a53422bdd8d99be68b7580adc2708d62
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Mon Aug 21 14:19:46 2017 -0400

    Merge pull request #15405 from nimrodshn/fix_nonexistent_containers_in_report
    
    Fix non existent container showing in report
    (cherry picked from commit e5ff9de1256593406408912636225c61ec696c3e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1484895