Allow rate assignment to enterprise for ContainerImages

[isimluk]

Relates to https://bugzilla.redhat.com/show_bug.cgi?id=1426467

@miq-bot add_label bug, chargeback, euwe/yes
@miq-bot assign @gtanzillo

[miq-bot]

Checked commit isimluk@e70ed2d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

[zeari]

Overall i think its correct to inherit the rate from the enterprise here since we do inherit from the 'Selected Containers Provider' but we need to confirm with @simon3z and Loicavenel .
questions to consider:

• when would we use the default rate and when the enterprise/provider rate?
• what are the use cases here when considering the label based assignments?

[zeari]

@Loicavenel

[simon3z]

@isimluk I think I am missing something... in your PR #12608 you removed this from ChargebackContainerImage:

-  def get_rate_parents(_perf)
-    # get rates from image tags only
-    []
-  end

and added:

+  def parents_determining_rate
+    case resource_type
...
+    when Container.name
+      []
+    end
+  end

The correctness of this change isn't evident to me. I would expected that the entity involved here was ContainerImage (and its parent).

Bottom line I don't know when this Container.name case is used and in what Chargeback type (cc @zeari)

[zeari]

@simon3z this is the type if the metric. Container image chargeback goes over container metrics since metrics for images doesnt make a lot of sense.

[simon3z]

@simon3z this is the type if the metric. Container image chargeback goes over container metrics since metrics for images doesnt make a lot of sense.

That makes sense but if the method is named parents_determining_rate and there's no reference anywhere to metrics how someone would know? Even the evaluated variable has a generic resource_type name.

So is the current value [parent_ems] returning the "Default Container Image Rate"? Because if that's not the case I would have expected an empty array here [].

[zeari]

@simon3z well it is app/models/metric/chargeback_helper.rb

So is the current value [parent_ems] returning the "Default Container Image Rate"? Because if that's not the case I would have expected an empty array here [].

nope, if theres an assigned rate to the provider it get the rate from the Selected Containers provider assignment.

[Loicavenel]

@zeari to make sure we all get it right:

• Chargeback by Project, there are 2 possibility of Rate assignment

  Rate assigned to "The Enterprise"
  OR
  Rate assigned by "Container provider"

• Chargeback by Image (it where I am not 100% sure)

  Rate assigned to "The Enterprise"
  All images will have the same rate applied
  Rate assigned to "Container provider"
  All images in the same provider will have the same rate applied
  Rate assigned to "Labelled Image"
  Image with a label where there is a rate assigned with you this rate, other images will use the "default container image rate"

In all the cases is a rate is not assigned, chargeback report will be empty.

CORRECT?

I am letting Tagged Image out of the discussion for now.

[simon3z]

@zeari @isimluk @gtanzillo @Loicavenel it seems to me that we would need to discuss some aspects here and I think we can do it on Thursday in our meeting.
Patch is trivial but I think it uncovered some parts that need attention.

[isimluk]

Patch is trivial but I think it uncovered some parts that need attention.

Could you please expand on this? What do you mean by uncovered parts?

By the way, what was the result of discussion? I didn't get the invite. 😢

[simon3z]

@isimluk we didn't discuss this last week. Let's do it this week (same meeting you joined a couple of weeks ago with @zeari and @gtanzillo).

[zeari]

CORRECT?

@Loicavenel Yes (sorry for the ultra late reply)

[isimluk]

I would prefer to keep all the technical details written in the pr. I have another meeting in the given time slot.

[simon3z]

The decision in the meeting was to produce the two specific examples of possible Chargeback Assignments/Reports to leverage the existing parent_ems and the new MiqEnterprise.my_enterprise relationships.

Unless we have these examples there is no way for us and QE to assess correctness and we don't want to incur in an another case where the side-effect of these enable an unsupported report.

If it's not possible to leverage/test any of these (parent_ems / MiqEnterprise.my_enterprise) safest approach is to keep this list empty [].

[isimluk]

Well, that's sounds good. Being thorough and diligent tends to pay of.

Although, I have a feeling that the following information may have got lost in translation.

This is pr is a fix for customer bug and it has been verified in their environment. Actually, it is a fix for regression introduced in https://github.com/ManageIQ/manageiq/pull/10677/files#diff-9cccefb74f0723b14c5bae2ab4474a53R108

[simon3z]

This is pr is a fix for customer bug and it has been verified in their environment. Actually, it is a fix for regression introduced in https://github.com/ManageIQ/manageiq/pull/10677/files#diff-9cccefb74f0723b14c5bae2ab4474a53R108

@isimluk the fix for the customer bug was delivered with #14079 where the use-case is clear (ContainerProject chargeback).
I am asking to have a clear use-case for these two as well because for as much as we discussed this in the meeting we couldn't find a way to leverage these so far.

[isimluk]

@isimluk the fix for the customer bug was delivered with #14079 where the use-case is clear
(ContainerProject chargeback).

Aha. I was confident it was about both, ContainerImage and ContainerProject. I think we dive deep into ContainerImage on the last two calls. But I could be wrong, it has been a long time... Sorry for confusion then.

[simon3z]

@isimluk instead of closing this could you transform it in a patch to remove parent_ems (leaving an empty array []) so that we can continue the discussion?
(You should probably change the title as well, I just don't want to lose track of this discussion)

@miq-bot add_label providers/containers

[isimluk]

I am afraid to remove parent_ems, as I feel there is lack of clarity of what is going on. It seems we are going in circles, perhaps we can sit down and write a docs how this is supposed to work? What is the matrix of all the possible assignments? More importantly, it would be helpful to put down all the possible use-cases and requirements users may have.

Please note the parent_ems was added in 31fac86 and it seems to have to do something with docker labels assignments. The labels can be assigned to EMSes.

I am all confused. I don't want to remove it unless there is more clarity. In one of the previous messages, you said safest approach is to keep this list empty [].. I beg to differ on that one, I thinḱ the safest approach is change only stuff we have full understanding of.

Additionally, it feels to me that if we merge this pr, we could remove Default Container Image Rate. I see Default Container Image Rate as a mechanism to achieve the Enterprise level assignment. We could drop a whole lot of code, if we merged this. Although, in past, we went with Default Container Image Rate and there is no documentation why.