Skip to content

fix!: remove startup logs eof for streaming #8528

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

Merged
merged 2 commits into from
Jul 18, 2023
Merged

fix!: remove startup logs eof for streaming #8528

merged 2 commits into from
Jul 18, 2023

Conversation

kylecarbs
Copy link
Member

We have external utilities like logstream-kube that may send logs after an agent shuts down unexpectedly to report additional information. In a recent change we stopped accepting these logs, which broke these utilities.

In the future we'll rename startup logs to agent logs or something more generalized so this is less confusing in the future.

@kylecarbs kylecarbs requested review from mafredri and coadler July 14, 2023 22:28
@kylecarbs kylecarbs self-assigned this Jul 14, 2023
@kylecarbs kylecarbs marked this pull request as ready for review July 14, 2023 22:28
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine but I can give it a proper look tomorrow.

if state.ReadyAt.Valid {
return
}
// Just keep listening - more logs might come in the future!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the ready state stuff higher up too.

@kylecarbs
Copy link
Member Author

@mafredri this can just wait until Monday, no biggie.

We have external utilities like logstream-kube that may send
logs after an agent shuts down unexpectedly to report additional
information. In a recent change we stopped accepting these logs,
which broke these utilities.

In the future we'll rename startup logs to agent logs or something
more generalized so this is less confusing in the future.
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just remembered the current log fetching in cli/cliui.Agent relies on the websocket closing when streaming startup logs.

I pushed a fix for this and as long as the WebUI doesn't depend on the ws closing behavior, I think we're good.

@mafredri mafredri changed the title fix: remove startup logs eof for streaming fix!: remove startup logs eof for streaming Jul 17, 2023
@github-actions github-actions bot added the release/breaking This label is applied to PRs to detect breaking changes as part of the release process label Jul 17, 2023
@mafredri
Copy link
Member

Marked this as breaking since this will break coder server/clients (e.g. older clients will continue streaming startup logs forever, when blocking startup script is used).

@kylecarbs kylecarbs merged commit 98164f6 into main Jul 18, 2023
@kylecarbs kylecarbs deleted the startupeof branch July 18, 2023 15:57
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release/breaking This label is applied to PRs to detect breaking changes as part of the release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants