8000 Allow migration of non-persistent sessions to persistent sessions by ahus1 · Pull Request #29377 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Allow migration of non-persistent sessions to persistent sessions - #29377

Merged
ahus1 merged 5 commits into
keycloak:mainfrom
ahus1:is-29375-migrate-to-persistent-sessions
May 22, 2024
Merged

Allow migration of non-persistent sessions to persistent sessions#29377
ahus1 merged 5 commits into
keycloak:mainfrom
ahus1:is-29375-migrate-to-persistent-sessions

Conversation

@ahus1
@ahus1 ahus1 commented May 8, 2024 •
edited
Loading
Copy link
Copy Markdown
Member

Closes #29375

@ahus1 ahus1 self-assigned this May 8, 2024
This was referenced May 8, 2024
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ahus1
ahus1 commented May 8, 2024
Copy link
Copy Markdown
Member Author

@mhajas - this is still missing a test. I'd be happy for your feedback if this goes into the right direction. Thanks!

@ahus1
ahus1 force-pushed the is-29375-migrate-to-persistent-sessions branch 2 times, most recently from fc33145 to 32fb237 Compare May 13, 2024 11:56
@ahus1
ahus1 force-pushed the is-29375-migrate-to-persistent-sessions branch 2 times, most recently from 54cfca5 to e6929c5 Compare May 13, 2024 14:42
@keycloak-github-bot keycloak-github-bot Bot mentioned this pull request May 13, 2024
Closed
@ahus1
ahus1 marked this pull request as ready for review May 14, 2024 08:43
@ahus1
ahus1 requested a review from a team as a code owner May 14, 2024 08:43
@ahus1
ahus1 marked this pull request as draft May 14, 2024 08:50
@ahus1 ahus1 mentioned this pull request May 14, 2024
Merged
6 tasks done
@ahus1
Allow migration of non-persistent sessions to persistent sessions dur…
87eb74d 
…ing upgrades

Closes keycloak#29375

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 force-pushed the is-29375-migrate-to-persistent-sessions branch from e6929c5 to 87eb74d Compare May 15, 2024 14:49
pruivo
pruivo reviewed May 15, 2024
View reviewed changes
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
Comment thread ...pan/src/main/java/org/keycloak/models/sessions/infinispan/PersistentUserSessionProvider.java Resolved
@ahus1
Updating the docs after the review
2b5852e 
Closes keycloak#29375

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 force-pushed the is-29375-migrate-to-persistent-sessions branch from f24c6ee to 17cbce9 Compare May 16, 2024 16:48
@ahus1
Adding a test
e105dba 
Closes keycloak#29375

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 force-pushed the is-29375-migrate-to-persistent-sessions branch from 17cbce9 to e105dba Compare May 16, 2024 17:50
@thomasdarimont
thomasdarimont commented May 16, 2024
Copy link
Copy Markdown
Contributor

This will be quite useful to have!
For environments with many sessions (1-10 million) the migration might take a while. In this case it would also be helpful to let users know how many sessions will be migrated, in order to estimate the overall duration of the migration.
How about adding a log message when the migration starts, with the information how many sessions are going to be migrated?

@ahus1
ahus1 marked this pull request as ready for review May 17, 2024 06:19
@ahus1
ahus1 requested review from a team as code owners May 17, 2024 06:19
@ahus1
ahus1 commented May 17, 2024 •
edited
Loading
Copy link
Copy Markdown
Member Author

In this case it would also be helpful to let users know how many sessions will be migrated, in order to estimate the overall duration of the migration.

@thomasdarimont - thank you for the suggestion. I think this would be a good follow-up PR. I will need some advice on how to use the estimate counting to avoid an expensive operation.

I added it as an optional task to #28265

@ahus1
ahus1 requested review from mhajas and pruivo May 17, 2024 06:25
@ahus1
ahus1 commented May 17, 2024
Copy link
Copy Markdown
Member Author

@mhajas / @pruivo - I'd think this is now ready for a final review, a test exists and it is green.

I know that we have #29592 pending, but maybe we can squash-merge this one first and then tackle the last one.

ryanemerson
ryanemerson reviewed May 21, 2024
View reviewed changes
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
Comment thread docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc Resolved
@ahus1
Resolving review comments in the docs
01e8061 
Closes keycloak#29375

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 requested a review from ryanemerson May 21, 2024 12:31
@ahus1
ahus1 commented May 21, 2024
Copy link
Copy Markdown
Member Author

@ryanemerson - I've updated the docs, please re-review. Thanks!

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed May 21, 2024
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot
keycloak-github-bot Bot commented May 21, 2024
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testMigrateSession

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected:<3> but was:<0>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

@ahus1
Nitpick resolved
d70312d 
Closes keycloak#29375

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 requested a review from ryanemerson May 21, 2024 15:28
@ahus1
ahus1 enabled auto-merge (squash) May 21, 2024 15:43
@keycloak-github-bot
keycloak-github-bot Bot commented May 21, 2024
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testMigrateSession

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected:<3> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.model.session.SessionTimeoutsTest#testOfflineUserClientIdleTimeoutSmallerThanSessionNoRefresh

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected null, but was:<org.keycloak.models.sessions.infinispan.AuthenticatedClientSessionAdapter@44bbe1d1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotNull(Assert.java:756)
	at org.junit.Assert.assertNull(Assert.java:738)
	at org.junit.Assert.assertNull(Assert.java:748)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed May 21, 2024
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ahus1
ahus1 commented May 22, 2024
Copy link
Copy Markdown
Member Author

@dbubenheim - we're currently working on persistent sessions, see discussion #28271 over there. Given our previous discussion, I assume you might be interested in that feature as it IMHO has the potential to simplify your current setup.

I'd hope we get this change into main before KC25 is released. KC25 is currently scheduled for end of May, and I wonder if you would be interested and have the time to test this migration with your setup before or after our release.

mhajas
mhajas approved these changes May 22, 2024
View reviewed changes
@mhajas mhajas left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @ahus1! This is a great addition for people that use some sort of persistence in their current setup

@ahus1
ahus1 merged commit 80de3a0 into keycloak:main May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanemerson ryanemerson ryanemerson approved these changes
@pruivo pruivo pruivo approved these changes
+2 more reviewers
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
@mhajas mhajas mhajas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ahus1 ahus1

Labels

flaky-test team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow migration of non-persistent sessions to persistent sessions

5 participants

@ahus1 @thomasdarimont @ryanemerson @pruivo @mhajas
0