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

[Martin Kosek]

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.

> 
> I would like to see --skip-version-check also in ipa-server-upgrade, for
> consistency with ipactl.
> 
> In the spec file ipa-server-upgrade is run with --quiet, so why redirect stdout
> to /dev/null?