Refact ceph-ansible #1710
Refact ceph-ansible #1710
Conversation
guits
force-pushed
the
refact
branch
2 times, most recently
from
31d7416
to
82211e2
Compare
There was a problem hiding this comment.
A couple questions:
-
Could we move only the config in
ceph-commonandceph-docker-commoninto the newceph-defaultsrole? It gets to be quite a lot if we include config from the other roles as well. This would also still generate sample group_vars files for each role which I think is nice. -
Why do we need both
ceph-configandceph-defaults? Isceph-configdoing anything besides creating the dependency onceph-defaults?
roles/ceph-client/defaults/main.yml
Outdated
| @@ -1,18 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
All these symlinks shouldn't be necessary. As long as ceph-defaults is included in the same play as another role then that role should inherit all of the config in ceph-defaults/defaults/main.yml.
These could be done by making ceph-defaults a dependency for the other roles or just including it directly in a playbook like:
- hosts: all
roles:
- ceph-defaults
- ceph-common
- ceph-mon
In that example ceph-common and ceph-mon should have access to all configuration set in ceph-defaults.
There was a problem hiding this comment.
Thanks for your comments @andrewschoen .
The idea is to have ceph-defaults handling only defaults variables and ceph-config for stuff like setting fsid. Do you think merging these two roles would be better?
Don't you think it could be cool to keep the default behavior which is including ceph-common or ceph-docker-common from other roles but having the possibility to dismiss it as well (see here) so we can set dependencies in site.yml as you described above?
Even if the CI seems to be happy, this PR is not finished at all, so, please let me know if you have any thoughts or suggestions.
There was a problem hiding this comment.
@guits yeah, I think it might make sense to combine ceph-config and ceph-defaults. I guess I just don't see the use case for ceph-defaults on it's own. Seems like you'd always want to use ceph-config so that fsid is populated correctly.
I do like the behavior added with no_deps. I still think that the configuration for all roles besides ceph-common and ceph-docker-common should stay with the respective role though. Anything that is common across roles or needed for ceph.conf generation should be included in ceph-config or ceph-defaults.
|
@leseb @andrewschoen I removed the symlinks |
guits
force-pushed
the
refact
branch
4 times, most recently
from
a5554d4
to
7828b85
Compare
guits
force-pushed
the
refact
branch
11 times, most recently
from
3afd39e
to
adfa2d5
Compare
|
jenkins test jewel-ansible2.2-update_cluster |
guits
force-pushed
the
refact
branch
2 times, most recently
from
2337e24
to
2d7bc38
Compare
site-docker.yml.sample
Outdated
|
|
||
| - hosts: mgrs | ||
| become: True | ||
| roles: | ||
| - ceph-defaults |
There was a problem hiding this comment.
ceph-defaults and ceph-docker-common should only be used when: "ceph_release_num.{{ ceph_stable_release }} > ceph_release_num.jewel"
roles/ceph-common/defaults/main.yml
Outdated
| # this is only here for usage with the rolling_update.yml playbook | ||
| # do not ever change this here | ||
| rolling_update: false | ||
| no_dep: false |
There was a problem hiding this comment.
I don't think you're using no_dep anymore are you?
guits
force-pushed
the
refact
branch
2 times, most recently
from
413ebae
to
214b5ff
Compare
| @@ -91,6 +91,7 @@ | |||
| - not containerized_deployment | |||
|
|
|||
| roles: | |||
| - ceph-defaults | |||
There was a problem hiding this comment.
You're gonna need either ceph-common or ceph-docker-common here depending on the value of containerized_deployment. Same for the rest of the plays in this playbook.
guits
force-pushed
the
refact
branch
2 times, most recently
from
195de18
to
ae3a688
Compare
guits
force-pushed
the
refact
branch
7 times, most recently
from
6887e8a
to
815d054
Compare
WIP
Fixes: #1681, #1698
Signed-off-by: Guillaume Abrioux gabrioux@redhat.com