Skip to content
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

Add filter_by_tags option #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martialblog
Copy link

Hi,

this PR adds a filter_by_tags option which allows the inventory instances to be filtered by one or more tags.

Fixes #115

 - This option allows the inventory instances to be filtered by one or more tags.
@martialblog
Copy link
Author

Using the existing get_filters() function wasn't possible since it only applies to the CS list API, as far as I can tell.

Thus I think the only way is to check all instances the API returned for their tags.

Suggestions for improvements are welcome.

@rvalle
Copy link
Collaborator

rvalle commented Dec 12, 2022

Normally I have been using tags to define groups, but I think this implementation adds value.

Ultimately you would be able to use tags to cherry pick what goes into the inventory.

@rvalle
Copy link
Collaborator

rvalle commented Dec 12, 2022

However, there was another addition to the inventory plugin pending to implement because it was dependent on ACS api changes that were being implemented.

That was re. the ability to group by VPC. see #106

@martialblog would you be confortable if we abuse this PR to include support for VPC groups?

@rvalle
Copy link
Collaborator

rvalle commented Dec 12, 2022

My guess is that adding support for groups by VPC would require the following addition too:

INVENTORY_NORMALIZATION_J2 = '''
---
instance:

 .....

 {% if instance.vpc is defined %}
   vpc: {{ instance.vpc }}
 {% endif %}

Unfortunately I need to update my ACS to the required version in order to fully test this works. but if you can, I appreciate.

@martialblog
Copy link
Author

@rvalle Thanks for the feedback. Cherry picking the inventory was the use case that I had in front of me.

I can add the instance.vpc in a Commit, not sure if I be able to test it anytime soon though. But seems like a minor change

@rvalle
Copy link
Collaborator

rvalle commented Dec 13, 2022

Ok, I just reviewed the VPC implementation in ACS and this is actually per NIC.
If the network is part of a VPC the vpcname is returned with the nic attributes.
As VMs can have multiple NICs that means the VPCs would be just like tags, as VMs can belong to multiple.
Sorry for the mess, maybe we can leave this for its own PR, as it seems more complicated than I anticipated.

@rvalle
Copy link
Collaborator

rvalle commented Dec 13, 2022

Using the existing get_filters() function wasn't possible since it only applies to the CS list API, as far as I can tell.

Have you tried this route?

The current implementation builds the filter in the query to ACS, and there is the possibility to add a filter do deal with tags.

Did you notice?

https://cloudstack.apache.org/api/apidocs-4.17/apis/listVirtualMachines.html

it is possible to filter by tags when querying virtual machines, this is what we need.

I can see that the current add_filter implementation is designed to deal with entities for witch and ID needs to be worked out. For example, the filter expect an domainid, but you want to filter by domain name or id. Then there is an intermediate query to deal with that indirection, all domains are queried and the first domain ID for which NAME or ID matches any of the returned domains is used.

In the case of tag filters we don't need any indirection, because the query is not based on another entity but a direct value, so add_filter is not applicable because we already have the value we are going to use for filtering: the tag dictionary.

I think a bit of refractory is required to differentiate between adding a filter that involves an entity, and a filter that deals with a direct value.

perhaps we could rename the current add_filter to add_entity_filter and create another function add_value_filter where no entity is queried first to guess which ID should be used.

What do you think?

@resmo
Copy link
Member

resmo commented Dec 13, 2022

I came across a nice implementation of a "generic" filter which filters out after the api query: https://github.com/vultr/ansible-collection-vultr/blob/main/plugins/inventory/vultr.py#L119

IMHO we should implement the additional filter_by_x filters to be applied for querying the API to return a subset of hosts.

@martialblog
Copy link
Author

Hi, I don't have a ACS API in front of me at the moment to refactor this. The filter_by_x option sounds good.

@resmo resmo force-pushed the master branch 2 times, most recently from 2e1f31b to 52fcebc Compare December 2, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inventory plugin filter by tags
3 participants