8000 support setting periodSeconds and failureThreashold in the Keyclock CR by KyriosGN0 · Pull Request #40117 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

support setting periodSeconds and failureThreashold in the Keyclock CR #40117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 13, 2025
Merged

support setting periodSeconds and failureThreashold in the Keyclock CR #40117

merged 9 commits into from
Jun 13, 2025

Conversation

KyriosGN0
Copy link
Contributor
@KyriosGN0 KyriosGN0 commented Jun 1, 2025 •
edited
Loading

Signed-off-by: AvivGuiser avivguiser@gmail.com

fixes #21995

@KyriosGN0 KyriosGN0 requested review from a team as code owners June 1, 2025 15:46
vmuzikar
vmuzikar reviewed Jun 2, 2025
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@KyriosGN0 Thank you for the PR!

It'd be good to also have a mention about this in the advanced Operator guide.

@vmuzikar vmuzikar requested a review from shawkins June 2, 2025 09:35
@KyriosGN0
Copy link
Contributor Author

@KyriosGN0 Thank you for the PR!

It'd be good to also have a mention about this in the advanced Operator guide.

i have added a section about it

shawkins
shawkins reviewed Jun 2, 2025
View reviewed changes
Copy link
Contributor
@shawkins shawkins 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 @KyriosGN0 overall everything looks good. Just a couple of comments.

@KyriosGN0 KyriosGN0 requested review from shawkins and vmuzikar June 2, 2025 17:35
shawkins
shawkins previously approved these changes Jun 2, 2025
View reviewed changes
Copy link
Contributor
@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM - thank you @KyriosGN0

@KyriosGN0
Copy link
Contributor Author

@vmuzikar @shawkins anything i can do about the CI failing?

@shawkins
Copy link
Contributor
shawkins commented Jun 3, 2025

@KyriosGN0 If this is happening reproducibly, then it will take some further investigation to trap reason why it's failing. There's nothing in the logs that help determine why the instance isn't becoming ready - we do try to obtain the current or previous pod log from what is failing, but nothing is showing up.

@KyriosGN0
Copy link
Contributor Author

locally this specific test passes

@shawkins
Copy link
Contributor
shawkins commented Jun 4, 2025

locally this specific test passes

I reran the tests and it's passing now as expected. We'll need to address the flakiness of that test in a separate issue, but I don't recall it being something that commonly fails.

@KyriosGN0 KyriosGN0 requested a review from shawkins June 4, 2025 18:09
@KyriosGN0
Copy link
Contributor Author

@shawkins @vmuzikar i don't mean to be nagging, i fixed the conflict i had and then i noticed that the test i wrote ws confusing (i always built readiness probe)

keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jun 6, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

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
Copy link

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.cluster.RealmInvalidationClusterTest#crudWithFailover

Keycloak CI - Store IT (mariadb)

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

@KyriosGN0
Copy link
Contributor Author

@shawkins @vmuzikar can i get a retest? Thanks in advance

@shawkins
Copy link
Contributor
shawkins commented Jun 9, 2025

@shawkins @vmuzikar can i get a retest? Thanks in advance

Hopefully another pr will stablize the olm tests soon, we'll then pick that up here and let it run again.

keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jun 10, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

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
Copy link

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.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTest

Keycloak CI - Base IT

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest.assertForbiddenLogin(SAMLServletAdapterTest.java:518)
...

Report flaky test

shawkins
shawkins previously approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor
@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @KyriosGN0

@vmuzikar do you want to give it a look?

@shawkins shawkins self-assigned this Jun 11, 2025
vmuzikar
vmuzikar approved these changes Jun 12, 2025
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

vmuzikar
vmuzikar requested changes Jun 12, 2025
View reviewed changes
docs/guides/operator/advanced-configuration.adoc Outdated
periodSeconds: 20
failureThreshold: 5
----

===== Probe Timeouts
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we might want to remove the next section "Probe Timeouts" for using pod templated. It's superseded by this new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated docs, thanks!

@KyriosGN0 KyriosGN0 requested review from vmuzikar and shawkins June 12, 2025 13:59
vmuzikar
vmuzikar approved these changes Jun 12, 2025
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

@KyriosGN0 Thanks.

@vmuzikar vmuzikar enabled auto-merge (squash) June 12, 2025 18:37
KyriosGN0 and others added 9 commits June 13, 2025 07:21
@KyriosGN0 @shawkins
add probe spec
9255c92 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
make default for probes if not configured, add skeleton test files
a3ad278 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
fix tests
73d2238 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
fix tests
4a4d605 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
add docs
902b8a1 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
move test to unittest and apiserver test
6339caa 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
adding asserts to check new fields
e9b4b49 
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0 @shawkins
fix test
ef14b5d 
Signed-off-by: AvivGuiser <aviv.guiser@placer.ai>
@KyriosGN0 @shawkins
update docs
75e8f9e 
Signed-off-by: AvivGuiser <aviv.guiser@placer.ai>
@vmuzikar vmuzikar merged commit 7736ca2 into keycloak:main Jun 13, 2025
76 checks passed
@KyriosGN0 KyriosGN0 deleted the probes branch June 27, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@vmuzikar vmuzikar vmuzikar approved these changes

@shawkins shawkins Awaiting requested review from shawkins

Assignees

@shawkins shawkins

Labels
flaky-test team/cloud-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configurable probes in the Operator
3 participants
@KyriosGN0 @shawkins @vmuzikar
0