Re: [Freeipa-devel] [PATCH 0473-0476]DNS Locations: Prologue

[Jan Cholasta]

On 25.5.2016 18:28, Martin Basti wrote:



On 24.05.2016 12:10, Martin Basti wrote:



On 23.05.2016 07:44, Jan Cholasta wrote:

On 20.5.2016 15:32, Martin Basti wrote:



On 20.05.2016 12:30, Petr Spacek wrote:

On 18.5.2016 12:43, Martin Basti wrote:


On 12.05.2016 16:16, Martin Basti wrote:



On 12.05.2016 11:01, Martin Basti wrote:


On 11.05.2016 09:41, Martin Basti wrote:


On 10.05.2016 18:56, Petr Spacek wrote:

On 10.5.2016 15:38, Petr Spacek wrote:

On 10.5.2016 15:26, Martin Basti wrote:

On 10.05.2016 15:23, Petr Spacek wrote:

On 10.5.2016 14:44, Martin Basti wrote:

On 10.05.2016 14:33, Petr Spacek wrote:

On 6.5.2016 10:20, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/2008

Patches attached.


freeipa-mbasti-0473-DNS-Locations-Always-create-DNS-related-privileges.patch





From 9a936740da7cdacec150acc92a45041a98ce7cb3 Mon
Sep 17
00:00:00 2001
From: Martin Basti 
Date: Wed, 4 May 2016 17:33:52 +0200
Subject: [PATCH 1/4] DNS Locations: Always create DNS
related
privileges

DNS privileges are important for handling DNS locations
which can be
created without DNS servers in IPA topology. We will also
need this
privileges presented for future feature 'External DNS
support'

Seems reasonable, ACK.



freeipa-mbasti-0474-DNS-Locations-add-new-attributes-and-objectclasses.patch





From a7766da5fd1a72884308d4206c9cde262f5c8d35 Mon
Sep 17
00:00:00 2001
From: Martin Basti 
Date: Thu, 5 May 2016 11:12:00 +0200
Subject: [PATCH 2/4] DNS Locations: add new attributes and
objectclasses

http://www.freeipa.org/page/V4/DNS_Location_Mechanism

https://fedorahosted.org/freeipa/ticket/2008
---
 install/share/60ipadns.ldif | 4 
 1 file changed, 4 insertions(+)

diff --git a/install/share/60ipadns.ldif
b/install/share/60ipadns.ldif
index
e0ed0ab869cea0478d9640bb509c6267abed1a01..31c2f71f8566d04a05709f1359b20e6fa51921ce




100644
--- a/install/share/60ipadns.ldif
+++ b/install/share/60ipadns.ldif
@@ -70,9 +70,13 @@ attributeTypes: (
2.16.840.1.113730.3.8.5.25 NAME
'idnsSecKeyRevoke' DESC 'DNSKE
 attributeTypes: ( 2.16.840.1.113730.3.8.5.26 NAME
'idnsSecKeySep' DESC
'DNSKEY SEP flag (equivalent to bit 15): RFC 4035' EQUALITY
booleanMatch
SYNTAX 1.3.6.1.4.1.1466.115.121.1.7 SINGLE-VALUE X-ORIGIN
'IPA v4.1' )
 attributeTypes: ( 2.16.840.1.113730.3.8.5.27 NAME
'idnsSecAlgorithm' DESC
'DNSKEY algorithm: string used as mnemonic' EQUALITY
caseIgnoreIA5Match
SUBSTR caseIgnoreIA5SubstringsMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.26
SINGLE-VALUE X-ORIGIN 'IPA v4.1' )
 attributeTypes: ( 2.16.840.1.113730.3.8.5.28 NAME
'idnsSecKeyRef' DESC
'PKCS#11 URI of the key' EQUALITY caseExactMatch
SINGLE-VALUE SYNTAX
1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'IPA v4.1' )
+attributeTypes: ( 2.16.840.1.113730.3.8.5.32 NAME
'ipaLocation' DESC
'Reference to IPA location' EQUALITY distinguishedNameMatch
SYNTAX
1.3.6.1.4.1.1466.115.121.1.12 SINGLE-VALUE X-ORIGIN 'IPA
v4.4' )
+attributeTypes: ( 2.16.840.1.113730.3.8.5.33 NAME
'ipaLocationWeight' DESC
'Weight for the server in IPA location' EQUALITY
integerMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.27 SINGLE-VALUE X-ORIGIN 'IPA
v4.4' )
 objectClasses: ( 2.16.840.1.113730.3.8.6.0 NAME
'idnsRecord'
DESC 'dns
Record, usually a host' SUP top STRUCTURAL MUST idnsName
MAY ( cn $
idnsAllowDynUpdate $ dNSTTL $ dNSClass $ aRecord $
aAAARecord $
a6Record $
nSRecord $ cNAMERecord $ pTRRecord $ sRVRecord $
tXTRecord $
mXRecord $
mDRecord $ hInfoRecord $ mInfoRecord $ aFSDBRecord $
SigRecord $
KeyRecord
$ LocRecord $ nXTRecord $ nAPTRRecord $ kXRecord $
certRecord $
dNameRecord
$ dSRecord $ sSHFPRecord $ rRSIGRecord $ nSECRecord $
DLVRecord $
TLSARecord $ UnknownRecord $ RPRecord $ APLRecord $
IPSECKEYRecord $
DHCIDRecord $ HIPRecord $ SPFRecord ) )
 objectClasses: ( 2.16.840.1.113730.3.8.6.1 NAME
'idnsZone' DESC
'Zone
class' SUP idnsRecord STRUCTURAL MUST ( idnsZoneActive $
idnsSOAmName $
idnsSOArName $ idnsSOAserial $ idnsSOArefresh $
idnsSOAretry $
idnsSOAexpire $ idnsSOAminimum ) MAY ( idnsUpdatePolicy $
idnsAllowQuery $
idnsAllowTransfer $ idnsAllowSyncPTR $ idnsForwardPolicy $
idnsForwarders $
idnsSecInlineSigning $ nSEC3PARAMRecord ) )
 objectClasses: ( 2.16.840.1.113730.3.8.6.2 NAME
'idnsConfigObject' DESC
'DNS global config options' STRUCTURAL MAY (
idnsForwardPolicy $
idnsForwarders $ idnsAllowSyncPTR $ idnsZoneRefresh $
idnsPersistentSearch ) )
 objectClasses: ( 2.16.840.1.113730.3.8.12.18 NAME
'ipaDNSZone'
SUP top
AUXILIARY MUST idnsName MAY managedBy X-ORIGIN 'IPA v3' )
 objectClasses: ( 2.16.840.1.113730.3.8.6.3 NAME
'idnsForwardZone' DESC
'Forward Zone class' SUP top STRUCTURAL MUST ( idnsName $
idnsZoneActive )
MAY ( idnsForwarders $ idnsForwardPolicy ) )
 objectClasses: ( 2.16.840.1.113730.3.8.6.4 NAME
'idnsSecKey'
DESC 'DNSSEC
key metadata' STRUCTURAL MUST ( idnsSecKeyRef $
idnsSecKeyCreated $
idnsSecAlgorithm ) MAY ( idnsSecKeyPublish $
idnsSecKeyActivate $
idnsSecKeyInactive $ 

Re: [Freeipa-devel] [WIP] Thin client

[Jan Cholasta]

On 25.5.2016 18:17, Martin Basti wrote:



On 25.05.2016 16:07, Jan Cholasta wrote:

On 25.5.2016 15:03, David Kupka wrote:

On 18/05/16 08:01, Jan Cholasta wrote:

On 16.5.2016 16:35, Martin Basti wrote:



On 09.05.2016 14:07, Jan Cholasta wrote:

On 6.5.2016 14:32, Martin Basti wrote:



On 28.04.2016 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza


I did not test it yet, I just checked the code

* automount: do not inherit automountlocation_tofiles from
LDAPQuery *
LGTM

* dns: move all dnsrecord code called on client to a single class *
LGTM

* dns: do not rely on server data structures in code called on
client *
1)
you forgot to increment VERSION


This was deliberate, as it will no longer be necessary to bump
VERSION
for backward compatible changes after this whole patchset is merged.
But we're not there yet, so fixed.


How we should handle VERSION after your patches?


Otherwise LGTM

* otptoken: fix import of DN *
ACK

* otptoken_yubikey: fix otptoken_add_yubikey arguments *
1)
you forgot to increment VERSION


Fixed.



2)
Did you find out why this was issue?
-del answer['value']# Why does this cause an error if
omitted?
 -del answer['summary']  # Why does this cause an error if
omitted?


The command definition was not complete, it was missing has_output.



Otherwise LGTM

* vault: move client-side code to the module level *
LGTM

* vault: copy arguments of client commands from server
counterparts *
1)
you forgot to increment Version


Fixed.



* ipalib: use relative imports for cross-plugin imports *
1) Missing explanation for future generations why this change is
needed
in commit message


Fixed.



The other commits I will check later.
Martin^2



Okay. Thanks.



* frontend: remove the unused Command.soft_validate method
LGTM

* frontend: perform argument value validation only on server
LGTM

* frontend: do not forward argument defaults to server
I'm not a fan of returning  None in promt_param function, but I havent
found anything better to use.


It always returned None for unset params.


LGTM

* ipalib: optimize API.txt
this contains a lot of black magic, but because this is mainly copy of
current to proper places, LGTM


It's actually mostly cut-n-paste.



* makeaci: load additional plugins using API.add_module
Looks good, but I miss explanation why is this change needed


The next change would be impossible without this.



* plugable: replace API.import_plugins with new API.add_package
LGTM


* ipalib, ipaserver: migrate all plugins to Registry-based
registration
ACK

* ipalib, ipaserver: fix incorrect API.register calls in docstrings
LGTM


* plugable: remove the unused deprecated API.register method
LGTM


* plugable: switch API to Registry-based plugin discovery
LGTM

* frontend: merge baseldap.CallbackRegistry into Command
LGTM

*frontend: move the interactive_prompt callback type to Command
 LGTM

Martin^2







Hello,
first batch of 30 patches from Honza's trac-4739 branch
(https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be
pushed into master.
All upto "frontend: allow commands to have an argument named `name`" got
over numerous test cycles in last week+ and is working as expected
now, ACK.


Thanks for the review.



Honzo, please rebase and push them, thanks!



Attaching the patches for reference.

Pushed to master: 2b50fc617024f18a81e97f30f75ed1b9221c4055



Guys, you forgot to test it with newer pylint.


I don't see any "BuildRequires: newer pylint" in the spec file.



Patch attached.


LGTM, although the commit message is wrong - this is not related to thin 
client at all, PublicError and PublicMessage had .kw since forever.


--
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] Provisioning throughput

[thierry bordaz]

On 05/25/2016 08:49 PM, Rob Crittenden wrote:

thierry bordaz wrote:


Hello,

Thanks for all the feedbacks. I updated the design accordingly and with
additional tests results
(http://www.freeipa.org/page/V4/Performance_Improvements#Proposed_improvements) 


Several improvements can be done, in particular in DS plugins (memberof,
retroCL), but for "easy" benefit provisioning will be done with memberof
disabled followed by fixup.

It remains some aspects that are not clear to me:

  * For best performance, DS tuning and provisioning/fixup would
preferably be done under 'directory manager'
That means prompting DM password and writing it into temporary file.
Is that a concern ?
  * Fixup requires that we know the filters matching the provisioned
entries. For example :
  o (objectClass=inetorgperson)
  o (objectClass=ipausergroup)
  o (objectClass=ipahost)
  o (objectClass=ipahostgroup)
  o (objectClass=ipasudorule)
  o (objectClass=ipahbacrule)

The set of objectclass could be hardcode or provided in the
provisioning CLI option
What to do if an entry in in the provision file does not match
any of those filter ? Should it stop without starting the
provisioning ?
  * The CLI doing the provisioning could be something like 'ipa
provision ' or should it be a separated command e.g.
ipa-bulk-load ?


It depends. There is a migration command now, ipa migrate-ds, that 
adds records and is impacted by this. There is also the possibility of 
looping calls to ipa [user|group|etc]-add.


I agree that migration and bulk load can be linked. If migration 
dump/update a set of entries before filling them into a new instance it 
could use bulk load.
For set loop of ipa -add, I think they add many others direct 
operations (mainly SRCH) before doing the ADD in order to check 
coherency. bulk load looks more straightforward.


Would it be reasonable to require bulk import to be done on an IPA 
master so we can leverage the ldapi socket?
Do you mean using ldapi to reduce network latency or automember or 
something else ?


thanks
theirry


rob



thanks
thierry

On 05/13/2016 10:18 AM, Ludwig Krispenz wrote:


On 05/13/2016 09:42 AM, Petr Spacek wrote:

On 13.5.2016 09:26, Martin Kosek wrote:

On 05/12/2016 04:16 PM, Ludwig Krispenz wrote:

On 05/12/2016 03:45 PM, Ludwig Krispenz wrote:

On 05/12/2016 02:16 PM, Petr Vobornik wrote:

On 05/10/2016 05:50 PM, thierry bordaz wrote:

On 05/05/2016 03:44 PM, Petr Vobornik wrote:

On 05/04/2016 02:20 PM, thierry bordaz wrote:

Hello,

   I have been doing some tests/measures using
https://github.com/freeipa/freeipa-tools/blob/master/create-test-data.py. 




   The tool creates a set of typical 
users/hosts/groups... to

import with a
   ldapadd.

   I wrote down some finding in
http://www.freeipa.org/page/V4/Performance_Improvements#Provisioning_throughput_and_DS_plugins. 





   I still have to do some cleanup around the performance
but the
basic of a
   possible improvement is to do provisioning in several
steps
(disabling
   plugins, provisioning, enabling plugin, running fixup
tasks).

   Before going further in the design I wanted to share
those ideas
and know if
   it raise any concern.

   thanks
   thierry


Hi Thierry,

Thanks for the analysis. Very nice.

Knowing this will help us suggesting workarounds also for old
releases.

Couple questions:

Have you tested retrCL disabled with memberOf enabled. It seems
that it
would eliminate 550K adds and 0.8M searches. What would be the
time
improvement?

Do you know what is the time when memberof is enabled but
slapi-nis and
retroCL are disabled?
The culprit of the performance issue is very likely related to 
SRCH

(internal) triggered by memberof.

If retroCL is disabled and memberof enabled, #SRCH is 13.8M.
If retroCL is disabled, slapi-nis disabled and memberof enabled
#SRCH is
14.8
When all of them are enabled the #SRCH is 15M.

You are right if retroCL is disabled the #ADD drops but it has no
significant effect on the duration.

ok, thanks for the analysis


Regarding the duration of the provisioning, values are not
really stable
as performance of VM fluctuates. But as soon as memberof is
enabled the
provisioning lasts > 4hours where the same provisioning lasts
6mins as
soon as memberof is disabled.

I need to confirm the average time for internal searches but
assuming
1ms per SRCH it consumes >90% of the provisioning.



   From the text it was not clear to me, if you find or
investigate
possible improvements in memberof plugin which would improve the
performance without stopping and starting DS.
As was discussed at mtg, have you tried if the DS restart is 
really

necessary?

memberof plugin can be enabled and disabled while the server is
running, BUT
to achieve this the "enable-dynamic-plugins" feature has to be
turned on. And
then any enable/disable of a plugin would try to do it 

Re: [Freeipa-devel] Provisioning throughput

[Rob Crittenden]

thierry bordaz wrote:



On 05/25/2016 08:49 PM, Rob Crittenden wrote:

thierry bordaz wrote:


Hello,

Thanks for all the feedbacks. I updated the design accordingly and with
additional tests results
(http://www.freeipa.org/page/V4/Performance_Improvements#Proposed_improvements)

Several improvements can be done, in particular in DS plugins (memberof,
retroCL), but for "easy" benefit provisioning will be done with memberof
disabled followed by fixup.

It remains some aspects that are not clear to me:

  * For best performance, DS tuning and provisioning/fixup would
preferably be done under 'directory manager'
That means prompting DM password and writing it into temporary file.
Is that a concern ?
  * Fixup requires that we know the filters matching the provisioned
entries. For example :
  o (objectClass=inetorgperson)
  o (objectClass=ipausergroup)
  o (objectClass=ipahost)
  o (objectClass=ipahostgroup)
  o (objectClass=ipasudorule)
  o (objectClass=ipahbacrule)

The set of objectclass could be hardcode or provided in the
provisioning CLI option
What to do if an entry in in the provision file does not match
any of those filter ? Should it stop without starting the
provisioning ?
  * The CLI doing the provisioning could be something like 'ipa
provision ' or should it be a separated command e.g.
ipa-bulk-load ?


It depends. There is a migration command now, ipa migrate-ds, that
adds records and is impacted by this. There is also the possibility of
looping calls to ipa [user|group|etc]-add.


I agree that migration and bulk load can be linked. If migration
dump/update a set of entries before filling them into a new instance it
could use bulk load.
For set loop of ipa -add, I think they add many others direct
operations (mainly SRCH) before doing the ADD in order to check
coherency. bulk load looks more straightforward.


I just wonder if some (all) of this could be done manually. Document how 
to turn off memberof, do the import whatever way is appropriate, then 
run the fixup? I'm not sure what you had in mind.


I don't want to think small but do we expect to be importing a slew of 
hosts, sudorules, etc? I guess the potential is there but would it be on 
the same scale as users? If you focus only on users/groups does that 
change the use case at all?



Would it be reasonable to require bulk import to be done on an IPA
master so we can leverage the ldapi socket?

Do you mean using ldapi to reduce network latency or automember or
something else ?


To avoid the DM password issues. ldapi autobinds to DM when the id is root.

rob

--
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] Provisioning throughput

[Rob Crittenden]

thierry bordaz wrote:


Hello,

Thanks for all the feedbacks. I updated the design accordingly and with
additional tests results
(http://www.freeipa.org/page/V4/Performance_Improvements#Proposed_improvements)
Several improvements can be done, in particular in DS plugins (memberof,
retroCL), but for "easy" benefit provisioning will be done with memberof
disabled followed by fixup.

It remains some aspects that are not clear to me:

  * For best performance, DS tuning and provisioning/fixup would
preferably be done under 'directory manager'
That means prompting DM password and writing it into temporary file.
Is that a concern ?
  * Fixup requires that we know the filters matching the provisioned
entries. For example :
  o (objectClass=inetorgperson)
  o (objectClass=ipausergroup)
  o (objectClass=ipahost)
  o (objectClass=ipahostgroup)
  o (objectClass=ipasudorule)
  o (objectClass=ipahbacrule)

The set of objectclass could be hardcode or provided in the
provisioning CLI option
What to do if an entry in in the provision file does not match
any of those filter ? Should it stop without starting the
provisioning ?
  * The CLI doing the provisioning could be something like 'ipa
provision ' or should it be a separated command e.g.
ipa-bulk-load ?


It depends. There is a migration command now, ipa migrate-ds, that adds 
records and is impacted by this. There is also the possibility of 
looping calls to ipa [user|group|etc]-add.


Would it be reasonable to require bulk import to be done on an IPA 
master so we can leverage the ldapi socket?


rob



thanks
thierry

On 05/13/2016 10:18 AM, Ludwig Krispenz wrote:


On 05/13/2016 09:42 AM, Petr Spacek wrote:

On 13.5.2016 09:26, Martin Kosek wrote:

On 05/12/2016 04:16 PM, Ludwig Krispenz wrote:

On 05/12/2016 03:45 PM, Ludwig Krispenz wrote:

On 05/12/2016 02:16 PM, Petr Vobornik wrote:

On 05/10/2016 05:50 PM, thierry bordaz wrote:

On 05/05/2016 03:44 PM, Petr Vobornik wrote:

On 05/04/2016 02:20 PM, thierry bordaz wrote:

Hello,

   I have been doing some tests/measures using
https://github.com/freeipa/freeipa-tools/blob/master/create-test-data.py.


   The tool creates a set of typical users/hosts/groups... to
import with a
   ldapadd.

   I wrote down some finding in
http://www.freeipa.org/page/V4/Performance_Improvements#Provisioning_throughput_and_DS_plugins.



   I still have to do some cleanup around the performance
but the
basic of a
   possible improvement is to do provisioning in several
steps
(disabling
   plugins, provisioning, enabling plugin, running fixup
tasks).

   Before going further in the design I wanted to share
those ideas
and know if
   it raise any concern.

   thanks
   thierry


Hi Thierry,

Thanks for the analysis. Very nice.

Knowing this will help us suggesting workarounds also for old
releases.

Couple questions:

Have you tested retrCL disabled with memberOf enabled. It seems
that it
would eliminate 550K adds and 0.8M searches. What would be the
time
improvement?

Do you know what is the time when memberof is enabled but
slapi-nis and
retroCL are disabled?

The culprit of the performance issue is very likely related to SRCH
(internal) triggered by memberof.

If retroCL is disabled and memberof enabled, #SRCH is 13.8M.
If retroCL is disabled, slapi-nis disabled and memberof enabled
#SRCH is
14.8
When all of them are enabled the #SRCH is 15M.

You are right if retroCL is disabled the #ADD drops but it has no
significant effect on the duration.

ok, thanks for the analysis


Regarding the duration of the provisioning, values are not
really stable
as performance of VM fluctuates. But as soon as memberof is
enabled the
provisioning lasts > 4hours where the same provisioning lasts
6mins as
soon as memberof is disabled.

I need to confirm the average time for internal searches but
assuming
1ms per SRCH it consumes >90% of the provisioning.



   From the text it was not clear to me, if you find or
investigate
possible improvements in memberof plugin which would improve the
performance without stopping and starting DS.

As was discussed at mtg, have you tried if the DS restart is really
necessary?

memberof plugin can be enabled and disabled while the server is
running, BUT
to achieve this the "enable-dynamic-plugins" feature has to be
turned on. And
then any enable/disable of a plugin would try to do it dynamically
an dnot
wait for the restart.
And I think not all plugins are able to handle this, TomasB was
once working
on it for IPA plugins, but it was not completed as far as I know

but enabling dynamic plugins can be done without restart, so what
can be done is.
- enable dynamic plugins
- disable memberof
- do some work
- enable memberof
- disable dynamic plugins

Please see
https://fedorahosted.org/freeipa/ticket/4203#comment:9
I do not think this will be that easy. We would first need to invest
into

Re: [Freeipa-devel] Provisioning throughput

[thierry bordaz]

Hello,

Thanks for all the feedbacks. I updated the design accordingly and with 
additional tests results 
(http://www.freeipa.org/page/V4/Performance_Improvements#Proposed_improvements)
Several improvements can be done, in particular in DS plugins (memberof, 
retroCL), but for "easy" benefit provisioning will be done with memberof 
disabled followed by fixup.


It remains some aspects that are not clear to me:

 * For best performance, DS tuning and provisioning/fixup would
   preferably be done under 'directory manager'
   That means prompting DM password and writing it into temporary file.
   Is that a concern ?
 * Fixup requires that we know the filters matching the provisioned
   entries. For example :
 o (objectClass=inetorgperson)
 o (objectClass=ipausergroup)
 o (objectClass=ipahost)
 o (objectClass=ipahostgroup)
 o (objectClass=ipasudorule)
 o (objectClass=ipahbacrule)

   The set of objectclass could be hardcode or provided in the
   provisioning CLI option
   What to do if an entry in in the provision file does not match
   any of those filter ? Should it stop without starting the
   provisioning ?
 * The CLI doing the provisioning could be something like 'ipa
   provision ' or should it be a separated command e.g.
   ipa-bulk-load ?

thanks
thierry

On 05/13/2016 10:18 AM, Ludwig Krispenz wrote:


On 05/13/2016 09:42 AM, Petr Spacek wrote:

On 13.5.2016 09:26, Martin Kosek wrote:

On 05/12/2016 04:16 PM, Ludwig Krispenz wrote:

On 05/12/2016 03:45 PM, Ludwig Krispenz wrote:

On 05/12/2016 02:16 PM, Petr Vobornik wrote:

On 05/10/2016 05:50 PM, thierry bordaz wrote:

On 05/05/2016 03:44 PM, Petr Vobornik wrote:

On 05/04/2016 02:20 PM, thierry bordaz wrote:

Hello,

   I have been doing some tests/measures using
https://github.com/freeipa/freeipa-tools/blob/master/create-test-data.py. 



   The tool creates a set of typical users/hosts/groups... to
import with a
   ldapadd.

   I wrote down some finding in
http://www.freeipa.org/page/V4/Performance_Improvements#Provisioning_throughput_and_DS_plugins. 




   I still have to do some cleanup around the performance 
but the

basic of a
   possible improvement is to do provisioning in several 
steps

(disabling
   plugins, provisioning, enabling plugin, running fixup 
tasks).


   Before going further in the design I wanted to share 
those ideas

and know if
   it raise any concern.

   thanks
   thierry


Hi Thierry,

Thanks for the analysis. Very nice.

Knowing this will help us suggesting workarounds also for old 
releases.


Couple questions:

Have you tested retrCL disabled with memberOf enabled. It seems 
that it
would eliminate 550K adds and 0.8M searches. What would be the 
time

improvement?

Do you know what is the time when memberof is enabled but 
slapi-nis and

retroCL are disabled?

The culprit of the performance issue is very likely related to SRCH
(internal) triggered by memberof.

If retroCL is disabled and memberof enabled, #SRCH is 13.8M.
If retroCL is disabled, slapi-nis disabled and memberof enabled 
#SRCH is

14.8
When all of them are enabled the #SRCH is 15M.

You are right if retroCL is disabled the #ADD drops but it has no
significant effect on the duration.

ok, thanks for the analysis

Regarding the duration of the provisioning, values are not 
really stable
as performance of VM fluctuates. But as soon as memberof is 
enabled the
provisioning lasts > 4hours where the same provisioning lasts 
6mins as

soon as memberof is disabled.

I need to confirm the average time for internal searches but 
assuming

1ms per SRCH it consumes >90% of the provisioning.


   From the text it was not clear to me, if you find or 
investigate

possible improvements in memberof plugin which would improve the
performance without stopping and starting DS.

As was discussed at mtg, have you tried if the DS restart is really
necessary?
memberof plugin can be enabled and disabled while the server is 
running, BUT
to achieve this the "enable-dynamic-plugins" feature has to be 
turned on. And
then any enable/disable of a plugin would try to do it dynamically 
an dnot

wait for the restart.
And I think not all plugins are able to handle this, TomasB was 
once working

on it for IPA plugins, but it was not completed as far as I know
but enabling dynamic plugins can be done without restart, so what 
can be done is.

- enable dynamic plugins
- disable memberof
- do some work
- enable memberof
- disable dynamic plugins

Please see
https://fedorahosted.org/freeipa/ticket/4203#comment:9
I do not think this will be that easy. We would first need to invest 
into
updating FreeIPA plugins to work with dynamic plugins setting and 
then we could

do things alike above.

It looks like that for FreeIPA 4.4, we will need to live with DS 
restart unless

there is some workaround...
couldn't the scenario I outline above with enabling dynamic plugins 
only temporary work, are 

Re: [Freeipa-devel] [WIP] Thin client

[Martin Basti]

On 25.05.2016 16:07, Jan Cholasta wrote:

On 25.5.2016 15:03, David Kupka wrote:

On 18/05/16 08:01, Jan Cholasta wrote:

On 16.5.2016 16:35, Martin Basti wrote:



On 09.05.2016 14:07, Jan Cholasta wrote:

On 6.5.2016 14:32, Martin Basti wrote:



On 28.04.2016 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza


I did not test it yet, I just checked the code

* automount: do not inherit automountlocation_tofiles from 
LDAPQuery *

LGTM

* dns: move all dnsrecord code called on client to a single class *
LGTM

* dns: do not rely on server data structures in code called on
client *
1)
you forgot to increment VERSION


This was deliberate, as it will no longer be necessary to bump 
VERSION

for backward compatible changes after this whole patchset is merged.
But we're not there yet, so fixed.


How we should handle VERSION after your patches?


Otherwise LGTM

* otptoken: fix import of DN *
ACK

* otptoken_yubikey: fix otptoken_add_yubikey arguments *
1)
you forgot to increment VERSION


Fixed.



2)
Did you find out why this was issue?
-del answer['value']# Why does this cause an error if
omitted?
 -del answer['summary']  # Why does this cause an error if
omitted?


The command definition was not complete, it was missing has_output.



Otherwise LGTM

* vault: move client-side code to the module level *
LGTM

* vault: copy arguments of client commands from server 
counterparts *

1)
you forgot to increment Version


Fixed.



* ipalib: use relative imports for cross-plugin imports *
1) Missing explanation for future generations why this change is
needed
in commit message


Fixed.



The other commits I will check later.
Martin^2



Okay. Thanks.



* frontend: remove the unused Command.soft_validate method
LGTM

* frontend: perform argument value validation only on server
LGTM

* frontend: do not forward argument defaults to server
I'm not a fan of returning  None in promt_param function, but I havent
found anything better to use.


It always returned None for unset params.


LGTM

* ipalib: optimize API.txt
this contains a lot of black magic, but because this is mainly copy of
current to proper places, LGTM


It's actually mostly cut-n-paste.



* makeaci: load additional plugins using API.add_module
Looks good, but I miss explanation why is this change needed


The next change would be impossible without this.



* plugable: replace API.import_plugins with new API.add_package
LGTM


* ipalib, ipaserver: migrate all plugins to Registry-based 
registration

ACK

* ipalib, ipaserver: fix incorrect API.register calls in docstrings
LGTM


* plugable: remove the unused deprecated API.register method
LGTM


* plugable: switch API to Registry-based plugin discovery
LGTM

* frontend: merge baseldap.CallbackRegistry into Command
LGTM

*frontend: move the interactive_prompt callback type to Command
 LGTM

Martin^2







Hello,
first batch of 30 patches from Honza's trac-4739 branch
(https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be
pushed into master.
All upto "frontend: allow commands to have an argument named `name`" got
over numerous test cycles in last week+ and is working as expected
now, ACK.


Thanks for the review.



Honzo, please rebase and push them, thanks!



Attaching the patches for reference.

Pushed to master: 2b50fc617024f18a81e97f30f75ed1b9221c4055



Guys, you forgot to test it with newer pylint.

Patch attached.
From 5714b4b1dcad5618f2dccd79dcd5022ea859ed63 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 25 May 2016 18:12:57 +0200
Subject: [PATCH] fix pylint false positive errors related to thin client

pylint 1.5 reports 'kw' as 'no-member' for PublicError and
PublicMessage. It is false positive in both cases.

https://fedorahosted.org/freeipa/ticket/4739
---
 pylint_plugins.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pylint_plugins.py b/pylint_plugins.py
index 7460d9f4a4c45fd036a32da3e746a4836791a0c2..631d85ab1d9e79b7b90cd1c7742e64a5b74e4f61 100644
--- a/pylint_plugins.py
+++ b/pylint_plugins.py
@@ -114,6 +114,7 @@ ipa_class_members = {
 'ipalib.errors.PublicError': [
 'msg',
 'strerror',
+'kw',
 ],
 'ipalib.errors.SingleMatchExpected': [
 'found',
@@ -128,6 +129,7 @@ ipa_class_members = {
 'msg',
 'strerror',
 'type',
+'kw',
 ],
 'ipalib.parameters.Param': [
 'cli_name',
-- 
2.5.5

-- 
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] [PATCH 0484] remove unused code from automount plugin

[Martin Basti]

On 25.05.2016 09:11, Stanislav Laznicka wrote:


LGTM, could you please just add the ticket to the commit message?


On 05/20/2016 04:28 PM, Martin Basti wrote:




On 20.05.2016 15:03, Martin Basti wrote:

The removed code is unused for long time.

Patch attached.




https://fedorahosted.org/freeipa/attachment/ticket/5899/





updated patch attached.
From 7c29292f7fde89a046d17e166a1a64b598b54458 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 19 May 2016 10:24:43 +0200
Subject: [PATCH] Remove unused variables in automount plugin

https://fedorahosted.org/freeipa/ticket/4739
---
 ipalib/plugins/automount.py | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index 85d7d13212de59cb5cb8d57583d2c3c9d4f2b71c..f5128572b7d1e3f21a4359bbbe0ad3081be5f863 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -308,13 +308,9 @@ class automountlocation_tofiles(LDAPQuery):
 __doc__ = _('Generate automount files for a specific location.')
 
 def execute(self, *args, **options):
-ldap = self.obj.backend
+self.api.Command['automountlocation_show'](args[0])
 
-location = self.api.Command['automountlocation_show'](args[0])
-
-maps = []
 result = self.api.Command['automountkey_find'](args[0], u'auto.master')
-truncated = result['truncated']
 maps = result['result']
 
 # maps, truncated
@@ -328,7 +324,6 @@ class automountlocation_tofiles(LDAPQuery):
 mapnames.append(info)
 key = info.split(None)
 result = self.api.Command['automountkey_find'](args[0], key[0])
-truncated = result['truncated']
 keys[info] = result['result']
 # TODO: handle truncated results, same as above
 
@@ -343,7 +338,6 @@ class automountlocation_tofiles(LDAPQuery):
 for m in orphanmaps:
 key = m['automountmapname']
 result = self.api.Command['automountkey_find'](args[0], key[0])
-truncated = result['truncated']
 orphankeys.append(result['result'])
 
 return dict(result=dict(maps=maps, keys=keys,
@@ -463,7 +457,6 @@ class automountlocation_import(Command):
 mapfile = am[1].replace('"','')
 am[1] = os.path.basename(am[1])
 maps[am[1]] = mapfile
-info = ' '.join(am[1:])
 
 # Add a new key to the auto.master map for the new map file
 try:
-- 
2.5.5

-- 
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] [Testplan Review] Server Roles

[Oleg Fayans]

Hi guys. Here is a rather schematic (as neither the feature not the
design document is not complete) of the server roles testplan. Could you
please review it and tell me what is missing?

http://www.freeipa.org/page/V4/Server_Roles/Test_Plan

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

-- 
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] Thin client

[David Kupka]

On 18/05/16 08:01, Jan Cholasta wrote:

On 16.5.2016 16:35, Martin Basti wrote:



On 09.05.2016 14:07, Jan Cholasta wrote:

On 6.5.2016 14:32, Martin Basti wrote:



On 28.04.2016 14:45, Jan Cholasta wrote:

Hi,

I have pushed my thin client WIP branch to GitHub:
.

All commits up to "ipalib: use relative imports for cross-plugin
imports" should be good for review. The rest is subject to change
(WARNING: I will force push into this branch).

Honza


I did not test it yet, I just checked the code

* automount: do not inherit automountlocation_tofiles from LDAPQuery *
LGTM

* dns: move all dnsrecord code called on client to a single class *
LGTM

* dns: do not rely on server data structures in code called on client *
1)
you forgot to increment VERSION


This was deliberate, as it will no longer be necessary to bump VERSION
for backward compatible changes after this whole patchset is merged.
But we're not there yet, so fixed.


How we should handle VERSION after your patches?


Otherwise LGTM

* otptoken: fix import of DN *
ACK

* otptoken_yubikey: fix otptoken_add_yubikey arguments *
1)
you forgot to increment VERSION


Fixed.



2)
Did you find out why this was issue?
-del answer['value']# Why does this cause an error if
omitted?
 -del answer['summary']  # Why does this cause an error if
omitted?


The command definition was not complete, it was missing has_output.



Otherwise LGTM

* vault: move client-side code to the module level *
LGTM

* vault: copy arguments of client commands from server counterparts *
1)
you forgot to increment Version


Fixed.



* ipalib: use relative imports for cross-plugin imports *
1) Missing explanation for future generations why this change is needed
in commit message


Fixed.



The other commits I will check later.
Martin^2



Okay. Thanks.



* frontend: remove the unused Command.soft_validate method
LGTM

* frontend: perform argument value validation only on server
LGTM

* frontend: do not forward argument defaults to server
I'm not a fan of returning  None in promt_param function, but I havent
found anything better to use.


It always returned None for unset params.


LGTM

* ipalib: optimize API.txt
this contains a lot of black magic, but because this is mainly copy of
current to proper places, LGTM


It's actually mostly cut-n-paste.



* makeaci: load additional plugins using API.add_module
Looks good, but I miss explanation why is this change needed


The next change would be impossible without this.



* plugable: replace API.import_plugins with new API.add_package
LGTM


* ipalib, ipaserver: migrate all plugins to Registry-based registration
ACK

* ipalib, ipaserver: fix incorrect API.register calls in docstrings
LGTM


* plugable: remove the unused deprecated API.register method
LGTM


* plugable: switch API to Registry-based plugin discovery
LGTM

* frontend: merge baseldap.CallbackRegistry into Command
LGTM

*frontend: move the interactive_prompt callback type to Command
 LGTM

Martin^2







Hello,
first batch of 30 patches from Honza's trac-4739 branch 
(https://github.com/jcholast/freeipa/commits/trac-4739) is ready to be 
pushed into master.
All upto "frontend: allow commands to have an argument named `name`" got 
over numerous test cycles in last week+ and is working as expected 
now, ACK.


Honzo, please rebase and push them, thanks!

--
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] Questions on git

[Jan Pazdziora]

On Wed, May 25, 2016 at 12:17:18PM +0200, Christian Heimes wrote:
> 
> I don't know why github either forces merge commits or a single squashed
> commit. If I would have to guess, I would assume it has philosophical
> reasons. It took many years to get github to add an alternative to merge

The reason I was given when discussing it with a GitHub person was
that it's a performance issue. They are worried that people would use
it for multi-hundred-commit branches and the WebUI would not be able
to provide the same fast response as for single diff/commit.

-- 
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat

-- 
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 0089-0093] Authentication Indicators

[Sumit Bose]

On Tue, May 24, 2016 at 12:21:43PM -0400, Nathaniel McCallum wrote:
> New versions again. This time I just removed the stray "TODO: assign
> OID" line in the commit as it no longer applies.

ACK to patches 1-4.

Patch 5 can be committed independently and needs some additional
discussion, see below.

bye,
Sumit

> 
> On Tue, 2016-05-24 at 12:08 -0400, Nathaniel McCallum wrote:
> > I have attached new versions of the patches. Comments below.
> > 
> > On Tue, 2016-05-24 at 15:25 +0200, Sumit Bose wrote:
> > > On Thu, May 12, 2016 at 05:33:26PM -0400, Nathaniel McCallum wrote:
> > > > On Fri, 2016-05-06 at 14:44 +0200, Sumit Bose wrote:
> > > > > On Wed, May 04, 2016 at 05:33:55PM -0400, Nathaniel McCallum
> > > > > wrote:

...

> > > > From c9e2b50248493fb5a283cf8c88c8e20c312d6348 Mon Sep 17 00:00:00
> > > > 2001
> > > > From: Nathaniel McCallum 
> > > > Date: Wed, 4 May 2016 17:08:45 -0400
> > > > Subject: [PATCH 5/5] Enable service authentication indicator
> > > > management
> > > > 
> > > 
> > > For me the patch looks good, but it would be nice if someone more
> > > used
> > > to our usage of python can have a short look to see if all
> > > conventioens
> > > are met. ACK for the functionality.
> > 
> > I would like for us to merge the first four patches first and then
> > concentrate on this one.
> > 
> > In particular, the following issue needs to be discussed. We
> > currently
> > only have two, hard-coded indicator values: otp and radius. Thus, we
> > use a StrEnum for this property. However, in the long-term, I'd like
> > to
> > have more flexibility; such as per-token indicators. This implies
> > String.
> > 
> > Is there some way to do StrEnum now and easily migrate to String
> > later?
> > I think this will break API. Thoughts?
> > 

-- 
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] [PATCH 0104-0109] DNS upgrade: change forwarding policy to "only" if private IPs are used

[Martin Basti]

On 20.05.2016 12:19, Petr Spacek wrote:

On 11.5.2016 12:08, Martin Basti wrote:


On 03.05.2016 14:59, Petr Spacek wrote:

Hello,

DNS upgrade: change forwarding policy to "only" if private IPs are used.

https://fedorahosted.org/freeipa/ticket/5710

This is the upgrade part. I will add one more patch to print a warning in
dnsforwardzone* commands to avoid surprises. Please do not close the ticket
yet.




1)
Upgrade failed with 'BindInstance' object has no attribute
'named_conf_get_directive'
IPA server upgrade failed: Inspect /var/log/ipaupgrade.log and run command
ipa-server-upgrade manually.
('IPA upgrade failed.', 1)
The ipa-server-upgrade command failed. See /var/log/ipaupgrade.log for more
information

2016-05-11T08:26:20Z ERROR Upgrade failed with 'BindInstance' object has no
attribute 'named_conf_get_directive'
2016-05-11T08:26:20Z DEBUG Traceback (most recent call last):
   File
"/usr/lib/python2.7/site-packages/ipaserver/install/upgradeinstance.py", line
213, in __upgrade
 self.modified = (ld.update(self.files) or self.modified)
   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
line 917, in update
 self._run_updates(all_updates)
   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
line 889, in _run_updates
 self._run_update_plugin(update['plugin'])
   File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
line 862, in _run_update_plugin
 restart_ds, updates = self.api.Updater[plugin_name]()
   File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 1418, in
__call__
 return self.execute(**options)
   File "/usr/lib/python2.7/site-packages/ipaserver/install/plugins/dns.py",
line 547, in execute
 self.update_global_named_conf_forwarder(bind)
   File "/usr/lib/python2.7/site-packages/ipaserver/install/plugins/dns.py",
line 508, in update_global_named_conf_forwarder
 if bind.named_conf_get_directive(
AttributeError: 'BindInstance' object has no attribute 
'named_conf_get_directive'

2016-05-11T08:26:20Z DEBUG Traceback (most recent call last):
   File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
447, in start_creation
 run_step(full_msg, method)
   File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
437, in run_step
 method()
   File
"/usr/lib/python2.7/site-packages/ipaserver/install/upgradeinstance.py", line
221, in __upgrade
 raise RuntimeError(e)
RuntimeError: 'BindInstance' object has no attribute 'named_conf_get_directive'

PATCH * Add ipaDNSVersion option to dnsconfig* commands and use new attribute *
2)
+Int('ipadnsversion?',
+label=_('IPA DNS version'),
+),

Shouldn't be this part of System: Read DNS Configuration permission?

3)
-def postprocess_result(self, result):
+def postprocess_result(self, result, show_version):
  if not any(param in result['result'] for param in self.params):
  result['summary'] = unicode(_('Global DNS configuration is 
empty'))

show_version param was added but I don't see it used in this patch.

4)
+Int('ipadnsversion?',
+label=_('IPA DNS version'),
+),

Could we add comment here that this option is accessible only from installers
and upgrade?

5)
+for config_option in container_entry.get("ipaConfigString", []):
+matched = re.match("^DNSVersion\s+(?P\d+)$",
+   config_option, flags=re.I)
+if matched:
+version = int(matched.group("version"))

Shouldn't we print error if version cannot be parsed?

PATCH  * DNS upgrade: separate backup logic to make it reusable *

LGTM

PATCH * Add function ipapython.dnsutil.related_to_auto_empty_zone() *

7)
I'm curious why do you need to check superdomains?

PATCH * DNS upgrade: change forwarding policy to = only for conflicting
forward zones*

8)
+self.log.debug('Zone %s was sucessfully modified to use '
+   'forward policy "only"', zone['idnsname'][0])
<---missing empty line>
+def execute(self, **options):

PATCH * DNS upgrade: change global forwarding policy in LDAP to "only" if
private IPs are used *
9)
- dnsutil.related_to_auto_empty_zone(zone.get('idnsname')[0])
+dnsutil.related_to_auto_empty_zone(
+dnsutil.DNSName(zone.get('idnsname')[0]))

Should be in previous commit

10)
-return
+return False, []
This should be fixed in the previous commit

PATCH * DNS upgrade: change global forwarding policy in named.conf to "only"
if private IPs are used *
11)
IMO this is an upgrade of configuration and this should be in
ipaserver/install/server/upgrade.py, upgrade plugins are used only for
updating of LDAP values

Unless you really want to use this as precedence, but then it requires broader
discussion.

12)

bind.named_conf_get_directive
should be
bindinstance.named_conf_get_directive

see 1)

This new 

Re: [Freeipa-devel] [PATCH 0110] DNS: Warn if forwarding policy conflicts with automatic empty zone

[Martin Basti]

On 04.05.2016 10:43, Petr Spacek wrote:

Hello,

DNS: Warn if forwarding policy conflicts with automatic empty zones

Forwarding policy "first" or "none" may conflicts with some automatic empty
zones. Queries for zones specified by RFC 6303 will ignore
forwarding and recursion and always result in NXDOMAIN answers.

This is not detected and warned about. Global forwarding is equivalent
to forward zone ".".

Example:
Forward zone 1.10.in-addr.arpa with policy "first"
will not forward anything because BIND will automatically prefer
automatic empty zone "10.in-addr.arpa." which is authoritative.

https://fedorahosted.org/freeipa/ticket/5710


This is last patch in the series so the ticket can be closed when all relevant
patches are pushed.






You forgot to update tests

_ 
test_dns.test_command[0087: dnsconfig_mod: Update global DNS settings] 
__


self = 0x7fcef3ef2510>, index = 87
declarative_test_definition = {'command': ('dnsconfig_mod', [], 
{'idnsforwarders': ['172.16.31.80'], 'version': '2.166'}), 'desc': 
'Update global DN...arders': ['172.16.31.80']}, 'summary': None, 
'value': None}, 'nice': '0087: dnsconfig_mod: Update global DNS settings'}


def test_command(self, index, declarative_test_definition):
"""Run an individual test

The arguments are provided by the pytest plugin.
"""
if callable(declarative_test_definition):
declarative_test_definition(self)
else:
>   self.check(**declarative_test_definition)

test_xmlrpc/xmlrpc_test.py:313:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

test_xmlrpc/xmlrpc_test.py:325: in check
self.check_output(nice, cmd, args, options, expected, extra_check)
test_xmlrpc/xmlrpc_test.py:368: in check_output
assert_deepequal(expected, got, nice)
util.py:361: in assert_deepequal
assert_deepequal(e_sub, g_sub, doc, stack + (key,))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _


expected = [{'code': 13006, 'message':  at 
0x7fcef426c758>, 'name': 'DNSServerValidationWarning', 'type': 'warning'}]
got = [{'code': 13021, 'message': "Forwarding policy conflicts with some 
automatic empty zones. Queries for zones specified ...': The DNS 
operation timed out after 10.0008428097 seconds.", 'name': 
'DNSServerValidationWarning', 'type': 'warning'}]
doc = '0087: dnsconfig_mod: Update global DNS settings', stack = 
('messages',)


def assert_deepequal(expected, got, doc='', stack=tuple()):
"""
Recursively check for type and equality.

If a value in expected is callable then it will used as a 
callback to

test for equality on the got value. The callback is passed the got
value and returns True if equal, False otherwise.

If the tests fails, it will raise an ``AssertionError`` with 
detailed
information, including the path to the offending value.  For 
example:


>>> expected = [u'Hello', dict(world=u'how are you?')]
>>> got = [u'Hello', dict(world='how are you?')]
>>> expected == got
True
>>> assert_deepequal(expected, got, doc='Testing my nested data')
Traceback (most recent call last):
  ...
AssertionError: assert_deepequal: type(expected) is not type(got).
  Testing my nested data
  type(expected) = 
  type(got) = 
  expected = u'how are you?'
  got = 'how are you?'
  path = (0, 'world')

Note that lists and tuples are considered equivalent, and the 
order of

their elements does not matter.
"""
if isinstance(expected, tuple):
expected = list(expected)
if isinstance(got, tuple):
got = list(got)
if isinstance(expected, DN):
if isinstance(got, six.string_types):
got = DN(got)
if not (isinstance(expected, Fuzzy) or callable(expected) or 
type(expected) is type(got)):

raise AssertionError(
TYPE % (doc, type(expected), type(got), expected, got, 
stack)

)
if isinstance(expected, (list, tuple)):
if len(expected) != len(got):
raise AssertionError(
>   LEN % (doc, len(expected), len(got), expected, got, 
stack)

)
E   AssertionError: assert_deepequal: list length mismatch.
E 0087: dnsconfig_mod: Update global DNS settings
E len(expected) = 1
E len(got) = 2
E expected = 

Re: [Freeipa-devel] Questions on git

[Martin Kosek]

On 05/25/2016 11:55 AM, Christian Heimes wrote:
> On 2016-05-25 11:46, Martin Kosek wrote:
>> On 05/25/2016 10:03 AM, Jan Pazdziora wrote:
>>> On Mon, May 23, 2016 at 04:24:38PM +0200, Florence Blanc-Renaud wrote:

 - I start working on a specific issue and decide to create a branch on my
 git repository (on my laptop)
 git clone git://git.fedorahosted.org/git/freeipa.git
 git branch -b issue
>>>
>>> This likely needs to be
>>>
>>>  git checkout -b issue
>>>
 - When the tests are ok and I want to submit a patch, can I stay on the
 branch "issue" to create the patch or should I merge first with the main
 branch? If a merge is required, is it recommended to pull then merge or
 merge then pull?
>>>
>>> As mentioned by Martin, you are looking for rebase, not merge. Rebase
>>> will re-create commits from the branch on top of other branch (master,
>>> most likely), omitting changes that got to master in the mean time,
>>> and giving you chance to resolve conflicts with whatever other changes
>>> might have gone to master, so that others have as clean experience as
>>> possible.
>>>
>>> If you look at FreeIPA's history (I like gitk for that), you will see
>>> that merge commits are very rarely used. The reason for keeping the
>>> history linear (and thus rebasing on master often) is that it forces
>>> the author to be explicit about the diffs, plus git tools for
>>> introspecting history often choke on parallel branches that get
>>> merged.
>>
>> +1, we want to keep that. For example, even though we already had some
>> discussions about adopting github workflow (pull reuqests) as the main 
>> vehicle
>> for patch reviews, we would still prefer to avoid merging and keep rebasing -
>> the history is much cleaner that way.
> 
> +1 against merge commits
> 
> A couple of months ago github introduced a new option. The green merge
> button can be configured to either do a merge commit, squash all commits
> in the branch or both.
> https://help.github.com/articles/about-pull-request-merge-squashing/

Interesting. Do you know why github forces the squashing? It would be better if
we can simply have the commits rebased and applied to master branch.

> I guess we can use squashed merges for the majority of simple PRs.

I expect we will anyway end up with extending "ipatool push" command to do some
magic with github API, Trac and Bugzilla as we are used now. Except that it
won't accept list of patches in file, but rathe a link/PR number. But this is
of course up to dicsussion.

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] Questions on git

[Christian Heimes]

On 2016-05-25 11:46, Martin Kosek wrote:
> On 05/25/2016 10:03 AM, Jan Pazdziora wrote:
>> On Mon, May 23, 2016 at 04:24:38PM +0200, Florence Blanc-Renaud wrote:
>>>
>>> - I start working on a specific issue and decide to create a branch on my
>>> git repository (on my laptop)
>>> git clone git://git.fedorahosted.org/git/freeipa.git
>>> git branch -b issue
>>
>> This likely needs to be
>>
>>  git checkout -b issue
>>
>>> - When the tests are ok and I want to submit a patch, can I stay on the
>>> branch "issue" to create the patch or should I merge first with the main
>>> branch? If a merge is required, is it recommended to pull then merge or
>>> merge then pull?
>>
>> As mentioned by Martin, you are looking for rebase, not merge. Rebase
>> will re-create commits from the branch on top of other branch (master,
>> most likely), omitting changes that got to master in the mean time,
>> and giving you chance to resolve conflicts with whatever other changes
>> might have gone to master, so that others have as clean experience as
>> possible.
>>
>> If you look at FreeIPA's history (I like gitk for that), you will see
>> that merge commits are very rarely used. The reason for keeping the
>> history linear (and thus rebasing on master often) is that it forces
>> the author to be explicit about the diffs, plus git tools for
>> introspecting history often choke on parallel branches that get
>> merged.
> 
> +1, we want to keep that. For example, even though we already had some
> discussions about adopting github workflow (pull reuqests) as the main vehicle
> for patch reviews, we would still prefer to avoid merging and keep rebasing -
> the history is much cleaner that way.

+1 against merge commits

A couple of months ago github introduced a new option. The green merge
button can be configured to either do a merge commit, squash all commits
in the branch or both.
https://help.github.com/articles/about-pull-request-merge-squashing/

I guess we can use squashed merges for the majority of simple PRs.

Christian




signature.asc
Description: OpenPGP digital signature
-- 
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] Questions on git

[Martin Kosek]

On 05/25/2016 10:03 AM, Jan Pazdziora wrote:
> On Mon, May 23, 2016 at 04:24:38PM +0200, Florence Blanc-Renaud wrote:
>>
>> - I start working on a specific issue and decide to create a branch on my
>> git repository (on my laptop)
>> git clone git://git.fedorahosted.org/git/freeipa.git
>> git branch -b issue
> 
> This likely needs to be
> 
>  git checkout -b issue
> 
>> - When the tests are ok and I want to submit a patch, can I stay on the
>> branch "issue" to create the patch or should I merge first with the main
>> branch? If a merge is required, is it recommended to pull then merge or
>> merge then pull?
> 
> As mentioned by Martin, you are looking for rebase, not merge. Rebase
> will re-create commits from the branch on top of other branch (master,
> most likely), omitting changes that got to master in the mean time,
> and giving you chance to resolve conflicts with whatever other changes
> might have gone to master, so that others have as clean experience as
> possible.
> 
> If you look at FreeIPA's history (I like gitk for that), you will see
> that merge commits are very rarely used. The reason for keeping the
> history linear (and thus rebasing on master often) is that it forces
> the author to be explicit about the diffs, plus git tools for
> introspecting history often choke on parallel branches that get
> merged.

+1, we want to keep that. For example, even though we already had some
discussions about adopting github workflow (pull reuqests) as the main vehicle
for patch reviews, we would still prefer to avoid merging and keep rebasing -
the history is much cleaner that way.

-- 
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] [PATCH 0477] upgrade: always start CA

[Stanislav Laznicka]

On 05/20/2016 03:00 PM, Martin Basti wrote:


On 19.05.2016 13:34, Stanislav Laznicka wrote:


Also, I tried to upgrade from 4.2.4 to 4.3.1 and it seems that it 
might be necessary to start the service even earlier in the upgrade 
logic. Attached is the trace that occurred during the upgrade.


I sent the whole log earlier accidentally, hopefully it will not 
arrive here as well.


On 05/19/2016 11:10 AM, Stanislav Laznicka wrote:


NACK, see my comments below

+# following upgrade steps require running CA
This is a nitpicky nitpick but could you please change this comment 
for # the following ...

Took me a while to understand what you were trying to say here.
+if ca_running and not ca.is_running():
+ca.stop('pki-tomcat')
+elif not ca_running and ca.is_running():
+ca.start('pki-tomcat')
+
You should swap ca.stop and ca.start here, you're stopping the 
service when it's stopped and starting it when it's already running.

Shame, shame, shame on me.



On 05/12/2016 04:34 PM, Martin Basti wrote:

Patch attached.

https://fedorahosted.org/freeipa/ticket/5868











I moved starting of CA to the earlier phase and swapped start/stop to 
correct order.


Patch attached.

Seems to work as expected now. ACK.
-- 
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] [PATCH 0484] remove unused code from automount plugin

[Stanislav Laznicka]

LGTM, could you please just add the ticket to the commit message?


On 05/20/2016 04:28 PM, Martin Basti wrote:




On 20.05.2016 15:03, Martin Basti wrote:

The removed code is unused for long time.

Patch attached.




https://fedorahosted.org/freeipa/attachment/ticket/5899/




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