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 <pvobo...@redhat.com>
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 <pvobo...@redhat.com>
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 919af9d7c63e65f2731daef8ace2f1baf6be58f7..22bf6a8f6a53343cba3869ad5924131c7861d819 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 matched %d groups' \
@@ -765,8 +765,8 @@ can use their Kerberos accounts.''')
 
             context['has_upg'] = ldap.has_upg()
 
-            valid_gids = []
-            invalid_gids = []
+            valid_gids = set()
+            invalid_gids = set()
             migrate_cnt = 0
             context['migrate_cnt'] = 0
             for entry_attrs in entries:
-- 
2.1.0

From 5d54f4e66e01f1fbd72337abc8593962c9345305 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Mon, 23 Mar 2015 12:15:52 +0100
Subject: [PATCH] migrate-ds: remove unused def_group_gid context property

it's no longer used anywhere
---
 ipalib/plugins/migration.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 9846b3e2fecd754346ba4748972df233fe6ca04b..919af9d7c63e65f2731daef8ace2f1baf6be58f7 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -758,12 +758,10 @@ can use their Kerberos accounts.''')
                 def_group = config.get('ipadefaultprimarygroup')
                 context['def_group_dn'] = api.Object.group.get_dn(def_group)
                 try:
-                    g_attrs = ldap.get_entry(context['def_group_dn'], ['gidnumber', 'cn'])
+                    ldap.get_entry(context['def_group_dn'], ['gidnumber', 'cn'])
                 except errors.NotFound:
                     error_msg = _('Default group for new users not found')
                     raise errors.NotFound(reason=error_msg)
-                if 'gidnumber' in g_attrs:
-                    context['def_group_gid'] = g_attrs['gidnumber'][0]
 
             context['has_upg'] = ldap.has_upg()
 
-- 
2.1.0

From 806070153e6fc6464a1abfaecdd920acf1d8c919 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Fri, 20 Mar 2015 18:00:19 +0100
Subject: [PATCH] migrate-ds: skip default group option

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.

https://fedorahosted.org/freeipa/ticket/4950
---
 API.txt                     |  3 ++-
 VERSION                     |  4 ++--
 ipalib/plugins/migration.py | 18 ++++++++++++++----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/API.txt b/API.txt
index f747765d7f9c87761fed0277cd59d1bc3fbd57e9..346e35fda536f8411f8ea8f2dc32af4caebf3fca 100644
--- a/API.txt
+++ b/API.txt
@@ -2450,7 +2450,7 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
 output: PrimaryKey('value', None, None)
 command: migrate_ds
-args: 2,18,4
+args: 2,19,4
 arg: Str('ldapuri', cli_name='ldap_uri')
 arg: Password('bindpw', cli_name='password', confirm=False)
 option: DNParam('basedn?', cli_name='base_dn')
@@ -2466,6 +2466,7 @@ option: Str('groupignoreobjectclass*', autofill=True, cli_name='group_ignore_obj
 option: Str('groupobjectclass+', autofill=True, cli_name='group_objectclass', csv=True, default=(u'groupOfUniqueNames', u'groupOfNames'))
 option: Flag('groupoverwritegid', autofill=True, cli_name='group_overwrite_gid', default=False)
 option: StrEnum('schema?', autofill=True, cli_name='schema', default=u'RFC2307bis', values=(u'RFC2307bis', u'RFC2307'))
+option: Bool('use_def_group?', autofill=True, cli_name='use_default_group', default=True)
 option: DNParam('usercontainer', autofill=True, cli_name='user_container', default=ipapython.dn.DN('ou=people'))
 option: Str('userignoreattribute*', autofill=True, cli_name='user_ignore_attribute', csv=True, default=())
 option: Str('userignoreobjectclass*', autofill=True, cli_name='user_ignore_objectclass', csv=True, default=())
diff --git a/VERSION b/VERSION
index b584eb4584ea45881e5329a846dae0df7e231844..8824bbfe4f323e687fb739ea51495402752284f5 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=116
-# Last change: tbordaz - Add stageuser_add command"
+IPA_API_VERSION_MINOR=117
+# Last change: pvoborni - added --use-default-group option to migrate-ds
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index b89ddac3ec2de26e9021f64a5f648f30f16f1eb5..9846b3e2fecd754346ba4748972df233fe6ca04b 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -21,7 +21,7 @@ import re
 from ldap import MOD_ADD
 
 from ipalib import api, errors, output
-from ipalib import Command, Password, Str, Flag, StrEnum, DNParam, File
+from ipalib import Command, Password, Str, Flag, StrEnum, DNParam, File, Bool
 from ipalib.cli import to_cli
 from ipalib.plugable import Registry
 from ipalib.plugins.user import NO_UPG_MAGIC
@@ -269,7 +269,8 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
 def _post_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx):
     assert isinstance(dn, DN)
 
-    _update_default_group(ldap, ctx, False)
+    if 'def_group_dn' in ctx:
+        _update_default_group(ldap, ctx, False)
 
     if 'description' in entry_attrs and NO_UPG_MAGIC in entry_attrs['description']:
         entry_attrs['description'].remove(NO_UPG_MAGIC)
@@ -602,6 +603,14 @@ class migrate_ds(Command):
             doc=_('Load CA certificate of LDAP server from FILE'),
             default=None
         ),
+        Bool('use_def_group?',
+            cli_name='use_default_group',
+            label=_('Add to default group'),
+            doc=_('Add migrated users without a group to a default group '
+                  '(default: true)'),
+            default=True,
+            autofill=True,
+        ),
     )
 
     has_output = (
@@ -745,7 +754,7 @@ can use their Kerberos accounts.''')
                     blacklists[blacklist] = tuple()
 
             # get default primary group for new users
-            if 'def_group_dn' not in context:
+            if 'def_group_dn' not in context and options.get('use_def_group'):
                 def_group = config.get('ipadefaultprimarygroup')
                 context['def_group_dn'] = api.Object.group.get_dn(def_group)
                 try:
@@ -836,7 +845,8 @@ can use their Kerberos accounts.''')
                     api.log.info("%d %ss migrated. %s elapsed." % (migrate_cnt, ldap_obj_name, total_dur))
                 api.log.debug("%d %ss migrated, duration: %s (total %s)" % (migrate_cnt, ldap_obj_name, d, total_dur))
 
-        _update_default_group(ldap, context, True)
+        if 'def_group_dn' in context:
+            _update_default_group(ldap, context, True)
 
         return (migrated, failed)
 
-- 
2.1.0

From a4294d0492f33aa7efbff3d19d5b2e3630e7b282 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Wed, 18 Mar 2015 18:50:38 +0100
Subject: [PATCH] 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 queary 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
---
 ipalib/plugins/migration.py | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index c8379420d539ac35901d99f981b4c8e2f0f89ffc..b89ddac3ec2de26e9021f64a5f648f30f16f1eb5 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -18,6 +18,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import re
+from ldap import MOD_ADD
 
 from ipalib import api, errors, output
 from ipalib import Command, Password, Str, Flag, StrEnum, DNParam, File
@@ -291,31 +292,25 @@ def _update_default_group(ldap, ctx, force):
         try:
             (result, truncated) = ldap.find_entries(searchfilter,
                 [''], DN(api.env.container_user, api.env.basedn),
-                scope=ldap.SCOPE_SUBTREE, time_limit = -1)
+                scope=ldap.SCOPE_SUBTREE, time_limit=-1, size_limit=-1)
         except errors.NotFound:
             api.log.debug('All users have default group set')
             return
-        new_members = []
-        group_entry_attrs = ldap.get_entry(group_dn, ['member'])
-        existing_members = set(group_entry_attrs.get('member', []))
-        for m in result:
-            if m.dn not in existing_members:
-                new_members.append(m.dn)
 
-        if new_members:
-            members = group_entry_attrs.setdefault('member', [])
-            members.extend(new_members)
-
-            try:
-                ldap.update_entry(group_entry_attrs)
-            except errors.EmptyModlist:
-                pass
+        member_dns = [m.dn for m in result]
+        modlist = [(MOD_ADD, 'member', ldap.encode(member_dns))]
+        try:
+            with ldap.error_handler():
+                ldap.conn.modify_s(str(group_dn), modlist)
+        except errors.DatabaseError as e:
+            api.log.error('Adding new members to default group failed: %s \n'
+                          'members: %s', e, ','.join(member_dns))
 
         e = datetime.datetime.now()
         d = e - s
         mode = " (forced)" if force else ""
-        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)
 
 # GROUP MIGRATION CALLBACKS AND VARS
 
-- 
2.1.0

From c02c21e7c74b0cf920f2331c1564ff371ab4bf9a Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Thu, 7 May 2015 12:41:12 +0200
Subject: [PATCH] cli: differentiate Flag a Bool when autofill is set

With previous behavior there was no difference between Flag and Bool if
- autofill == True
- default = some value

It prevented to have a boolean which is set by default to true, but could
be set to False if users wants to without prompting in interactive shell.
---
 ipalib/cli.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipalib/cli.py b/ipalib/cli.py
index bbf616d14f81b92ac6255b62313b66a95d2801ac..fc6e2303919d4db724d97f839d9a1b71752dfc10 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -48,7 +48,7 @@ from errors import (PublicError, CommandError, HelpError, InternalError,
                     NoSuchNamespaceError, ValidationError, NotFound,
                     NotConfiguredError, PromptFailed)
 from constants import CLI_TAB, LDAP_GENERALIZED_TIME_FORMAT
-from parameters import File, Str, Enum, Any
+from parameters import File, Str, Enum, Any, Flag
 from text import _
 from ipapython.version import API_VERSION
 from ipapython.dnsutil import DNSName
@@ -1150,7 +1150,7 @@ class cli(backend.Executioner):
                 continue
             if option.password and self.env.interactive:
                 kw['action'] = 'store_true'
-            elif option.type is bool and option.autofill:
+            elif isinstance(option, Flag):
                 if option.default is True:
                     kw['action'] = 'store_false'
                 else:
-- 
2.1.0

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

Reply via email to