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

Rewrite script/files loading to be be url based #1059

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
aa4b090
Rewrite script/files loading to be be url based
mstoykov Jun 7, 2019
4b91966
fixup! Rewrite script/files loading to be be url based
mstoykov Jun 27, 2019
a894e80
fixup! Rewrite script/files loading to be be url based
mstoykov Jul 4, 2019
3843bc9
fixup! Rewrite script/files loading to be be url based
mstoykov Jul 4, 2019
d27a5f0
Merge remote-tracking branch 'origin/master' into refactor/RewriteaAr…
mstoykov Jul 4, 2019
14e49b1
Rename FSes to Filesystems
mstoykov Jul 4, 2019
2222235
Fix lll error
mstoykov Jul 4, 2019
53f10f5
Add tests for old archive support
mstoykov Jul 4, 2019
ef49dfd
Fix saving and loading archive filename and pwd
mstoykov Jul 5, 2019
3047dec
Fix pwd sometimes missing it's slash at the end
mstoykov Jul 5, 2019
8add590
Fix not recognizing local files which are not starting with . or /
mstoykov Jul 5, 2019
a484a31
Fix lifting config files from the filesystems in the memmapfs and tha…
mstoykov Jul 5, 2019
3e65e7e
Add comment about UnprependPathFs
mstoykov Jul 5, 2019
0832273
Better names and abstration for UnprependPathFs
mstoykov Jul 5, 2019
56a4c37
Refactor normalizedFs to be a ChangePathFs
mstoykov Jul 5, 2019
2fe2d63
rename changepathfs file
mstoykov Jul 5, 2019
622409c
Implement caching and reading of cache for github/cdnjs urls
mstoykov Jul 8, 2019
ac764f3
Refactor the walk function in lib.archive a bit
mstoykov Jul 8, 2019
2e6bce1
Fix name of dumpMemMapFsToBuf
mstoykov Jul 9, 2019
edf5d79
Add test for ChangePathFs and some fixes
mstoykov Jul 9, 2019
c2e4417
Merge remote-tracking branch 'origin/master' into refactor/RewriteaAr…
mstoykov Jul 9, 2019
4bdd575
Check usage of CacheOnReadFs in archive
mstoykov Jul 10, 2019
5d40270
Add test for LstatIfPossible to ChangePathFs
mstoykov Jul 10, 2019
518e58f
Fix wronly calling Mkdir instead of MkdirAll in test
mstoykov Jul 10, 2019
34809f1
test a little bit more of the ChangePathFs.OpenFile code
mstoykov Jul 10, 2019
701df42
Add TrimPathSeparatorFs test
mstoykov Jul 10, 2019
fbda1ae
Fix trimpathseparator on windows
mstoykov Jul 10, 2019
d7be446
Add to TestTrimAferoPathSeparatorFs
mstoykov Jul 10, 2019
8f34ca7
Fix file and directory permissions and times
mstoykov Jul 11, 2019
df56303
Hardlink the to the 'data' file in the archive
mstoykov Jul 11, 2019
4f23674
Merge remote-tracking branch 'origin/master' into refactor/RewriteaAr…
mstoykov Jul 11, 2019
6b08d61
Don't print the warning for schemeless URLs multiple times
mstoykov Jul 11, 2019
b2b681c
Fixes for golangci and not correctly reading scheme urls from the cache
mstoykov Jul 15, 2019
dee4693
Add test testing error on main script not in fs
mstoykov Jul 15, 2019
e6b6ee2
Add tests for bad filename/pwd in archives
mstoykov Jul 15, 2019
1ee2b16
Add test for malformed metadata in archive
mstoykov Jul 15, 2019
ab0bc8a
s/travers/traverse
mstoykov Jul 15, 2019
55b90d0
Fix relative and absolute paths on windows
mstoykov Jul 15, 2019
97a658e
loader.Resolve: Don't change pwd when it's missing slash on the end
mstoykov Jul 15, 2019
723c49e
If a command gets an absolute path don't try it as relative
mstoykov Jul 15, 2019
3b3b905
Add test with funky paths to the archive
mstoykov Jul 16, 2019
67c6b13
100% test coverage for cmd#readSource
mstoykov Jul 16, 2019
fd5d380
Support running and archiving when giving scripts with stdin
mstoykov Jul 16, 2019
60f2295
Move cmd#readSource to loader#ReadSource
mstoykov Jul 16, 2019
305bf81
Move cmd#createFilesystems to loader#CreateFilesystems
mstoykov Jul 16, 2019
f57c992
typo
mstoykov Jul 17, 2019
a9498eb
case insensitive anonymizaiton for windows paths
mstoykov Jul 18, 2019
9249ba4
Add the os under which the archive was made in the archive
mstoykov Jul 23, 2019
20152f1
Merge remote-tracking branch 'origin/master' into refactor/RewriteaAr…
mstoykov Jul 23, 2019
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
7 changes: 4 additions & 3 deletions api/v1/setup_teardown_routes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"testing"
"time"

Expand All @@ -35,7 +36,6 @@ import (
"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/lib/types"
"github.com/manyminds/api2go/jsonapi"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
null "gopkg.in/guregu/null.v3"
Expand Down Expand Up @@ -130,10 +130,11 @@ func TestSetupData(t *testing.T) {
},
}
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.name, func(t *testing.T) {
runner, err := js.New(
&lib.SourceData{Filename: "/script.js", Data: testCase.script},
afero.NewMemMapFs(),
&lib.SourceData{URL: &url.URL{Path: "/script.js"}, Data: testCase.script},
nil,
lib.RuntimeOptions{},
)
require.NoError(t, err)
Expand Down
9 changes: 4 additions & 5 deletions cmd/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ package cmd
import (
"os"

"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)
Expand Down Expand Up @@ -51,8 +50,8 @@ An archive is a fully self-contained test run, and can be executed identically e
return err
}
filename := args[0]
fs := afero.NewOsFs()
src, err := readSource(filename, pwd, fs, os.Stdin)
filesystems := createFilesystems()
src, err := readSource(filename, pwd, filesystems, os.Stdin)
Copy link
Member

Choose a reason for hiding this comment

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

Same as cmd/run.go... I think you can currently re-bundle archives, which is a somewhat nice feature, for example as a way to change their options: k6 archive -u 20 -d 2m oldArchiveWithOtherOptions.tar. So, passing the caching filesystem probably isn't for the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome .. I was intending on having this as feature that upgrades the archive to a new version :). This is not actually a problem because after the archive is read it's filesystems are used not the ones that are provided here. Those are used if the file is not an archive but a script

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but then it means that we have a potentially multi-megabyte tar archive saved in memory in that cachedFS that won't be garbage-collected. Not a huge issue for k6 archive, unless the original tar is huge, but a minor issue for k6 run and k6 cloud

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would it not be garbage-collected ? We don't save any reference to anything of the fs in ReadSource and newRunner completely ignores it if it's an archive. I don't see why it won't be collected

Copy link
Member

Choose a reason for hiding this comment

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

Because the root object of that memory tree would be filesystems here (and in the other commands), which persists for the whole duration of the command execution. So, the tar archive will continue to be present there in its files, due to the fsext.CacheOnReadFs code, I think

if err != nil {
return err
}
Expand All @@ -62,7 +61,7 @@ An archive is a fully self-contained test run, and can be executed identically e
return err
}

r, err := newRunner(src, runType, fs, runtimeOptions)
r, err := newRunner(src, runType, filesystems, runtimeOptions)
if err != nil {
return err
}
Expand All @@ -71,7 +70,7 @@ An archive is a fully self-contained test run, and can be executed identically e
if err != nil {
return err
}
conf, err := getConsolidatedConfig(fs, Config{Options: cliOpts}, r)
conf, err := getConsolidatedConfig(filesystems["file"], Config{Options: cliOpts}, r)
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
Expand Down
9 changes: 4 additions & 5 deletions cmd/cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"github.com/loadimpact/k6/stats/cloud"
"github.com/loadimpact/k6/ui"
"github.com/pkg/errors"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/spf13/pflag"

Expand Down Expand Up @@ -71,8 +70,8 @@ This will execute the test on the Load Impact cloud service. Use "k6 login cloud
}

filename := args[0]
fs := afero.NewOsFs()
src, err := readSource(filename, pwd, fs, os.Stdin)
filesystems := createFilesystems()
src, err := readSource(filename, pwd, filesystems, os.Stdin)
na-- marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
Expand All @@ -82,7 +81,7 @@ This will execute the test on the Load Impact cloud service. Use "k6 login cloud
return err
}

r, err := newRunner(src, runType, fs, runtimeOptions)
r, err := newRunner(src, runType, filesystems, runtimeOptions)
if err != nil {
return err
}
Expand All @@ -91,7 +90,7 @@ This will execute the test on the Load Impact cloud service. Use "k6 login cloud
if err != nil {
return err
}
conf, err := getConsolidatedConfig(fs, Config{Options: cliOpts}, r)
conf, err := getConsolidatedConfig(filesystems["file"], Config{Options: cliOpts}, r)
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
Expand Down
17 changes: 10 additions & 7 deletions cmd/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (

"github.com/loadimpact/k6/js"
"github.com/loadimpact/k6/lib"
"github.com/spf13/afero"
"github.com/spf13/cobra"
)

Expand All @@ -43,8 +42,8 @@ var inspectCmd = &cobra.Command{
if err != nil {
return err
}
fs := afero.NewOsFs()
src, err := readSource(args[0], pwd, fs, os.Stdin)
filesystems := createFilesystems()
src, err := readSource(args[0], pwd, filesystems, os.Stdin)
if err != nil {
return err
}
Expand All @@ -59,20 +58,24 @@ var inspectCmd = &cobra.Command{
return err
}

var opts lib.Options
var (
opts lib.Options
b *js.Bundle
)
switch typ {
case typeArchive:
arc, err := lib.ReadArchive(bytes.NewBuffer(src.Data))
var arc *lib.Archive
arc, err = lib.ReadArchive(bytes.NewBuffer(src.Data))
if err != nil {
return err
}
b, err := js.NewBundleFromArchive(arc, runtimeOptions)
b, err = js.NewBundleFromArchive(arc, runtimeOptions)
if err != nil {
return err
}
opts = b.Options
case typeJS:
b, err := js.NewBundle(src, fs, runtimeOptions)
b, err = js.NewBundle(src, filesystems, runtimeOptions)
if err != nil {
return err
}
Expand Down
44 changes: 31 additions & 13 deletions cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"io"
"io/ioutil"
"net/http"
"net/url"
"os"
"os/signal"
"path/filepath"
Expand All @@ -43,6 +44,7 @@ import (
"github.com/loadimpact/k6/js"
"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/lib/consts"
"github.com/loadimpact/k6/lib/fsext"
"github.com/loadimpact/k6/lib/types"
"github.com/loadimpact/k6/loader"
"github.com/loadimpact/k6/ui"
Expand Down Expand Up @@ -116,8 +118,8 @@ a commandline interface for interacting with it.`,
return err
}
filename := args[0]
fs := afero.NewOsFs()
src, err := readSource(filename, pwd, fs, os.Stdin)
filesystems := createFilesystems()
src, err := readSource(filename, pwd, filesystems, os.Stdin)
na-- marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
Expand All @@ -127,7 +129,7 @@ a commandline interface for interacting with it.`,
return err
}

r, err := newRunner(src, runType, fs, runtimeOptions)
r, err := newRunner(src, runType, filesystems, runtimeOptions)
if err != nil {
return err
}
Expand All @@ -138,7 +140,7 @@ a commandline interface for interacting with it.`,
if err != nil {
return err
}
conf, err := getConsolidatedConfig(fs, cliConf, r)
conf, err := getConsolidatedConfig(filesystems["file"], cliConf, r)
if err != nil {
return err
}
Expand Down Expand Up @@ -503,29 +505,45 @@ func init() {
runCmd.Flags().AddFlagSet(runCmdFlagSet())
}

func createFilesystems() map[string]afero.Fs {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is the right place for this function - putting it in cmd isn't very testable because of potential import loops. Can we move it to somewhere in lib, since it doesn't actually depend on anything here?

Also, as I've mentioned to you in our chat, the abstractions (or rather, their lack) currently feel wrong to me. Why does the top-level code in cmd care about the fact that users can run local or remote scripts? And even more so - why does it care that we can cache those things? These seem like implementation details that shouldn't even be "known" in this part of the codebase... The code in cmd should just take whatever string the user supplied and pass it on to something else that deals with it - and either run or show a nice error (containing that exact same string). The role of that "something else" would then be to deal with all of the resolving, caching, fetching, figuring out the type (js/archive), etc. - the code in cmd should be oblivious to most of those details.

Maybe that should be in a separate issue/pull request, but judging by the uses in createFilesystems I've commented on above, it is somewhat problematic even now 😕

// We want to eliminate disk access at runtime, so we set up a memory mapped cache that's
// written every time something is read from the real filesystem. This cache is then used for
// successive spawns to read from (they have no access to the real disk).
// Also initialize the same for `https` but the caching is handled manually in the loader package
return map[string]afero.Fs{
"file": fsext.NewCacheOnReadFs(
fsext.NewUnprependPathFs(afero.NewOsFs(), afero.FilePathSeparator),
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
afero.NewMemMapFs(), 0),
"https": afero.NewMemMapFs(),
}
}

// Reads a source file from any supported destination.
func readSource(src, pwd string, fs afero.Fs, stdin io.Reader) (*lib.SourceData, error) {
func readSource(src, pwd string, filesystems map[string]afero.Fs, stdin io.Reader) (*lib.SourceData, error) {
if src == "-" {
data, err := ioutil.ReadAll(stdin)
if err != nil {
return nil, err
}
return &lib.SourceData{Filename: "-", Data: data}, nil
return &lib.SourceData{URL: &url.URL{Path: "-", Scheme: "file"}, Data: data}, nil
}
abspath := filepath.Join(pwd, src)
if ok, _ := afero.Exists(fs, abspath); ok {
src = abspath
pwdURL := &url.URL{Scheme: "file", Path: filepath.ToSlash(filepath.Clean(pwd)) + "/"}
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
srcURL, err := loader.Resolve(pwdURL, filepath.ToSlash(src))
if err != nil {
return nil, err
}
return loader.Load(fs, pwd, src)
return loader.Load(filesystems, srcURL, src)
}

// Creates a new runner.
func newRunner(src *lib.SourceData, typ string, fs afero.Fs, rtOpts lib.RuntimeOptions) (lib.Runner, error) {
func newRunner(
src *lib.SourceData, typ string, filesystems map[string]afero.Fs, rtOpts lib.RuntimeOptions,
) (lib.Runner, error) {
switch typ {
case "":
return newRunner(src, detectType(src.Data), fs, rtOpts)
return newRunner(src, detectType(src.Data), filesystems, rtOpts)
case typeJS:
return js.New(src, fs, rtOpts)
return js.New(src, filesystems, rtOpts)
case typeArchive:
arc, err := lib.ReadArchive(bytes.NewReader(src.Data))
if err != nil {
Expand Down
17 changes: 8 additions & 9 deletions cmd/runtime_options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ package cmd
import (
"bytes"
"fmt"
"net/url"
"os"
"runtime"
"strings"
"testing"

"github.com/loadimpact/k6/lib"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -220,11 +220,11 @@ func TestEnvVars(t *testing.T) {

runner, err := newRunner(
&lib.SourceData{
Data: []byte(jsCode),
Filename: "/script.js",
Data: []byte(jsCode),
URL: &url.URL{Path: "/script.js"},
},
typeJS,
afero.NewOsFs(),
nil,
rtOpts,
)
require.NoError(t, err)
Expand All @@ -234,16 +234,15 @@ func TestEnvVars(t *testing.T) {
assert.NoError(t, archive.Write(archiveBuf))

getRunnerErr := func(rtOpts lib.RuntimeOptions) (lib.Runner, error) {
r, err := newRunner(
return newRunner(
&lib.SourceData{
Data: []byte(archiveBuf.Bytes()),
Filename: "/script.tar",
Data: archiveBuf.Bytes(),
URL: &url.URL{Path: "/script.js"},
},
typeArchive,
afero.NewOsFs(),
nil,
rtOpts,
)
return r, err
}

_, err = getRunnerErr(lib.RuntimeOptions{})
Expand Down
7 changes: 3 additions & 4 deletions converter/har/converter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (

"github.com/loadimpact/k6/js"
"github.com/loadimpact/k6/lib"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -57,9 +56,9 @@ func TestBuildK6RequestObject(t *testing.T) {
v, err := buildK6RequestObject(req)
assert.NoError(t, err)
_, err = js.New(&lib.SourceData{
Filename: "/script.js",
Data: []byte(fmt.Sprintf("export default function() { res = http.batch([%v]); }", v)),
}, afero.NewMemMapFs(), lib.RuntimeOptions{})
URL: &url.URL{Path: "/script.js"},
Data: []byte(fmt.Sprintf("export default function() { res = http.batch([%v]); }", v)),
}, nil, lib.RuntimeOptions{})
assert.NoError(t, err)
}

Expand Down
Loading