Bug 1382347 - Auto-tagging from name=value and name=VALUE labels breaks refresh
Summary: Auto-tagging from name=value and name=VALUE labels breaks refresh
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat CloudForms Management Engine
Classification: Red Hat
Component: Providers
Version: 5.6.0
Hardware: All
OS: All
unspecified
high
Target Milestone: GA
: 5.8.0
Assignee: Beni Paskin-Cherniavsky
QA Contact: Einat Pacifici
URL:
Whiteboard: container
: 1378477 (view as bug list)
Depends On:
Blocks: 1390696 1390698
TreeView+ depends on / blocked
 
Reported: 2016-10-06 12:34 UTC by Beni Paskin-Cherniavsky
Modified: 2020-01-17 16:00 UTC (History)
12 users (show)

Fixed In Version: 5.8.0.0
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1390696 1390698 (view as bug list)
Environment:
Last Closed: 2017-06-12 16:07:42 UTC
Category: Bug
Cloudforms Team: Container Management
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)

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 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.


Note You need to log in before you can comment on or make changes to this bug.