Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Martin Babinsky 

On 04/21/2016 12:12 PM, Petr Vobornik wrote:

On 04/21/2016 10:41 AM, Ludwig Krispenz wrote:


On 04/21/2016 10:11 AM, Martin Babinsky wrote:

On 04/21/2016 09:21 AM, Jan Cholasta wrote:

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1].
A CI
test suite is also included.

There are some issues with the patch I would like to discuss in
more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to
port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?


I don't think it applies anymore. These messages were there so the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.


+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology,"
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining
services"
"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g.
topology
state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep
the
two
entities separate without some hacking around framework
capabilities


I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are
safe.




3.) since actions in post-callback require a knowledge about
topology
state gathered in pre-callback, I had to store some information in
the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is
this
okay?


Why can't the master remove itself?


Because it removes its own replication agreements hence any
changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't
replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry


That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.


What kind of cleanup is it? Can it be done before instead?



Well most of the code can be run in pre-callback if all the checks are
passed/forced.

However there is a check for deleted segments and this one should be
run after the removal of master entry to see if topology plugin
removed all dangling segments pointing to master. I am not quite sure
if it make sense to run this check in the master which is being removed.

no, it is not guranteed that the information on the removed master will
be correct. If the del is applied to the to be removed master the
topology plugin will only on a master which is remaining start the
removal of segments, these will alos be replicated back to the removed
master, but the repl agreements to this master will also be removed, so
no gurantee which mods will be available on the removed master, and you
should also be able to remove a master if it is down - so applying the
full removal on a remaining server makes sense.

What is the behaviour if the removal of a server would disconnect
topology ?




What would be the use-case for master to remove itself?

The only one I see, which was proposed in design page is that in
`ipa-server-install --uninstall` the installer would call `ipa
server-del $to_be_removed` on different replica so that uninstallation
would be done in single step. But effectively this is not removing
itself from API point of view.

Calling `ipa server-del $me` without subsequent uninstallation seems
pointless to me. In `ipa-re

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Martin Babinsky 

On 04/21/2016 12:22 PM, Ludwig Krispenz wrote:


On 04/21/2016 12:12 PM, Petr Vobornik wrote:

On 04/21/2016 10:41 AM, Ludwig Krispenz wrote:

On 04/21/2016 10:11 AM, Martin Babinsky wrote:

On 04/21/2016 09:21 AM, Jan Cholasta wrote:

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:

On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1].
A CI
test suite is also included.

There are some issues with the patch I would like to discuss in
more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to
port
these as messages to the command which results in quite
voluminous
response sent back to the frontend. Should we try to reduce the
output?

I don't think it applies anymore. These messages were there so
the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.

+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology,"
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining
services"
"performing cleanup"
"Deleted server {server}"



2.) In the original discussion[2] we assumed that the cleanup
part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g.
topology
state from pre-callback, messages) i have merged it to
server-del.
That
makes the code rather unwieldy but I found it difficult to keep
the
two
entities separate without some hacking around framework
capabilities

I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.

The patch adds a force option, which allows you to re-run
server-del
even if the master entry does not exist anymore, so I think we are
safe.


3.) since actions in post-callback require a knowledge about
topology
state gathered in pre-callback, I had to store some
information in
the
command's context. Sorry about that, if you know about some
way to
circumvent me, let me know.

Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this
topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an
ad-hoc
forwarding of the request to other master that can do the
job. Is
this
okay?

Why can't the master remove itself?


Because it removes its own replication agreements hence any
changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't
replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the
master
entry

That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.

What kind of cleanup is it? Can it be done before instead?


Well most of the code can be run in pre-callback if all the checks are
passed/forced.

However there is a check for deleted segments and this one should be
run after the removal of master entry to see if topology plugin
removed all dangling segments pointing to master. I am not quite sure
if it make sense to run this check in the master which is being
removed.

no, it is not guranteed that the information on the removed master will
be correct. If the del is applied to the to be removed master the
topology plugin will only on a master which is remaining start the
removal of segments, these will alos be replicated back to the removed
master, but the repl agreements to this master will also be removed, so
no gurantee which mods will be available on the removed master, and you
should also be able to remove a master if it is down - so applying the
full removal on a remaining server makes sense.

What is the behaviour if the removal of a server would disconnect
topology ?



What would be the use-case for master to remove itself?

I don't think that there is a use case except uninstall, but this
doesen't prevent users to run the command. And then there are a few
options how to handle this.
- accept it and try to do the best
- reject it and say it should be run on any other master
- automatically execute this on another master - I think this is
Martin's current plan


Yes, also if you r

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Ludwig Krispenz 


On 04/21/2016 12:12 PM, Petr Vobornik wrote:

On 04/21/2016 10:41 AM, Ludwig Krispenz wrote:

On 04/21/2016 10:11 AM, Martin Babinsky wrote:

On 04/21/2016 09:21 AM, Jan Cholasta wrote:

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:

On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1].
A CI
test suite is also included.

There are some issues with the patch I would like to discuss in
more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to
port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?

I don't think it applies anymore. These messages were there so the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.

+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology,"
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining
services"
"performing cleanup"
"Deleted server {server}"



2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g.
topology
state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep
the
two
entities separate without some hacking around framework
capabilities

I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.

The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are
safe.


3.) since actions in post-callback require a knowledge about
topology
state gathered in pre-callback, I had to store some information in
the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.

Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is
this
okay?

Why can't the master remove itself?


Because it removes its own replication agreements hence any
changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't
replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry

That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.

What kind of cleanup is it? Can it be done before instead?


Well most of the code can be run in pre-callback if all the checks are
passed/forced.

However there is a check for deleted segments and this one should be
run after the removal of master entry to see if topology plugin
removed all dangling segments pointing to master. I am not quite sure
if it make sense to run this check in the master which is being removed.

no, it is not guranteed that the information on the removed master will
be correct. If the del is applied to the to be removed master the
topology plugin will only on a master which is remaining start the
removal of segments, these will alos be replicated back to the removed
master, but the repl agreements to this master will also be removed, so
no gurantee which mods will be available on the removed master, and you
should also be able to remove a master if it is down - so applying the
full removal on a remaining server makes sense.

What is the behaviour if the removal of a server would disconnect
topology ?



What would be the use-case for master to remove itself?
I don't think that there is a use case except uninstall, but this 
doesen't prevent users to run the command. And then there are a few 
options how to handle this.

- accept it and try to do the best
- reject it and say it should be run on any other master
- automatically execute this on another master - I think this is 
Martin's current plan


The only one I see, which was proposed in design page is that i

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Petr Vobornik 
On 04/21/2016 10:41 AM, Ludwig Krispenz wrote:
> 
> On 04/21/2016 10:11 AM, Martin Babinsky wrote:
>> On 04/21/2016 09:21 AM, Jan Cholasta wrote:
>>> On 19.4.2016 12:42, Martin Babinsky wrote:
 On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:
>
> On 04/14/2016 10:59 AM, Martin Babinsky wrote:
>> On 04/14/2016 08:24 AM, Jan Cholasta wrote:
>>> On 13.4.2016 17:10, Rob Crittenden wrote:
 Martin Babinsky wrote:
> This is a WIP patch which moves the `ipa-replica-manage del`
> subcommand
> to the 'server-del' API method and exposes it as CLI command[1].
> A CI
> test suite is also included.
>
> There are some issues with the patch I would like to discuss in
> more
> detail on the list:
>
> 1.) In the original subcommand there was a lot of output (mostly
> print
> statements) during all stages of master removal. I have tried to
> port
> these as messages to the command which results in quite voluminous
> response sent back to the frontend. Should we try to reduce the
> output?

 I don't think it applies anymore. These messages were there so the
 user
 would know something was happening. If it is an API command there
 isn't
 much we can do other than add something to the cli to print "This
 could
 take a bit" before making the call.
>>>
>>> +1
>>>
>> This is already implemented in PoC. So I guess we may reduce the
>> output only to the following:
>>
>>
>> In CLI print:
>> "Removing {server} from replication topology, "
>> "please wait...
>>
>> The adding info messages:
>>
>> "checking topology connectivity" | "skipping topology connectivity
>> check"
>> "checking remaining services" | "skipping check for remaining
>> services"
>> "performing cleanup"
>> "Deleted server {server}"
>>
>>

> 2.) In the original discussion[2] we assumed that the cleanup part
> would
> me a separate API method called during server_del postcallback.
> However
> since the two objects ended up sharing a lot of state (e.g.
> topology
> state from pre-callback, messages) i have merged it to server-del.
> That
> makes the code rather unwieldy but I found it difficult to keep
> the
> two
> entities separate without some hacking around framework
> capabilities

 I haven't looked at the code but as a general principal having
 separate
 operations has saved our bacon on more than one occasion.
>>>
>>> The patch adds a force option, which allows you to re-run server-del
>>> even if the master entry does not exist anymore, so I think we are
>>> safe.
>>>

> 3.) since actions in post-callback require a knowledge about
> topology
> state gathered in pre-callback, I had to store some information in
> the
> command's context. Sorry about that, if you know about some way to
> circumvent me, let me know.

 Looks like it is the only way since you are extending server_del.
 Another option would be to drop pre/post and add all this topology
 stuff
 directly to server_del execute.

> 4.) The master can not remove itself. I have implemented an ad-hoc
> forwarding of the request to other master that can do the job. Is
> this
> okay?
>>>
>>> Why can't the master remove itself?
>>>
>> Because it removes its own replication agreements hence any
>> changes in
>> DIT (like removed principals, s4u2 proxy targets etc.) won't
>> replicate
>> to other masters.
> It shouldn't remove replication agreements, in fact this should be
> prevented by the topology plugin.
> The removal of the agreements will be triggered by removing the master
> entry

 That is true, but there is a plenty of cleanup code that is executed
 *after* the master entry is removed and these changes would not
 replicate if the agreements were removed by topology plugin in the
 meanwhile.
>>>
>>> What kind of cleanup is it? Can it be done before instead?
>>>
>>
>> Well most of the code can be run in pre-callback if all the checks are
>> passed/forced.
>>
>> However there is a check for deleted segments and this one should be
>> run after the removal of master entry to see if topology plugin
>> removed all dangling segments pointing to master. I am not quite sure
>> if it make sense to run this check in the master which is being removed.
> no, it is not guranteed that the information on the removed master will
> be correct. If the del is applied to the to be removed master the
> topology plugin will only on a master which is remaining star

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Ludwig Krispenz 


On 04/21/2016 10:11 AM, Martin Babinsky wrote:

On 04/21/2016 09:21 AM, Jan Cholasta wrote:

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1].
A CI
test suite is also included.

There are some issues with the patch I would like to discuss in 
more

detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to
port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?


I don't think it applies anymore. These messages were there so the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.


+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining 
services"

"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g. 
topology

state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep 
the

two
entities separate without some hacking around framework 
capabilities


I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are
safe.




3.) since actions in post-callback require a knowledge about
topology
state gathered in pre-callback, I had to store some information in
the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is
this
okay?


Why can't the master remove itself?

Because it removes its own replication agreements hence any 
changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't 
replicate

to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry


That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.


What kind of cleanup is it? Can it be done before instead?



Well most of the code can be run in pre-callback if all the checks are 
passed/forced.


However there is a check for deleted segments and this one should be 
run after the removal of master entry to see if topology plugin 
removed all dangling segments pointing to master. I am not quite sure 
if it make sense to run this check in the master which is being removed.
no, it is not guranteed that the information on the removed master will 
be correct. If the del is applied to the to be removed master the 
topology plugin will only on a master which is remaining start the 
removal of segments, these will alos be replicated back to the removed 
master, but the repl agreements to this master will also be removed, so 
no gurantee which mods will be available on the removed master, and you 
should also be able to remove a master if it is down - so applying the 
full removal on a remaining server makes sense.


What is the behaviour if the removal of a server would disconnect topology ?


--
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael 
O'Neill

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Martin Babinsky 

On 04/21/2016 09:21 AM, Jan Cholasta wrote:

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1].
A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to
port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?


I don't think it applies anymore. These messages were there so the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.


+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining services"
"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep the
two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are
safe.




3.) since actions in post-callback require a knowledge about
topology
state gathered in pre-callback, I had to store some information in
the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is
this
okay?


Why can't the master remove itself?


Because it removes its own replication agreements hence any changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry


That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.


What kind of cleanup is it? Can it be done before instead?



Well most of the code can be run in pre-callback if all the checks are 
passed/forced.


However there is a check for deleted segments and this one should be run 
after the removal of master entry to see if topology plugin removed all 
dangling segments pointing to master. I am not quite sure if it make 
sense to run this check in the master which is being removed.


--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Jan Cholasta 

On 19.4.2016 12:42, Martin Babinsky wrote:

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly
print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?


I don't think it applies anymore. These messages were there so the
user
would know something was happening. If it is an API command there
isn't
much we can do other than add something to the cli to print "This
could
take a bit" before making the call.


+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity
check"
"checking remaining services" | "skipping check for remaining services"
"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep the
two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having
separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are
safe.




3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in
the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is
this
okay?


Why can't the master remove itself?


Because it removes its own replication agreements hence any changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry


That is true, but there is a plenty of cleanup code that is executed
*after* the master entry is removed and these changes would not
replicate if the agreements were removed by topology plugin in the
meanwhile.


What kind of cleanup is it? Can it be done before instead?

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-21 Thread Jan Cholasta 

On 19.4.2016 13:49, Martin Babinsky wrote:

On 04/14/2016 10:48 AM, Martin Babinsky wrote:

On 04/14/2016 08:42 AM, Jan Cholasta wrote:

Hi,

On 13.4.2016 16:49, Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.



`server-del` now accepts the following options:
* `--cleanup`: perform a cleanup after an already deleted master


I would prefer if this was actually called --force, for reasons
explained in the design thread:
.



* `--force-removal`: force master removal, i.e. ignore topology errors


So, this is actually the all-powerful --force option we always try to
avoid, but with a different name (and not very good one - if you are
removing something, what other than removal would you need to force?).

Could you split this into separate options?


There are actually two checks that we need to pass/bypass before we can
remove the master entry and run all the cleanup shenanigans:

1.) the topology is not disconnected already or is not being
disconnected by the action

2.) the action does leave at least one CA/DNS server, does not remove
DNSSec keymaster and we can promote other master to CA renewal master

So IIUC we would need three options actually:

* one that bypasses topology checks ('--ignore-topology-disconnect')
* one that bypasses the check for remaining services
('--ignore-last-services?')
* one that will cleanup leftovers only, ignoring NotFound error
('--cleanup'), this one is already there


Actually '--force' should replace '--cleanup' as it does basically the
same job.


Right.


What about the remaining two proposed options?


--ignore-topology-disconnect is good. The other one should use "role" 
rather than "service", e.g. --ignore-last-of-role.


--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-19 Thread Martin Babinsky 

On 04/14/2016 10:48 AM, Martin Babinsky wrote:

On 04/14/2016 08:42 AM, Jan Cholasta wrote:

Hi,

On 13.4.2016 16:49, Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.



`server-del` now accepts the following options:
* `--cleanup`: perform a cleanup after an already deleted master


I would prefer if this was actually called --force, for reasons
explained in the design thread:
.


* `--force-removal`: force master removal, i.e. ignore topology errors


So, this is actually the all-powerful --force option we always try to
avoid, but with a different name (and not very good one - if you are
removing something, what other than removal would you need to force?).

Could you split this into separate options?


There are actually two checks that we need to pass/bypass before we can
remove the master entry and run all the cleanup shenanigans:

1.) the topology is not disconnected already or is not being
disconnected by the action

2.) the action does leave at least one CA/DNS server, does not remove
DNSSec keymaster and we can promote other master to CA renewal master

So IIUC we would need three options actually:

* one that bypasses topology checks ('--ignore-topology-disconnect')
* one that bypasses the check for remaining services
('--ignore-last-services?')
* one that will cleanup leftovers only, ignoring NotFound error
('--cleanup'), this one is already there


Actually '--force' should replace '--cleanup' as it does basically the 
same job. What about the remaining two proposed options?



Honza







--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-19 Thread Martin Babinsky 

On 04/14/2016 11:46 AM, Ludwig Krispenz wrote:


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del`
subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the
output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


+1


This is already implemented in PoC. So I guess we may reduce the
output only to the following:


In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity check"
"checking remaining services" | "skipping check for remaining services"
"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part
would
me a separate API method called during server_del postcallback.
However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del.
That
makes the code rather unwieldy but I found it difficult to keep the
two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are safe.




3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology
stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Why can't the master remove itself?


Because it removes its own replication agreements hence any changes in
DIT (like removed principals, s4u2 proxy targets etc.) won't replicate
to other masters.

It shouldn't remove replication agreements, in fact this should be
prevented by the topology plugin.
The removal of the agreements will be triggered by removing the master
entry


That is true, but there is a plenty of cleanup code that is executed 
*after* the master entry is removed and these changes would not 
replicate if the agreements were removed by topology plugin in the 
meanwhile.




Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or
other
testing (e.g. there might be valid reasons to do this)? If so that
seems
ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.


5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error
back to
the user. This makes a large part of 'TestServerDel' suite to fail.
How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to
keep
long-lived connections alive.


Impossible, everything is synchronous.




6.) There are some in-place imports of server-side code in some
places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.


I wouldn't worry about this, I'm going to move most plugin code to
ipaserver as part of the thin client feature anyway ;-)

Honza









--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-14 Thread Ludwig Krispenz 


On 04/14/2016 10:59 AM, Martin Babinsky wrote:

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:
This is a WIP patch which moves the `ipa-replica-manage del` 
subcommand

to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the 
output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


+1

This is already implemented in PoC. So I guess we may reduce the 
output only to the following:



In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity check"
"checking remaining services" | "skipping check for remaining services"
"performing cleanup"
"Deleted server {server}"




2.) In the original discussion[2] we assumed that the cleanup part 
would
me a separate API method called during server_del postcallback. 
However

since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. 
That
makes the code rather unwieldy but I found it difficult to keep the 
two

entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are safe.




3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology 
stuff

directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Why can't the master remove itself?

Because it removes its own replication agreements hence any changes in 
DIT (like removed principals, s4u2 proxy targets etc.) won't replicate 
to other masters.
It shouldn't remove replication agreements, in fact this should be 
prevented by the topology plugin.

The removal of the agreements will be triggered by removing the master entry


Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or 
other
testing (e.g. there might be valid reasons to do this)? If so that 
seems

ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.


5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error 
back to
the user. This makes a large part of 'TestServerDel' suite to fail. 
How

should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to 
keep

long-lived connections alive.


Impossible, everything is synchronous.



6.) There are some in-place imports of server-side code in some 
places.

I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.


I wouldn't worry about this, I'm going to move most plugin code to
ipaserver as part of the thin client feature anyway ;-)

Honza






--
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael 
O'Neill

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-14 Thread Martin Babinsky 

On 04/13/2016 05:10 PM, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or other
testing (e.g. there might be valid reasons to do this)? If so that seems
ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.

One of the use-cases for this feature is to run server-del as a part of 
uninstallation of domain-level 1 master. In this case, you cannot run 
the command on the host and some other master has to put our host down 
so you would have to forward the request somewhere else anyway.


Other case I was thinking about was running the command from a client 
machine that talks primarily to a server that should be removed. In this 
case the user can either get some 'I cannot kill myself, talk to someone 
else' response, or the server could forward the request elsewhere and 
then return the result.

5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to keep
long-lived connections alive.


6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.

rob



[1] https://fedorahosted.org/freeipa/ticket/5588
[2]
https://www.redhat.com/archives/freeipa-devel/2016-March/msg00335.html








--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-14 Thread Martin Babinsky 

On 04/14/2016 08:24 AM, Jan Cholasta wrote:

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


+1

This is already implemented in PoC. So I guess we may reduce the output 
only to the following:



In CLI print:
"Removing {server} from replication topology, "
"please wait...

The adding info messages:

"checking topology connectivity" | "skipping topology connectivity check"
"checking remaining services" | "skipping check for remaining services"
"performing cleanup"
"Deleted server {server}"





2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del
even if the master entry does not exist anymore, so I think we are safe.




3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Why can't the master remove itself?

Because it removes its own replication agreements hence any changes in 
DIT (like removed principals, s4u2 proxy targets etc.) won't replicate 
to other masters.


Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or other
testing (e.g. there might be valid reasons to do this)? If so that seems
ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.


5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to keep
long-lived connections alive.


Impossible, everything is synchronous.




6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.


I wouldn't worry about this, I'm going to move most plugin code to
ipaserver as part of the thin client feature anyway ;-)

Honza




--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-14 Thread Martin Babinsky 

On 04/14/2016 08:42 AM, Jan Cholasta wrote:

Hi,

On 13.4.2016 16:49, Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.



`server-del` now accepts the following options:
* `--cleanup`: perform a cleanup after an already deleted master


I would prefer if this was actually called --force, for reasons
explained in the design thread:
.


* `--force-removal`: force master removal, i.e. ignore topology errors


So, this is actually the all-powerful --force option we always try to
avoid, but with a different name (and not very good one - if you are
removing something, what other than removal would you need to force?).

Could you split this into separate options?

There are actually two checks that we need to pass/bypass before we can 
remove the master entry and run all the cleanup shenanigans:


1.) the topology is not disconnected already or is not being 
disconnected by the action


2.) the action does leave at least one CA/DNS server, does not remove 
DNSSec keymaster and we can promote other master to CA renewal master


So IIUC we would need three options actually:

* one that bypasses topology checks ('--ignore-topology-disconnect')
* one that bypasses the check for remaining services 
('--ignore-last-services?')
* one that will cleanup leftovers only, ignoring NotFound error 
('--cleanup'), this one is already there



Honza




--
Martin^3 Babinsky

--
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Jan Cholasta 

Hi,

On 13.4.2016 16:49, Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.



`server-del` now accepts the following options:
* `--cleanup`: perform a cleanup after an already deleted master


I would prefer if this was actually called --force, for reasons 
explained in the design thread: 
.



* `--force-removal`: force master removal, i.e. ignore topology errors


So, this is actually the all-powerful --force option we always try to 
avoid, but with a different name (and not very good one - if you are 
removing something, what other than removal would you need to force?).


Could you split this into separate options?

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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Jan Cholasta 

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


+1




2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del 
even if the master entry does not exist anymore, so I think we are safe.





3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Why can't the master remove itself?



Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or other
testing (e.g. there might be valid reasons to do this)? If so that seems
ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.


5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to keep
long-lived connections alive.


Impossible, everything is synchronous.




6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.


I wouldn't worry about this, I'm going to move most plugin code to 
ipaserver as part of the thin client feature anyway ;-)


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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Rob Crittenden 

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user 
would know something was happening. If it is an API command there isn't 
much we can do other than add something to the cli to print "This could 
take a bit" before making the call.



2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate 
operations has saved our bacon on more than one occasion.



3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del. 
Another option would be to drop pre/post and add all this topology stuff 
directly to server_del execute.



4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Assisted suicide?

What does this effectively do? Potentially disconnect it from the 
topology, perhaps to run as standalone for upgrade, integration or other 
testing (e.g. there might be valid reasons to do this)? If so that seems 
ok to me, or if too hacky you could just spit out a message directing 
them to disconnect from another host, but that would be strange in the 
UI I think.



5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely 
sure this is possible with the framework but it might be one way to keep 
long-lived connections alive.



6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for 
inspiration, it does this.


rob



[1] https://fedorahosted.org/freeipa/ticket/5588
[2] https://www.redhat.com/archives/freeipa-devel/2016-March/msg00335.html





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

[Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Martin Babinsky 
This is a WIP patch which moves the `ipa-replica-manage del` subcommand 
to the 'server-del' API method and exposes it as CLI command[1]. A CI 
test suite is also included.


There are some issues with the patch I would like to discuss in more 
detail on the list:


1.) In the original subcommand there was a lot of output (mostly print 
statements) during all stages of master removal. I have tried to port 
these as messages to the command which results in quite voluminous 
response sent back to the frontend. Should we try to reduce the output?


2.) In the original discussion[2] we assumed that the cleanup part would 
me a separate API method called during server_del postcallback. However 
since the two objects ended up sharing a lot of state (e.g. topology 
state from pre-callback, messages) i have merged it to server-del. That 
makes the code rather unwieldy but I found it difficult to keep the two 
entities separate without some hacking around framework capabilities


3.) since actions in post-callback require a knowledge about topology 
state gathered in pre-callback, I had to store some information in the 
command's context. Sorry about that, if you know about some way to 
circumvent me, let me know.


4.) The master can not remove itself. I have implemented an ad-hoc 
forwarding of the request to other master that can do the job. Is this okay?


5.) Since the original behavior of 'chekc_deleted_segments' was kept, 
the code can sometimes hang waiting for removal of some segments, 
especially during forced removal in wonky topologies. This can cause 
gateway timeout in JSON-RPC call and report false positive error back to 
the user. This makes a large part of 'TestServerDel' suite to fail. How 
should we handle this? Should we keep the original behavior?


6.) There are some in-place imports of server-side code in some places. 
I'm not very happy about it and would be glad if we can agree on a way 
to fix this.



[1] https://fedorahosted.org/freeipa/ticket/5588
[2] https://www.redhat.com/archives/freeipa-devel/2016-March/msg00335.html

--
Martin^3 Babinsky
From e807b266a126e9708329871362aa52a7e15f4183 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 8 Apr 2016 16:29:04 +0200
Subject: [PATCH] integration test suite for server-del

https://fedorahosted.org/freeipa/ticket/5588
---
 ipatests/test_integration/tasks.py   |  32 ++-
 ipatests/test_integration/test_caless.py |  11 +-
 ipatests/test_integration/test_server_del.py | 307 +++
 3 files changed, 335 insertions(+), 15 deletions(-)
 create mode 100644 ipatests/test_integration/test_server_del.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 70f4fa7fd96f9d993332996f087577d1c88f828e..f3be216ee75a5d25e0722cb21ac64ad43318b623 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -57,10 +57,10 @@ def check_arguments_are(slice, instanceof):
 and third arguments are integers
 """
 def wrapper(func):
-def wrapped(*args):
+def wrapped(*args, **kwargs):
 for i in args[slice[0]:slice[1]]:
 assert isinstance(i, instanceof), "Wrong type: %s: %s" % (i, type(i))
-return func(*args)
+return func(*args, **kwargs)
 return wrapped
 return wrapper
 
@@ -721,7 +721,7 @@ def clean_replication_agreement(master, replica):
 
 
 @check_arguments_are((0, 3), Host)
-def create_segment(master, leftnode, rightnode):
+def create_segment(master, leftnode, rightnode, suffix=DOMAIN_SUFFIX_NAME):
 """
 creates a topology segment. The first argument is a node to run the command
 :returns: a hash object containing segment's name, leftnode, rightnode
@@ -731,7 +731,7 @@ def create_segment(master, leftnode, rightnode):
 lefthost = leftnode.hostname
 righthost = rightnode.hostname
 segment_name = "%s-to-%s" % (lefthost, righthost)
-result = master.run_command(["ipa", "topologysegment-add", DOMAIN_SUFFIX_NAME,
+result = master.run_command(["ipa", "topologysegment-add", suffix,
  segment_name,
  "--leftnode=%s" % lefthost,
  "--rightnode=%s" % righthost], raiseonerr=False)
@@ -743,7 +743,7 @@ def create_segment(master, leftnode, rightnode):
 return {}, result.stderr_text
 
 
-def destroy_segment(master, segment_name):
+def destroy_segment(master, segment_name, suffix=DOMAIN_SUFFIX_NAME):
 """
 Destroys topology segment.
 :param master: reference to master object of class Host
@@ -753,7 +753,7 @@ def destroy_segment(master, segment_name):
 kinit_admin(master)
 command = ["ipa",
"topologysegment-del",
-   DOMAIN_SUFFIX_NAME,
+   suffix,
segment_name]
 result = master.run_command(command, raiseonerr=False)
 return result.returncode, result.stderr_text
@@ -1181,