-
Notifications
You must be signed in to change notification settings - Fork 7
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 early rotation for roles with WALs, handle multiple WALs per role #28
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
dac711c
Guard against premature rotation
ea46314
Add more logging
59eafe9
Fix logged version number
fb8d6db
Stop current rotation loop if we hit a premature rotation condition
61fa8a7
Add more WAL logging
2cf0db2
More WAL logging
64e5a98
gofmt rotation.go
calvn 5ec68fb
Fix early rotation for roles with WALs, handle multiple WALs per role
tomhjp e3eb497
Review changes:
tomhjp 30ce185
Switch from warning to error to correct HTTP status code from 400 -> 500
tomhjp 2eff49f
* Delete WALs on failed role creation or role deletion
tomhjp f89ce21
Fix a bug in role deletion WAL cleanup, add some WAL tests
tomhjp 3cedec4
Delete test debug lines
tomhjp 7c7d98d
Fix manual rotate not respecting WAL ID, more unit testing
tomhjp ad73e4d
Add tests for processing of stored WALs, remove bad assertion
tomhjp 76f9f03
Remove old password from WAL
tomhjp 8dcd840
Remove unnecessary multierror
tomhjp 4eac5a1
Distinguish between log lines
tomhjp cfbda98
Add missing debug logs for cleaning up WALs, tidy build.sh
tomhjp fa193fa
Add error to debug messages
tomhjp 78553e3
Add last check on newPassword
tomhjp 70e4c7b
Minor test improvements ported from DB PR
tomhjp b32b685
Log line ordering
tomhjp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want to do the WAL delete before the role deletion, that way we have an avenue to retry the operation if the WAL cleanup fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried implementing this, but I think I'd prefer to leave WAL cleanup as a best effort, because although it has its own consistency problems, the other order does as well. If we delete WALs before the role and fail, we'll need to push the item back onto the queue, which itself could fail. In practice though, deleting roles and WALs both rely on the same storage layer, so at least these partial failure scenarios should be very rare.
We could address the inconsistency of leaving WALs lying around fairly robustly by keying WALs on a UUID whose lifetime is attached to a role's storage entry instead of keying WALs on the role name. That obviously wouldn't be a backport candidate though. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, let's leave it as is for now and revisit any improvements in a subsequent PR