Remove LCOW (step 5): volumes/mounts: remove LCOW code (alternative)#42520
Remove LCOW (step 5): volumes/mounts: remove LCOW code (alternative)#42520cpuguy83 merged 7 commits intomoby:masterfrom
Conversation
64a4c5f to
e53c546
Compare
beac221 to
24e129a
Compare
f4623cf to
5fa7e7e
Compare
|
@tianon ptal if you like this approach better than the other one (i.e., keeping the functionality) |
|
😬 guess I need to update the commit message for the first commit (I was happy that it was green ✅ ); can do so if we decide to go the route in this PR. |
|
Yeah, I think this makes more sense if we plan to keep LCOW in a different form -- there was some debate on today's maintainers call as to whether that's even something we want, so I guess we need to decide on that before we can really say for sure whether we can just bring down the axe or whether we need to keep all three parsers. 😄 |
…formatting - Remove the windowsparser.HasResource() override, as it was the same on both Windows and Linux - Move the rxLCOWDestination to the lcowParser code - Move the rwModes variable to a generic (non-platform-specific) file, as it's used both for the windowsParser and the linuxParser - Some minor formatting and linting changes Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This changes mounts.NewParser() to create a parser for the current operatingsystem, instead of one specific to a (possibly non-matching, in case of LCOW) OS. With the OS-specific handling being removed, the "OS" parameter is also removed from `daemon.verifyContainerSettings()`, and various other container-related functions. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's only testing the LinuxParser, so moving it to a file specific to that code. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This adds constructors for the Linux, Windows, and LCOW, to allow using these parsers externally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This allows stubbing the provider for a test without side effects for other tests, making it no longer needed to reset it to its original value in a defer() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
5fa7e7e to
8e3f9fd
Compare
|
@tianon @AkihiroSuda @cpuguy83 I re-ordered some of the commits, and updated the PR description to explain what this PR does, and how it differs from the other PR. |
cpuguy83
left a comment
There was a problem hiding this comment.
I think it'll be easier to keep the the argument to NewParser... at least since we aren't removing this parsing I don't see the benefit in ripping out the parts that enable switching.
Do you want to keep all the code that passes it, or just that argument (and by default call it with Mostly I tried to get rid of all the |
|
ping @cpuguy83 ^^ could you explain what you wanted to see here? |
| // - Variation on hostdir but can be a drive followed by colon as well | ||
| // - If a path, must be absolute. Can include spaces | ||
| // - Drive cannot be c: (explicitly checked in code, not RegEx) | ||
| const rxLCOWDestination = `(?P<destination>/(?:[^\\/:*?"<>\r\n]+[/]?)*)` |
There was a problem hiding this comment.
This isn't new, so I'm not suggesting we change it now, but shouldn't this (and the friends it moved away from) be regexp.MustCompile'd?
There was a problem hiding this comment.
Yes, probably makes sense to do so (help me remind in case I forget)
|
I don't see any evidence that there will be a containerd LCOW implementation in the foreseeable future, so why not remove the code for now? |
|
@justincormack there was some back-and-forth on that. That said, the remaining code in this package is fairly limited, and is just what's in https://github.com/thaJeztah/docker/blob/remove_lcow_step5_alternative/volume/mounts/lcow_parser.go (and some tests in the |
relates to (part of) #42170
alternative for #42517
closes #42517
This alternative to #42517 preserves the LCOW parsing code for mounts (in case we will be using LCOW using containerd's LCOW implementation). By default, a native parser is now used (matching the host (and thus container's) platform.
After taking care of the above, tests have also be split to be per-driver, and some refactoring to improve the tests (being able to run them "parallel").
details below:
volume/mounts: move some code to correct location, and minor linting/formatting
Windows and Linux
used both for the windowsParser and the linuxParser
volume/mounts: remove "containerOS" argument from NewParser (LCOW code)
This changes mounts.NewParser() to create a parser for the current operatingsystem,
instead of one specific to a (possibly non-matching, in case of LCOW) OS.
With the OS-specific handling being removed, the "OS" parameter is also removed
from
daemon.verifyContainerSettings(), and various other container-relatedfunctions.
Split tests per parser
Have tests per-parser, instead of grouping all tests in a single test (with a
test-set for each parser inside that test). All tests have been kept, at the
cost of some duplication in the test-cases that overlapped / were the same
for multiple parsers.
Add constructors for each parser
This adds constructors for the Linux, Windows, and LCOW, to allow using these
parsers externally, instead of only through
NewParser().Don't use global variable for fileinfoprovider
The code used a global / package-level
currentFileInfoProviderthat was usedto stub the actual fileInfoProvider during tests. Using a package-level variable
for this could cause issues when trying to run tests in parallel, as well as
making the tests more complicated (tests had to reset the provider in a defer).
Now that we have a constructor for each driver, we can set up the fileinfoprovider
in those constructors, and stub the implementation for specific instances, instead
of globally.
Use sub-tests, and use gotest.tools
These tests were running many test-cases, which required a lot of glue code to
construct useful errors if "one of those" failed.
Using regular sub-tests instead to make debugging easier, and use gotest.tools
to take care of useful errors.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)