search

SUSE / DeepSea

A collection of Salt files for deploying, managing and automating Ceph.
GNU General Public License v3.0
160 stars 75 forks source link

cephfs: fix pool creation prerequisite (bsc#1149557) #1750

Closed jan--f closed 4 years ago

jan--f commented 4 years ago

With unless the state is executed when one condition returns false. For example it'll try to create the cephfs_data pool if the pool is present but the cephfs instance is not yet. Negating the condition and using onlyif yields the expected behaviour.

Signed-off-by: Jan Fajerski jfajerski@suse.com

swiftgist commented 4 years ago

I think it's still a bit awkward to read with the number of negations. Wouldn't 

{% if salt['cmd.run']('ceph fs ls') == "" %}

around all the steps be a bit clearer. If ceph fs returns nothing, then create/enable both pools, etc.

That would also address the issue from the bug where cephfs_data had been manually created as an EC and the unless/ifonly conditional would have exactly one test.

jan--f commented 4 years ago

I think it's still a bit awkward to read with the number of negations. Wouldn't

{% if salt['cmd.run']('ceph fs ls') == "" %}

around all the steps be a bit clearer. If ceph fs returns nothing, then create/enable both pools, etc.

That would also address the issue from the bug where cephfs_data had been manually created as an EC and the unless/ifonly conditional would have exactly one test.

Tbh I'm not a big fan of adding Jinja to states. The different execution contexts and times have often made things very awkward. Do you think this is too confusing even with the comments?

swiftgist commented 4 years ago

Tbh I'm not a big fan of adding Jinja to states. The different execution contexts and times have often made things very awkward. Do you think this is too confusing even with the comments?

I understand the reluctance to add more Jinja. :) Considering Martin's comment about the union/intersection of the two commands and a bit of the mental gymnastics to understand when the command is called, my thinking was that reducing each test to a single line would be more obvious.

Is it too confusing even with the comments? I don't know honestly. I know what it does. Considering this achieves the goal and fixes the original issue, I'd say go ahead and merge.

jschmid1 commented 4 years ago

@jan--f Please create a backport so that we can merge those two together

jschmid1 commented 4 years ago

@jan--f Please create a backport so that we can merge those two together

ignore this. I should've checked the next tab :>