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

[Jan Cholasta]

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.

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?

-- 
Jan Cholasta