lib/fs: Add test reproducing missing mtimefs issue (ref #9677) #9687
Conversation
43a1a29 to
0773582
Compare
lib/fs/fakefs.go
Outdated
| return os.ErrNotExist | ||
| } | ||
| if fs.timePrecisionSecond { | ||
| mtime = mtime.Round(time.Second) |
There was a problem hiding this comment.
If I were a file system this would be truncate and not round, but I guess as this is the more evil behavior it might be better in fakefs :p
lib/fs/filesystem_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestRepro9677(t *testing.T) { |
There was a problem hiding this comment.
Pet peeve, explain what the test is supposed to test :)
There was a problem hiding this comment.
Added something to the name that could give a hint, and wrote a comment trying to explain it.
Doing so I noticed another thing: We create an FS key for cached FS that includes options. So a FS with and without mtimefs should produce distinct keys, preventing everything we saw?! Clearly I am either misunderstanding what that key mechanism does or it's broken. I'll have a look at that too.
There was a problem hiding this comment.
Yeah, mtime FS "swallows" it's own option, thus it's not included in the cache key. So instead of only caching the cache, I could also fix the cache key, in one of two ways (or both): Include the types of underlying/wrapped filesystems in the key. And/or add the mtime option to the Filesystem.Options result. I'll probably do both regardless, seems like the right thing to do.
Or no, neither. Adding an option for the mtime FS wrapper is inconsistent, we don't do it for other wrappers either. And considering wrapped filesystems in the case FS cache makes sense, but I prefer not caching the filesystem at all, rendering the change moot.
0773582 to
4b9e37f
Compare
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in #9687, and I intend to redo the caseFilesystem reordering after this. Ref: #9677 Followup to: #9687
…hing#9701) This would have addressed a recent issue that arose when re-ordering our "filesystem layers". Specifically moving the caseFilesystem to the outermost layer. The previous cache included the filesystem, and as such all the layers below. This isn't desirable (to put it mildly), as you can create different variants of filesystems with different layers for the same path and options. Concretely this did happen with the mtime layer, which isn't always present. A test for the mtime related breakage was added in syncthing#9687, and I intend to redo the caseFilesystem reordering after this. Ref: syncthing#9677 Followup to: syncthing#9687
The test is quite odd and specific, but it does reproduce the issue that caused #9677, so I'd propose to add it to have a simple regression test for the basic scenario. Also the option to the fakefs might come handy for other scenarios where you want to quickly test some behaviour on a filesystem without nanosecond precision, without actually needing access to one.