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

cli: stream both stdout and stderr when following an alloc. #16556

Merged
merged 14 commits into from
Apr 4, 2023

Conversation

jrasell
Copy link
Member

@jrasell jrasell commented Mar 20, 2023

This update changes the behaviour when following logs from an
allocation, so that both stdout and stderr files streamed when the
operator supplies the follow flag. The previous behaviour is held
when all other flags and situations are provided.

Closes #2123

jrasell added 2 commits March 20, 2023 11:36
This update changes the behaviour when following logs from an
allocation, so that both stdout and stderr files streamed when the
operator supplies the follow flag. The previous behaviour is held
when all other flags and situations are provided.
command/alloc_logs.go Outdated Show resolved Hide resolved

// End the streaming
r.Close()
_ = r.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of this PR, but do we need to close(cancel) here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot; I can fix this up in another PR.

command/alloc_logs.go Outdated Show resolved Hide resolved
command/alloc_logs.go Outdated Show resolved Hide resolved
helper/cli/ui.go Outdated Show resolved Hide resolved
helper/cli/ui.go Outdated Show resolved Hide resolved
helper/cli/ui.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Looking good!
image

There seems to always be some empty lines first, but I don't know where those are coming from. Left some random comments, but nothing major so feel free to ignore them

command/alloc_logs.go Outdated Show resolved Hide resolved
command/alloc_logs.go Outdated Show resolved Hide resolved
Comment on lines +232 to +237
if l.follow && !(l.stderr || l.stdout || l.tail || l.numLines > 0 || l.numBytes > 0) {
if err := l.tailMultipleFiles(client, alloc); err != nil {
l.Ui.Error(fmt.Sprintf("Failed to tail stdout and stderr files: %v", err))
return 1
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if l.follow && !(l.stderr || l.stdout || l.tail || l.numLines > 0 || l.numBytes > 0) {
if err := l.tailMultipleFiles(client, alloc); err != nil {
l.Ui.Error(fmt.Sprintf("Failed to tail stdout and stderr files: %v", err))
return 1
}
} else {
if l.follow && !(l.stderr || l.stdout || l.tail || l.numLines > 0 || l.numBytes > 0) {
if err := l.tailMultipleFiles(client, alloc); err != nil {
l.Ui.Error(fmt.Sprintf("Failed to tail stdout and stderr files: %v", err))
return 1
}
return 0
}

By returning early we can remove the else clause and dedent the rest of the code, which may be nice, but very much a style nit-pick 😄

Comment on lines 244 to 292
r, readErr = l.followFile(client, alloc, follow, task, logType, api.OriginEnd, offset)
r, readErr = l.followFile(client, alloc, l.follow, l.task, logType, api.OriginEnd, offset)

// If numLines is set, wrap the reader
if numLines != -1 {
r = NewLineLimitReader(r, int(numLines), int(numLines*bytesToLines), 1*time.Second)
if l.numLines != -1 {
r = NewLineLimitReader(r, int(l.numLines), int(l.numLines*bytesToLines), 1*time.Second)
}

if readErr != nil {
readErr = fmt.Errorf("Error tailing file: %v", readErr)
return fmt.Errorf("error tailing file: %v", readErr)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of the PR, but it may be worth flipping the lines to check for readErr != nil earlier since r will be nil

command/alloc_logs.go Outdated Show resolved Hide resolved
Comment on lines +351 to +352
stdoutFrames, stdoutErrCh := client.AllocFS().Logs(
alloc, true, l.task, api.FSLogNameStdout, api.OriginEnd, 1, cancel, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the docs I think passing a ctx.Done() instead of creating a cancel channel would work, and it would make the code look more like modern Go.

You can then even pass the context to the function (tailMultipleFiles(ctx, ...)) and have let the parent Run() handle the lifecycle of the request, so that things like signal handling are handled outside of these sub-functions.

func (l *AllocLogsCommand) Run(...) {
	// ...
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()
	// ...
	signalCh := make(chan os.Signal, 1)
	signal.Notify(signalCh, os.Interrupt, syscall.SIGTERM)
	go func() {
		for {
			select {
			case <-signalCh:
				return 0
			}
		}
	}()
	// ...
	tailMultipleFiles(ctx, ...)
	// ...
	followFile(ctx, ...)
}

func (l *AllocLogsCommand) tailMultipleFiles(ctx ...) error {
	// ...
	stderrFrames, stderrErrCh := client.AllocFS().Logs(
		alloc, true, l.task, api.FSLogNameStderr, api.OriginEnd, 1, ctx.Done(), nil)
	// ...
}

e2e/alloc_logs/alloc_logs_test.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.x backport to 1.5.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixing stdout and stderr logs
2 participants