Bug 1382347

Summary:	Auto-tagging from name=value and name=VALUE labels breaks refresh
Product:	Red Hat CloudForms Management Engine	Reporter:	Beni Paskin-Cherniavsky <cben>
Component:	Providers	Assignee:	Beni Paskin-Cherniavsky <cben>
Status:	CLOSED CURRENTRELEASE	QA Contact:	Einat Pacifici <epacific>
Severity:	high	Docs Contact:
Priority:	unspecified
Version:	5.6.0	CC:	bazulay, cben, cpelland, dron, fsimonce, jdeubel, jfrey, jhardy, lavenel, mtayer, obarenbo, zgalor
Target Milestone:	GA	Keywords:	TestOnly
Target Release:	5.8.0
Hardware:	All
OS:	All
Whiteboard:	container
Fixed In Version:	5.8.0.0	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:
Clones:	1390696 1390698 (view as bug list)		Environment:
Last Closed:	2017-06-12 16:07:42 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	Bug
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	Container Management	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	1390696, 1390698

Description Beni Paskin-Cherniavsky 2016-10-06 12:34:39 UTC

Description of problem:

This is probably the underlying bug that a customer encountered in https://bugzilla.redhat.com/show_bug.cgi?id=1378477.

ContainerLabelTagMapping.rb does some internal sanitization (when it has to create new tags) which folds case, folds most punctuation and truncates to 50 chars.
If 2 entities have same label name and different values that collide after this sanitization, it breaks the current logic, causing exception which aborts refresh.

Note: auto-tagging is still dormant undocumented feature, but we'll soon add UI to activate it.

Version-Release number of selected component (if applicable):
All the way from 5.6.0 which first included auto-tagging code to current master.

How reproducible:
Always
Disclaimer: I did not run the below Steps to reproduce

Steps to Reproduce:
1. Create ContainerLabelTagMapping rule from label_name: 'name', label_value: nil (means map all values) to some category.
TODO: By the time this bug gets to QE, document how to do this via UI we'll add.
2. oc label pod1 name=value
3. oc label pod2 name=valuE

Pending unit tests for the bug in:
https://github.com/ManageIQ/manageiq/compare/master...cben:auto-tagging-collision-test
(cover case, punctuation and length collisions)

Actual results:
Refresh crashes mid-way with exception `ActiveRecord::RecordInvalid: Validation failed: Name has already been taken`.
(visible in log and in provider's "last refresh status")
Only one of the pods is successfully auto tagged.

Expected results:
Tagging and refresh should succeed.
We're still unsure if tagging should create 2 different tags with descriptions reflecting the original case, or it's OK to create 1 tag to which both labels will map.

Additional info:
Kubernetes labels are case-sensitive. `n=v` is different from `n=V` and `N=v`.
You can have `n=v` and `N=V` simultaneously on same entity; `n=v` and `n=V` are exclusive but are possible on separate entities.

Comment 2 CFME Bot 2016-10-10 18:46:12 UTC

https://github.com/ManageIQ/manageiq/pull/11806

Comment 3 CFME Bot 2016-10-28 13:11:15 UTC

New commit detected on ManageIQ/manageiq/master:
https://github.com/ManageIQ/manageiq/commit/90342958cdd2ffbb459fe3f7151b3607e6e8a0aa

commit 90342958cdd2ffbb459fe3f7151b3607e6e8a0aa
Author:     Beni Cherniavsky-Paskin <cben>
AuthorDate: Wed Oct 26 18:57:48 2016 +0300
Commit:     Beni Cherniavsky-Paskin <cben>
CommitDate: Wed Oct 26 19:57:14 2016 +0300

    ContainerLabelTagMapping: Don't insert mappings while mapping, find existing tags.
    
    Finding existing tag if already inserted hopefully fixes:
    https://bugzilla.redhat.com/show_bug.cgi?id=1382347
    https://bugzilla.redhat.com/show_bug.cgi?id=1382361
    
    This will map name=value and name=VALUE labels to same tag,
    which does not reflect Kubernetes case-sensitive semantics,
    but is probably OK (or even desirable?).
    
    Also not polluting the table - having only user-created mappings there -
    will be helpful for UI #11591.
    
    TODO: tests for retag_entity

 app/models/container_label_tag_mapping.rb          | 57 +++++++++---------
 app/models/ems_refresh/save_inventory_container.rb |  4 +-
 spec/models/container_label_tag_mapping_spec.rb    | 67 +++++++++++-----------
 3 files changed, 63 insertions(+), 65 deletions(-)

Comment 4 Federico Simoncelli 2016-10-28 15:00:39 UTC

Should this be in POST?

Comment 5 Federico Simoncelli 2016-10-28 15:01:53 UTC

ON_DEV is correct (not backported yet).

Comment 6 Beni Paskin-Cherniavsky 2016-10-30 13:38:05 UTC

Is it correct that this (and the sister bug 1382361) have directly Target Release = 5.7.0 instead of Target = cfme-future and getting 5.7 + 5.6 clones?

Barak, I initially set target = 5.7, then corrected to future + 5.7? flag, then you moved back to 5.7 — was that intentional?

When will the BZ clones (at least for 5.6) get created?  After backport or before?  Sorry, I'm confused on the current workflow...

Comment 7 CFME Bot 2016-11-01 16:26:15 UTC

New commit detected on ManageIQ/manageiq/euwe:
https://github.com/ManageIQ/manageiq/commit/bbcda770928a952d649f8f3df48169294127e93b

commit bbcda770928a952d649f8f3df48169294127e93b
Author:     Gregg Tanzillo <gtanzill>
AuthorDate: Fri Oct 28 09:09:21 2016 -0400
Commit:     Oleg Barenboim <chessbyte>
CommitDate: Tue Nov 1 12:21:35 2016 -0400

    Merge pull request #11806 from cben/auto-tagging-noinsert
    
    Rewrite of auto-tagging, fixing multiple bugs
    (cherry picked from commit 9800efa91dfffd0f34314a27216ec8fe0c233b38)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1382347
    https://bugzilla.redhat.com/show_bug.cgi?id=1382361

 app/models/container_label_tag_mapping.rb          | 132 +++++------
 app/models/ems_refresh/save_inventory_container.rb |  35 ++-
 .../kubernetes/container_manager/refresh_parser.rb |  32 ++-
 .../openshift/container_manager/refresh_parser.rb  |  12 +-
 spec/models/container_label_tag_mapping_spec.rb    | 253 ++++++++++++++-------
 .../container_manager/refresh_parser_spec.rb       |  14 +-
 .../kubernetes/container_manager/refresher_spec.rb |  46 +++-
 .../container_manager/refresh_parser_spec.rb       |   3 +-
 8 files changed, 341 insertions(+), 186 deletions(-)

Comment 8 Beni Paskin-Cherniavsky 2016-11-01 16:30:57 UTC

Backported to 5.7.

Comment 11 Mooli Tayer 2016-11-15 16:59:38 UTC

*** Bug 1378477 has been marked as a duplicate of this bug. ***

Comment 12 Einat Pacifici 2017-03-05 10:29:16 UTC

Verified. 
Created mapping rules and then refreshed provider. 
The refresh completed successfully and all objects were assigned correct labels according to mapping.