Fix chargeback for container Images with rate assigning by docker label #12851

zeari · 2016-11-24T21:58:46Z

This includes work on persisting openshift container images as well as fixes to ContainerImageChargeback reports

Fix for image names under filter:

@simon3z @enoodle
@miq-bot add_label providers/containers, chargeback, wip

miq-bot · 2016-11-27T15:56:24Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

zeari · 2016-11-27T15:59:53Z

app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb

-          :docker_labels         => parse_identifying_attributes(docker_metadata[:Config][:Labels],
-                                                                 'docker_labels', "openshift"),
+          :docker_labels         => docker_labels,
+          :tags                  => map_labels('ContainerImage', labels + docker_labels)


Allow mapping of both kubernetes labels and docker labels to tags
@simon3z

@zeari you can't merge labels and docker_labels... what if they have the same key but different values? Which one should win? You should keep them separate.

@simon3z Im only merging them when sending them to be mapped into tags. This would allow users to turn docker labels into tags for ContainerImages. I think its unlikely that someone would set a rule on something that would be common for both docker-labels and kubernetes labels.
@cben What would happen in the above case which they have the same key but different values?

@simon3z Im only merging them when sending them to be mapped into tags.

@zeari it's not about how many times or where. If you do it even once you still have to answer the questions above (what if they have the same key but different values? Which one should win? why?).

Is this used by the Chargeback for Images? (IIRC we were basing it on labels)

Let's remove everything that could cause issues and that it's not vital for the use-cases we identified.

Ooh, docker labels, shiny :)

[First, are these concatenable? Is docker_labels array of {:name, :value} hashes?]
I think the mapping code would map both. With same name and 2 values, you might get 2 tags.
Is that what we want? The Right Thing is probably a new mapping column, letting user define independent mappings for k8s labels vs docker labels (and maybe open it to other kinds of custom attrs). Though I'm not excited about making auto-tagging more complicated :-\

Are you also adding docker labels as new virtual custom-attr columns in reports?
There, would you merge a k8s label & docker label of same name into one "column"?
We probably should be consistent between direct label querying / mapped tag querying.

Are you covering docker labels only on images?
https://docs.docker.com/engine/userguide/labels-custom-metadata/ lists several entities.
P.S. This PR does way too much, but you knew that :)

[First, are these concatenable? Is docker_labels array of {:name, :value} hashes?]
I think the mapping code would map both. With same name and 2 values, you might get 2 tags.

We could filter the list...

Is that what we want? The Right Thing is probably a new mapping column, letting user define independent mappings for k8s labels vs docker labels (and maybe open it to other kinds of custom attrs). Though I'm not excited about making auto-tagging more complicated :-\

I feel doing this is a whole lot easier than adding label rate assignment code which is turning out as a lot but I may be wrong. Besides, tags are much more integrated into various miq features so this will solve multiple use case scenarios

Are you covering docker labels only on images?

yes

Mitigating note: docker labels recommend com.domain.reversed.label format — if followed, collisions with k8s labels should be rare.

I think the mapping code would map both.

Correction: UI creates categories with single_value=true, so this either won't work, or will be "inconsistent" in DB. Can fix, but let's understand behavior first.

I feel doing this is a whole lot easier than adding label rate assignment code which is turning out as a lot but I may be wrong. Besides, tags are much more integrated into various miq features so this will solve multiple use case scenarios

Fair enough. 👍 to smaller euwe changes and 👍 to less chargeback-special code.
Do we have a longer-term plan?

UX: will we show k8s labels / docker labels together as just "labels", or will we separate them?

Will we want independent k8s labels / docker labels mappings?
I'm concerned that if we allow ambiguous "either one" mappings now, and want to separate later, a later migration won't be able to know which was meant.
In any case, we can't do schema changes in euwe.

Will we eventually have direct MiqExpression (virtual column) support for docker labels? Will they show merged with k8s labels or separate?

Will chargeback rate assignment eventually support [any kind of] labels?
Is there any hope rate assigment will become MiqExpression based to stop playing catch-up?

[aside: Chargeback is not just technical & feature debt, it's a continuous attractor of new debt :-(. IMHO we must allocate more time to gradually make it more based on the core infrastructure...]

enoodle · 2016-11-28T14:34:19Z

@zeari, @simon3z If this PR replaces #12711 then we should add blocker, bug and euwe/yes labels here too.

miq-bot · 2016-11-29T15:01:49Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

simon3z · 2016-12-01T10:11:07Z

Will we want independent k8s labels / docker labels mappings?

@cben @zeari yes

zeari · 2016-12-05T13:45:04Z

@gtanzillo @lpichler Adding tests, please review.
cc @simon3z

zeari · 2016-12-05T13:50:58Z

@miq-bot remove_label wip

lpichler · 2016-12-06T17:07:59Z

@zeari
What is archived ? Is this expected output ?

zeari · 2016-12-06T17:10:36Z

What is archived ? Is this expected output ?

Yes or No depending on if this project was deleted or not - We save old entities\metrics for chargeback.

zeari · 2016-12-06T17:33:16Z

What is archived ? Is this expected output ?

Fixed

zeari · 2016-12-06T17:33:37Z

@miq-bot add_label euwe/yes

simon3z · 2016-12-06T17:34:33Z

@enoodle please review this as well (it contains some of your PRs and we want to make sure they're integrated properly)

miq-bot · 2016-12-06T19:52:41Z

Checked commits zeari/manageiq@43d581e~...f42f061 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
25 files checked, 20 offenses detected

app/controllers/chargeback_controller.rb

❗ - Line 630, Col 9 - Style/Next - Use next to skip iteration.
❗ - Line 751, Col 7 - Style/AccessorMethodName - Do not prefix reader method names with get_.
❕ - Line 243, Col 121 - Metrics/LineLength - Line is too long. [138/120]
❕ - Line 762, Col 121 - Metrics/LineLength - Line is too long. [125/120]

app/helpers/container_image_helper/textual_summary.rb

❕ - Line 8, Col 121 - Metrics/LineLength - Line is too long. [146/120]

app/helpers/ui_constants.rb

❗ - Line 527, Col 33 - Style/IndentationWidth - Use 2 (not 1) spaces for indentation.

app/models/chargeback.rb

❕ - Line 24, Col 121 - Metrics/LineLength - Line is too long. [144/120]

app/models/chargeback/rates_cache.rb

❕ - Line 11, Col 121 - Metrics/LineLength - Line is too long. [128/120]

app/models/container_image.rb

❕ - Line 24, Col 121 - Metrics/LineLength - Line is too long. [128/120]
❕ - Line 25, Col 121 - Metrics/LineLength - Line is too long. [142/120]

app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb

❕ - Line 174, Col 7 - Metrics/AbcSize - Assignment Branch Condition size for parse_openshift_image is too high. [29.63/20]

app/models/metric/chargeback_helper.rb

❕ - Line 25, Col 121 - Metrics/LineLength - Line is too long. [140/120]

app/models/mixins/assignment_mixin.rb

❕ - Line 51, Col 121 - Metrics/LineLength - Line is too long. [134/120]

app/services/container_dashboard_service.rb

❕ - Line 70, Col 121 - Metrics/LineLength - Line is too long. [133/120]

app/views/report/_form_filter_chargeback.html.haml

⚠️ - Line 96 - Line is too long. [182/160]

spec/models/chargeback_container_image_spec.rb

❗ - Line 128, Col 15 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.
❗ - Line 135, Col 17 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.
❗ - Line 176, Col 65 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❕ - Line 11, Col 121 - Metrics/LineLength - Line is too long. [130/120]

spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb

❕ - Line 203, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for assert_specific_container_image is too high. [20.02/20]

h-kataria · 2016-12-06T19:54:08Z

UI changes look good.

zeari · 2016-12-08T15:02:29Z

@gtanzillo @lpichler I cant chery pick this into euwe without conflicts and Im having a hard time mapping out the PR's this depends on. is it ok to have euwe specific commits?( #13030)

gtanzillo · 2016-12-08T16:17:36Z

@zeari Yes, that's the way to go what it becomes too difficult to cherry-pick.

chessbyte · 2016-12-13T16:03:34Z

Backported to Euwe via #13030

miq-bot added core/chargeback providers/containers wip labels Nov 24, 2016

zeari force-pushed the combined_cb_images branch from ab9eb63 to c8dca7c Compare November 27, 2016 15:54

zeari commented Nov 27, 2016

View reviewed changes

zeari force-pushed the combined_cb_images branch from c8dca7c to 508d8fb Compare November 29, 2016 12:40

zeari force-pushed the combined_cb_images branch 6 times, most recently from a68f505 to a2aac99 Compare December 5, 2016 13:32

zeari changed the title ~~[WIP] Fix chargeback for container Images with rate assigning by docker label~~ Fix chargeback for container Images with rate assigning by docker label Dec 5, 2016

miq-bot removed the wip label Dec 5, 2016

zeari force-pushed the combined_cb_images branch 2 times, most recently from dac3f66 to ac28c4c Compare December 5, 2016 14:06

chessbyte added bug ui labels Dec 5, 2016

chessbyte assigned simon3z Dec 5, 2016

Erez Freiberger and others added 5 commits December 6, 2016 12:08

Container Images: collect images from OpenShift

43d581e

adding openshift image data to UI

b611ad2

adding labels and custom attributes to container image

546b7b0

adding docker_lables

44aabaa

Disconnect container images

594016a

zeari force-pushed the combined_cb_images branch from 80ed182 to bfaecdf Compare December 6, 2016 17:07

zeari force-pushed the combined_cb_images branch from bfaecdf to e83ebad Compare December 6, 2016 17:21

miq-bot added the euwe/yes label Dec 6, 2016

zeari force-pushed the combined_cb_images branch from 55c17e8 to f42f061 Compare December 6, 2016 19:50

Ari Zellner added 3 commits December 6, 2016 21:50

Apply a specific default rate to CB for container images

6f4f7c6

Add rate assignment by docker labels to Chargeback for container images

31fac86

Fix rubocop warnings

f42f061

zeari closed this Dec 6, 2016

zeari reopened this Dec 6, 2016

zeari mentioned this pull request Dec 6, 2016

[EUWE] Fix chargeback for container Images with rate assigning by docker label #13030

Merged

gtanzillo merged commit e268895 into ManageIQ:master Dec 6, 2016

chessbyte assigned gtanzillo and unassigned simon3z Dec 7, 2016

chessbyte added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 7, 2016

enoodle mentioned this pull request Dec 7, 2016

Container Images: Collect container images from Openshift #10692

Closed

lpichler mentioned this pull request Dec 7, 2016

Prioritize rate with tag of VM when selecting from more rates #12534

Merged

zeari mentioned this pull request Dec 8, 2016

Add tags as columns in containerImageChargeback #12305

Closed

chessbyte added the blocker label Dec 12, 2016

chessbyte added euwe/backported and removed euwe/yes labels Dec 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix chargeback for container Images with rate assigning by docker label #12851

Fix chargeback for container Images with rate assigning by docker label #12851

zeari commented Nov 24, 2016 •

edited

Loading

miq-bot commented Nov 27, 2016

zeari Nov 27, 2016

simon3z Nov 28, 2016

zeari Nov 29, 2016

simon3z Nov 29, 2016

cben Nov 29, 2016

zeari Nov 29, 2016

cben Nov 29, 2016

enoodle commented Nov 28, 2016

miq-bot commented Nov 29, 2016

simon3z commented Dec 1, 2016

zeari commented Dec 5, 2016

zeari commented Dec 5, 2016

lpichler commented Dec 6, 2016

zeari commented Dec 6, 2016

zeari commented Dec 6, 2016

zeari commented Dec 6, 2016

simon3z commented Dec 6, 2016

miq-bot commented Dec 6, 2016

h-kataria commented Dec 6, 2016

zeari commented Dec 8, 2016 •

edited

Loading

gtanzillo commented Dec 8, 2016

chessbyte commented Dec 13, 2016

Fix chargeback for container Images with rate assigning by docker label #12851

Fix chargeback for container Images with rate assigning by docker label #12851

Conversation

zeari commented Nov 24, 2016 • edited Loading

miq-bot commented Nov 27, 2016

zeari Nov 27, 2016

Choose a reason for hiding this comment

simon3z Nov 28, 2016

Choose a reason for hiding this comment

zeari Nov 29, 2016

Choose a reason for hiding this comment

simon3z Nov 29, 2016

Choose a reason for hiding this comment

cben Nov 29, 2016

Choose a reason for hiding this comment

zeari Nov 29, 2016

Choose a reason for hiding this comment

cben Nov 29, 2016

Choose a reason for hiding this comment

enoodle commented Nov 28, 2016

miq-bot commented Nov 29, 2016

simon3z commented Dec 1, 2016

zeari commented Dec 5, 2016

zeari commented Dec 5, 2016

lpichler commented Dec 6, 2016

zeari commented Dec 6, 2016

zeari commented Dec 6, 2016

zeari commented Dec 6, 2016

simon3z commented Dec 6, 2016

miq-bot commented Dec 6, 2016

h-kataria commented Dec 6, 2016

zeari commented Dec 8, 2016 • edited Loading

gtanzillo commented Dec 8, 2016

chessbyte commented Dec 13, 2016

zeari commented Nov 24, 2016 •

edited

Loading

zeari commented Dec 8, 2016 •

edited

Loading