Skip to content

tsh: split large desktop recordings into multiple files#33784

Merged
zmb3 merged 1 commit intomasterfrom
zmb3/split-avi-export
Nov 7, 2023
Merged

tsh: split large desktop recordings into multiple files#33784
zmb3 merged 1 commit intomasterfrom
zmb3/split-avi-export

Conversation

@zmb3
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 commented Oct 20, 2023

AVI files contain 32-bit pointers, which means we cant just add frames to a single file until the end of time. This change properly detects when a recording export has reached the maximum size of an AVI file and splits the recording into multiple files.

changelog: Split large desktop recordings into multiple files during export.

Closes #33109
Closes #33110

@zmb3 zmb3 requested a review from stevenGravy October 20, 2023 22:59
@github-actions github-actions Bot added size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 20, 2023
@zmb3 zmb3 force-pushed the zmb3/split-avi-export branch from 31ab6c3 to 4ee8ead Compare October 20, 2023 23:08
AVI files contain 32-bit pointers, which means we cant just add
frames to a single file until the end of time. This change properly
detects when a recording export has reached the maximum size of
an AVI file and splits the recording into multiple files.

Closes #33109
Closes #33110
@zmb3 zmb3 force-pushed the zmb3/split-avi-export branch from 4ee8ead to b150775 Compare October 20, 2023 23:08
// writeMovie writes the events for the specified session into one or more movie files
// beginning with the specified prefix. It returns the number of frames that were written and an error.
func writeMovie(ctx context.Context, ss events.SessionStreamer, sid session.ID, prefix string,
write func(format string, args ...any) (int, error)) (frames int, err error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we can't just use our logger ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because loggers do not output user-friendly output. They are for devs for troubleshooting, not for end users.

// so we indicate to the user when we wrote something
if frames > 0 {
fmt.Printf("wrote recording to %v\n", fname)
_, err = writeMovie(cf.Context, authClient, session.ID(cf.SessionID), filenamePrefix, fmt.Printf)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either here or within the function we should write out that we are starting the writing of recordings.

As it takes time to keep writing more files I would append the "wrote uuid.avi" with

Starting recording export
wrote uuid.avi
Continuing export to next file.
...

Otherwise you could think there is a error since it output that it wrote the file but doesn't tell you it's continuing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

At some point I'd like to add a progress bar - that's a larger change though.

Until then, I think I prefer being less noisy.

@stevenGravy
Copy link
Copy Markdown
Contributor

stevenGravy commented Oct 23, 2023

Exporting large and small recordings looking good. Nice that you get the progress as it goes so someone could start reviewing the audit as they get it. Gave some feedback on enhancing the output so the user knows the output is starting and continuing.

@zmb3
Copy link
Copy Markdown
Collaborator Author

zmb3 commented Nov 7, 2023

@gabrielcorado PTAL

Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the delay.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 7, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@zmb3 zmb3 added this pull request to the merge queue Nov 7, 2023
Merged via the queue into master with commit fe40a22 Nov 7, 2023
@zmb3 zmb3 deleted the zmb3/split-avi-export branch November 7, 2023 20:27
@public-teleport-github-review-bot
Copy link
Copy Markdown

@zmb3 See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop access export cannot export larger then 4gb AVI content

4 participants