-
Notifications
You must be signed in to change notification settings - Fork 54
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
Fix prerequisites in cephadm-preflight #284
Conversation
I misinterpreted the clients group and what its purpose was supposed to be, hence my latest commit. But there is still the problem, that the clients group is left untouched by the preflight playbook. This is also fixed my this PR. If you are still unsure as to what this is supposed to accomplish, here a comparison in playbook runs. Here rocky9-01 is admin, but not client and osd. The rest (02-04) is admin, client and osd. existing prefligt playbook
As you can see, the clients are skipped, no packages are installed. proposed preflight playbook
The clients are now not immediately skipped anymore and the required packages are installed. |
Requesting @asm0deuz for review. |
jenkins test el8-functional |
jenkins test el9-functional |
@droidben It looks good to me but you need to squash your commits into one (now there are 5 commits) |
c26ce06
to
cfacb65
Compare
@asm0deuz Commits have been squashed |
@asm0deuz I recommend to merge this soon so the sourcebranch does not go into conflict again |
@droidben There are still 3 commits, there should only be one "added lvm2 to infra packages". You need to stash and remove the 2 other ones |
@asm0deuz There is only 1 commit from me, all others are because the branch got out of sync and I had to merge the upstream diffs. |
3712e78
to
cfacb65
Compare
@asm0deuz Could you please clarify what is taking so long? With the time more merge conflicts will inevitably appear. And if you, as per your last comment, do not want me to update my branch, please at least merge this soon. In the meantime, I have reset to the original commit, so that you have to do the merging yourself if that's what you wanted. |
@droidben Just rebase it and I'll do the merge if the tests pass. |
@asm0deuz Rebased to match devel, tests are passing. |
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.
LGTM
8cf47aa
to
0f53dbe
Compare
@droidben To fix this conflit, just do the following:
This should fix the issue. |
90ed533
to
19bc342
Compare
19bc342
to
40ce4c8
Compare
@asm0deuz This is exactly what was done, but due to the long wait there will be the merge conflict somewhere, we cannot forgo it. :) |
I noticed that clients configured with
cephadm-preflight.yml
were not getting prerequisite packages installed. This was due to a wrong conditional in the playbook.Also
lvm2
is an official prerequisite as per the docs so I added it to the infra packages in the defaults as not all distros in all feature-flavors come with it installed.