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
Adds failure checking if OSD is not created #2599
Conversation
@@ -70,6 +80,11 @@ | |||
- not containerized_deployment | |||
- item.0.partitions|length == 0 | |||
|
|||
register: osd |
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 won't work because of the changed_when: false
.
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 is this task set to changed_when false? Can we change it? It isn't present in the same command for manually prepare ceph.
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.
nvm I'll work around it
@@ -2,6 +2,8 @@ | |||
# use shell rather than docker module | |||
# to ensure osd disk prepare finishes before | |||
# starting the next task | |||
- set_fact: osd_created=False |
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.
- name: set_fact osd_created
set_fact:
osd_created: False
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.
done
@@ -26,6 +28,10 @@ | |||
- not osd_auto_discovery | |||
- containerized_deployment | |||
- item.0.partitions|length == 0 | |||
register: osd |
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.
Calling the register
the same name won't work since ansible will overwrite the previous one, even if the task is skipped. So you have to use different names for each 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.
That's the point. I want it to overwrite each one because the real state variable is osd_created
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.
Yes, in this approach there is no issue to register multiple times the same variable name.
It might be an issue if we wanted to have the possibility to deploy multiple osd scenario per host though.
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.
@guits Is that supported today? Are you asking me to change it or this is OK?
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.
@trozet this is OK, I was just trying to clarify.
- item.0.partitions|length == 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.
looks like this is fixing a different bug, which means you need a separate commit for that.
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.
oops. I must have accidentally done that with copy paste. Will remove.
@trozet let us know when this is ready for review again. |
4c8cc1d
to
ee92b95
Compare
@leseb please review again when you have a moment. |
- fail: | ||
msg: > | ||
OSD was never created. If using not using osd_auto_discovery, ensure the |
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.
extra space between .
and If
.
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.
I usually use 2 spaces after periods. I'll change them all to 1.
- "{{ parted_results.results | default([]) }}" | ||
- "{{ devices }}" | ||
- "{{ dedicated_devices }}" | ||
changed_when: false |
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.
Doesn't this break the register line 99? https://github.com/ceph/ceph-ansible/pull/2599/files#diff-bc1034a6fab2180f375254c274e02ed6R99
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.
Can you elaborate on your comment some more? I'm not sure what you think may not work with the register on 99.
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 are you concerned because of the changed_when: false
because it's changing the "changed" attribute of a registered variable ? If yes, the register L99 isn't affected since they are not applied on the same 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.
Yeah my bad, missed the indent and the block :). Not an issue.
- fail: | ||
msg: > | ||
OSD was never created. Ensure the specified devices do not have |
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
@@ -2,6 +2,10 @@ | |||
# use shell rather than docker module |
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 comment should be moved L9
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.
done
@@ -26,6 +30,10 @@ | |||
- not osd_auto_discovery | |||
- containerized_deployment | |||
- item.0.partitions|length == 0 | |||
register: osd | |||
|
|||
- set_fact: osd_created=True |
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.
please, use the same syntax than above (L5-7, which is what we use in the whole playbook)
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.
done
@@ -28,6 +32,10 @@ | |||
- containerized_deployment | |||
- osd_objectstore == 'filestore' | |||
- item.0.partitions|length == 0 | |||
register: osd | |||
|
|||
- set_fact: osd_created=True |
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
- "{{ devices }}" | ||
- "{{ dedicated_devices }}" | ||
changed_when: false | ||
- set_fact: osd_created=True |
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
- "{{ dedicated_devices }}" | ||
changed_when: false | ||
|
||
- set_fact: osd_created=True |
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
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.
@trozet you missed this one ;)
register: osd | ||
|
||
- set_fact: osd_created=True |
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
@trozet did you push your changes? |
ee92b95
to
083456e
Compare
- "{{ dedicated_devices }}" | ||
changed_when: false | ||
|
||
- set_fact: osd_created=True |
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.
@trozet you missed this one ;)
083456e
to
ba2cf6f
Compare
@guits please review again |
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.
sorry, one more thing.
when: osd.changed | ||
|
||
- name: fail if OSD was never created |
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.
oops missed this one, no uppercase in name
's 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.
@leseb done. Out of curiosity is that an ansible standards type of thing? Or is that just something decided on for this particular project?
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 is something we decided on this particular project.
when: osd.changed | ||
|
||
- name: fail if OSD was never created |
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
ba2cf6f
to
1bb5d81
Compare
jenkins test luminous-ansible2.4-docker_cluster |
jenkins test luminous-ansible2.4-bluestore_osds_non_container |
@trozet could you please rebase on master? |
@guits sorry I missed this. Will rebase. |
There are several different types of OSD that may be created depending on a users environment/settings. However, in the case that no conditions are met for OSD creation, ceph-ansible completes and no OSD is created. resolves ceph#2598 Signed-off-by: Tim Rozet <trozet@redhat.com>
1bb5d81
to
f4a0973
Compare
jenkins test this please |
It'd be nice to resurrect something similar once #3187 merges. |
Closing this due to inactivity, feel free to re-open, thanks! |
There are several different types of OSD that may be created depending
on a users environment/settings. However, in the case that no
conditions are met for OSD creation, ceph-ansible completes and no OSD
is created.
resolves #2598