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

2015-05-04 Thread Jan Cholasta

Dne 29.4.2015 v 15:27 Martin Basti napsal(a):

On 29/04/15 13:22, Martin Kosek wrote:

On 04/29/2015 12:59 PM, Martin Kosek wrote:

On 04/29/2015 12:50 PM, Martin Basti wrote:

On 29/04/15 12:39, Martin Kosek wrote:

On 04/29/2015 12:15 PM, Martin Basti wrote:

On 29/04/15 08:52, Jan Cholasta wrote:

Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):

On 04/29/2015 07:34 AM, Jan Cholasta wrote:

...

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.


Yes that is reason.

No, it's not.

I will update design as well

Nope. This decision was never made this way, AFAIR. --debug is what
all the
main tools (ipa-server-install, ipa-replica-install,
ipa-client-install) use
and we never agreed that we want to change it.

In fact, I think I remember some discussion from Devconf.cz time,
when we
mentioned that the ipa-ldap-updater has it the deprecated status
wrong way,
that we want --debug. CCing Simo since he may have been in the
conversation.

http://freeipa.org/page/V3/Logging_and_output

In commands that currently have it, the `-d, --debug` option will
become a
deprecated alias for --verbose.

I see, I must somehow missed that aspect of the miniframework. Well,
question
is - is it really a good decision and thing we should do?

I.e. slowly moving towards --verbose option, deprecating --debug,
given we use
--debug in most commands and people are using it? This could cause
lot of
unnecessary churn in stable distributions that would wish to rebase
to FreeIPA,
like CentOS or RHEL - and for what reason?

I will be against removing --debug option from the main commands
unless there
is a very good reason and justification to do so.

Martin

I talked to Martin in person. If --debug option is not removed and is
kept in
the old commands and you really want to go with the --verbose option
crusade, I
can live with it.

Martin

Updated patches attached.

* Removed --debug version
* I also added log message that version check was skipped



Thanks, ACK.

Pushed to master: 3debc7b2b54b7926dcd2b26a37b3dc0677c3bc61

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Kosek
On 04/29/2015 12:15 PM, Martin Basti wrote:
 On 29/04/15 08:52, Jan Cholasta wrote:
 Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):
 On 04/29/2015 07:34 AM, Jan Cholasta wrote:
...
 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.

 Yes that is reason.

No, it's not.

I will update design as well

Nope. This decision was never made this way, AFAIR. --debug is what all the
main tools (ipa-server-install, ipa-replica-install, ipa-client-install) use
and we never agreed that we want to change it.

In fact, I think I remember some discussion from Devconf.cz time, when we
mentioned that the ipa-ldap-updater has it the deprecated status wrong way,
that we want --debug. CCing Simo since he may have been in the conversation.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Basti

On 29/04/15 12:39, Martin Kosek wrote:

On 04/29/2015 12:15 PM, Martin Basti wrote:

On 29/04/15 08:52, Jan Cholasta wrote:

Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):

On 04/29/2015 07:34 AM, Jan Cholasta wrote:

...

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.


Yes that is reason.

No, it's not.

I will update design as well

Nope. This decision was never made this way, AFAIR. --debug is what all the
main tools (ipa-server-install, ipa-replica-install, ipa-client-install) use
and we never agreed that we want to change it.

In fact, I think I remember some discussion from Devconf.cz time, when we
mentioned that the ipa-ldap-updater has it the deprecated status wrong way,
that we want --debug. CCing Simo since he may have been in the conversation.

http://freeipa.org/page/V3/Logging_and_output

In commands that currently have it, the `-d, --debug` option will 
become a deprecated alias for --verbose.


--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Kosek
On 04/29/2015 12:50 PM, Martin Basti wrote:
 On 29/04/15 12:39, Martin Kosek wrote:
 On 04/29/2015 12:15 PM, Martin Basti wrote:
 On 29/04/15 08:52, Jan Cholasta wrote:
 Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):
 On 04/29/2015 07:34 AM, Jan Cholasta wrote:
 ...
 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.

 Yes that is reason.
 No, it's not.

 I will update design as well

 Nope. This decision was never made this way, AFAIR. --debug is what all the
 main tools (ipa-server-install, ipa-replica-install, ipa-client-install) use
 and we never agreed that we want to change it.

 In fact, I think I remember some discussion from Devconf.cz time, when we
 mentioned that the ipa-ldap-updater has it the deprecated status wrong way,
 that we want --debug. CCing Simo since he may have been in the conversation.
 http://freeipa.org/page/V3/Logging_and_output
 
 In commands that currently have it, the `-d, --debug` option will become a
 deprecated alias for --verbose.

I see, I must somehow missed that aspect of the miniframework. Well, question
is - is it really a good decision and thing we should do?

I.e. slowly moving towards --verbose option, deprecating --debug, given we use
--debug in most commands and people are using it? This could cause lot of
unnecessary churn in stable distributions that would wish to rebase to FreeIPA,
like CentOS or RHEL - and for what reason?

I will be against removing --debug option from the main commands unless there
is a very good reason and justification to do so.

Martin

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Kosek
On 04/29/2015 12:59 PM, Martin Kosek wrote:
 On 04/29/2015 12:50 PM, Martin Basti wrote:
 On 29/04/15 12:39, Martin Kosek wrote:
 On 04/29/2015 12:15 PM, Martin Basti wrote:
 On 29/04/15 08:52, Jan Cholasta wrote:
 Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):
 On 04/29/2015 07:34 AM, Jan Cholasta wrote:
 ...
 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.

 Yes that is reason.
 No, it's not.

 I will update design as well

 Nope. This decision was never made this way, AFAIR. --debug is what all the
 main tools (ipa-server-install, ipa-replica-install, ipa-client-install) use
 and we never agreed that we want to change it.

 In fact, I think I remember some discussion from Devconf.cz time, when we
 mentioned that the ipa-ldap-updater has it the deprecated status wrong way,
 that we want --debug. CCing Simo since he may have been in the conversation.
 http://freeipa.org/page/V3/Logging_and_output

 In commands that currently have it, the `-d, --debug` option will become a
 deprecated alias for --verbose.
 
 I see, I must somehow missed that aspect of the miniframework. Well, question
 is - is it really a good decision and thing we should do?
 
 I.e. slowly moving towards --verbose option, deprecating --debug, given we use
 --debug in most commands and people are using it? This could cause lot of
 unnecessary churn in stable distributions that would wish to rebase to 
 FreeIPA,
 like CentOS or RHEL - and for what reason?
 
 I will be against removing --debug option from the main commands unless there
 is a very good reason and justification to do so.
 
 Martin

I talked to Martin in person. If --debug option is not removed and is kept in
the old commands and you really want to go with the --verbose option crusade, I
can live with it.

Martin

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread 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?

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Basti

On 29/04/15 08:52, Jan Cholasta wrote:

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.



Yes that is reason. I will update design as well

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-29 Thread Martin Basti

On 29/04/15 13:22, Martin Kosek wrote:

On 04/29/2015 12:59 PM, Martin Kosek wrote:

On 04/29/2015 12:50 PM, Martin Basti wrote:

On 29/04/15 12:39, Martin Kosek wrote:

On 04/29/2015 12:15 PM, Martin Basti wrote:

On 29/04/15 08:52, Jan Cholasta wrote:

Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):

On 04/29/2015 07:34 AM, Jan Cholasta wrote:

...

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.


Yes that is reason.

No, it's not.

I will update design as well

Nope. This decision was never made this way, AFAIR. --debug is what all the
main tools (ipa-server-install, ipa-replica-install, ipa-client-install) use
and we never agreed that we want to change it.

In fact, I think I remember some discussion from Devconf.cz time, when we
mentioned that the ipa-ldap-updater has it the deprecated status wrong way,
that we want --debug. CCing Simo since he may have been in the conversation.

http://freeipa.org/page/V3/Logging_and_output

In commands that currently have it, the `-d, --debug` option will become a
deprecated alias for --verbose.

I see, I must somehow missed that aspect of the miniframework. Well, question
is - is it really a good decision and thing we should do?

I.e. slowly moving towards --verbose option, deprecating --debug, given we use
--debug in most commands and people are using it? This could cause lot of
unnecessary churn in stable distributions that would wish to rebase to FreeIPA,
like CentOS or RHEL - and for what reason?

I will be against removing --debug option from the main commands unless there
is a very good reason and justification to do so.

Martin

I talked to Martin in person. If --debug option is not removed and is kept in
the old commands and you really want to go with the --verbose option crusade, I
can live with it.

Martin

Updated patches attached.

* Removed --debug version
* I also added log message that version check was skipped

--
Martin Basti

From aa1aaa52060f4927f4f8e20a80f8735be7a1a3a3 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 2 Apr 2015 14:14:15 +0200
Subject: [PATCH 1/3] Server Upgrade: ipa-server-upgrade command

https://fedorahosted.org/freeipa/ticket/4904
---
 freeipa.spec.in |  2 +
 install/tools/Makefile.am   |  1 +
 install/tools/ipa-server-upgrade| 12 ++
 install/tools/man/Makefile.am   |  1 +
 install/tools/man/ipa-server-upgrade.1  | 40 ++
 ipaserver/install/ipa_server_upgrade.py | 72 +
 6 files changed, 128 insertions(+)
 create mode 100644 install/tools/ipa-server-upgrade
 create mode 100644 install/tools/man/ipa-server-upgrade.1
 create mode 100644 ipaserver/install/ipa_server_upgrade.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 608242b5adbc43efbbf0ae30a6d7a933bebc1084..c661fe574464fdba1b1a8c64710d44a012ec8ede 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -660,6 +660,7 @@ fi
 %{_sbindir}/ipa-replica-manage
 %{_sbindir}/ipa-csreplica-manage
 %{_sbindir}/ipa-server-certinstall
+%{_sbindir}/ipa-server-upgrade
 %{_sbindir}/ipa-ldap-updater
 %{_sbindir}/ipa-otptoken-import
 %{_sbindir}/ipa-compat-manage
@@ -804,6 +805,7 @@ fi
 %{_mandir}/man1/ipa-replica-prepare.1.gz
 %{_mandir}/man1/ipa-server-certinstall.1.gz
 %{_mandir}/man1/ipa-server-install.1.gz
+%{_mandir}/man1/ipa-server-upgrade.1.gz
 %{_mandir}/man1/ipa-dns-install.1.gz
 %{_mandir}/man1/ipa-ca-install.1.gz
 %{_mandir}/man1/ipa-kra-install.1.gz
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index b791a8c748a18602f88522c3a0e3d74499700ae0..e5d45c47966a503da9f25aec13175793a36962e4 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -16,6 +16,7 @@ sbin_SCRIPTS =			\
 	ipa-replica-manage	\
 	ipa-csreplica-manage	\
 	ipa-server-certinstall  \
+	ipa-server-upgrade	\
 	ipactl			\
 	ipa-compat-manage	\
 	ipa-nis-manage		\
diff --git a/install/tools/ipa-server-upgrade b/install/tools/ipa-server-upgrade
new file mode 100644
index ..781b0d3dbcf2b1b5493126ad3d7eb74032864dbb
--- /dev/null
+++ b/install/tools/ipa-server-upgrade
@@ -0,0 +1,12 @@
+#!/usr/bin/python2
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+# Documentation can be found at:
+# http://freeipa.org/page/LdapUpdate
+# 

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

2015-04-28 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-27 Thread Martin Basti

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

From 9ba1dc90b12c70053a926e3dbfbd6aaa4d9fc920 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 2 Apr 2015 14:14:15 +0200
Subject: [PATCH 1/3] Server Upgrade: ipa-server-upgrade command

https://fedorahosted.org/freeipa/ticket/4904
---
 freeipa.spec.in |  2 +
 install/tools/Makefile.am   |  1 +
 install/tools/ipa-server-upgrade| 12 ++
 install/tools/man/Makefile.am   |  1 +
 install/tools/man/ipa-server-upgrade.1  | 40 ++
 ipaserver/install/ipa_server_upgrade.py | 72 +
 6 files changed, 128 insertions(+)
 create mode 100644 install/tools/ipa-server-upgrade
 create mode 100644 install/tools/man/ipa-server-upgrade.1
 create mode 100644 ipaserver/install/ipa_server_upgrade.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 608242b5adbc43efbbf0ae30a6d7a933bebc1084..c661fe574464fdba1b1a8c64710d44a012ec8ede 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -660,6 +660,7 @@ fi
 %{_sbindir}/ipa-replica-manage
 %{_sbindir}/ipa-csreplica-manage
 %{_sbindir}/ipa-server-certinstall
+%{_sbindir}/ipa-server-upgrade
 %{_sbindir}/ipa-ldap-updater
 %{_sbindir}/ipa-otptoken-import
 %{_sbindir}/ipa-compat-manage
@@ -804,6 +805,7 @@ fi
 %{_mandir}/man1/ipa-replica-prepare.1.gz
 %{_mandir}/man1/ipa-server-certinstall.1.gz
 %{_mandir}/man1/ipa-server-install.1.gz
+%{_mandir}/man1/ipa-server-upgrade.1.gz
 %{_mandir}/man1/ipa-dns-install.1.gz
 %{_mandir}/man1/ipa-ca-install.1.gz
 %{_mandir}/man1/ipa-kra-install.1.gz
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index b791a8c748a18602f88522c3a0e3d74499700ae0..e5d45c47966a503da9f25aec13175793a36962e4 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -16,6 +16,7 @@ sbin_SCRIPTS =			\
 	ipa-replica-manage	\
 	ipa-csreplica-manage	\
 	ipa-server-certinstall  \
+	ipa-server-upgrade	\
 	ipactl			\
 	ipa-compat-manage	\
 	ipa-nis-manage		\
diff --git a/install/tools/ipa-server-upgrade b/install/tools/ipa-server-upgrade
new file mode 100644
index ..747024847a7c9d8837b4968395e2c63648a792cf
--- /dev/null
+++ 

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

2015-04-27 Thread Martin Basti

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

--
Martin Basti

From 9ba1dc90b12c70053a926e3dbfbd6aaa4d9fc920 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 2 Apr 2015 14:14:15 +0200
Subject: [PATCH 1/3] Server Upgrade: ipa-server-upgrade command

https://fedorahosted.org/freeipa/ticket/4904
---
 freeipa.spec.in |  2 +
 install/tools/Makefile.am   |  1 +
 install/tools/ipa-server-upgrade| 12 ++
 install/tools/man/Makefile.am   |  1 +
 install/tools/man/ipa-server-upgrade.1  | 40 ++
 ipaserver/install/ipa_server_upgrade.py | 72 +
 6 files changed, 128 insertions(+)
 create mode 100644 install/tools/ipa-server-upgrade
 create mode 100644 install/tools/man/ipa-server-upgrade.1
 create mode 100644 ipaserver/install/ipa_server_upgrade.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 608242b5adbc43efbbf0ae30a6d7a933bebc1084..c661fe574464fdba1b1a8c64710d44a012ec8ede 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -660,6 +660,7 @@ fi
 %{_sbindir}/ipa-replica-manage
 %{_sbindir}/ipa-csreplica-manage
 %{_sbindir}/ipa-server-certinstall
+%{_sbindir}/ipa-server-upgrade
 %{_sbindir}/ipa-ldap-updater
 %{_sbindir}/ipa-otptoken-import
 %{_sbindir}/ipa-compat-manage
@@ -804,6 +805,7 @@ fi
 %{_mandir}/man1/ipa-replica-prepare.1.gz
 %{_mandir}/man1/ipa-server-certinstall.1.gz
 %{_mandir}/man1/ipa-server-install.1.gz
+%{_mandir}/man1/ipa-server-upgrade.1.gz
 %{_mandir}/man1/ipa-dns-install.1.gz
 %{_mandir}/man1/ipa-ca-install.1.gz
 %{_mandir}/man1/ipa-kra-install.1.gz
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index b791a8c748a18602f88522c3a0e3d74499700ae0..e5d45c47966a503da9f25aec13175793a36962e4 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -16,6 +16,7 @@ sbin_SCRIPTS =			\
 	ipa-replica-manage	\
 	ipa-csreplica-manage	\
 	ipa-server-certinstall  \
+	ipa-server-upgrade	\
 	ipactl			\
 	ipa-compat-manage	\
 	ipa-nis-manage		\
diff --git a/install/tools/ipa-server-upgrade b/install/tools/ipa-server-upgrade
new file mode 100644
index 

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

2015-04-27 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-23 Thread Martin Basti

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.

--
Martin Basti

From 06250c0e49ffc25ae4f7c3de53675ef40fb4ec79 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 2 Apr 2015 14:14:15 +0200
Subject: [PATCH 1/3] Server Upgrade: ipa-server-upgrade command

https://fedorahosted.org/freeipa/ticket/4904
---
 freeipa.spec.in |  2 +
 install/tools/Makefile.am   |  1 +
 install/tools/ipa-server-upgrade| 12 ++
 install/tools/man/Makefile.am   |  1 +
 install/tools/man/ipa-server-upgrade.1  | 40 ++
 ipaserver/install/ipa_server_upgrade.py | 72 +
 6 files changed, 128 insertions(+)
 create mode 100644 install/tools/ipa-server-upgrade
 create mode 100644 install/tools/man/ipa-server-upgrade.1
 create mode 100644 ipaserver/install/ipa_server_upgrade.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 8d58f2568e1de418c25cb1bd34fc7d4736a15e54..0e262d445b80a279488fa77860168f46a0c8a045 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -660,6 +660,7 @@ fi
 %{_sbindir}/ipa-replica-manage
 %{_sbindir}/ipa-csreplica-manage
 %{_sbindir}/ipa-server-certinstall
+%{_sbindir}/ipa-server-upgrade
 %{_sbindir}/ipa-ldap-updater
 %{_sbindir}/ipa-otptoken-import
 %{_sbindir}/ipa-compat-manage
@@ -804,6 +805,7 @@ fi
 %{_mandir}/man1/ipa-replica-prepare.1.gz
 %{_mandir}/man1/ipa-server-certinstall.1.gz
 %{_mandir}/man1/ipa-server-install.1.gz
+%{_mandir}/man1/ipa-server-upgrade.1.gz
 %{_mandir}/man1/ipa-dns-install.1.gz
 %{_mandir}/man1/ipa-ca-install.1.gz
 %{_mandir}/man1/ipa-kra-install.1.gz
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index b791a8c748a18602f88522c3a0e3d74499700ae0..e5d45c47966a503da9f25aec13175793a36962e4 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -16,6 +16,7 @@ sbin_SCRIPTS =			\
 	ipa-replica-manage	\
 	ipa-csreplica-manage	\
 	ipa-server-certinstall  \
+	ipa-server-upgrade	\
 	ipactl			\
 	ipa-compat-manage	\
 	ipa-nis-manage		\
diff --git a/install/tools/ipa-server-upgrade b/install/tools/ipa-server-upgrade
new file mode 100644
index ..747024847a7c9d8837b4968395e2c63648a792cf
--- /dev/null
+++ b/install/tools/ipa-server-upgrade
@@ -0,0 +1,12 @@
+#!/usr/bin/python2
+#

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

2015-04-21 Thread Jan Cholasta

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).


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?


+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.



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.



Honza

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-21 Thread Martin Basti

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

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-20 Thread David Kupka

On 04/16/2015 05:14 PM, Martin Basti wrote:

On 15/04/15 16:26, Martin Basti wrote:

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.




Updated patches attached.

--
Martin Basti


Hi,
thanks for the patches. Could you please split them correctly?
I mean patch 227 and 228. In patch 227 you add whole file 
ipa_server_upgrade.py and in patch 228 add forgotten import and change 
option description slightly.


Otherwise it works for me.

--
David Kupka

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code


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

2015-04-16 Thread Martin Basti

On 15/04/15 16:26, Martin Basti wrote:

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.





Updated patches attached.

--
Martin Basti

From c7421c744d3b42c5530324df7a84632900a77d14 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 2 Apr 2015 14:14:15 +0200
Subject: [PATCH 1/3] Server Upgrade: ipa-server-upgrade command

---
 freeipa.spec.in |  2 +
 install/tools/Makefile.am   |  1 +
 install/tools/ipa-server-upgrade| 12 +
 install/tools/man/Makefile.am   |  1 +
 install/tools/man/ipa-server-upgrade.1  | 96 +
 ipaserver/install/ipa_server_upgrade.py | 74 +
 6 files changed, 186 insertions(+)
 create mode 100644 install/tools/ipa-server-upgrade
 create mode 100644 install/tools/man/ipa-server-upgrade.1
 create mode 100644 ipaserver/install/ipa_server_upgrade.py

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 8d58f2568e1de418c25cb1bd34fc7d4736a15e54..0e262d445b80a279488fa77860168f46a0c8a045 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -660,6 +660,7 @@ fi
 %{_sbindir}/ipa-replica-manage
 %{_sbindir}/ipa-csreplica-manage
 %{_sbindir}/ipa-server-certinstall
+%{_sbindir}/ipa-server-upgrade
 %{_sbindir}/ipa-ldap-updater
 %{_sbindir}/ipa-otptoken-import
 %{_sbindir}/ipa-compat-manage
@@ -804,6 +805,7 @@ fi
 %{_mandir}/man1/ipa-replica-prepare.1.gz
 %{_mandir}/man1/ipa-server-certinstall.1.gz
 %{_mandir}/man1/ipa-server-install.1.gz
+%{_mandir}/man1/ipa-server-upgrade.1.gz
 %{_mandir}/man1/ipa-dns-install.1.gz
 %{_mandir}/man1/ipa-ca-install.1.gz
 %{_mandir}/man1/ipa-kra-install.1.gz
diff --git a/install/tools/Makefile.am b/install/tools/Makefile.am
index b791a8c748a18602f88522c3a0e3d74499700ae0..e5d45c47966a503da9f25aec13175793a36962e4 100644
--- a/install/tools/Makefile.am
+++ b/install/tools/Makefile.am
@@ -16,6 +16,7 @@ sbin_SCRIPTS =			\
 	ipa-replica-manage	\
 	ipa-csreplica-manage	\
 	ipa-server-certinstall  \
+	ipa-server-upgrade	\
 	ipactl			\
 	ipa-compat-manage	\
 	ipa-nis-manage		\
diff --git a/install/tools/ipa-server-upgrade b/install/tools/ipa-server-upgrade
new file mode 100644
index ..747024847a7c9d8837b4968395e2c63648a792cf
--- /dev/null
+++ b/install/tools/ipa-server-upgrade
@@ -0,0 +1,12 @@
+#!/usr/bin/python2
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+# Documentation can be found at:
+# http://freeipa.org/page/LdapUpdate
+# http://www.freeipa.org/page/V4/Server_Upgrade_Refactoring
+
+from ipaserver.install.ipa_server_upgrade import IPAServerUpgrade
+
+IPAServerUpgrade.run_cli()
diff --git a/install/tools/man/Makefile.am b/install/tools/man/Makefile.am
index 38c049c79fbd2ce22888b47ee576c4574e98c45b..6db1776191ca855986a152dbd4854a0dc1b744d7 100644
--- a/install/tools/man/Makefile.am
+++ b/install/tools/man/Makefile.am
@@ -12,6 +12,7 @@ man1_MANS = \
 	ipa-replica-prepare.1		\
 	ipa-server-certinstall.1	\
 	ipa-server-install.1		\
+	ipa-server-upgrade.1		\
 	ipa-dns-install.1		\
 	ipa-adtrust-install.1		\
 	ipa-ca-install.1		\
diff --git a/install/tools/man/ipa-server-upgrade.1 b/install/tools/man/ipa-server-upgrade.1
new file mode 100644
index ..9b5387301faf749515dcee6f7f5025d9916b2882
--- /dev/null
+++ b/install/tools/man/ipa-server-upgrade.1
@@ -0,0 +1,96 @@
+.\
+.\ Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+.\
+
+.TH ipa-server-upgrade 1 April 02 2015 FreeIPA FreeIPA Manual Pages
+.SH NAME
+ipa\-server\-upgrade \- upgrade IPA server
+.SH SYNOPSIS
+ipa\-server\-upgrade [options]
+.SH DESCRIPTION
+ipa\-server\-upgrade is used to upgrade IPA server when the IPA packages are being updated. It is not intended to be executed by end\-users.
+
+ipa\-server\-upgrade will:
+
+* update LDAP schema
+* process all files with the extension .update in /usr/share/ipa/updates (including update plugins).
+* upgrade local configurations of IPA services
+
+An update file describes an LDAP entry and a set of operations to be performed on that entry. It can be used to add new entries or modify existing entries.
+
+Blank lines and lines beginning with # are ignored.
+
+There are 7 keywords:
+
+* default: the starting value
+* add: add a value (or values) to an attribute
+* remove: remove a value (or values) from an attribute
+* only: set an attribute to this
+* onlyifexist: set an attribute to this only if the entry exists
+* deleteentry: remove the entry
+* replace: replace an existing value, format is old: new
+* addifnew: add a new attribute and