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.

Sign up for GitHub

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

More cmd/ refactoring to simplify test loading and support integration tests #2412

Merged
merged 10 commits into from
Mar 18, 2022
Merged

More cmd/ refactoring to simplify test loading and support integration tests #2412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8e8da96
Refactor test loading by moving it to a new helper function
na-- Mar 6, 2022
66d2f1f
Refactor abort signal handling into a separate helper
na-- Mar 6, 2022
37ade41
Move K6_TYPE / --type / -t out of the globalFlags to RuntimeOptions
na-- Mar 6, 2022
7671dc7
Simplify createOutputs()
na-- Mar 6, 2022
b1c270d
Refactor cmd.rootCommand to support easy integration tests
na-- Mar 7, 2022
0581cc4
Fix --log-output=file support for relative paths and add a test
na-- Mar 7, 2022
3b3e386
Rename and document better the cmd/inspect.go internals
na-- Mar 7, 2022
85c5169
Remove js.Bundle.IsExecutable() because it's not needed anymore
na-- Mar 7, 2022
1455a62
Move some output init calls away from the Engine
na-- Mar 8, 2022
6ffdb95
Improve some code identifier names and comments
na-- Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions cmd/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"path/filepath"
"strconv"
"sync"
"syscall"
"time"

"github.com/fatih/color"
Expand Down Expand Up @@ -190,13 +189,10 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
}

// Trap Interrupts, SIGINTs and SIGTERMs.
sigC := make(chan os.Signal, 2)
globalState.signalNotify(sigC, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
defer globalState.signalStop(sigC)
go func() {
sig := <-sigC
gracefulStop := func(sig os.Signal) {
logger.WithField("sig", sig).Print("Stopping cloud test run in response to signal...")
// Do this in a separate goroutine so that if it blocks the second signal can stop the execution
// Do this in a separate goroutine so that if it blocks, the
// second signal can still abort the process execution.
go func() {
stopErr := client.StopCloudTestRun(refID)
if stopErr != nil {
Expand All @@ -206,11 +202,12 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
}
globalCancel()
}()

sig = <-sigC
}
hardStop := func(sig os.Signal) {
logger.WithField("sig", sig).Error("Aborting k6 in response to signal, we won't wait for the test to end.")
os.Exit(int(exitcodes.ExternalAbort))
}()
Comment on lines +206 to -249
Copy link
Contributor

Choose a reason for hiding this comment

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

The names here are not ... good IMO.

I was really confused that gracefulStop does something while hardStop just logs and does not os.Exit.

I would argue they are not great in the handleTestAbortSignals either.

I would recommend adding on in front the names in all places. This still isn't great, but I feel a lot better about onHardStop not calling os.Exit explicitly and only logging, while onGracefulStop actually doing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe just have os.Exit in both hardStops 🤷 I kind of feel like this DRY-ing of the code isn't really worth it

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 for gracefulStop and onHardStop
Regarding the DRY, keep in mind that this will be used at least 2 more times, in k6 coordinate and k6 agent

Copy link
Member Author

Choose a reason for hiding this comment

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

(for expediency, I will address this in the next PR)

}
stopSignalHandling := handleTestAbortSignals(globalState, gracefulStop, hardStop)
defer stopSignalHandling()

et, err := lib.NewExecutionTuple(test.derivedConfig.ExecutionSegment, test.derivedConfig.ExecutionSegmentSequence)
if err != nil {
Expand Down
36 changes: 36 additions & 0 deletions cmd/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@ package cmd

import (
"fmt"
"os"
"syscall"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
"gopkg.in/guregu/null.v3"

"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib/types"
)

Expand Down Expand Up @@ -87,3 +90,36 @@ func printToStdout(gs *globalState, s string) {
gs.logger.Errorf("could not print '%s' to stdout: %s", s, err.Error())
}
}

// Trap Interrupts, SIGINTs and SIGTERMs and call the given.
func handleTestAbortSignals(gs *globalState, firstHandler, secondHandler func(os.Signal)) (stop func()) {
sigC := make(chan os.Signal, 2)
done := make(chan struct{})
gs.signalNotify(sigC, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)

go func() {
select {
case sig := <-sigC:
firstHandler(sig)
case <-done:
return
}

select {
case sig := <-sigC:
if secondHandler != nil {
secondHandler(sig)
}
Comment on lines +110 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this nil check needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so, if os.Exit() is here. And I kind of like it to be here, to softly force the user to acknowledge that a second Ctrl+C should always abort the execution almost immediately, not be something optional.

// If we get a second signal, we immediately exit, so something like
// https://github.com/k6io/k6/issues/971 never happens again
os.Exit(int(exitcodes.ExternalAbort))
case <-done:
return
}
}()

return func() {
close(done)
gs.signalStop(sigC)
}
}
18 changes: 6 additions & 12 deletions cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"os"
"runtime"
"sync"
"syscall"
"time"

"github.com/spf13/afero"
Expand Down Expand Up @@ -182,21 +181,16 @@ a commandline interface for interacting with it.`,
)

// Trap Interrupts, SIGINTs and SIGTERMs.
sigC := make(chan os.Signal, 2)
globalState.signalNotify(sigC, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
defer globalState.signalStop(sigC)
go func() {
sig := <-sigC
gracefulStop := func(sig os.Signal) {
logger.WithField("sig", sig).Debug("Stopping k6 in response to signal...")
lingerCancel() // stop the test run, metric processing is cancelled below

// If we get a second signal, we immediately exit, so something like
// https://github.com/k6io/k6/issues/971 never happens again
sig = <-sigC
}
hardStop := func(sig os.Signal) {
logger.WithField("sig", sig).Error("Aborting k6 in response to signal")
globalCancel() // not that it matters, given the following command...
os.Exit(int(exitcodes.ExternalAbort))
}()
}
stopSignalHandling := handleTestAbortSignals(globalState, gracefulStop, hardStop)
defer stopSignalHandling()

// Initialize the engine
initBar.Modify(pb.WithConstProgress(0, "Init VUs..."))
Expand Down