[MM-59066] server/job: prevent deletion of non-finished jobs #30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpoile
approved these changes
Jun 25, 2024
server/job_test.go
Outdated
Comment on lines
42
to
50
| t.Run("Empty dump location", func(t *testing.T) { | ||
| job := &DumpJob{ | ||
| DumpLocation: "", | ||
| } | ||
|
|
||
| success, err := job.DeleteDump(plugin) | ||
| require.NoError(t, err) | ||
| require.False(t, success) | ||
| }) |
There was a problem hiding this comment.
Would it be a good idea to check if the fs was asked to delete anything?
There was a problem hiding this comment.
Added an assertion for that, created a temp file and then checked if it was deleted or not.
agnivade
approved these changes
Jun 26, 2024
hanzei
reviewed
Jul 2, 2024
DHaussermann
approved these changes
Jul 4, 2024
There was a problem hiding this comment.
Tested and passed
Confirmed I could see the bug: Both the data and dump folder located here server/data/plugin-data/mattermost-plugin-metrics were being deleted
- Tested to ensure the
datafolder is no longer getting deleted - The
dumpfolder does get deleted as expected sinceRemove Allhas been clicked - The
dumpfolder does get recreated once the next queued job completes - Regression tested other plugin functionality around the path to the data folder as well as the download and remove options for existing dumps.
No issues found.
LGTM!
Huge thanks to @isacikgoz for teaching me about this plugin and helping me with adding a delay to make repro conditions much easier. 🎉
|
Heads up @isacikgoz I don't have permission to modify labels in this repo. I cannot remove the QA Review label. |
hanzei
added
3: Reviews Complete
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
An issue has been pointed out that if a “Remove All” request is being made during a job is running will led to a case that plugin to delete entire data directory. So, the logic was deleting the directory of the dump. But the problem is, if a job receives a request to be deleted, and it doesn’t have a dump directory (which would be set after the job finishes), it will have an empty value. And if the empty value is given to filestore, the entire data directory will be deleted.
Ticket Link
https://mattermost.atlassian.net/browse/MM-59066