-
Notifications
You must be signed in to change notification settings - Fork 312
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
cluster/audit: audit log in seconds maybe leads to file overwriten #879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
===========================================
- Coverage 52.17% 10.78% -41.40%
===========================================
Files 263 130 -133
Lines 19030 9888 -9142
===========================================
- Hits 9929 1066 -8863
- Misses 7562 8583 +1021
+ Partials 1539 239 -1300
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
887f4c0
to
3344d2f
Compare
@lucklove PTAL |
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.
LGTM, brilliant!
Awesome!!! I think this is what bother us for a very long time: #666 (I closed that because it didn't reproduce these days, but I'm sure your PR finally fixed that) |
@@ -103,7 +103,7 @@ func ShowAuditLog(dir string, auditID string) error { | |||
return errors.Trace(err) | |||
} | |||
|
|||
t := time.Unix(ts, 0) | |||
t := time.Unix(ts/1e9, 0) |
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.
This is not compatible with the existing audit files ?
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.
Sorry for my careless, @9547 can your please open another PR to fix this?
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.
Sorry, I was inconsiderate, I'll fix in another PR
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.
What problem does this PR solve?
In my local dev's env, I've found the audit log file was base52 encoded with
time.Now().Unix()
, while at the same time, if the command is run fast enough, then the later file with the same name will cover the previous one.In my local dev, I'm running
tiup-dm test-cmd
the below case maybe(occasional) failed of this line
tiup/tests/tiup-dm/test_cmd.sh
Line 28 in 11535d2
What is changed and how it works?
Check List
Tests
Code changes
Side effects
Related changes
Release notes: