site.yaml.sample: do not try to gather facts on "non-ceph" hosts

[insatomcat]

The site.yaml.sample makes ceph-ansible try to gather facts on group "all" which may contains hosts that have nothing to do with ceph-ansible, and/or even unreachable hosts which will make the playbook fail.
This commit will replace the "all" group by "ansible_play_hosts", containing only ceph-ansible related hosts.

[asm0deuz]

@insatomcat Your scenario isn't supported, only ceph related nodes should be in the inventory file. skipping unreachable hosts may lead to unexpected results or end users might not even notice that the playbook hasn't been run on all their hosts. Am I missing something here?

[insatomcat]

Well my usecase is using ceph-ansible among other playbooks to setup a virtualization cluster (ceph-ansible obviously for ceph, but I also configure networking, pacemaker/corosync, libvirt, etc.). My inventory contains machines that have nothing to do with ceph (guests that will run on the cluster "later" and that are not yet existing when we deploy ceph, client "testing" machines that do not access ceph directly, etc).
I'm using ceph-ansible with "import-playbook" and even if I use "--limit" to restrict to ceph machines, "group['all']" will not respect that. Anyway, for this I have to patch the site.yml and so I thought I'd propose this upstream.

I find it weird that ceph-ansible has to use the "all" group, I feel it should not really be concerned of what's in the inventory if it does not belong to the groups it has to deal with.

[guits]

@insatomcat what about the similar task in site-container.yml.sample ?

[guits]

after discussing this with @clwluvw , we came up with the idea that maybe creating a variable ceph_group_name that would default to "all" (ceph_group_name: all) could be a decent approach ? Need to be tested though.

[insatomcat]

@insatomcat what about the similar task in site-container.yml.sample ?

I'm not using containerized deployment so I did not hit this task, but I guess the issue is the same.

after discussing this with @clwluvw , we came up with the idea that maybe creating a variable ceph_group_name that would default to "all" (ceph_group_name: all) could be a decent approach ? Need to be tested though.

We are talking about the site sample files. The user will usually rename those files and they should work most of the time as is. Of course in my use case I have to modify the sample file.
Your idea would make the change I have to make a bit different, but fundamentally would not change much, I would still have to modify the default from "all" to "ansible_play_hosts".
So my question is really about changing this default behavior (use the whole inventory).
I feel that the default should be not to use "all".

[guits]

the issue with not using all is that it would break the playbook execution when using the --limit option.

[insatomcat]

the issue with not using all is that it would break the playbook execution when using the --limit option.

how so ?

[guits]

the issue with not using all is that it would break the playbook execution when using the --limit option.

how so ?

for instance, if you run the playbook with --limit osds, then you won't have monitors' facts. It means any tasks later in the playbook which has a delegate_to: to monitor node(s) will fail.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

[insatomcat]

for instance, if you run the playbook with --limit osds, then you won't have monitors' facts. It means any tasks later in the playbook which has a delegate_to: to monitor node(s) will fail.

Would is be complicated to create a "ceph" group, including all other groups? and gather facts for this group?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically closed due to inactivity. Please re-open if these changes are still required.