Skip to content

Conversation

liulinC
Copy link
Collaborator

@liulinC liulinC commented Jul 31, 2025

commit 92377bfe2fffd762ab922a015a93afa314ae000f (HEAD -> private/linl/CA-414418, origin/private/linl/CA-414418)
Author: Lin Liu <[email protected]>
Date:   Thu Jul 31 07:25:30 2025 +0000

    CA-414418: Perf: save user validate result and apply to sessions
    
    For all sessions created by external/AD users, session revalidate
    will check whether the users are still acitve, and kick off the
    session accordingly.
    
    However, xapi check the user for every session. The problem here
    is lots of session are created by only a few users. (for the case
    of CVAD and ControlUP). This would cause lot of duplicated check
    for the same user again and again, which is slow and waste lots
    of resources.
    
    To fix the issue, [(user_sid, check_result)] is defined for every
    round of check. The check result is saved so later check for the
    session with same user can just be applied.
    
    Signed-off-by: Lin Liu <[email protected]>

commit b0e2b5bfd2791dfa38ee9e4c9cd9480f7a01016e
Author: Lin Liu <[email protected]>
Date:   Thu Jul 31 06:17:32 2025 +0000

    CA-414418: Detection of AD account removal does not cause logout
    
    For performance, during revalidate existing sessions, xapi query
    subject details from xapi db first, if the subject is suspend,
    then goes to AD, to make sure unblocked user can login.
    
    There is a backend thread to update xapi db subject information
    from AD. However, it can not handle the case that the subject
    is removed. (and should not remove the subject for user until
    user remove it explictly). Thus, the subject information is not
    updated and keep alive.
    
    In this case, subject revalidate always got session not suspend
    from xapi db.
    
    The issue is fixed by query subject information from AD direclty,
    and session revalidate thread handle the removed subject properly
    to kick off the sessions
    
    For performance, there is a follow up commit to resovle that
    
    Signed-off-by: Lin Liu <[email protected]>
    ```

liulinC added 2 commits July 31, 2025 08:02
For performance, during revalidate existing sessions, xapi query
subject details from xapi db first, if the subject is suspend,
then goes to AD, to make sure unblocked user can login.

There is a backend thread to update xapi db subject information
from AD. However, it can not handle the case that the subject
is removed. (and should not remove the subject for user until
user remove it explictly). Thus, the subject information is not
updated and keep alive.

In this case, subject revalidate always got session not suspend
from xapi db.

The issue is fixed by query subject information from AD direclty,
and session revalidate thread handle the removed subject properly
to kick off the sessions

For performance, there is a follow up commit to resovle that

Signed-off-by: Lin Liu <[email protected]>
For all sessions created by external/AD users, session revalidate
will check whether the users are still acitve, and kick off the
session accordingly.

However, xapi check the user for every session. The problem here
is lots of session are created by only a few users. (for the case
of CVAD and ControlUP). This would cause lot of duplicated check
for the same user again and again, which is slow and waste lots
of resources.

To fix the issue, [(user_sid, check_result)] is defined for every
round of check. The check result is saved so later check for the
session with same user can just be applied.

Signed-off-by: Lin Liu <[email protected]>
|> fst
else
suspended
is_subject_suspended ~__context ~cache:false authenticated_user_sid
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this solves the original bug?
AFAICT the initial query with 'cache:true' found that the account is missing (suspended), and the 2nd query with 'cache:false' said that it is not suspended (so I was guessing that winbind might also be caching something), but I may have misinterpreted the logs.

Copy link
Collaborator Author

@liulinC liulinC Jul 31, 2025

Choose a reason for hiding this comment

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

No, the initial query with cache:true will always succeed and found the account not suspended.
(As xapi db update thread never update it due to NOT_FOUND exception)
The update db thread can not handle it, refer to my comments in the CA ticket.
And I verified it can resolve the issue.

@minglumlu
Copy link
Member

I think the perf improvement is great!

@liulinC liulinC force-pushed the private/linl/CA-414418 branch from ae3957d to faf3179 Compare August 1, 2025 12:55
- Rename session_timeout -> session_timed_out
- Assoc List -> Map to store the check result
- Apply check direclty instead of remembering check status

Signed-off-by: Lin Liu <[email protected]>
@liulinC liulinC force-pushed the private/linl/CA-414418 branch from faf3179 to 2778096 Compare August 1, 2025 13:26
@lindig
Copy link
Contributor

lindig commented Aug 1, 2025

Looking from the outside it seems like we are adding a cache for authentication results. But would it not be better to add this cache on a lower level to minimise changes for client code?

@liulinC liulinC added this pull request to the merge queue Aug 2, 2025
@liulinC
Copy link
Collaborator Author

liulinC commented Aug 2, 2025

Looking from the outside it seems like we are adding a cache for authentication results. But would it not be better to add this cache on a lower level to minimise changes for client code?

This is not exposed client API. so, no client code update is required.

Merged via the queue into xapi-project:master with commit 5313809 Aug 2, 2025
15 checks passed
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.

4 participants