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

[Jan Cholasta]

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.

-- 
Jan Cholasta