Re: [Freeipa-devel] [PATCH] 635 wait for memberof plugin when doing reverse members
Jan Zelený wrote: Rob Crittenden wrote: Jan Zeleny wrote: Rob Crittenden wrote: Jan Zelený wrote: Rob Crittendenwrote: Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 rob About that FIXME you got there: I'm not sure if it wouldn't be better to handle the possible exception right in the wait_for_memberof method (I guess it depends on what exception are we expecting and what are we going to do with it?). If you want the exception to reach the calling function, I'd like to see some kind of exception handling in that function - either to let the user know that the error occurred during this waiting or maybe to disregard the exception and continue normal operation. The types of exceptions could run the gambit but I was wondering what would happen if we were looping and some other user deleted the role. The next search for it would fail with NotFound. Granted this isn't a very friendly message to return to someone after adding a member to the group but it does sort of make sense (someone deleted it concurrently). It seemed best to just let this filter up to the caller. Yes, I understand that. But my point was that it would be more user friendy to catch this exception in the calling function and adjust the error message to the situation. Otherwise user can get completely out-of-context error message, like "user not found" when working with groups or something like that. Some nitpicking: I'm confused - in the doc string you state that "this will loop for 6+ seconds" and a couple lines below, you have a comment "Don't sleep for more that 6 seconds" - is there a mistake ar are these two statements unrelated? Yeah, I was afraid that might be confusing. I'll wait .3 seconds 20 times so 6 seconds. There are a few LDAP calls which take a bit of time as well, so it will be 6+ seconds if it goes the whole time. Ok, thanks for explanation Jan Ok, I added a catch-all in case something goes horribly wrong. rob ack Jan pushed to master ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 635 wait for memberof plugin when doing reverse members
Jan Zeleny wrote: Rob Crittenden wrote: Jan Zelený wrote: Rob Crittenden wrote: Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 rob About that FIXME you got there: I'm not sure if it wouldn't be better to handle the possible exception right in the wait_for_memberof method (I guess it depends on what exception are we expecting and what are we going to do with it?). If you want the exception to reach the calling function, I'd like to see some kind of exception handling in that function - either to let the user know that the error occurred during this waiting or maybe to disregard the exception and continue normal operation. The types of exceptions could run the gambit but I was wondering what would happen if we were looping and some other user deleted the role. The next search for it would fail with NotFound. Granted this isn't a very friendly message to return to someone after adding a member to the group but it does sort of make sense (someone deleted it concurrently). It seemed best to just let this filter up to the caller. Yes, I understand that. But my point was that it would be more user friendy to catch this exception in the calling function and adjust the error message to the situation. Otherwise user can get completely out-of-context error message, like "user not found" when working with groups or something like that. Some nitpicking: I'm confused - in the doc string you state that "this will loop for 6+ seconds" and a couple lines below, you have a comment "Don't sleep for more that 6 seconds" - is there a mistake ar are these two statements unrelated? Yeah, I was afraid that might be confusing. I'll wait .3 seconds 20 times so 6 seconds. There are a few LDAP calls which take a bit of time as well, so it will be 6+ seconds if it goes the whole time. Ok, thanks for explanation Jan Ok, I added a catch-all in case something goes horribly wrong. rob >From 59e21c840e096ebf693bc9cdcc93c939b06cfd2c Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Mon, 13 Dec 2010 11:10:37 -0500 Subject: [PATCH] Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 --- ipalib/errors.py | 17 +++ ipalib/plugins/baseldap.py | 65 +-- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/ipalib/errors.py b/ipalib/errors.py index fe2b01e..d9bf385 100644 --- a/ipalib/errors.py +++ b/ipalib/errors.py @@ -1157,6 +1157,23 @@ class ManagedGroupExistsError(ExecutionError): errno = 4024 format = _('Unable to create private group. Group \'%(group)s\' already exists.') + +class ReverseMemberError(ExecutionError): +""" +**4025** Raised when verifying that all reverse members have been added or removed. + +For example: + +>>> raise ReverseMemberError(verb=u'added', exc=u'Group \'foo\' not found.') +Traceback (most recent call last): + ... +A problem was encounted when verifying that all members were added: Group 'foo' not found. +""" + +errno = 4025 +format = _('A problem was encounted when verifying that all members were %(verb)s: %(exc)s') + + class BuiltinError(ExecutionError): """ **4100** Base class for builtin execution errors (*4100 - 4199*). diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 3299f80..d010cd9 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -22,8 +22,9 @@ Base classes for LDAP plugins. import re import json +import time -from ipalib import crud, errors +from ipalib import api, crud, errors from ipalib import Method, Object from ipalib import Flag, Int, List, Str from ipalib.base import NameSpace @@ -175,6 +176,50 @@ def get_effective_rights(ldap, dn, attrs=None): return rdict +def wait_for_memberof(keys, entry_start, completed, show_command, adding=True): +""" +When adding or removing reverse members we are faking an update to +object A by updating the member attribute in object B. The memberof +plugin makes this work by adding or removing the me
Re: [Freeipa-devel] [PATCH] 635 wait for memberof plugin when doing reverse members
Jan Zelený wrote: Rob Crittenden wrote: Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 rob About that FIXME you got there: I'm not sure if it wouldn't be better to handle the possible exception right in the wait_for_memberof method (I guess it depends on what exception are we expecting and what are we going to do with it?). If you want the exception to reach the calling function, I'd like to see some kind of exception handling in that function - either to let the user know that the error occurred during this waiting or maybe to disregard the exception and continue normal operation. The types of exceptions could run the gambit but I was wondering what would happen if we were looping and some other user deleted the role. The next search for it would fail with NotFound. Granted this isn't a very friendly message to return to someone after adding a member to the group but it does sort of make sense (someone deleted it concurrently). It seemed best to just let this filter up to the caller. Some nitpicking: I'm confused - in the doc string you state that "this will loop for 6+ seconds" and a couple lines below, you have a comment "Don't sleep for more that 6 seconds" - is there a mistake ar are these two statements unrelated? Yeah, I was afraid that might be confusing. I'll wait .3 seconds 20 times so 6 seconds. There are a few LDAP calls which take a bit of time as well, so it will be 6+ seconds if it goes the whole time. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 635 wait for memberof plugin when doing reverse members
Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 rob >From f02cb4f4988e01d651005e182f5c0245121e6fc8 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Tue, 7 Dec 2010 23:22:00 -0500 Subject: [PATCH] Give the memberof plugin time to work when adding/removing reverse members. When we add/remove reverse members it looks like we're operating on group A but we're really operating on group B. This adds/removes the member attribute on group B and the memberof plugin adds the memberof attribute into group A. We need to give the memberof plugin a chance to do its work so loop a few times, reading the entry to see if the number of memberof is more or less what we expect. Bail out if it is taking too long. ticket 560 --- ipalib/plugins/baseldap.py | 59 +-- 1 files changed, 56 insertions(+), 3 deletions(-) diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 6b7153b..99d3d4f 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -22,8 +22,9 @@ Base classes for LDAP plugins. import re import json +import time -from ipalib import crud, errors +from ipalib import api, crud, errors from ipalib import Method, Object from ipalib import Flag, Int, List, Str from ipalib.base import NameSpace @@ -175,6 +176,50 @@ def get_effective_rights(ldap, dn, attrs=None): return rdict +def wait_for_memberof(keys, entry_start, completed, show_command, adding=True): +""" +When adding or removing reverse members we are faking an update to +object A by updating the member attribute in object B. The memberof +plugin makes this work by adding or removing the memberof attribute +to/from object A, it just takes a little bit of time. + +This will loop for 6+ seconds, retrieving object A so we can see +if all the memberof attributes have been updated. +""" +if completed == 0: +# nothing to do +return api.Command[show_command](keys[-1])['result'] + +if 'memberof' in entry_start: +starting_memberof = len(entry_start['memberof']) +else: +starting_memberof = 0 + +# Loop a few times to give the memberof plugin a chance to add the +# entries. Don't sleep for more than 6 seconds. +memberof = 0 +x = 0 +while x < 20: +# sleep first because the first search, even on a quiet system, +# almost always fails to have memberof set. +time.sleep(.3) +x = x + 1 + +# FIXME: put a try/except around here? I think it is probably better +# to just let the exception filter up to the caller. +entry_attrs = api.Command[show_command](keys[-1])['result'] +if 'memberof' in entry_attrs: +memberof = len(entry_attrs['memberof']) + +if adding: +if starting_memberof + completed >= memberof: +break +else: +if starting_memberof + completed <= memberof: +break + +return entry_attrs + class LDAPObject(Object): """ Object representing a LDAP entry. @@ -1326,6 +1371,9 @@ class LDAPAddReverseMember(LDAPModReverseMember): else: attrs_list = self.obj.default_attributes +# Pull the record as it is now so we can know how many members +# there are. +entry_start = self.api.Command[self.show_command](keys[-1])['result'] completed = 0 failed = {'member': {self.reverse_attr: []}} for attr in options.get(self.reverse_attr, []): @@ -1351,7 +1399,8 @@ class LDAPAddReverseMember(LDAPModReverseMember): except errors.PublicError, e: failed['member'][self.reverse_attr].append((attr, unicode(msg))) -entry_attrs = self.api.Command[self.show_command](keys[-1])['result'] +# Wait for the memberof plugin to update the entry +entry_attrs = wait_for_memberof(keys, entry_start, completed, self.show_command, adding=True) for callback in self.POST_CALLBACKS: if hasattr(callback, 'im_self'): @@ -1429,6 +1478,9 @@ class LDAPRemoveReverseMember(LDAPModReverseMember): else: attrs_list = self.obj.default_attributes +# Pull the record as it is now so we can know how many members +# there are. +entry_start = self.api.Command[self.show_command](keys[-1])['result'] completed = 0 failed = {'member': {self.reverse_attr: []}} for attr in options.get(self.reverse_attr,