- Notifications
You must be signed in to change notification settings - Fork947
fix: add constraint and runtime check for provisioner logs size limit#18893
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
| err = s.Database.UpdateProvisionerJobLogsLength(ctx, database.UpdateProvisionerJobLogsLengthParams{ | ||
| ID: parsedID, | ||
| LogsLength: int32(newLogSize), // #nosec G115 - Log output length is limited to 1MB (2^20) which fits in an int32. | ||
| }) | ||
| if err != nil { | ||
| if database.IsProvisionerJobLogsLimitError(err) { | ||
| err = s.Database.UpdateProvisionerJobLogsOverflowed(ctx, database.UpdateProvisionerJobLogsOverflowedParams{ | ||
| ID: parsedID, | ||
| LogsOverflowed: true, | ||
| }) | ||
| if err != nil { | ||
| s.Logger.Error(ctx, "failed to set logs overflowed flag", slog.F("job_id", parsedID), slog.Error(err)) | ||
| } | ||
| return &proto.UpdateJobResponse{ | ||
| Canceled: job.CanceledAt.Valid, | ||
| }, nil | ||
| } | ||
| s.Logger.Error(ctx, "failed to update logs length", slog.F("job_id", parsedID), slog.Error(err)) | ||
| return nil, xerrors.Errorf("update logs length: %w", err) | ||
| } |
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 think it might be a good idea to store the current logs length and overflow state as variables in this Go code. With that change we would update the length var when anInsert call is successful, and we can determine prior to insertion whether the log we want to insert will cause the overflow. That way we can avoid the extras.Database.UpdateProvisionerJobLogsLength call on every log line after an overflow has occurred.
Uh oh!
There was an error while loading.Please reload this page.
WIP, lovely PR description imminent.