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

[Martin Basti]

On 29/04/15 07:34, 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.
>
> 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?
>
Because --quiet is not quiet enough.

It prints upgrade steps to stdout.

ipa-server-upgrade --quiet
Upgrading IPA:
   [1/9]: stopping directory server
   [2/9]: saving configuration
   [3/9]: disabling listeners
   [4/9]: enabling DS global lock
   [5/9]: starting directory server
   [6/9]: upgrading server
   [7/9]: stopping directory server
   [8/9]: restoring configuration
   [9/9]: starting directory server
Done.
IPA upgrade failed.


-- 
Martin Basti