fmherschel / SAPHanaSR-old-private

15 stars 8 forks source link

No default value for PREFER_SITE_TAKEOVER defined even though decription states otherwise #19

Closed fdanapfel closed 4 years ago

fdanapfel commented 9 years ago

According to the description the default value for PREFER_SITE_TAKEOVER should be "yes":

<parameter name="PREFER_SITE_TAKEOVER" unique="0" required="0">
    <longdesc lang="en">Should cluster/RA prefer to switchover to slave instance instead of restarting master locally? Default="yes"
    no: Do prefer restart locally
    yes: Do prefer takever to remote site
    </longdesc>
    <shortdesc lang="en">Local or site recover preferred?</shortdesc>
    <content type="boolean" default="yes" />

However looking at the code in the saphana_init function no default is specified if OCF_RESKEY_PREFER_SITE_TAKEOVER is not set: PreferSiteTakeover="$OCF_RESKEY_PREFER_SITE_TAKEOVER" (for other parameters like "AUTMATED_REGISTER" a default is specified: AUTOMATED_REGISTER="${OCF_RESKEY_AUTOMATED_REGISTER:-false}")

fmherschel commented 9 years ago

That's true. We should decide, if we follow the docu or the code ;-) Maybe it makes sense to force the user to set this option active (required="1"). Or we set the default like PreferSiteTakeover="${OCF_RESKEY_PREFER_SITE_TAKEOVER:-true}".

Advantage changing the code: If a customer in the past did NOT set this value they do not get a failure about wrong configuration (missing parameter). Disadvantage changing the code: If we now set a default not set before we change the behaviour of the RA - at least we need to encounter that! Maybe I could do that if we have (suse) internally a bugzilla crying for the unset default.

I did not found that one, because in all our recommendations and samples we actively set this parameter to pro-active document what the configuration is asking for: prefer site takeover or not.

fdanapfel commented 9 years ago

For now I have changed our Documentation to state that there is no default value for this parameter and that the parameter must be set to the desired value during configuration of the resource.

Therefore the simplest option might be to remove the "yes" in "content type="boolean" default="yes"" and set "required="1" in the description of the resource agent.

fmherschel commented 8 years ago

Hi Frank, I reviewed currently this issue and from todays perpective I would say that changing the "required" to "true" (1) might also break customer installations as their cluster would stop all resources which would not have this option set :/ So maybe your first idea to follow the initial documentation and to count it as a bug, that the default is not set correctly is even the less worse option. What do you think about that? If we agree in this issue and solving that in the way you initially have suggested I would patch that asap.

Sorry for the loop :) Regards Fabian

fmherschel commented 7 years ago

Hi Frank, an empty value of leads into the code which is also used for a set value "false". So we will change the docu in one of the next updates and we add the default "false" to the code to be independent from code changes in ocf_is_true. Thanks for helping here. Regards Fabian

fdanapfel commented 4 years ago

Closing this issue since it was fixed in new repo: https://github.com/SUSE/SAPHanaSR/commit/5b85a83e453cbd9f7c228fa423b5ef56fef64585