Skip to content

WIP: Remove legacy LCOW code #42170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
thaJeztah wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

WIP: Remove legacy LCOW code #42170

thaJeztah wants to merge 2 commits into from
+39 −58

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Mar 19, 2021 •
edited
Loading

The LCOW implementation in dockerd has been deprecated in favor of re-implementation in containerd (in progress). Microsoft started removing the LCOW V1 code from the build dependencies we use in Microsoft/opengcs in microsoft/opengcs@e972b27 (microsoft/opengcs#390), soon to be part of Microsoft/hcshhim (microsoft/hcsshim#973).

The (pre microsoft/opengcs@e972b27) version of opengcs we need for LCOW v1 is no longer compatible with current versions of runc/libcontainer (see microsoft/opengcs#396), which means that we wouldn't be able to update our runtime (containerd, runc) dependencies without breaking LCOW v1.

With the above; and given that LCOW has been deprecated (but so far, not actively removed), we should make a start to remove this code.

WARNING:

I should be clear that this is not ready to be merged, and a really WIP branch. This branch is just "blindly" ripping out all LCOW-related code/code-paths. See it as a "what if we removed all bits.." to make visible where the LCOW code-paths live, and so that we can discuss what parts should be removed, and possibly, what parts should be kept (For example, if we want to support LCOW based on containerd, possibly some of this code is still useful)

@thaJeztah thaJeztah added status/2-code-review impact/deprecation area/lcow Issues and PR's related to the experimental LCOW feature kind/refactor PR's that refactor, or clean-up code labels Mar 19, 2021
@thaJeztah thaJeztah force-pushed the remove_lcow branch 3 times, most recently from 6aef7fc to 39ea4ba Compare March 19, 2021 16:03
@thaJeztah thaJeztah changed the title WIP: Remove defunct LCOW code WIP: Remove legacy LCOW code Mar 19, 2021
@thaJeztah thaJeztah force-pushed the remove_lcow branch 3 times, most recently from 5cf1d4e to 44a7424 Compare March 19, 2021 17:17
@thaJeztah
Copy link
Member Author

thaJeztah commented Mar 19, 2021

hmm.. flaky test?

[2021-03-19T17:39:38.015Z] === Failed
[2021-03-19T17:39:38.015Z] === FAIL: daemon/logger TestCopierWithSized/With_RingLogger (0.00s)
[2021-03-19T17:39:38.015Z]     --- FAIL: TestCopierWithSized/With_RingLogger (0.00s)
[2021-03-19T17:39:38.015Z]         copier_test.go:265: invalid character '\x00' looking for beginning of value
[2021-03-19T17:39:38.015Z] 
[2021-03-19T17:39:38.015Z] === FAIL: daemon/logger TestCopierWithSized (0.00s)

@thaJeztah thaJeztah mentioned this pull request Jun 1, 2021
Merged
@thaJeztah thaJeztah mentioned this pull request Jun 9, 2021
Merged
This was referenced Jun 10, 2021
Merged
Merged
@thaJeztah thaJeztah force-pushed the remove_lcow branch 3 times, most recently from 53b460c to 5b2fad2 Compare June 11, 2021 19:16
@thaJeztah thaJeztah mentioned this pull request Jun 11, 2021
Closed
@thaJeztah thaJeztah mentioned this pull request Jul 2, 2021
Merged
@thaJeztah thaJeztah force-pushed the remove_lcow branch 4 times, most recently from 41ba270 to 404655e Compare July 27, 2021 11:31
@thaJeztah thaJeztah mentioned this pull request Jul 27, 2021
Merged
This was referenced Jul 27, 2021
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 29, 2021
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 9, 2021
Merged
@thaJeztah thaJeztah force-pushed the remove_lcow branch 2 times, most recently from af2e6fa to ddeb49e Compare August 9, 2021 09:32
@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 9, 2021 •
edited
Loading

Two failures that look related, but will have to look if the test may be wrong, or if it's an existing issue that wasn't hit before (possibly if base-image was "from scratch", it would be testing for Linux?)

=== Failed
=== FAIL: github.com/docker/docker/builder/dockerfile TestCmd (0.00s)
    dispatchers_test.go:272: assertion failed: 
        --- expectedCommand
        +++ sb.state.runConfig.Cmd
          strslice.StrSlice{
        - 	"cmd",
        + 	"cmd /S /C ",
        - 	"/S",
        - 	"/C",
        - 	"./executable",
          }
        

=== FAIL: github.com/docker/docker/builder/dockerfile TestEntrypoint (0.00s)
    dispatchers_test.go:329: assertion failed: 
        --- expectedEntrypoint
        +++ sb.state.runConfig.Entrypoint
          strslice.StrSlice{
        - 	"cmd",
        + 	"cmd /S /C ",
        - 	"/S",
        - 	"/C",
        - 	"/usr/sbin/nginx",
          }

@thaJeztah thaJeztah mentioned this pull request Mar 11, 2022
Merged
thaJeztah
thaJeztah commented Aug 31, 2022
View reviewed changes
builder/dockerfile/dispatchers_windows.go
return []string{}, runConfig.ArgsEscaped
}

if os == "windows" { // ie WCOW
Copy link
Member Author

@thaJeztah thaJeztah Aug 31, 2022

Choose a reason for hiding this comment

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

Maybe this is the cause of the failure; could this be a "from scratch" case?

@thaJeztah thaJeztah mentioned this pull request Jul 12, 2023
Closed
15 tasks
@thaJeztah thaJeztah mentioned this pull request Nov 15, 2024
Merged
@thaJeztah thaJeztah mentioned this pull request Dec 23, 2024
Merged
@thaJeztah
builder/dockerfile: remove os parameter for default Shells (LCOW)
e088609 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Fix TestCmd, TestEntrypoint
53e4b26 
These tests were previously using the linux format, probably
because the image is empty

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@olljanat olljanat mentioned this pull request Jul 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi will be requested when the pull request is marked ready for review tonistiigi is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@thaJeztah