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

[David Kupka]

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?

-- 
David Kupka