-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Wip RGW NFS #1780
Wip RGW NFS #1780
Conversation
@alfredodeza should I remove the tests I wrote until #1781 is merged? |
- name: add custom repo | ||
- name: fetch nfs-ganesha development apt repo file | ||
uri: | ||
url: https://shaman.ceph.com/api/repos/nfs-ganesha/next/latest/{{ ansible_distribution | lower }}/xenial/flavors/{{ nfs_ganesha_flavor }}/repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb is there an ansible variable I can use to substitute for "xenial" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try ansible_distribution_release
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove the tests, just update them for the new API
class TestNFSs(object): | ||
|
||
@pytest.mark.no_docker | ||
def test_nfs_services_are_running(self, node, Service): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this will now need to be host
instead of Service
@pytest.mark.no_docker | ||
def test_nfs_services_are_running(self, node, Service): | ||
service_name = "nfs-ganesha" | ||
assert Service(service_name).is_running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this needs to be host.service(service_name).is_running
assert Service(service_name).is_running | ||
|
||
@pytest.mark.no_docker | ||
def test_nfs_services_are_enabled(self, node, Service): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -114,6 +114,10 @@ fetch_directory: ~/ceph-ansible-keys | |||
#ceph_stable_release: dummy | |||
#ceph_stable_repo: "{{ ceph_mirror }}/debian-{{ ceph_stable_release }}" | |||
|
|||
#nfs_ganesha_stable: true # use stable repos for nfs-ganesha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb do the vars added in this file also need to be copied over to group_vars/all.yml.sample?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you add a var in all.yml.sample and then call ./generate_group_vars_sample.sh
the rhcs.yml.sample will automatically be updated.
a77db8b
to
c7d6957
Compare
state: "{{ (upgrade_ceph_packages|bool) | ternary('latest','present') }}" | ||
when: nfs_group_name in group_names | ||
|
||
- name: install red hat storage nfs file gateway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb @ktdreyer I don't need to do any repo configuration for nfs-ganesha if someone is running rhcs as long as this is set to true (https://github.com/ceph/ceph-ansible/blob/wip-rgw-nfs/group_vars/all.yml.sample#L161) right?
If someone is running rhcs, nfs-ganesha and it's fsal and ntirpc repos will be in the same repo as the rest of the ceph bits come from right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
a305b87
to
5241f59
Compare
when: | ||
- nfs_ganesha_dev is defined | ||
- nfs_ganesha_dev_yum_repo is defined | ||
- nfs_ganesha_stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct here? Don't you want either nfs_ganesha_stable
or nfs_ganesha_dev
?
return_content: yes | ||
register: nfs_ganesha_dev_yum_repo | ||
when: | ||
- nfs_ganesha_dev is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default nfs_ganesha_dev
and nfs_ganesha_stable
in ceph-defaults/defaults/main.yml
and avoid these x is defined
checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewschoen that could work. What's the rule of thumb on whether to have variables in group_vars/all.yml and ceph-defaults/defaults/main.yml and one of the daemon specific main.ymls like ceph-nfs/defaults/main.yml for example?
35dfda5
to
1775735
Compare
@alfredodeza I updated the tests to the new API. Do i need to worry about any of the tests that didn't pass? @leseb This PR should be ready to merge. I'm not sure if I followed all the best practices in terms of the things I have commented on in the PR. |
@@ -114,6 +114,10 @@ dummy: | |||
#ceph_stable_release: dummy | |||
#ceph_stable_repo: "{{ ceph_mirror }}/debian-{{ ceph_stable_release }}" | |||
|
|||
#nfs_ganesha_stable: true # use stable repos for nfs-ganesha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb is this file, along with group_vars/rhcs.yml.sample the correct place to have these variables? At first I had put them in roles/ceph-nfs/defaults/main.yml since the changes only matter on nfs nodes but after I saw how ceph_dev and ceph_stable are in this file and followed suite.
@@ -38,21 +38,39 @@ | |||
changed_when: false | |||
when: ceph_stable_uca | |||
|
|||
- name: add custom repo | |||
- name: fetch nfs-ganesha development apt repo file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb would it make sense to move the nfs-ganesha repo generation and installation tasks to files under roles/ceph-nfs/tasks since they only apply to nfs nodes?
3dc656f
to
0eb948c
Compare
when: | ||
- nfs_ganesha_stable is defined | ||
- nfs_ganesha_stable_deb_repo is defined | ||
- nfs_ganesha_stable | ||
- not ansible_distribution == "Debian" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be "ansible_distribution == 'Debian'"?
jenkins test this please |
acdf6ea
to
585ec81
Compare
jenkins test luminous-ansible2.3-centos7_cluster |
when: nfs_obj_gw | ||
|
||
- name: set access key | ||
set_fact: | ||
ceph_nfs_rgw_access_key: "{{ (rgwuser.stdout | from_json)['keys'][0]['access_key'] }}" | ||
when: nfs_obj_gw | ||
delegate_to: "{{ groups[mon_group_name][0] }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb the reason this task and the next task are delegated to the monitor is because it uses the rgwuser register in the "create rgw nfs user" task above.
@leseb as long as the failures below aren't due to my PR, it's ready to be reviewed and merged. https://2.jenkins.ceph.com/job/ceph-ansible-prs-luminous-ansible2.3-centos7_cluster/334/ |
jenkins test luminous-ansible2.3-centos7_cluster |
@@ -0,0 +1,18480 @@ | |||
luminous-ansible2.3-xenial_cluster installed: alabaster==0.7.9,amqp==2.1.4,ansible==2.3.1.0,apipkg==1.4,asn1crypto==0.22.0,Babel==2.3.4,backports.functools-lru-cache==1.2.1,backports.ssl-match-hostname==3.5.0.1,bcrypt==3.1.3,Beaker==1.5.4,beautifulsoup4==4.6.0,billiard==3.5.0.2,blessings==1.5,bodhi==2.9.1,bodhi-client==2.9.1,boto==2.45.0,boto3==1.4.6,botocore==1.6.0,bunch==1.0.1,cached-property==1.3.0,CCColUtils==1.5,celery==4.0.2,ceph-detect-init==1.0.1,ceph-disk==1.0.0,cephfs==0,cffi==1.10.0,chardet==2.3.0,CherryPy==3.5.0,click==6.7,coverage==4.4.1,cryptography==2.0.3,cssselect==0.9.2,Cython==0.25.2,decorator==4.0.11,docker-py==1.10.6,docker-pycreds==0.2.1,dockerfile-parse==0.0.5,dockerpty==0.4.1,docopt==0.6.2,docutils==0.13.1,ecdsa==0.13,enum34==1.1.6,execnet==1.4.1,facebook-scribe==2.0,fedpkg==1.29,file-magic==0.3.0,funcsigs==1.0.2,future==0.16.0,futures==3.1.1,gitdb2==2.0.2,GitPython==2.1.5,gpg==1.8.0,gssapi==1.2.0,html5lib==0.999,httplib2==0.9.2,idna==2.6,imagesize==0.7.1,iniparse==0.4,ipaddr==2.1.10,ipaddress==1.0.18,IPy==0.81,Jinja2==2.9.6,jmespath==0.9.0,jsonschema==2.5.1,kerberos==1.2.5,kitchen==1.2.4,kobo==0.5.2,kombu==4.0.2,langtable==0.0.37,lockfile==0.11.0,logutils==0.3.5,lxml==3.7.2,Mako==1.0.6,MarkupSafe==1.0,mock==2.0.0,munch==2.1.0,nose==1.3.7,notario==0.0.11,ntplib==0.3.3,offtrac==0.1.0,olefile==0.44,openidc-client==0.3.0,ordered-set==2.0.0,osbs-client==0.39.1,packagedb-cli==2.14.1,paramiko==2.2.1,Paste==2.0.3,pbr==1.10.0,pecan==1.2.1,Pillow==4.1.1,pluggy==0.4.0,ply==3.9,prettytable==0.7.2,pwquality==1.3.0,py==1.4.34,pyasn1==0.3.3,pycparser==2.18,pycrypto==2.6.1,pycryptopp==0.6.0.1206569328141510525648634803928199668821045408958,pycurl==7.43.0,Pygments==2.2.0,pygobject==3.24.1,pygpgme==0.3,pykickstart==2.35,pyliblzma==0.5.3,pymod2pkg==0.6.1,PyNaCl==1.1.2,pyOpenSSL==16.2.0,pyparted==3.10.7,PySocks==1.5.7,pytest==3.2.1,pytest-xdist==1.16.0,python-augeas==0.5.0,python-bugzilla==2.1.0,python-dateutil==2.6.0,python-dmidecode==3.12.2,python-fedora==0.9.0,python-keyczar==0.71rc0,python-meh==0.43,python-yubico==1.3.2,pytz==2016.10,pyudev==0.21.0,pyusb==1.0.0,pyxattr==0.5.3,PyYAML==3.12,qpid-python==1.36.0,rados==0,rbd==0,rdopkg==0.45.0,repoze.lru==0.4,requests==2.13.0,requests-kerberos==0.10.0,rhmsg==0.5,rhpkg==1.32,rpkg==1.50,rpm-python==4.13.0.1,rpmdiff==4.2,rsa==3.4.2,s3transfer==0.1.10,simplegeneric==0.8.1,simplejson==3.10.0,singledispatch==3.4.0.3,six==1.10.0,slip==0.6.4,slip.dbus==0.6.4,smmap2==2.0.3,snowballstemmer==1.2.1,Sphinx==1.6.3,sphinx-rtd-theme==0.2.4,sphinxcontrib-websupport==1.0.1,SQLAlchemy==1.1.13,SSSDConfig==1.15.3,systemd-python==234,Tempita==0.5.1,testinfra==1.6.0,texttable==0.9.0,thrift==0.9.3,thrift3babeltrace==0.2.2,tox==2.7.0,typing==3.5.2.2,urlgrabber==3.10.1,urllib3==1.20,vine==1.1.3,virtualenv==15.1.0,waitress==1.0.2,WebOb==1.7.3,websocket-client==0.40.0,WebTest==2.0.27,Werkzeug==0.11.10,Whoosh==2.7.4,yum-metadata-parser==1.1.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's that? Please remove it :)
config_type: ini | ||
|
||
- name: cat ganesha conf file to view overrides | ||
command: cat /etc/ganesha/ganesha.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a leftover? Please remove this task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -194,7 +198,11 @@ dummy: | |||
#ceph_dev_branch: master # development branch you would like to use e.g: master, wip-hack | |||
#ceph_dev_sha1: latest # distinct sha1 to use, defaults to 'latest' (as in latest built) | |||
|
|||
#nfs_ganesha_dev: false # use development repos for nfs-ganesha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you must edit ceph-defaults/defaults/main.yml and then run generate_group_vars_sample.sh
- (nfs_obj_gw or nfs_file_gw) | ||
- not ansible_distribution == "Debian" | ||
when: | ||
- nfs_ganesha_stable is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this check as the var must be defined in ceph-common/defaults/main.yml
- not ansible_distribution == "Debian" | ||
when: | ||
- nfs_ganesha_stable is defined | ||
- nfs_ganesha_stable_deb_repo is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
return_content: yes | ||
register: nfs_ganesha_apt_repo | ||
when: | ||
- nfs_ganesha_dev is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
group: root | ||
backup: yes | ||
when: | ||
- nfs_ganesha_dev is defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
585ec81
to
45fcae5
Compare
@leseb I've taken all of your your new comments into consideration, so once again this PR should be good to go. One questions I did have was right now as long as there's an nfs node, nfs-ganesha repo setup and installation is happening on all nodes. Do you think I should move all the nfs related installations tasks to roles/ceph-nfs/tasks/installs/ since those nfs-ganesha reposityory setup and installation only needs to happen on nfs nodes and not every node. |
d68f59a
to
eb8c486
Compare
|
||
Sectype = sys,krb5,krb5i,krb5p; | ||
|
||
FSAL { | ||
Name = CEPH; | ||
} | ||
|
||
{{ ganesha_ceph_export_overrides }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb is it ok that these variable and others like it are undefined and undeclared? in defaults/main.yml they are declared like this:
#ganesha_ceph_export_overrides:
eb8c486
to
017206a
Compare
when: | ||
- (nfs_obj_gw or nfs_file_gw) | ||
- not ansible_distribution == "Debian" | ||
when: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the trailing space
config_type: ini | ||
|
||
- name: cat ganesha conf file to view overrides | ||
command: cat /etc/ganesha/ganesha.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -0,0 +1,25 @@ | |||
import pytest | |||
import json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you import it but json
is not used.
017206a
to
07c4a5d
Compare
Signed-off-by: Ali Maredia <amaredia@redhat.com>
Signed-off-by: Ali Maredia <amaredia@redhat.com>
@leseb I took your newest changes into account and fixed them. That task where the ganesha.conf is cat'd out is just for my debugging purposes since I can't see the conf when the jenkins tests are run. That task has been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing and we will be good to go.
|
||
- name: generate ganesha configuration file | ||
action: config_template | ||
args: | ||
src: "{{ lookup('env', 'ANSIBLE_ROLES_PATH') | default (playbook_dir + '/roles', true) }}/ceph-common/templates/ganesha.conf.j2" | ||
src: "{{ lookup('env', 'ANSIBLE_ROLES_PATH') | default (playbook_dir + '/roles', true) }}/ceph-nfs/templates/ganesha.conf.j2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This task needs a notify for the handler to work please add:
notify:
- restart ceph nfss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as @leseb comment,
just need to add the missing notify and it should be ok to be merged
07c4a5d
to
f8171e8
Compare
config_type: ini | ||
notify: | ||
- restart ceph nfss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb done.
Signed-off-by: Ali Maredia <amaredia@redhat.com>
Signed-off-by: Ali Maredia <amaredia@redhat.com>
The error on ceph-ansible-prs-luminous-ansible2.3-switch_to_containers is expected, the error is in switch-from-non-containerized-to-containerized-ceph-daemons.yml. |
This PR is ready to be merged.