Skip to content

Remove LCOW (step 1)#42451

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:remove_lcow_step1
Jun 8, 2021
Merged

Remove LCOW (step 1)#42451
cpuguy83 merged 1 commit intomoby:masterfrom
thaJeztah:remove_lcow_step1

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jun 1, 2021

taken from #42170, but splitting into smaller chunks

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 (soon to be part of Microsoft/hcshhim),
which means that we need to start removing this code.

This first step removes the lcow graphdriver, the LCOW initialization code, and
some LCOW-related utilities.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review impact/changelog 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 Jun 1, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Jun 1, 2021
@thaJeztah thaJeztah marked this pull request as ready for review June 1, 2021 12:42
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 1, 2021

relates to #39533

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 1, 2021

@AkihiroSuda @tonistiigi @cpuguy83 ptal

AkihiroSuda
AkihiroSuda reviewed Jun 1, 2021
View reviewed changes
project/PACKAGERS.md Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Jun 1, 2021

Choose a reason for hiding this comment

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

(Off-topic: PACKAGERS.md seems completely outdated 😅)

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jun 1, 2021

Choose a reason for hiding this comment

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

Yes, it needs to be cleaned up

AkihiroSuda
AkihiroSuda approved these changes Jun 1, 2021
View reviewed changes
pkg/system/path_windows.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Jun 1, 2021

Choose a reason for hiding this comment

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

Can we just export this as CheckSystemDriveAndRemoveDriveLetter?

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Jun 1, 2021

Choose a reason for hiding this comment

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

We could, but the downside is we'll have to maintain GoDoc for both Windows and Linux separately, plus making sure that the signatures are in sync; having the "exported" version in a shared file, and the implementations separate makes sure we don't diverge.

@thaJeztah
Remove LCOW code (step 1)
e047d98 
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 (soon to be part of Microsoft/hcshhim),
which means that we need to start removing this code.

This first step removes the lcow graphdriver, the LCOW initialization code, and
some LCOW-related utilities.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the remove_lcow_step1 branch from ad53489 to e047d98 Compare June 3, 2021 19:16
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 3, 2021

this one is failing on windows (flaky test: #42459)

=== RUN   TestNetworkDBIslands

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 5, 2021

Failures on arm64:

TestConcurrencyNoWait tracked through #42458

=== RUN   TestConcurrencyNoWait
--- FAIL: TestConcurrencyNoWait (0.16s)
    iptables_test.go:217:  (iptables failed: iptables -t nat -A DOCKEREST -p tcp -d 192.168.1.1 --dport 1234 -j DNAT --to-destination 172.17.0.1:4321 ! -i lo: Another app is currently holding the xtables lock. Perhaps you want to use the -w option?
         (exit status 4))

TestExistsRaw created tracking issue: #42469

=== RUN   TestExistsRaw
--- FAIL: TestExistsRaw (0.10s)
    iptables_test.go:288: Failed to detect rule. i=2

Failure on amd64

TestDiff created tracking issue #42470

=== RUN   TestDiff
--- FAIL: TestDiff (1.03s)
    diff_test.go:42: assertion failed: 
        --- expected
        +++ items
        {[]container.ContainerChangeResponseItem}:
        	-: []container.ContainerChangeResponseItem{{Kind: 0x01, Path: "/foo"}, {Kind: 0x01, Path: "/foo/bar"}}
        	+: []container.ContainerChangeResponseItem(nil)

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 5, 2021

This one failed again (on arm64)

=== RUN   TestExistsRaw
--- FAIL: TestExistsRaw (0.04s)
    iptables_test.go:288: Failed to detect rule. i=2

Otherwise everything is green

@cpuguy83 @tianon ptal

tianon
tianon approved these changes Jun 7, 2021
View reviewed changes
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.

Just one minor question -- overall LGTM

The following (deleted) comment was helpful context for me while reviewing this PR:

// ValidatePlatform determines if a platform structure is valid. This function
// is used for LCOW, and is a no-op on non-windows platforms.

(It's about halfway down the diff, but it's deleted in a lot of places and IMO this comment helps explain why so it's the best place to start. 😄)

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jun 8, 2021

It's ✅ green again!

@tianon tianon mentioned this pull request Jun 8, 2021
Merged
cpuguy83
cpuguy83 approved these changes Jun 8, 2021
View reviewed changes
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

@cpuguy83 cpuguy83 merged commit a7ea29a into moby:master Jun 8, 2021
@thaJeztah thaJeztah deleted the remove_lcow_step1 branch June 8, 2021 20:51
@thaJeztah thaJeztah mentioned this pull request Jul 29, 2021
Merged
@thaJeztah thaJeztah changed the title Remove LCOW code (step 1) Remove LCOW (step 1) Jul 29, 2021
@jhuntwork jhuntwork mentioned this pull request Dec 5, 2022
Closed
@thaJeztah thaJeztah mentioned this pull request Jul 12, 2023
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@johnstep johnstep Awaiting requested review from johnstep

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @tianon @cpuguy83 @AkihiroSuda