-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 32 commits
aa4b090
4b91966
a894e80
3843bc9
d27a5f0
14e49b1
2222235
53f10f5
ef49dfd
3047dec
8add590
a484a31
3e65e7e
0832273
56a4c37
2fe2d63
622409c
ac764f3
2e6bce1
edf5d79
c2e4417
4bdd575
5d40270
518e58f
34809f1
701df42
fbda1ae
d7be446
8f34ca7
df56303
4f23674
6b08d61
b2b681c
dee4693
e6b6ee2
1ee2b16
ab0bc8a
55b90d0
97a658e
723c49e
3b3b905
67c6b13
fd5d380
60f2295
305bf81
f57c992
a9498eb
9249ba4
20152f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import ( | |
"io" | ||
"io/ioutil" | ||
"net/http" | ||
"net/url" | ||
"os" | ||
"os/signal" | ||
"path/filepath" | ||
|
@@ -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" | ||
|
@@ -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 | ||
} | ||
|
@@ -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 | ||
} | ||
|
@@ -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(afero.NewOsFs(), cliConf, r) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -503,29 +505,55 @@ func init() { | |
runCmd.Flags().AddFlagSet(runCmdFlagSet()) | ||
} | ||
|
||
func createFilesystems() map[string]afero.Fs { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 Maybe that should be in a separate issue/pull request, but judging by the uses in |
||
// 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 | ||
osfs := afero.NewOsFs() | ||
if runtime.GOOS == "windows" { | ||
// This is done so that we can continue to use paths with /|"\" through the code but also to | ||
// be easier to travers the cachedFs later as it doesn't work very well if you have windows | ||
mstoykov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// volumes | ||
osfs = fsext.NewTrimFilePathSeparatorFs(osfs) | ||
} | ||
return map[string]afero.Fs{ | ||
"file": fsext.NewCacheOnReadFs(osfs, 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) (*loader.SourceData, error) { | ||
if src == "-" { | ||
data, err := ioutil.ReadAll(stdin) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &lib.SourceData{Filename: "-", Data: data}, nil | ||
return &loader.SourceData{URL: &url.URL{Path: "-", Scheme: "file"}, Data: data}, nil | ||
} | ||
pwdURL := &url.URL{Scheme: "file", Path: filepath.ToSlash(filepath.Clean(pwd)) + "/"} | ||
mstoykov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
srcLocalPath := filepath.Join(pwd, src) | ||
na-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ok, _ := afero.Exists(filesystems["file"], srcLocalPath); ok { | ||
// there is file on the local disk ... lets use it :) | ||
return loader.Load(filesystems, &url.URL{Scheme: "file", Path: srcLocalPath}, src) | ||
} | ||
abspath := filepath.Join(pwd, src) | ||
if ok, _ := afero.Exists(fs, abspath); ok { | ||
src = abspath | ||
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 *loader.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 { | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 fork6 run
andk6 cloud
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 itsfiles
, due to thefsext.CacheOnReadFs
code, I think