Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 6436 - MOD on a large group slow if substring index is present #6437

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jchapma
Copy link
Contributor

@jchapma jchapma commented Dec 4, 2024

Bug Description: If the substring index is configured for the group membership attribute ( member or uniqueMember ), the removal of a member from a large static group is pretty slow.

Fix Description: A solution to this issue would be to introduce a new index to track a membership atttribute index. In the interm, we add a check to healthcheck to inform the user of the implications of this configuration.

Fixes: #6436

Reviewed by:

Bug Description: If the substring index is configured for the group
membership attribute ( member or uniqueMember ), the removal of a
member from a large static group is pretty slow.

Fix Description: A solution to this issue would be to introduce
a new index to track a membership atttribute index. In the interm,
we add a check to healthcheck to inform the user of the implications
of this configuration.

Fixes: 389ds#6436

Reviewed by:
Copy link
Contributor

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

standalone = topology_st.standalone

log.info('Enable MO plugin')
plugin = MemberOfPlugin(standalone)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required to enable memberof ?
IMHO healthcheck should only warn when a membership attribute is indexed in substring, independently of memberof being enabled/disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this on Tuesday and decided that is was easier to check for this configuration from the memberof plugin, similar to the check for the equality index type.

report['fix'] = report['fix'].replace('YOUR_INSTANCE', self._instance.serverid)
report['items'].append(suffix)
report['items'].append(attr)
report['check'] = f'memberof:substring_index'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 'memberof' ? I would expect the admin to check the need of 'attr' substring_index

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will change this

report['fix'] = report['fix'].replace('YOUR_INSTANCE', self._instance.serverid)
report['items'].append(suffix)
report['items'].append(attr)
report['check'] = f'memberof:substring_index'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same

backends = Backends(self._instance).list()
attrs = self.get_attr_vals_utf8_l("memberofgroupattr")
container = self.get_attr_val_utf8_l("nsslapd-plugincontainerscope")
#import pdb; pdb.set_trace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unneeded leftover

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, my bad

Comment on lines 868 to 869
except:
# No index at all, bad
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a 'No index at all' error, I think, we should be more specific with the except: and specify the exact exception expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from other memberof attr checks, but you're right. This test is to check if a membership attribute contains a substring index, if the index doesn't exist then we have nothing to report.

Comment on lines 867 to 868
except:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, what I mean is that we should not handle bare exceptions like that (and in Python, in general).

I think it should follow this logic (it's just an example, the actual exceptions should be something else):

try:
    result = check()
except ValueError:
    handle_value_error()
except TypeError:
    handle_type_error()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MOD on a large static group takes too long if substring index is present.
4 participants