Skip to content
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

Skip session recording reservation files (filessesion) #13826

Merged
merged 9 commits into from
Jun 28, 2022

Conversation

gabrielcorado
Copy link
Contributor

@gabrielcorado gabrielcorado commented Jun 23, 2022

Closes #13640 by skipping reservation files when completing a session recording upload. To do so, now we create the reservation files with a different extension so we can differentiate them from regular upload parts.

Context

Session recording modes introduce "disk reservation", where Teleport creates placeholder files to guarantee that the node has enough disk space to record sessions. This reservation happens every time a new part file is required. Those files are replaced with recording contents on the UploadPart flow.

In scenarios where the upload couldn't run (for example, if the node is killed during a session), the reservation file was left on the node. When the UploadComplete was executed, it created broken recording files (.tar), causing the uploads to fail (as we can see in the logs presented on the related issue).

@gabrielcorado gabrielcorado self-assigned this Jun 23, 2022
@github-actions github-actions bot requested review from alistanis and espadolini June 23, 2022 21:32
@github-actions github-actions bot added the audit-log Issues related to Teleports Audit Log label Jun 23, 2022
@espadolini
Copy link
Contributor

Side note: don't we lose the disk space reservation the moment we open the file for writing again in openUploadPart?

@gabrielcorado
Copy link
Contributor Author

Side note: don't we lose the disk space reservation the moment we open the file for writing again in openUploadPart?

@espadolini Do you mean it because we truncate the file when opening?

@espadolini
Copy link
Contributor

Do you mean it because we truncate the file when opening?

Yes, as opposed to writing whatever content we need and then truncating before closing.

@gabrielcorado
Copy link
Contributor Author

gabrielcorado commented Jun 24, 2022

@zmb3 I've updated the PR with @espadolini suggestion: create the reservation files with a different extension, so we don't need to check their content.

Yes, as opposed to writing whatever content we need and then truncating before closing.

I have updated the PR with this also. Thanks for pointing it out.

Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

Never mucked around the session recording code so take this approval with a grain of salt

@gabrielcorado gabrielcorado enabled auto-merge (squash) June 28, 2022 14:35
@gabrielcorado gabrielcorado merged commit 97d6c74 into master Jun 28, 2022
@gabrielcorado gabrielcorado deleted the gabrielcorado/remove-reservation-files branch June 28, 2022 20:24
gabrielcorado added a commit that referenced this pull request Jun 29, 2022
Skip session recording reservation files (filessesion) (#13826)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-log Issues related to Teleports Audit Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning - Skipped session recording
4 participants