8000 Fix FipsDistTest failures by mabartos · Pull Request #23871 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Fix FipsDistTest failures - #23871

Closed
mabartos wants to merge 1 commit into
keycloak:mainfrom
mabartos:fix_fips
Closed

Fix FipsDistTest failures#23871
mabartos wants to merge 1 commit into
keycloak:mainfrom
mabartos:fix_fips

Conversation

@mabartos
@mabartos mabartos commented Oct 10, 2023
Copy link
Copy Markdown
Member

Fixes #23786

@ghost ghost added the flaky-test label Oct 10, 2023
This was referenced Oct 10, 2023
Closed
Closed
@ghost
ghost commented Oct 10, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below 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.ui.account2.LDAPAccountTest#createdNotVisibleTest

Keycloak CI - Account Console IT (firefox)

org.openqa.selenium.NoSuchElementException: 
Unable to locate element: #username
For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az574-436', ip: '10.1.0.220', os.name: 'Linux', os.arch: 'amd64', os.version: '6.2.0-1012-azure', java.version: '17.0.8.1'
...

Report flaky test

ghost
ghost reviewed Oct 10, 2023
View reviewed changes
@ghost ghost 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

< 8000 div class="js-timeline-item js-timeline-progressive-focus-container" data-gid="IC_kwDOAKnDVc5ov3Xp">
@mabartos
mabartos commented Oct 11, 2023
Copy link
Copy Markdown
Member Author

I was able to reproduce the issue for the FipsDistTest on my local machine. From the test execution log, the test fails due to the IllegalThreadStateException exception, which is thrown during obtaining exitValue from the executed process (basically executing commands on dist), which hasn't stopped yet. It means there's some problem with destroying the process. The process is not killed even after a specified 10s timeout for waiting for the process execution.

It only happens when the execution of the commands on the built distribution fails with an error and the keepAlive is set to true, which breaks the shutdown flow. It means that the distribution should be stopped manually, but the ShutdownContext is already executed for other components. It means that the CacheManager is destroyed and the ArC CDI as well, which is visible in the error logs.

In these cases, the distribution should be properly stopped as well, even when keepAlive=true.

It's related only to the testing environment, and the distribution or FIPS are NOT affected. Everything works as expected when manually executed on the distribution or when keepAlive=false.

@miquelsi @vmuzikar Could you please check it? Thanks

@mabartos
mabartos marked this pull request as ready for review October 11, 2023 10:39
@mabartos
mabartos requested review from a team as code owners October 11, 2023 10:39
@ghost ghost added the team/cloud-native label Oct 11, 2023
@mabartos
mabartos requested review from miquelsi and vmuzikar October 11, 2023 10:39
@mabartos mabartos self-assigned this Oct 11, 2023
vmuzikar
vmuzikar requested changes Oct 12, 2023
View reviewed changes
@vmuzikar vmuzikar 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.

As discussed offline, we should double check that it's not a server issue. It feels weird KC gets into a half-dead state when it can't be properly exitted.

@mabartos mabartos added the status/hold PR should not be merged. On hold for later. label Oct 13, 2023
@mabartos
mabartos commented Oct 18, 2023
Copy link
Copy Markdown
Member Author

@vmuzikar As we've discussed, the problem might be somewhere else; I can confirm it is. Thanks for pointing it out.

I was able to reproduce the failures for the latest upstream. Not reproducible for 22.x.

It seems the problem's cause is hidden behind the RESTEasy Reactive. I've tried to exclude the PR #15728 in the upstream and the failures disappeared as well.

I'll close this PR and provide a better one after further analysis.

@mabartos mabartos closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar requested changes
@miquelsi miquelsi Awaiting requested review from miquelsi
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

@mabartos mabartos

Labels

flaky-test status/hold PR should not be merged. On hold for later. team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failure: FipsDistTest

2 participants

@mabartos @vmuzikar
0