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

Added filters option to openstack inventory plugin. #51401

Closed
wants to merge 3 commits into from

Conversation

Turmio
Copy link

@Turmio Turmio commented Jan 28, 2019

SUMMARY

Added support for OpenStack SDK filters option to inventory plugin.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

OpenStack inventory plugin

ADDITIONAL INFORMATION

I use our case as example:

We want to create automatically environment from pull request (multiple virtual machines, networks etc.). Our and 3rd party software are installed and configured with Ansible. We use Heat templates to describe resources and use unique identifier for each environment.

With filters implementation we can use metadata to get only those virtual machine belonging to one pull request:

filters:
  metadata:
    stack_identifier: 'PR-123'

Signed-off-by: Turo Soisenniemi <turo.soisenniemi@outlook.com>
@ansibot
Copy link
Contributor

ansibot commented Jan 28, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. inventory Inventory category needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. openstack support:community This issue/PR relates to code supported by the Ansible community. labels Jan 28, 2019
@opendev-zuul
Copy link

opendev-zuul bot commented Jan 28, 2019

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

  • openstacksdk-ansible-devel-functional-devstack : RETRY_LIMIT in 1m 09s

@opendev-zuul
Copy link

opendev-zuul bot commented Jan 29, 2019

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

  • openstacksdk-ansible-devel-functional-devstack : RETRY_LIMIT in 1m 11s

@Turmio
Copy link
Author

Turmio commented Jan 29, 2019

Okay, it seems that pull request description is evaluated in openstack tests:
2019-01-29 03:02:08.227497 | localhost | "msg": "The task includes an option with an undefined variable. The error was: 'pull_request_id' is undefined\n\nThe error appears to have been in '/var/lib/zuul/builds/122999eb97a043d08f3e5afc25053dda/trusted/project_1/git.openstack.org/openstack-infra/zuul-jobs/roles/add-fileserver/tasks/main.yaml': line 18, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: Add fileserver to inventory\n ^ here\n"

I'll change the example and retest.

@Turmio
Copy link
Author

Turmio commented Jan 29, 2019

recheck

@opendev-zuul
Copy link

opendev-zuul bot commented Jan 29, 2019

Build succeeded (third-party-check pipeline).

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 6, 2019
@Turmio
Copy link
Author

Turmio commented Feb 11, 2019

ready_for_review

@Turmio
Copy link
Author

Turmio commented Apr 2, 2019

bot_status

@ansibot
Copy link
Contributor

ansibot commented Apr 2, 2019

Components

lib/ansible/plugins/inventory/openstack.py
support: community
maintainers: Shrews cloudnull dagnello emonty evrardjp juliakreger mnaser odyssey4me rcarrillocruz

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@ITD27M01
Copy link

ITD27M01 commented Apr 4, 2019

Hi @Turmio
What is the reason to use the "filters" instead of just "groups" metadata property? The current implementation of plugin has support for such property and you can use it to grouping the hosts within the plays.

For example the "metadata": {"groups": "linux, PR-123, development"} will add the host (OS server) to three groups and you can limit plays or use the host patterns such hosts: linux:&{{ pool_request_id }}

@ansibot
Copy link
Contributor

ansibot commented Apr 4, 2019

cc @gtema
click here for bot help

@Turmio
Copy link
Author

Turmio commented Apr 4, 2019

Hi @ITD27M01,

"filters" limits actual query done to the OpenStack instance. Actually we are using git commit sha as identifier. Adding it to filters, we can get only virtual machines that pass defined filters and use Ansible as in any static inventory. For example all matches only to those VMs.

Workflow:

  1. User pushes code to git repository
  2. CI creates new environment with Ansible + heat template and writes to git sha to ansible controller
  3. CI starts system installation process
  4. CI runs E2E tests
  5. CI destroys enviroment

Example openstack.yml:

---
plugin: openstack
expand_hostvars: false
keyed_groups:
  - prefix: ''
    key: openstack.metadata.vm_role
cache: true
cache_plugin: jsonfile
cache_connection: /tmp/inventory-cache/
private: true
filters:
  metadata:
    ci_stack: '{{ lookup("file","/etc/openstack/git.sha") }}'

This provides horizontally scalable environment without editing our playbooks for openstack. All OpenStack magic is done in inventory files.

@ITD27M01
Copy link

ITD27M01 commented Apr 5, 2019

Hello @Turmio

Firstly, All this stuff already is done by "group/groups" metadata keys and ansible hostpatterns:

https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/openstack.py#L272

https://docs.ansible.com/ansible/latest/user_guide/intro_patterns.html

In your case "vm_role" and "ci_stack" is overengineering and can be achieved by setting the "group" metadata key to the following:

"metadata": {"groups": "<vm_role>, <ci_stack>"}

In this way you will get the groups for "<vm_role>" and "<ci_stack>" in ansible inventory.

Secondly, all metadata keys alredy used for host groups (with "meta-" prefix):

https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/openstack.py#L306

So, these keys can be used for your task.

@Turmio
Copy link
Author

Turmio commented Apr 5, 2019

Hi @ITD27M01, it does not do the same. Filters are passed to OpenStackSDK which does the filtering before ansible can access any host.

I looked up the line you referenced and it gets servers from here:
https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/openstack.py#L206

This is line where I added filters. This change also improves performance because filtering is done server side before filtering with groups as you suggested. We have potentially hundreds of VMs up same time and need to access about 5-10.

I don't think using openstack sdk filters is overengineering as it is openstack way of filtering hosts. aws_ec2 inventory source also has filters:
https://docs.ansible.com/ansible/latest/plugins/inventory/aws_ec2.html

We cannot (or don't want to) use host patterns. We have product where we have defined groups, add-hoc commands etc. which should work on any environment without modifying host patterns.

@ITD27M01
Copy link

ITD27M01 commented Apr 5, 2019

@Turmio
Here is a false assumption. search_hosts you are using is doing the call to the list_hosts with the same arguments:

https://opendev.org/openstack/openstacksdk/src/branch/master/openstack/cloud/inventory.py#L76

So, all the work run on the client side in any way and performance will not be improved.

The nova API has limited filters for GET /servers API request:

https://developer.openstack.org/api-ref/compute/?expanded=list-servers-detail#list-servers

and metadata hash not in these keys.

I thought about improving performance too, but the change made should be more complex. Moving the host grouping deeper into the library (openstacksdk) will cause confusion and won't do any good - host grouping is already implemented in the ansible plugin.

The good way to use such filters are tags. They can be used to split large environment inside the project to small environments.

But there should be support for acceptable filters on openstacksdk side and after that, the filtering can be implemented in ansible plugin.

@Turmio
Copy link
Author

Turmio commented Apr 5, 2019

@ITD27M01, I'll admit that I did not check where filters are executed. I assumed it was server side based on my experience that it feels faster.

This change is not about grouping, but using openstack filters to limit servers before grouping. If you can limit servers before grouping I think performance gain starts from here: https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/openstack.py#L223

Also I think openstacksdk "filters" allow more complex scenarios than grouping only. Here is the actual filtering done:
https://github.com/openstack/openstacksdk/blob/9675c1fb0b065017a0fadf75f52d8193cce7dd24/openstack/cloud/_utils.py#L152

I missed your comment about automatic meta groups. That would us to define only filters and use meta groups only. Wouldn't that be what most users want? Use metadata as groups and don't want to see unnecessary servers and write host patterns.

@ITD27M01
Copy link

ITD27M01 commented Apr 5, 2019

@Turmio
Generally, I don't mind filters. But your choice is more of an illusion than a solution. In addition to groups, there will be a new level of abstraction that will make simple things difficult. I want the filters also but I offer to use the "native" filtering capability of Nova API (server side).

For plays with hostpattern 'all' the best choice is to use the '--limit' during ansible run. This limit support the same patterns:

ansible-playbook play.yaml --limit linux:\!vm_role:\&ci_stack

https://docs.ansible.com/ansible/latest/user_guide/playbooks_best_practices.html

@Turmio
Copy link
Author

Turmio commented Apr 7, 2019

Hello @ITD27M01

While I agree that server side API is better I disagree using it in this plugin. Inventory plugin has only one requirement and that is to openstacksdk. So I think it is natural to use "filters" from openstacksdk.

Also this solution is not illusion. It is generally good practice to filter as early it can be implemented. Filters and groups are implementation to different problem. "Filters" remove completely hosts that does not belong to that inventory and "groups" maps hosts to desired groups.

I don't understand how does this make simple things difficult. There is no need to use "filters" as it is optional and it adds capability to filter without creating unnecessary groups.

I know how to use host patterns. What I am trying to tell you is that we have product which provides ready to use playbooks. All user need is to select or create is an inventory. We have (currently) dozens of users. I don't want to train each of them (and future) users to use different limits with OpenStack than when using same playbooks and ad-hoc commands in different environment. That kind of destroys one of the reasons using Ansible as configuration management tools.

About using Nova API:
That would introduce new dependency and from your link can be seen that different version of Nova support different parameters for filtering. I think right way is to use "filters" from openstacksdk and add Nova API capability to sdk. That way every other user would also benefit from server side filtering and ansible plugin would not have another dependency.

About performance:
I will try tomorrow is there any performance difference and report it.

@ITD27M01
Copy link

ITD27M01 commented Apr 7, 2019

Hi @Turmio
I suggest taking benefits of both solutions. The first and big profit of filtering the whole inventory (anyway) is memory saving. So, here no difference which way is used when we have the 100000+ lines in inventory json. But the server-side filtering adds a significant performance boost and should not be neglected.
I suggest splitting the filter hash into two parts so the first part will be used for server-side filtering and second will be used for metadata filtering. I have used such a method in an internal project:

filters:
  status: 'ACTIVE'
  name: 'srv8-'
  tags:
    - staging
    - workers
  metadata:
     os_distro: 'rhel'
     ci_stack: '{{ lookup("file","/etc/openstack/git.sha") }}'

During inventory processing the "metadata" key should be removed from filters:

if isinstance(filters, dict) or isinstance(filters, string_types):
    metadata = {key: value for key, value in filters['metadata'].items()
                if value is not None}

    del filters['metadata']

    raw_data = cloud_inventory.list_hosts(
        expand=expand_hostvars, filters=filters, fail_on_cloud_config=fail_on_errors)

    source_data = raw_data
    for key, value in metadata.items():
        source_data = filter(lambda server: server['metadata'].get(key) == value, source_data)
else:
    source_data = cloud_inventory.list_hosts(
        expand=expand_hostvars, fail_on_cloud_config=fail_on_errors)

As you can see the OpenStackSDK needs a litle change:

    def list_hosts(self, expand=True, filters=None, fail_on_cloud_config=True):
        hostvars = []

        for cloud in self.clouds:
            try:
                # Cycle on servers
                for server in cloud.list_servers(detailed=expand, filters):
                    hostvars.append(server)
            except exceptions.OpenStackCloudException:
                # Don't fail on one particular cloud as others may work
                if fail_on_cloud_config:
                    raise

        return hostvars

What do you think?

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 11, 2019
@emonty
Copy link
Contributor

emonty commented Jul 17, 2019

shipit

Copy link
Contributor

@Shrews Shrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 17, 2019
@gtema
Copy link
Contributor

gtema commented Jul 17, 2019

shipit

@Turmio
Copy link
Author

Turmio commented Jul 17, 2019

Should this have affects_2.9 label instead of affects_2.8?

@@ -192,9 +210,14 @@ def parse(self, inventory, loader, path, cache=True):

expand_hostvars = self._config_data.get('expand_hostvars', False)
fail_on_errors = self._config_data.get('fail_on_errors', False)
filters = self.templar.template(self._config_data.get('filters', None))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of templating this particular option here it should be added in a more general way. Have you seen #58288?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

I haven't seen that PR but I'll agree that is the right way to do this.

This new option doesn't really make sense if templating is not included, In that case some other templating mechanism / scripting must be used to achieve dynamic / environment aware filtering.

How about merging this as it is for now and removing templating after #58288 is merged? There seems to failing tests and I wouldn't want to wait if it makes to 2.9.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this seems like a sensible approach.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked this as resolved based on @cloudnull's comment and because of #58288 is still failing in CI.

Copy link
Contributor

@s-hertel s-hertel Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I marked as unresolved because #60715 is based on #58288, and it is not failing CI. It just needs reviews. It also allows you to use inline vaults in the inventory config.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s-hertel nice!

I did not notice that, I only looked status of original PR. So I should wait that your PR is merged and revert that change which added templating?

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Jul 17, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 25, 2019
@@ -192,9 +210,14 @@ def parse(self, inventory, loader, path, cache=True):

expand_hostvars = self._config_data.get('expand_hostvars', False)
fail_on_errors = self._config_data.get('fail_on_errors', False)
filters = self.templar.template(self._config_data.get('filters', None))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this seems like a sensible approach.

@cloudnull
Copy link
Contributor

shipit

@ailjushkin
Copy link

ailjushkin commented Sep 25, 2019

Can you please explain what else should be done to merge this PR (finally)?

@ailjushkin
Copy link

ailjushkin commented Sep 26, 2019

Could anyone explain in simple words, what the problem with this filtering? Does openstack really cannot limit http response by metadata? Does it always return the whole project with thousands of servers for a client and client should process it on his side? If so, this sounds bad. Looks like it is lack of basic API functionality.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 29, 2019
@cloudnull
Copy link
Contributor

cloudnull commented Jan 7, 2020

hey @ailjushkin , IDK why this is taking so long to get in... :(

@Turmio it looks like its in need of a rebase at this point. Can you resolve the merge conflict so we can try and get this in, again?

@sshnaidm
Copy link
Contributor

Thanks for submitting patch for Openstack Ansible modules!
We moved Openstack Ansible modules to Openstack repositories.
Next patches should be submitted not with Ansible Github but with
Openstack Gerrit: https://review.opendev.org/#/q/project:openstack/ansible-collections-openstack
Please submit your code there from now.
Thanks for your contribution and sorry for inconvienience.

@ansibot ansibot added the new_plugin This PR includes a new plugin. label Mar 24, 2020
@ansibot ansibot added collection Related to Ansible Collections work collection:openstack.cloud needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md labels Apr 29, 2020
@bmillemathias
Copy link
Contributor

@sshnaidm so someone can close this PR ?

@sshnaidm
Copy link
Contributor

sshnaidm commented Jul 6, 2020

@sshnaidm so someone can close this PR ?

I think it's on Ansible cores, I don't have permissions to close it.

@s-hertel
Copy link
Contributor

s-hertel commented Jul 6, 2020

Closing as per #51401 (comment)

@s-hertel s-hertel closed this Jul 6, 2020
@ansible ansible locked and limited conversation to collaborators Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud collection Related to Ansible Collections work feature This issue/PR relates to a feature request. inventory Inventory category needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_plugin This PR includes a new plugin. openstack stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet