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

fix(rules): do not create cached connections for automated rules #763

Merged
merged 11 commits into from
Dec 7, 2021

Conversation

andrewazores
Copy link
Member

fix(rules): signal connections opened by rules may be immediately closed

Related to #756

fix(pom): remove itest connection cache size config

Remove connection cache size/TTL config, which used an incorrect/outdated
cache max size env var name, to better reflect a standard Cryostat
deployment using default cache settings (unlimited size, 10s TTL)

Fixes #762

fix(rules): do not create cached connections for automated rules

Related to #756

Automated rules processing will reuse existing cached connections if
available. If none are available then a new connection will be created and
NOT cached, in order to avoid automated rules causing evictions of cached
connections for other interactive clients

@andrewazores andrewazores marked this pull request as ready for review December 1, 2021 19:39
@andrewazores andrewazores requested review from jan-law and hareetd and removed request for jan-law December 1, 2021 19:39
@andrewazores andrewazores force-pushed the close-rules-connections branch 2 times, most recently from c40e650 to 9fea079 Compare December 1, 2021 22:35
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

Looks good, I just have a few comments/questions. Tested it out and is working as expected.

@andrewazores
Copy link
Member Author

@andrewazores
Copy link
Member Author

Hmm, that test keeps failing. I'll take a closer look at it. For now I'll mark this as a draft again while I investigate.

Remove connection cache size/TTL config, which used an incorrect/outdated cache max size env var name, to better reflect a standard Cryostat deployment using default cache settings (unlimited size, 10s TTL)

Fixes cryostatio#762
Related to cryostatio#756

Automated rules processing will reuse existing cached connections if available. If none are available then a new connection will be created and NOT cached, in order to avoid automated rules causing evictions of cached connections for other interactive clients
@andrewazores andrewazores force-pushed the close-rules-connections branch from a5d8ca1 to 24aa160 Compare December 6, 2021 21:10
@andrewazores andrewazores marked this pull request as ready for review December 6, 2021 22:26
@andrewazores
Copy link
Member Author

I think I've sorted out the failing test now.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

Besides my last comment, the code makes sense and works well when tested.

pom.xml Show resolved Hide resolved
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

The first run of integration tests failed, specifically ArchivedReportJwtDownloadIT:

image

After looking at the logs, seems like another case of #666, so likely independent of your PR changes.

The second run succeeded and the Rules API is working as expected.

@andrewazores andrewazores merged commit 7c010b2 into cryostatio:main Dec 7, 2021
@andrewazores andrewazores deleted the close-rules-connections branch December 7, 2021 19:35
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.

pom.xml uses incorrect/outdated connection cache size env var name
3 participants