-
Notifications
You must be signed in to change notification settings - Fork 897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Azure labeling and tagging support #17212
Conversation
c50e2bc
to
a488dd1
Compare
@@ -23,7 +23,7 @@ class ContainerLabelTagMapping < ApplicationRecord | |||
|
|||
require_nested :Mapper | |||
|
|||
TAG_PREFIXES = ['/managed/amazon:', '/managed/kubernetes:'].freeze | |||
TAG_PREFIXES = %w(_all_ amazon azure kubernetes).map { |name| "/managed/#{name}:" }.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the _all_
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are /managed/_all_:...
categories created anywhere?
It's a good idea to reserve a non-provider-specific prefix, currently mappings for "foo" label for all types get a /managed/kubernetes::foo
category (ALL_PREFIX
in UI — https://github.com/ManageIQ/manageiq-ui-classic/pull/3697/files#diff-95b50e997ace14a4e385c9a176eea25fR31).
When you actually change ALL_PREFIX
, you might need a migration for existing categories, and tags under them?
Also, remember these share a namespace with regular user-created categories. _all_
is too nondescriptive IMHO; perhaps something like mapped
?
@AlexanderZagaynov Let's skip the Once that's done you can remove the WIP label and we can get this merged. |
@miq-bot assign @blomquisg |
@miq-bot add_label enhancement, providers/cloud, blocker, fine/yes, gaprindashvili/yes |
a488dd1
to
d6a8720
Compare
@AlexanderZagaynov - I think we should get this PR in first and then focus on related PRs. If you remove the |
d6a8720
to
ccfe3c0
Compare
@@ -60,6 +60,7 @@ def self.references_to_tags(tag_references) | |||
private | |||
|
|||
def map_label(type, label) | |||
type = 'VmAzure' if type == 'Vm' && label[:source] == 'azure' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be a bad place to put it, there is only generic code. How are we handling Amazon @cben ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it in the collecting code, but there will be two places at least, and since this method looks like some gateway method for all related incoming data processing, I found it better to put this crutch code here.
Unfortunately all this label tag mapping code looks like bunch of a crutches for a crutches sitting on the crutches. I found it impossible to do a deep refactoring because of backporting needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't yet have 3 All/Vm/VmAzure levels of granularity. We have just All (nil
) and Vm, which this change is trying to change.
I agree though that hardcoding here isn't nice. It'd be very hard to guess this is happening when looking at caller code (it's already confusing that "type" is usually but not always a model name...).
I suggest modifying the signature to take a list of types a mapping can match. I suggest including nil
in it, for clarity. It may be easier to add new method instead of modifying existing method across repos:
def map_types(types, label) # naming is hard
types.collect_concat do |type|
map_name_type_value(label[:name], type, label[:value])
end
end
# TODO: use map_types() everywhere, remove this when unused
def map_label(type, label)
map_types([type, nil], label)
end
Then Azure refresh would call map_types(['VmAzure', 'Vm', nil], ...)
, and Amazon refresh map_types(['VmAmazon', 'Vm', nil], ...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actially this method called as map_label('Vm', { :name => 'label_name', :value => 'label_value', :source => 'azure', :section => 'labels' })
for Azure, and with :source => 'Amazon'
for Amazon. You proposal just introduces another crutch for this nightmarish code. I don't mind of a full refactoring, but not in the case we need that code to be backported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I just started writing that this is all a crutch to encode 2 fields — provider and type — in one column...
So, do you need additional granularity backported?
I strongly feel extra granularity is not crucial feature. So in existing releases, one would be able to map "Vm", of both providers — what's the harm in that? (IMHO one doesn't even need that, "<All>" mappings are good enough in practice!)
Let's stop adding crutches "because backporting". That's how we (well, I 😳) built this mess. Let's do minimal workable backporting, and instead refactor master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was your proposal to distinct providers using Vm
for Amazon and VmAzure
for Azure, remember?
I'm going to remove that line in favour of specific change in the Azure collecting code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore everything I said above
I misread the PR intention :-( After a chat, turns out we both are not interested in adding 3rd level of granularity.
OK, so IIUC the goal here is just Vm
= amazon vm, AzureVm
= azure vm, done this way so it can be backported without any migration. LGTM, though consider just passing "AzureVm" from azure refresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can, could we fill it in refresh, this is really a bad place to add it. Also it should be the right STI which is ...Providers::Azure::Vm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be the right STI which is ...Providers::Azure::Vm
It could be, not a must. these "type" strings are not really model names, just arbitrary string that should match between mapping created by UI and refresh calling map_labels().
(They started as base model names — "ContainerFoo" — which is not not always the STI type of the actual record being tagged. And I think amazon already departed from that to distinguish vm vs image?)
So you could choose ...Providers::Azure::Vm
(or anything you want), not sure that would be a win or more misleading, I'm good with anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that line was removed in favour of https://github.com/ManageIQ/manageiq-providers-azure/pull/231/files#diff-1f54967e2f7cd1793ef76031aa337270R287
app/models/classification.rb
Outdated
cond << c.id | ||
end | ||
c.class.in_region(region_id).exists?(cond) | ||
validates :description, :uniqueness => { :scope => %i(parent_id tag_id) }, :if => proc { |c| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexanderZagaynov can you describe why we need this? Amazon works without these changes, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description uniqueness previously was validated within parent_id
+ in_region
scope. If
condition was probably created because of bizarre in_region
scope, which differentiated through id
s range, not through some designated field.
My change just adds tag_id
to uniqueness scope, and introduces slight if
proc code refactoring using standard AR methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to defend the existing validation (:wink: I've stared at this for 2 years and I'm still not sure how to read it! :worried:), but please explain the change.
Wouldn't scoping on tag_id
too allow as many Classifications with same description (but different Tag) as you want, effectively losing uniqueness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cben to answer your question about uniqueness scope we need to find out first why do we need that uniqueness in the whole. I can't say that. But I can say that since tag
should be unique for each provider, and classification
should refer to the tag - it's impossible to have mapping for both Amazon
and Azure
to the same classification name without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha. Not sure I'm understand right. You mean we might want to simultaneously map say:
foo
, AzureVm -> "Foo" classification, /managed/azure:vm:foo
foo
, AmazonImage -> "Foo" classification, /managed/amazon:template:foo
(I'm probably off in exact type & resulting tag name)
?
So indeed that would allow user to see in UI:
foo=bar
azure vm tag mapped to a "Foo : bar" MIQ tag,
foo=baz
amazon image tag mapped to a "Foo : baz" MIQ tag.
However, that's a very misleading situation. It'd look like tags in same category, but actually there'd be 2 distinct categories with same name! Then when you'd want say a policy or report by tag in Foo category, you'd see 2 Foo categories, looking exactly the same, and would have to choose one...
If you really want to achieve simultaneous several types -> one category mappings, we should modify UI to not include mapping type in tag name, and to allow several ContainerLabelTagMapping to point to same category tag_id. (Mapper has no problem with that.)
The problem doing that in UI is "garbage collection" of categories — currently I delete category with the mapping, would have to do a check to see if it's still used...
BTW any change in how we construct category names probably needs data migration.
But I want to question do we really need this?
If one wants 2 things mapped to one category, just do an "<All>" mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can't. It will not work without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain full use case that doesn't work with current validation? (what mappings and what classifications are involved?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, probably my https://github.com/ManageIQ/manageiq/pull/17212/files#r181664339 wasn't clear enough. I did draw the picture to illustrate what's happeing inside.
Without that change we will not be able to add two classifications with the same description for a different providers, and all the labels will stick to the single mapping entry only, even from the different providers.
P.S. don't blame me for such weird data structure design, it was here already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, very clear diagram!
So I guessed right, and my response above applies. TLDR, trying to be clearer:
it's an existing limitation that you can't create 2 mappings with specific types onto one category.
The workaround is use a single "<All>" type mapping (labeled_resource_type=nil). Such mapping would map amazon vms, azure vms, but also amazon images, container nodes and other stuff — all onto a single category. Which doesn't hurt too much.
-
I think the workaround is acceptable, at least for backporting.
-
I don't think this change is right way to fix the limitation. It'd still map onto 2 categories, that look same but are actually distinct, causing confusion and usability problems (eg. can't know which one to pick for a report).
- In cases where mapping to 2 categories is useful, user can get same functional (but not visual) effect by using distinct descriptions (say "RedHat Location (AWS)" vs "RedHat Location (Azure)").
-
If we need to really fix this limitation, it'll take extra changes in UI — but the result would be several mappings to single category Classification/Tag, so no uniqueness problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ccfe3c0
to
4f19ed5
Compare
4f19ed5
to
29095d7
Compare
Checked commit AlexanderZagaynov@29095d7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
LGTM. I think this is safe to merge first? |
Azure labeling and tagging support (cherry picked from commit 585990b) https://bugzilla.redhat.com/show_bug.cgi?id=1568807
Gaprindashvili backport details:
|
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1493041
Related:
ManageIQ/manageiq-providers-azure#231
ManageIQ/manageiq-providers-azure#229
ManageIQ/manageiq-ui-classic#3697