Remove LCOW: pkg/containerfs: drop ContainerFS abstraction#44191
Remove LCOW: pkg/containerfs: drop ContainerFS abstraction#44191thaJeztah merged 11 commits intomoby:masterfrom
Conversation
The Driver interface was required for Linux Containers on Windows, which is no longer supported. Signed-off-by: Cory Snider <[email protected]>
Signed-off-by: Cory Snider <[email protected]>
Signed-off-by: Cory Snider <[email protected]>
They were needed for Linux Containers on Windows, which is no longer supported. Signed-off-by: Cory Snider <[email protected]>
With LCOW support removed, there is no need to support non-native file paths any longer. Signed-off-by: Cory Snider <[email protected]>
The Driver abstraction was needed for Linux Containers on Windows, support for which has since been removed. There is no direct equivalent to Lchmod() in the standard library so continue to use the containerd/continuity version. Signed-off-by: Cory Snider <[email protected]>
corhere
requested review from
cpuguy83,
johnstep and
tonistiigi
as code owners
corhere
added
the
area/lcow
Iterate towards dropping the type entirely. Signed-off-by: Cory Snider <[email protected]>
Drop the constructor and redundant string() type-casts. Signed-off-by: Cory Snider <[email protected]>
Signed-off-by: Cory Snider <[email protected]>
Now that the type of Container.BaseFS has been reverted to a string, values can never implement the extractor or archiver interfaces. Rip out the dead code to support archiving and unarchiving through those interfcaes. Signed-off-by: Cory Snider <[email protected]>
The TODO comment was in regards to allowing graphdriver plugins to provide their own ContainerFS implementations. The ContainerFS interface has been removed from Moby, so there is no longer anything which needs to be figured out. Signed-off-by: Cory Snider <[email protected]>
corhere
force-pushed
the
drop-containerfs-iface
branch
from
6b49e7a to
a5be811
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM! That's sooooo much cleaner ❤️
| // dest.path must be used because destPath has already been cleaned of any | ||
| // trailing slash | ||
| if endsInSlash(dest.root, dest.path) || destExistsAsDir { | ||
| if endsInSlash(dest.path) || destExistsAsDir { |
There was a problem hiding this comment.
In a follow-up, we can probably get rid of the endsInSlash() func and inline it; looks like this is the only location where it's used
thaJeztah
added
status/2-code-review
kind/refactor
tianon
left a comment
tianon
left a comment
There was a problem hiding this comment.
Looks good!
Reviewing commit-by-commit, it was a little weird to see it convert to a string type, then touch all the same lines to turn it into an alias, then remove it, but it made the progression of the removal really clear. 👍
Yes, was also considering if squashing was worth it, but it definitely makes the history more descriptive, so I think it's worth the trade-off of the extra code churn 👍 |
|
Let me bring this one in 👍 I did a quick check, and #44196 (which we may consider backporting) doesn't conflict with this one, so no issues on that front. |
4 participants
The
containerfs.ContainerFSandcontainerfs.Driverinterfaces were added in #34252 in order to support container root filesystems mounted in remote and foreign environments, specifically to support Linux Containers on Windows. LCOW support has been dropped from Moby, and with it the need to support container root filesystems mounted outside of the host's native filesystem hierarchy. Thecontainerfsinterfaces have served their purpose but are no longer needed. Retaining support for remote and foreign container filesystems is getting in the way of fixing bugs and making enhancements, so it is time to retire the abstraction.- What I did
Reverted large parts of #34252 by replacing uses of the
containerfs.ContainerFSandcontainerfs.Driverinterfaces with native filesystem and path manipulation APIs.- How I did it
golang.org/x/tools/cmd/eg,
gofmt -rand manual search-and-replace.- How to verify it
CI 🟢
- Description for the changelog
N/A. Not a user-visible change.
- A picture of a cute animal (not mandatory but encouraged)