Extend driver.ListLayers() #1563
Conversation
nalind
commented
Apr 10, 2023
•
nalind
commented
- Implement ListLayers() for the aufs, btrfs, and devicemapper drivers, along with a unit test for them.
- Stop filtering out directories with names that aren't 64-hex chars the overlay and vfs ListLayers() implementations, which is more a convention than a hard rule.
- Close() a dangling ReadCloser in NaiveCreateFromTemplate.
- Have layerStore.Wipe() try to remove remaining listed layers after it removes the layers that the layerStore knew of.
- Switch from using plain defer to using t.Cleanup() to handle deleting layers that tests create, have the addManyLayers() test function do so as well.
- Remove vfs.CopyDir, which near as I can tell isn't referenced anywhere.
nalind
force-pushed
the
listlayers
branch
2 times, most recently
from
ee6092a
to
922af86
Compare
|
LGTM |
nalind
force-pushed
the
listlayers
branch
4 times, most recently
from
f4d8893
to
1166982
Compare
|
the test failure seems related to the PR |
nalind
force-pushed
the
listlayers
branch
3 times, most recently
from
6ca0e65
to
972f7c7
Compare
nalind
force-pushed
the
listlayers
branch
9 times, most recently
from
e702ebc
to
abb0f09
Compare
drivers/graphtest/testutil.go
Outdated
| // error is returned | ||
| func removeLayer(t testing.TB, driver graphdriver.Driver, name string) { | ||
| err := driver.Remove(name) | ||
| for i := 100 * time.Millisecond; i <= 1000*time.Millisecond && err != nil && errors.Is(err, syscall.EBUSY); i += (100 * time.Millisecond) { |
There was a problem hiding this comment.
(I’m totally unfamiliar with this) BTW is it understood why retries are necessary? I’d naively expect the tests to be one of the more deterministic environments with no unexpected concurrent accessors, so what is keeping the volume busy if it’s not the tests?
Are the tests somehow triggering hard-to-cleanup concurrent goroutines/threads/processes that they don’t wait for, maybe?
There was a problem hiding this comment.
And if it is understood, it might be worth documenting somewhere around here.
There was a problem hiding this comment.
In isolated testing, I'd occasionally see the devicemapper driver returning EBUSY when we tried to remove layers, though this may have only been happening when deferred deletion wasn't being used. I'll see what happens if I remove that logic, and just have this helper expect success.
There was a problem hiding this comment.
There’s enough mentions of EBUSY throughout the codebase that I totally believe that there’s something (or multiple somethings) going on — I just could never find any specific description of what is actually the reason. The closest is #719 / moby/moby#36438 , which also isn’t really specific (and seems to suggest that the error is a genuine external user, not something that could be successfully retried).
So I’m mostly trying to get as much of the knowledge, if any, documented and explicit.
If this is working as designed and just requires a retry for good reasons, great, let’s document that.
If we know what is not working as designed, but needs a workaround short-term, fine, let’s document that.
If the cause is unknown but we know that something is failing, oh well, documenting that would still be nice.
If we are lucky, somebody will, sometime, find and fix the cause, and then look at this kind of loop and wonder whether it can be removed — and documenting why it was added would have been helpful.
Implement ListLayers() for the aufs, btrfs, and devicemapper drivers, along with a unit test for them. Stop filtering out directories with names that aren't 64-hex chars in vfs and overlay ListLayers() implementations, which is more a convention than a hard rule. Have layerStore.Wipe() try to remove remaining listed layers after it removes the layers that the layerStore knew of. Close() a dangling ReadCloser in NaiveCreateFromTemplate. Switch from using plain defer to using t.Cleanup() to handle deleting layers that tests create, have the addManyLayers() test function do so as well. Remove vfs.CopyDir, which near as I can tell isn't referenced anywhere. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
/approve |
