ngine-io / ansible-collection-cloudstack

CloudStack Ansible Collections
https://galaxy.ansible.com/ngine_io/cloudstack
GNU General Public License v3.0
21 stars 27 forks source link

Add filter_by_tags option #117

Open martialblog opened 1 year ago

martialblog commented 1 year ago

Hi,

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

Fixes #115

martialblog commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.