fix: add constraint and runtime check for provisioner logs size limit #18893
Conversation
bcpeinhardt
changed the title
bcpeinhardt
changed the title
| 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.
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 an Insert 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 extra s.Database.UpdateProvisionerJobLogsLength call on every log line after an overflow has occurred.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| @@ -8,6 +8,14 @@ WHERE | |||
| AND ( | |||
| id > @created_after | |||
| ) ORDER BY id ASC; | |||
|
|
|||
| -- name: GetProvisionerJobLogSize :one | |||
There was a problem hiding this comment.
I can remove this query now I think.
| @@ -87,7 +87,8 @@ export const AgentRow: FC<AgentRowProps> = ({ | |||
| logs.push({ | |||
| id: -1, | |||
| level: "error", | |||
| output: "Startup logs exceeded the max size of 1MB!", | |||
| output: | |||
| "Startup logs exceeded the max size of 1MB, and will not continue to be written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.", | |||
There was a problem hiding this comment.
should the other places for the logging also contain the more descriptive message, in WorkspaceBuildLogs.tsx and WorkspaceBuildPageView.tsx?
This PR sets a constraint of 1MB on the provisioner job logs written to the database. This is consistent with the constraint we place on workspace agent logs:
coder/coderd/database/dump.sql
Line 2030 in 4ac6be6
It also updates the message printed to the front end when workspace startup logs exceed the max, as it was causing some customers to think their startup script had failed to run.