Re: [Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group
On 05/07/2015 07:01 PM, Martin Basti wrote: On 07/05/15 13:17, Petr Vobornik wrote: On 05/06/2015 04:49 PM, Martin Basti wrote: On 05/05/15 10:59, Petr Vobornik wrote: On 04/30/2015 03:53 PM, Martin Basti wrote: On 10/04/15 12:55, Petr Vobornik wrote: The essential patch is 814. snip As we personally talked with Honza, and he agreed with param modification, then ACK for all patches. Pushed to master: master: * 2c1bca3b0f19f69471c993867113d13cbc54636a migrate-ds: optimize adding users to default group * fda96988444c8c01115f0e992abe1b71192998d5 migrate-ds: skip default group option * a6ca9800fa8b48b9fa71dc54cc036a17d35b6197 migrate-ds: remove unused def_group_gid context property * c3d99a28a0bfed2198712c98830c2011fd6be874 migrate-ds: optimize gid checks by utilizing dictionary nature of set * 3b0e81ce062e63a3ca45df1cd997196cc340f565 migrate-ds: log migrated group members only on debug level * 91b39acd6bca1a1054220291f4b4a0a4118e829c cli: differentiate Flag a Bool when autofill is set -- Petr Vobornik -- 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] 814-818 migrate-ds: optimize adding users to default group
On 07/05/15 13:17, Petr Vobornik wrote:
On 05/06/2015 04:49 PM, Martin Basti wrote:
On 05/05/15 10:59, Petr Vobornik wrote:
On 04/30/2015 03:53 PM, Martin Basti wrote:
On 10/04/15 12:55, Petr Vobornik wrote:
The essential patch is 814.
815 a proposal for new option.
816 and 818 are cleanup patches.
817 little optimization.
== [PATCH] 814 migrate-ds: optimize adding users to default group ==
Migrate-ds searches for user without a group and adds them to default
group. There is no point in checking if the user's selected by
previous query are not member of default group because they are not
member of any group.
The operation is also speeded up by not fetching the default group.
Users are added right away.
https://fedorahosted.org/freeipa/ticket/4950
NACK
Users haven't been added into ipa default group after migration.
Fixed in patch 815.
Just nitpick
1) too many parentheses
api.log.error(('Adding new members to default group
failed:
%s \n'
'members: %s') % (e,
(','.join(member_dns
You can use this instead:
api.log.error('Adding new members to default group
failed:
%s \n'
'members: %s', e, ','.join(member_dns))
Fixed.
== [PATCH] 815 migrate-ds: skip default group options ==
New option --use-default-group=False could be used to disable
adding of
migrated users into default group.
By default, the default group is no longer POSIX therefore it doesn't
fulfill the original idea of providing GID and therefore it could be
skipped during migration.
LGTM
the option got so the default would be applied.
+autofill=True,
== [PATCH] 816 migrate-ds: remove unused def_group_gid context
property ==
it's no longer used anywhere
1)
You can remove the unused variable 'g_attrs'
removed
== [PATCH] 817 migrate-ds: optimize gid checks by utilizing
dictionary
nature of set ==
LGTM
== [PATCH] 818 migrate-ds: log migrated group members only on debug
level ==
It pollutes error_log.
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
fixed and also changed "migrating %s user %s'" line to debug, which
was the actual reason for this patch.
Martin^2
Thanks.
1)
Please create new API file, probably missing autofill in API.txt:
Option 'use_def_group?' in command 'migrate_ds' in API file not found
Options count in migrate_ds of 18 doesn't match expected: 19
Option use_def_group? of command migrate_ds in ipalib, not in API file:
Bool('use_def_group?', autofill=True, cli_name='use_default_group',
default=True)
There are one or more changes to the API.
Either undo the API changes or update API.txt and increment the major
version in VERSION.
you could just wrote that I forgot to run ./makeapi ;)
2)
ipa: error: --use-default-group option does not take a value
Attached new patch #826 which should fix the issue. Not sure if
Honza(CCd) will like it.
"(default: true)" added to option description for better UX.
and a nitpick:
patch 814
1)
Why this change?
-api.log.debug('Adding %d users to group%s duration %s',
-len(new_members), mode, d)
+api.log.info('Adding %d users to group%s duration %s',
+ len(member_dns), mode, d)
So that there will be a record in a default(not debug) log that
something happened. The reason is that it also affects users, without
a group, that are already present on the system.
As we personally talked with Honza, and he agreed with param
modification, then
ACK for all patches.
--
Martin Basti
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group
On 05/06/2015 04:49 PM, Martin Basti wrote:
On 05/05/15 10:59, Petr Vobornik wrote:
On 04/30/2015 03:53 PM, Martin Basti wrote:
On 10/04/15 12:55, Petr Vobornik wrote:
The essential patch is 814.
815 a proposal for new option.
816 and 818 are cleanup patches.
817 little optimization.
== [PATCH] 814 migrate-ds: optimize adding users to default group ==
Migrate-ds searches for user without a group and adds them to default
group. There is no point in checking if the user's selected by
previous query are not member of default group because they are not
member of any group.
The operation is also speeded up by not fetching the default group.
Users are added right away.
https://fedorahosted.org/freeipa/ticket/4950
NACK
Users haven't been added into ipa default group after migration.
Fixed in patch 815.
Just nitpick
1) too many parentheses
api.log.error(('Adding new members to default group failed:
%s \n'
'members: %s') % (e,
(','.join(member_dns
You can use this instead:
api.log.error('Adding new members to default group failed:
%s \n'
'members: %s', e, ','.join(member_dns))
Fixed.
== [PATCH] 815 migrate-ds: skip default group options ==
New option --use-default-group=False could be used to disable adding of
migrated users into default group.
By default, the default group is no longer POSIX therefore it doesn't
fulfill the original idea of providing GID and therefore it could be
skipped during migration.
LGTM
the option got so the default would be applied.
+autofill=True,
== [PATCH] 816 migrate-ds: remove unused def_group_gid context
property ==
it's no longer used anywhere
1)
You can remove the unused variable 'g_attrs'
removed
== [PATCH] 817 migrate-ds: optimize gid checks by utilizing dictionary
nature of set ==
LGTM
== [PATCH] 818 migrate-ds: log migrated group members only on debug
level ==
It pollutes error_log.
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
fixed and also changed "migrating %s user %s'" line to debug, which
was the actual reason for this patch.
Martin^2
Thanks.
1)
Please create new API file, probably missing autofill in API.txt:
Option 'use_def_group?' in command 'migrate_ds' in API file not found
Options count in migrate_ds of 18 doesn't match expected: 19
Option use_def_group? of command migrate_ds in ipalib, not in API file:
Bool('use_def_group?', autofill=True, cli_name='use_default_group',
default=True)
There are one or more changes to the API.
Either undo the API changes or update API.txt and increment the major
version in VERSION.
you could just wrote that I forgot to run ./makeapi ;)
2)
ipa: error: --use-default-group option does not take a value
Attached new patch #826 which should fix the issue. Not sure if
Honza(CCd) will like it.
"(default: true)" added to option description for better UX.
and a nitpick:
patch 814
1)
Why this change?
-api.log.debug('Adding %d users to group%s duration %s',
-len(new_members), mode, d)
+api.log.info('Adding %d users to group%s duration %s',
+ len(member_dns), mode, d)
So that there will be a record in a default(not debug) log that
something happened. The reason is that it also affects users, without a
group, that are already present on the system.
--
Petr Vobornik
From 568b6b233d99b65af7d9d4d140164a7b693027a6 Mon Sep 17 00:00:00 2001
From: Petr Vobornik
Date: Thu, 9 Apr 2015 17:17:09 +0200
Subject: [PATCH] migrate-ds: log migrated group members only on debug level
It pollutes error_log.
---
ipalib/plugins/migration.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 22bf6a8f6a53343cba3869ad5924131c7861d819..8b7dd9ef6c5e16ef39997f04ca935c4de3e56aa9 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -338,11 +338,11 @@ def _pre_migrate_group(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwarg
continue
if m.endswith(search_bases['user']):
-api.log.info('migrating %s user %s' % (member_attr, m))
+api.log.debug('migrating %s user %s', member_attr, m)
m = DN((api.Object.user.primary_key.name, rdnval),
api.env.container_user, api.env.basedn)
elif m.endswith(search_bases['group']):
-api.log.info('migrating %s group %s' % (member_attr, m))
+api.log.debug('migrating %s group %s', member_attr, m)
m = DN((api.Object.group.primary_key.name, rdnval),
api.env.container_group, api.env.basedn)
else:
--
2.1.0
From 2560a055c8cec039fd8e109d8e33c61967783203 Mon Sep 17 00:00:00 2001
From: Petr Vobornik
Date: Mon, 23 Mar 2015 12:18:23 +0100
Subject: [PATCH] migrate-ds:
Re: [Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group
On 05/05/15 10:59, Petr Vobornik wrote:
On 04/30/2015 03:53 PM, Martin Basti wrote:
On 10/04/15 12:55, Petr Vobornik wrote:
The essential patch is 814.
815 a proposal for new option.
816 and 818 are cleanup patches.
817 little optimization.
== [PATCH] 814 migrate-ds: optimize adding users to default group ==
Migrate-ds searches for user without a group and adds them to default
group. There is no point in checking if the user's selected by
previous query are not member of default group because they are not
member of any group.
The operation is also speeded up by not fetching the default group.
Users are added right away.
https://fedorahosted.org/freeipa/ticket/4950
NACK
Users haven't been added into ipa default group after migration.
Fixed in patch 815.
Just nitpick
1) too many parentheses
api.log.error(('Adding new members to default group failed:
%s \n'
'members: %s') % (e,
(','.join(member_dns
You can use this instead:
api.log.error('Adding new members to default group failed:
%s \n'
'members: %s', e, ','.join(member_dns))
Fixed.
== [PATCH] 815 migrate-ds: skip default group options ==
New option --use-default-group=False could be used to disable adding of
migrated users into default group.
By default, the default group is no longer POSIX therefore it doesn't
fulfill the original idea of providing GID and therefore it could be
skipped during migration.
LGTM
the option got so the default would be applied.
+autofill=True,
== [PATCH] 816 migrate-ds: remove unused def_group_gid context
property ==
it's no longer used anywhere
1)
You can remove the unused variable 'g_attrs'
removed
== [PATCH] 817 migrate-ds: optimize gid checks by utilizing dictionary
nature of set ==
LGTM
== [PATCH] 818 migrate-ds: log migrated group members only on debug
level ==
It pollutes error_log.
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
fixed and also changed "migrating %s user %s'" line to debug, which
was the actual reason for this patch.
Martin^2
Thanks.
1)
Please create new API file, probably missing autofill in API.txt:
Option 'use_def_group?' in command 'migrate_ds' in API file not found
Options count in migrate_ds of 18 doesn't match expected: 19
Option use_def_group? of command migrate_ds in ipalib, not in API file:
Bool('use_def_group?', autofill=True, cli_name='use_default_group',
default=True)
There are one or more changes to the API.
Either undo the API changes or update API.txt and increment the major
version in VERSION.
2)
ipa: error: --use-default-group option does not take a value
and a nitpick:
patch 814
1)
Why this change?
-api.log.debug('Adding %d users to group%s duration %s',
-len(new_members), mode, d)
+api.log.info('Adding %d users to group%s duration %s',
+ len(member_dns), mode, d)
--
Martin Basti
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group
On 04/30/2015 03:53 PM, Martin Basti wrote:
On 10/04/15 12:55, Petr Vobornik wrote:
The essential patch is 814.
815 a proposal for new option.
816 and 818 are cleanup patches.
817 little optimization.
== [PATCH] 814 migrate-ds: optimize adding users to default group ==
Migrate-ds searches for user without a group and adds them to default
group. There is no point in checking if the user's selected by
previous query are not member of default group because they are not
member of any group.
The operation is also speeded up by not fetching the default group.
Users are added right away.
https://fedorahosted.org/freeipa/ticket/4950
NACK
Users haven't been added into ipa default group after migration.
Fixed in patch 815.
Just nitpick
1) too many parentheses
api.log.error(('Adding new members to default group failed:
%s \n'
'members: %s') % (e, (','.join(member_dns
You can use this instead:
api.log.error('Adding new members to default group failed:
%s \n'
'members: %s', e, ','.join(member_dns))
Fixed.
== [PATCH] 815 migrate-ds: skip default group options ==
New option --use-default-group=False could be used to disable adding of
migrated users into default group.
By default, the default group is no longer POSIX therefore it doesn't
fulfill the original idea of providing GID and therefore it could be
skipped during migration.
LGTM
the option got so the default would be applied.
+autofill=True,
== [PATCH] 816 migrate-ds: remove unused def_group_gid context
property ==
it's no longer used anywhere
1)
You can remove the unused variable 'g_attrs'
removed
== [PATCH] 817 migrate-ds: optimize gid checks by utilizing dictionary
nature of set ==
LGTM
== [PATCH] 818 migrate-ds: log migrated group members only on debug level ==
It pollutes error_log.
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
fixed and also changed "migrating %s user %s'" line to debug, which was
the actual reason for this patch.
Martin^2
--
Petr Vobornik
From 4d94d466a2baadc5bf7537b3a53cc1e33df7ec7c Mon Sep 17 00:00:00 2001
From: Petr Vobornik
Date: Thu, 9 Apr 2015 17:17:09 +0200
Subject: [PATCH] migrate-ds: log migrated group members only on debug level
It pollutes error_log.
---
ipalib/plugins/migration.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 934abea095f49ff716d453a5cf11192de16180cc..2e0120414d7ff980d90f5be9091e17b4dca96256 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -338,11 +338,11 @@ def _pre_migrate_group(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwarg
continue
if m.endswith(search_bases['user']):
-api.log.info('migrating %s user %s' % (member_attr, m))
+api.log.debug('migrating %s user %s', member_attr, m)
m = DN((api.Object.user.primary_key.name, rdnval),
api.env.container_user, api.env.basedn)
elif m.endswith(search_bases['group']):
-api.log.info('migrating %s group %s' % (member_attr, m))
+api.log.debug('migrating %s group %s', member_attr, m)
m = DN((api.Object.group.primary_key.name, rdnval),
api.env.container_group, api.env.basedn)
else:
--
2.1.0
From 32230d9f5b0a68c9e750a63ee973a521a12caefc Mon Sep 17 00:00:00 2001
From: Petr Vobornik
Date: Mon, 23 Mar 2015 12:18:23 +0100
Subject: [PATCH] migrate-ds: optimize gid checks by utilizing dictionary
nature of set
---
ipalib/plugins/migration.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 23474aabc92a00c9db41146dc394146a44826b66..934abea095f49ff716d453a5cf11192de16180cc 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -166,11 +166,11 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
'gidnumber', entry_attrs['gidnumber'][0], 'posixgroup',
[''], search_bases['group']
)
-valid_gids.append(entry_attrs['gidnumber'][0])
+valid_gids.add(entry_attrs['gidnumber'][0])
except errors.NotFound:
api.log.warn('GID number %s of migrated user %s does not point to a known group.' \
% (entry_attrs['gidnumber'][0], pkey))
-invalid_gids.append(entry_attrs['gidnumber'][0])
+invalid_gids.add(entry_attrs['gidnumber'][0])
except errors.SingleMatchExpected, e:
# GID number matched more groups, this should not happen
api.log.warn('GID number %s of migrated user %s should match 1 group, but it ma
Re: [Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group
On 10/04/15 12:55, Petr Vobornik wrote:
The essential patch is 814.
815 a proposal for new option.
816 and 818 are cleanup patches.
817 little optimization.
== [PATCH] 814 migrate-ds: optimize adding users to default group ==
Migrate-ds searches for user without a group and adds them to default
group. There is no point in checking if the user's selected by
previous query are not member of default group because they are not
member of any group.
The operation is also speeded up by not fetching the default group.
Users are added right away.
https://fedorahosted.org/freeipa/ticket/4950
NACK
Users haven't been added into ipa default group after migration.
Just nitpick
1) too many parentheses
api.log.error(('Adding new members to default group failed:
%s \n'
'members: %s') % (e, (','.join(member_dns
You can use this instead:
api.log.error('Adding new members to default group failed:
%s \n'
'members: %s', e, ','.join(member_dns))
== [PATCH] 815 migrate-ds: skip default group options ==
New option --use-default-group=False could be used to disable adding of
migrated users into default group.
By default, the default group is no longer POSIX therefore it doesn't
fulfill the original idea of providing GID and therefore it could be
skipped during migration.
LGTM
== [PATCH] 816 migrate-ds: remove unused def_group_gid context
property ==
it's no longer used anywhere
1)
You can remove the unused variable 'g_attrs'
== [PATCH] migrate-ds: optimize gid checks by utilizing dictionary
nature of set ==
LGTM
== [PATCH] migrate-ds: log migrated group members only on debug level ==
It pollutes error_log.
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
Martin^2
--
Martin Basti
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
