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

kcm: add GET_CRED_LIST for faster iteration #5546

Closed
wants to merge 3 commits into from
Closed

Conversation

pbrezina
Copy link
Member

For large caches, one IPC operation per credential dominates the cost of
iteration. Instead transfer the whole list of credentials to the client in
one IPC operation.

Resolves: #5545

This is a continuation of #5375. The first
pull requests addressed bottlenecks in sssd-kcm and reduced the test case
run time from 30 minutes to 2 minutes, this new operation takes it down to
9 seconds.

This file was copied from MIT Kerberos code, but we do not really
need it.
@pbrezina
Copy link
Member Author

Note that you need to build krb5 from upstream to test it: krb5/krb5#1165 (already merged)

@sumit-bose
Copy link
Contributor

Hi,

the patches work well for me.

The feature is already covered by the current KCM integration tests, i.e. if the krb5 client library supports the extension it will be used in the tests. However there is no indication in the tests if the GET_CRED_LIST or GET_CRED_UUID_LIST is used. I wonder if it would be good to have such indication to avoid regressions on platform where we know that the extension is supported?

About KCM_MIT_OFFSET, I think it would be worth to add a comment explaining where this is coming from, e.g. that it is the opcode of the first call of the MIT extension.

bye,
Sumit

@pbrezina
Copy link
Member Author

I opened additional PR against Kerberos since the fallback currently does not work: krb5/krb5#1177

For large caches, one IPC operation per credential dominates the cost
of iteration. Instead transfer the whole list of credentials to the
client in one IPC operation.

Resolves: SSSD#5545
@pbrezina
Copy link
Member Author

The feature is already covered by the current KCM integration tests, i.e. if the krb5 client library supports the extension it will be used in the tests. However there is no indication in the tests if the GET_CRED_LIST or GET_CRED_UUID_LIST is used. I wonder if it would be good to have such indication to avoid regressions on platform where we know that the extension is supported?

We can grep the logs and produce a debug message saying which operation was executed but this does not fail the test. I think the right thing to do is to let QA write a performance test that would be able to catch it? They just did manual verification for rhbz#1876514

About KCM_MIT_OFFSET, I think it would be worth to add a comment explaining where this is coming from, e.g. that it is the opcode of the first call of the MIT extension.

Done.

@sumit-bose
Copy link
Contributor

The feature is already covered by the current KCM integration tests, i.e. if the krb5 client library supports the extension it will be used in the tests. However there is no indication in the tests if the GET_CRED_LIST or GET_CRED_UUID_LIST is used. I wonder if it would be good to have such indication to avoid regressions on platform where we know that the extension is supported?

We can grep the logs and produce a debug message saying which operation was executed but this does not fail the test. I think the right thing to do is to let QA write a performance test that would be able to catch it? They just did manual verification for rhbz#1876514

ok

About KCM_MIT_OFFSET, I think it would be worth to add a comment explaining where this is coming from, e.g. that it is the opcode of the first call of the MIT extension.

Done.

Thanks for the update. The CI failure on rawhide does not look related, so ACK.

bye,
Sumit

@pbrezina pbrezina added the Ready to push Ready to push label Apr 6, 2021
@pbrezina
Copy link
Member Author

pbrezina commented Apr 6, 2021

Pushed PR: #5546

  • master
    • 560e247 - kcm: add GET_CRED_LIST for faster iteration
    • 81130b2 - kcm: add support for MIT extensions
    • 9a39ceb - kcm: remove unneeded kcm.h

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

Successfully merging this pull request may close these issues.

kcm: implement GET_CRED_LIST for faster iteration
4 participants