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 backward_sorted and backward_reverse_sorted ordering strategies #69801

Closed
wants to merge 1 commit into from

Conversation

segal90
Copy link

@segal90 segal90 commented Jun 1, 2020

SUMMARY

Add backward_sorted and backward_reverse_sorted ordering strategies.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Orders

ADDITIONAL INFORMATION

This ordering can be useful if you have hostnames like fw-dc1-01, fw-dc1-02, fw-dc2-01, fw-dc2-02.
With these orders you can run plays against 02 (backup) nods first then the 01 (primary) nodes.

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. 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. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 2, 2020
@samdoran
Copy link
Contributor

samdoran commented Jun 4, 2020

We're on the fence about accepting this feature. What is the use case for this? The test case gives the same result as reverse_sorted. At a minimum, the name needs to be changed to clarify exactly what it is doing because it's rather confusing as is. Should 'reverted name' be 'reversed name'?

needs_info

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jun 4, 2020
@segal90
Copy link
Author

segal90 commented Jun 5, 2020

Test cases give the same result because you need hostnames like fw-dc1-01, fw-dc1-02, fw-dc2-01, fw-dc2-02 for seeing the difference.
Let's see what will be the result with the different sorting:
sorted: fw-dc1-01, fw-dc1-02, fw-dc2-01, fw-dc2-02
reverse_sorted: fw-dc2-02, fw-dc2-01, fw-dc1-02, fw-dc1-01
backward_sorted: fw-dc1-01, fw-dc2-01, fw-dc1-02, fw-dc2-02
backward_reverse_sorted: fw-dc2-02, fw-dc1-02, fw-dc2-01, fw-dc1-01

As you can see with the 2 new algorithms you can run a play on primary or secondary nodes first. This can be handy if you have an active-backup HA solution in multiple data centers.

Putting DC's name before numbering in hostnames is pretty common in larger companies that have multiple DCs.

Should 'reverted name' be 'reversed name'?
Yes, you are right. Fixed.

@samdoran
Copy link
Contributor

samdoran commented Jun 5, 2020

Ok. Can you add a changelog fragment and update the test data? See this fragment as an example. This would be classified as minor_changes.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 8, 2020
@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Jun 8, 2020
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jun 8, 2020
@acozine
Copy link
Contributor

acozine commented Jun 10, 2020

@segal90. This isn't intuitive and can lead to unexpected results.

For example, if I had servers ending in -01 through -12, with backward_sorted I will get: -10, -01, -11, -02, -12, -03, etc.

Could you get the ordering you want using inventory or reverse_inventory instead?

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 10, 2020
@segal90
Copy link
Author

segal90 commented Jun 11, 2020

@acozine,
Inventory is not sorted and the order of the hosts can change on every run. This is a valid use case with any inventory plugin which pulls hosts from external sources (eg: databases).

This algorithm could be useful with active-backup HA clusters where you have only 2-3 hosts (eg: lots of network device pairs).

Probably you are right and it can lead to unexpected results but comparing to the benefit mentioned above, I think it worth.

@acozine
Copy link
Contributor

acozine commented Jun 16, 2020

@segal90 thanks for the response. If this functionality gets merged, the docs team can add a sample inventory to the playbooks_strategies page to illustrate the results of each ordering strategy.

@samdoran
Copy link
Contributor

@segal90 Would you be able to attend an IRC meeting to bring this up for discussion? ansible/community#541

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. has_issue labels Jun 24, 2020
@ansibot ansibot added 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. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 11, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 5, 2020
@bcoca
Copy link
Member

bcoca commented Jan 12, 2022

Hi!

Thanks very much for your submission to Ansible. It sincerely means a lot to us that you've taken time to contribute.

Unfortunately, we're not sure if we want this feature in the program, and I don't want this to seem confrontational. Our reasons for this are:

  • This is not something we wish to add to Ansible at this point in time
  • Historically we've added other sorting strategies but we do not wish for this to become a catch all for any future sorting algorithms people wish for in the future
  • It is probably better to create a dynamic inventory to achieve the sorting mechanism you want https://docs.ansible.com/ansible/latest/dev_guide/developing_inventory.html

However, we're absolutely always up for discussion. Since this is a really busy project, we don't always see comments on closed tickets, but want to encourage
open dialog. You can stop by the development list, and we'd be glad to talk about it - and we might even be persuaded otherwise!

In the future, sometimes starting a discussion on the development list prior to implementing a feature can make getting things included a little easier, but it's not always necessary.

Thank you once again for this and your interest in Ansible!

@bcoca bcoca closed this Jan 12, 2022
@ansible ansible locked and limited conversation to collaborators Jan 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. has_issue inventory Inventory category 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. pre_azp This PR was last tested before migration to Azure Pipelines. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants