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): handle deletion async #1095

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Oct 5, 2022

Fixes #1093

  • ignore cleanup failures, always respond 200
  • do not include cleanup failures in response body
  • handle deletion cleanup async
  • use RecordingTargetHelper to clean up

This change makes Automated Rule deletion happen asynchronously, similar to how Automated Rule activation is also asynchronous.

Prior to this change, creating an enabled Rule definition was a fast request that simply defined the Rule and returned. Any
recordings created by that rule were processed asynchronously and separately from the original request. Deleting a rule, on
the other hand, was synchronous - all matching targets would be tested again for rule applicability, and recordings would be
stopped on any matching targets. When all of those tasks were completed the deletion response would finally be sent back to the
requesting client, with a 500 status code indicating that at least one of those tasks failed and a 200 if everything succeeded.

With this change the deletion acts the same way as creation/activation. The rule definition deletion is the only thing blocked on
before the API response is sent to the client. If clean up is required then this is handled asynchronously in the background.

(Worth noting: Rule creation also used to be a synchronous request but was made async some time ago, and deletion was never updated the same way)

@andrewazores
Copy link
Member Author

andrewazores commented Oct 6, 2022

CI keeps failing here, I think due to #1083 mostly. #1096 should clean it up and make the tests more reliable.

The AutoRulesIT is the first test to fail in these bad runs and because it fails it doesn't properly clean up after itself. I think the extra targets get left running, and it also leaves credentials and rules definitions behind, so most tests that run afterward also fail because there are more targets than expected or the automated rule has triggered more recordings than expected, etc.

Copy link
Member

@tthvo tthvo 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 :D

@andrewazores andrewazores merged commit 97af7cf into cryostatio:main Oct 6, 2022
@andrewazores andrewazores deleted the rule-delete-async branch October 6, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Automated Rule deletion fails if some matched targets cannot be connected
2 participants