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

sssctl: Management of indexes on cache DBs. #6368

Closed
wants to merge 1 commit into from

Conversation

aplopez
Copy link
Contributor

@aplopez aplopez commented Sep 28, 2022

A new command was added to sssctl in order to manage indexes on the cache DBs.

sssctl is now able to create, list and delete indexes on the local caches. Indexes are useful for the new D-Bus ListByAttr() function.

sssctl cache-index create -a attr [-d domain]
sssctl cache-index delete -a attr [-d domain]
sssctl cache-index list [-a attr] [-d domain]

Task: https://issues.redhat.com/browse/SSSD-4981

:feature: sssctl is now able to create, list and delete indexes on the local caches. Indexes are useful for the new D-Bus ListByAttr() function.

:relnote: The new D-Bus function ListByAttr() allows the caller to look for users that have an attribute with a certain value. For performance reasons, it is recommended that the attribute is indexed both on the remote server and on the local cache. The sssctl tool now provides the cache-index command to help you manage indexes on the local cache.

@aplopez aplopez added the no-backport This should go to target branch only. label Sep 28, 2022
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Overall the PR seems good but there are several things to improve.

By the way, due to recent changes in POPT library you may need to free sss_tool_popt_ex()->action_str manually. Please check PR6350 for more information.

On top of that, I'd like to ask you, do I need to do anything special to test theses changes? I see you mention that the attribute needs to be indexed but I don't know if that's manually or automatically handled.

src/db/sysdb_ops.c Show resolved Hide resolved
src/db/sysdb_ops.c Outdated Show resolved Hide resolved
src/tools/sssctl/sssctl_data.c Show resolved Hide resolved
src/tools/sssctl/sssctl_data.c Outdated Show resolved Hide resolved
src/tools/sssctl/sssctl_data.c Outdated Show resolved Hide resolved
src/tools/sssctl/sssctl_data.c Show resolved Hide resolved
src/tools/sssctl/sssctl_data.c Outdated Show resolved Hide resolved
@aplopez
Copy link
Contributor Author

aplopez commented Sep 30, 2022

On top of that, I'd like to ask you, do I need to do anything special to test theses changes? I see you mention that the attribute needs to be indexed but I don't know if that's manually or automatically handled.

Nothing special to do. You just run the command and you can verify the @INDEXLIST entry to check the index was created. The reminder about creating the remote index is displayed because there is not much we can do here. This command affects only the local caches. Up to the user to create the indexes on the remote server. And it is for performance sake, not actually needed for the feature to work.

ldbsearch --url /var/lib/sss/db/cache_ldap.ldb -s base -b '@INDEXLIST'

@aplopez
Copy link
Contributor Author

aplopez commented Sep 30, 2022

By the way, due to recent changes in POPT library you may need to free sss_tool_popt_ex()->action_str manually. Please check PR6350 for more information.

Your PR is not pulled yet and I cannot free action_str as, without your changes, the string is not being duplicated and I'm freeing the command line, which generates a segmentation fault.

I'm not sure how to proceed here.

@aplopez
Copy link
Contributor Author

aplopez commented Sep 30, 2022

I'm not sure how to proceed here.

I missed I was requested to re-review your PR. I'll do it as soon as possible and when it is pulled, I'll be able to make those changes here.

@ikerexxe
Copy link
Contributor

I missed I was requested to re-review your PR. I'll do it as soon as possible and when it is pulled, I'll be able to make those changes here.

Perfect!

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Let's see once Iker's comments are resolved.

T.

src/tools/sssctl/sssctl_data.c Outdated Show resolved Hide resolved
src/db/sysdb_ops.c Show resolved Hide resolved
@aplopez
Copy link
Contributor Author

aplopez commented Sep 30, 2022

I added all the pending changes except the free(action_str) which would currently generate a segmentation fault and the english sentence that needs checking. @ikerexxe , @thalman Please feel free to review and comment, but don't approve until that last change is added.

@ikerexxe
Copy link
Contributor

ikerexxe commented Oct 3, 2022

While testing the changes I've found that sssctl crashes. @aplopez I've sent you an email with the steps to reproduce the error and some additional information.

@aplopez
Copy link
Contributor Author

aplopez commented Oct 3, 2022

While testing the changes I've found that sssctl crashes. @aplopez I've sent you an email with the steps to reproduce the error and some additional information.

Geez. It was the free(action_str) which, after explaining why it can't still be present, I forgot to remove.

Fixed now.

@ikerexxe
Copy link
Contributor

ikerexxe commented Oct 3, 2022

Fixed now.

Thanks! I've been able to test it and it seems good. I'm not giving my approval yet as the PR isn't in the final version, but so far so good.

@alexey-tikhonov
Copy link
Member

@aplopez, #6350 was merged.

A new command was added to sssctl in order to manage indexes on the
cache DBs.

sssctl cache-index create -a attr [-d domain]
sssctl cache-index delete -a attr [-d domain]
sssctl cache-index list [-a attr] [-d domain]

:feature: sssctl is now able to create, list and delete indexes on
          the local caches. Indexes are useful for the new D-Bus
          ListByAttr() function.

:relnote: The new D-Bus function ListByAttr() allows the caller to
          look for users that have an attribute with a certain value.
          For performance reasons, it is recommended that the
          attribute is indexed both on the remote server and on the
          local cache. The sssctl tool now provides the cache-index
          command to help you manage indexes on the local cache.
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

The code looks fine and I've tested the command without any problems so ack from my side.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Thank you for the patch, ACK

@pbrezina
Copy link
Member

pbrezina commented Oct 5, 2022

Pushed PR: #6368

  • master
    • 3e5251b - sssctl: Management of indexes on cache DBs.

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Oct 5, 2022
@pbrezina pbrezina closed this Oct 5, 2022
@aplopez aplopez deleted the sssctl branch November 25, 2022 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugzilla no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants