-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add integrity checks before uploading snapshots #402
Conversation
Are there any other full snapshot checks we should make? I didn't see any similar checks to make for compact snapshots, but maybe I missed something. |
The snapshot format makes some structural assumptions that cannot be enforced by protobuf. If these are violated, processing the snapshot may fail, or snapshot data may be misinterpreted and can cause bogus statistics to be recorded. Validate these assumptions before sending a snapshot.
This reverts commit 867b45b.
77a7709
to
d851aa6
Compare
This could cause problems for subsequent snapshot computations. Instead, just log an error and send a failed snapshot.
} | ||
|
||
func SendFailedFull(ctx context.Context, server *state.Server, collectionOpts state.CollectionOpts, logger *util.Logger) error { | ||
s := snapshot.FullSnapshot{FailedRun: true, CollectorErrors: logger.ErrorMessages} | ||
s := snapshot.FullSnapshot{FailedRun: true} |
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 ended up moving this into submitFull
because both callers were setting collector errors independently before invoking that helper--we might as well do it in the shared code path.
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.
nice! 👍
The snapshot format makes some structural assumptions that cannot be
enforced by protobuf. If these are violated, processing the snapshot
may fail, or snapshot data may be misinterpreted and can cause bogus
statistics to be recorded.
Validate these assumptions before sending a snapshot.
See discussion in #399 (review)