[Freeipa-devel] [PATCHES 0227-0229] Server upgrade: introduce ipa-server-upgrade command

[Martin Basti]

On 29/04/15 08:52, Jan Cholasta wrote:
> Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):
>> On 04/29/2015 07:34 AM, Jan Cholasta wrote:
>>> Dne 27.4.2015 v 18:23 David Kupka napsal(a):
>>>> On 04/27/2015 04:45 PM, Martin Basti wrote:
>>>>> On 27/04/15 13:38, Martin Basti wrote:
>>>>>> On 23/04/15 12:55, Martin Basti wrote:
>>>>>>> On 21/04/15 10:31, Martin Basti wrote:
>>>>>>>> On 21/04/15 08:12, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Dne 15.4.2015 v 16:26 Martin Basti napsal(a):
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4904
>>>>>>>>>>
>>>>>>>>>> Patches attached.
>>>>>>>>>>
>>>>>>>>>> Also ipa-upgradeconfig part is called as a subprocess. This 
>>>>>>>>>> will be
>>>>>>>>>> removed after installer modifications.
>>>>>>>>>>
>>>>>>>>>> This patch may cause temporal upgrade issues (corner cases), 
>>>>>>>>>> until
>>>>>>>>>> installer part will be finished.
>>>>>>>>>>
>>>>>>>>>> If somebody will be hit by them, please use 
>>>>>>>>>> --skip-version-check for
>>>>>>>>>> ipactl and ipa-server-upgrade.
>>>>>>>>>
>>>>>>>>> Regarding that option vs. --force: I think the common 
>>>>>>>>> assumption is
>>>>>>>>> that --force ignores *all* non-fatal errors, but you break that
>>>>>>>>> assumption in ipactl. IMO --force should both ignore errors in
>>>>>>>>> service startup *and* skip version check, and a new option should
>>>>>>>>> be added to just ignore errors in service startup (e.g.
>>>>>>>>> --ignore-service-failures).
>>>>>>>> Originally I used --force option to skip detection, but there was
>>>>>>>> objections against it on list.
>>>>>>>>
>>>>>>>> However, to have option --force, which set true for both
>>>>>>>> --ignore-service-failures and --skip-version-check options, 
>>>>>>>> might be
>>>>>>>> better.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> ipa-server-upgrade should probably also have --force, even if it
>>>>>>>>> does the same thing as --skip-version-check, again because 
>>>>>>>>> --force
>>>>>>>>> is common.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a weird API:
>>>>>>>>>
>>>>>>>>> +        if data_upgrade.badsyntax:
>>>>>>>>> +            raise admintool.ScriptError(
>>>>>>>>> +                'Bad syntax detected in upgrade file(s).', 1)
>>>>>>>>> +        elif data_upgrade.upgradefailed:
>>>>>>>>> +            raise admintool.ScriptError('IPA upgrade 
>>>>>>>>> failed.', 1)
>>>>>>>>> +        elif data_upgrade.modified:
>>>>>>>>> +            self.log.info('Data update complete')
>>>>>>>>> +        else:
>>>>>>>>> +            self.log.info('Data update complete, no data were
>>>>>>>>> modified')
>>>>>>>>>
>>>>>>>>> Why does not IPAUpgrade raise errors instead?
>>>>>>>>>
>>>>>>>> For historical reasons, I can investigate what would break this
>>>>>>>> change, I will send it in separate patch.
>>>>>>>>>
>>>>>>>>> +class IPAVersionError(Exception):
>>>>>>>>> +    pass
>>>>>>>>> +
>>>>>>>>> +class PlatformMismatchError(IPAVersionError):
>>>>>>>>> +    pass
>>>>>>>>> +
>>>>>>>>> +class DataUpgradeRequiredError(IPAVersionError):
>>>>>>>>> +    pass
>>>>>>>>> +
>>>>>>>>> +class DataInNewerVersionError(IPAVersionError):
>>>>>>>>> +    pass
>>>>>>>>>
>>>>>>>>> I don't like the "IPA" in "IPAVersionError", it does not tell you
>>>>>>>>> much about what kind of version is that. Also data version errors
>>>>>>>>> should only tell you what is wrong, not how you fix it. IMO 
>>>>>>>>> better
>>>>>>>>> names for these would be e.g. "UpgradeVersionError",
>>>>>>>>> "UpgradePlatformError", "UpgradeDataOlderVersionError",
>>>>>>>>> "UpgradeDataNewerVersionError". Similar for store_ipa_version and
>>>>>>>>> check_ipa_version.
>>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>> Why is it not an error if there is no version in 
>>>>>>>>> check_ipa_version?
>>>>>>>>> IMO it should, even if you then ignore the exception most of the
>>>>>>>>> time.
>>>>>>>> I can raise error in that case and ignore the exception.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Honza
>>>>>>>>>
>>>>>>>> Martin^2
>>>>>>>>
>>>>>>> Updated patches attached.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Updated patches attached
>>>>>>
>>>>>> -- 
>>>>>> Martin Basti
>>>>>>
>>>>>>
>>>>>
>>>>> Updated patch attached
>>>>>
>>>>
>>>> Looks good to me and works as expected. Honza, are you OK with the 
>>>> patches?
>>>>
>>>
>>> Some nitpicks:
>>>
>>> The command line tool class should be named "ServerUpgrade" rather than
>>> "IPAServerUpgrade" for consistency with others.
>>>
>>> The deprecated --debug option should not be used in new commands.
>>
>> Why is --debug option deprecated? I thought we wanted to deprecate 
>> --verbose
>> option as --debug is used in most our CLI tools. Well, except 
>> ipa-ldap-updated
>> which for some reasons marks --debug as deprecated. It does not 
>> matter now,
>> given the command is removed/changed.
>
> AdminTool provides --debug as a deprecated alias for --verbose when a 
> subclass requests it. It seems the decision to deprecate --debug was 
> already made back when AdminTool was introduced, so let's trust that 
> decision.
>
Yes that is reason. I will update design as well

-- 
Martin Basti