Skip to content

Remove LCOW (step 5): volumes/mounts: remove LCOW code (alternative)#42520

Merged
cpuguy83 merged 7 commits intomoby:masterfrom
thaJeztah:remove_lcow_step5_alternative
Jul 26, 2021
Merged

Remove LCOW (step 5): volumes/mounts: remove LCOW code (alternative)#42520
cpuguy83 merged 7 commits intomoby:masterfrom
thaJeztah:remove_lcow_step5_alternative

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jun 14, 2021

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

  • 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

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-related
functions.

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 currentFileInfoProvider that was used
to 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)

@thaJeztah thaJeztah force-pushed the remove_lcow_step5_alternative branch 2 times, most recently from 64a4c5f to e53c546 Compare June 22, 2021 15:29
@thaJeztah thaJeztah force-pushed the remove_lcow_step5_alternative branch 7 times, most recently from beac221 to 24e129a Compare June 29, 2021 20:53
@thaJeztah thaJeztah changed the title [WIP] Remove LCOW (step 5): volumes/mounts: remove LCOW code Remove LCOW (step 5): volumes/mounts: remove LCOW code (alternative) Jun 29, 2021
@thaJeztah thaJeztah marked this pull request as ready for review June 30, 2021 07:29
@thaJeztah thaJeztah force-pushed the remove_lcow_step5_alternative branch 2 times, most recently from f4623cf to 5fa7e7e Compare June 30, 2021 07:53
@thaJeztah
Copy link
Copy Markdown
Member Author

@tianon ptal if you like this approach better than the other one (i.e., keeping the functionality)

@thaJeztah
Copy link
Copy Markdown
Member Author

😬 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.

@tianon
Copy link
Copy Markdown
Member

tianon commented Jul 1, 2021

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. 😄

thaJeztah added 7 commits July 2, 2021 13:25
…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>
@thaJeztah thaJeztah force-pushed the remove_lcow_step5_alternative branch from 5fa7e7e to 8e3f9fd Compare July 2, 2021 12:14
@thaJeztah thaJeztah added area/lcow Issues and PR's related to the experimental LCOW feature impact/deprecation kind/refactor PR's that refactor, or clean-up code status/2-code-review labels Jul 2, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Jul 2, 2021
@thaJeztah
Copy link
Copy Markdown
Member Author

@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.

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thaJeztah
Copy link
Copy Markdown
Member Author

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 runtime.GOOS)?

Mostly I tried to get rid of all the if foo == "windows" and if container.OS != runtime.GOOS code that touches every bit of the code.

@thaJeztah
Copy link
Copy Markdown
Member Author

ping @cpuguy83 ^^ could you explain what you wanted to see here?

@thaJeztah
Copy link
Copy Markdown
Member Author

@tianon @cpuguy83 - per our discussion in the maintainers meeting; this LGTY?

Copy link
Copy Markdown
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// - 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]+[/]?)*)`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably makes sense to do so (help me remind in case I forget)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#42667 😏 😇

@justincormack
Copy link
Copy Markdown
Contributor

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?

@thaJeztah
Copy link
Copy Markdown
Member Author

@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 lcow_parser_test.go)

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/lcow Issues and PR's related to the experimental LCOW feature impact/deprecation kind/experimental kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants