Replace afero with the new Go io/fs package

[mstoykov]

During #1059 it became obvious afero has some problems specifically with it's MemMapFs, but also missing features in CacheOnReadFs, problems with Walk(which are due to the way MemMapFs works in some ways) and possibly many more.

Given that the cleanest (and I would argue very good) solution is for MemMapFs to be rewritten in a way that it fixes all it's bugs and is in general not hacky, I propose that we fork afero and use the go modules replace functionality. We should still try to merge all the fixes in the original repo, but unfortunately it looks pretty dead to me so it will probably take a while ...

This is definitely not high priority, as the current workarounds work, but it will still make the experience of using MemMapFs and afero much less error-prone and with less hacks.

[na--]

I think the issue should be called "Fork or replace afero", since it might be better if we use another library, instead of trying to fix all of the issues with afero, and then potentially inherit the maintenance burden of the forked library... For example, it might be worth investigating https://github.com/C2FO/vfs, since it seems much more active and it's supposedly inspired by afero. Or maybe https://github.com/twpayne/go-vfs, since it supposedly has afero compatibility. Or maybe this: https://godoc.org/golang.org/x/tools/godoc/vfs - this seems semi-official 😄

But yeah, if we end up sticking with afero and forking it, go modules (#1070) definitely seems like a prerequisite...

[na--]

To update this, it's probably worth waiting for https://tip.golang.org/pkg/io/fs/ and seeing what gets built on top of it

[na--]

Go 1.16 was released:

• https://golang.org/doc/go1.16#fs
• https://benjamincongdon.me/blog/2021/01/21/A-Tour-of-Go-116s-iofs-package/

[yorugac]

Since this issue mentions only package transition at the moment, I'd like to add a related suggestion. As part of the work here, if possible it'd be nice to change this map into a struct:

k6/loader/filesystems.go

Lines 44 to 47 in 9d6adcc

	return map[string]afero.Fs{
	"file": fsext.NewCacheOnReadFs(osfs, afero.NewMemMapFs(), 0),
	"https": afero.NewMemMapFs(),
	}

Also, is there going to be some kind of a design proposal for this issue? I.e. how interfaces would change with the transition and so on.

[mstoykov]

this map into a struct:

What is the expected benefit of this? Because currently one of the benefits of it being a map is that if we decide to add more protocols you just add them to the map and the rest of code should stay unchanged. I am technically not against it being a struct, just not certain what the benefit is going to be. More type safety?

Also, is there going to be some kind of a design proposal for this issue? I.e. how interfaces would change with the transition and so on.

Not really - whoever works on this will need to go through what io/fs provides and what we currently use and bridge the gap somehow. If at that point they want to make a design proposal before publishing (or even working on the PR, if that is possible) - it's fine, but part of the work here is to actually come with what needs to be done, as is the case with most issues ;)

[yorugac]

More type safety?

That + better readability.

Actually, my question about design was mostly addressed to @olegbespalov since he's assigned here 🙂. I guess I should have tagged him explicitly, sorry for confusion.

[olegbespalov]

@yorugac @mstoykov, thanks for commenting!

Changing the map to a struct is also something that I thought about when I was investigating this task. From my side, I add to a struct another property that I believe is important. I'm talking about better maintainability.

Not really - whoever works on this will need to go through what io/fs provides and what we currently use and bridge the gap somehow.

I see that the io/fs for now provides more readers' interfaces. So I'm not sure it will be possible to fully migrate to the io/fs from afero. What still can (and I believe should be done) is refactor towards the smaller interfaces. For example, we mostly pass the afero.Fs downstream, but do we need the whole filesystem abstraction in all places? 🤔

Also, if it stays the way it's right now, the task itself seems to be a bit vague 🤔 Maybe it's possible to list our main pain points somehow or if it's not only the bugs but also the whys we want to migrate.

For example:

• afero.Fs is too huge and replacing it with something tinier makes the design better. It improves testing possibilities. Makes code more flexible and improves maintainability.

[mstoykov]

I see that the io/fs for now provides more readers' interfaces. So I'm not sure it will be possible to fully migrate to the io/fs from afero.

Even now we have code on top of afero.FS and we definitely will need at least part of it. The idea here though is that we won't use a dependency that we have issues with, which we need to work around.
Addionally the places where we write to the afero.Fs are :

1. tests where we arguably create a test fs to be tested against - which in practice is a memmapfs
2. we write the config to the fs - only really going through afero.fs for testability - can probably have localized implementation
3. creating the filesystems when we read them from the archive - again something like a memmapfs
4. A specific hack for source read from stdin no idea how exactly this will need to be fixed. Connected to k6 path resolution doesn't work from CWD when running from stdin #1462 and probably a lot more involved than it was originally thought
5. Caching https urls which as mentioned in that comment there likely will be a lot better as afero.FS implementaiton where we just Open/Read the file and it does the request. Which will make both file and https filesystem have the same structure: cachingFS->realFsReadingFromSomewhere where we in practice don't have the second case when running from an archive.

This definitely isn't a small amount of work, but arguably most of the changes will be ... in the tests :(.

do we need the whole filesystem abstraction in all places?

Probably not, we do use quite little of it directly either way. By which I mean that we don't need the whole afero.FS interface, but we likely always need a whole "filesystem".

Also, if it stays the way it's right now, the task itself seems to be a bit vague

the main point is we have problems with afero and it's not maintained. The issue isn't really ... high priority as we also don't really change this part of the code, but when we have to, different bugs in afero make it harder to be certain one of the many afero.FS implementations won't break something.

I guess we also don't need 90% of what it gives us. Along the way io/fs was introduced which gave us at least a basis for a solution so at least we don't have to spin a whole knew library on our own (hopefully 🤞 ).

As with most "we don't like this we probably should change it" issues, specifying how the final solution will work is 90% of the work.

I do think that it might be better if we(you :P) first remove all usages of afero outside of lib/fsext and use only fsext everywhere and then try to remove afero from lib/fsext as well. This likely will :

1. split the work in (at least) two PRs which will be easier to review
2. help identify what actually is used by afero and what needs to be written to get io/fs to do the work.
3. after the first transition the code will be a lot more localized as well and we can probably even try not io/fs solutions if for some reason that is preferable.

p.s. From my memories of this code (and some recent going through parts of it) - there are a lot of hairy places, unfortunately, so doing this in multiple passes/PRs will definitely be preferable as breaking any minor corner case here has big consequences and while there are a ton of tests, you will likely need to touch most of those as well :(