Skip to content
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.

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

vendor: moby/sys mountinfo/v0.4.0 #41458

Merged
merged 3 commits into from
Nov 3, 2020
Merged

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 16, 2020

full diff: moby/sys@mountinfo/v0.1.3...mountinfo/v0.4.0

Note that this dependency uses submodules, providing "github.com/moby/sys/mount"
and "github.com/moby/sys/mountinfo". Our vendoring tool (vndr) currently doesn't
support submodules, so we vendor the top-level moby/sys repository (which contains
both) and pick the most recent tag, which could be either mountinfo/vXXX or
mount/vXXX.

github.com/moby/sys/mountinfo v0.4.0

Breaking changes:

  • PidMountInfo is now deprecated and will be removed before v1.0; users should switch to GetMountsFromReader

Fixes and improvements:

  • run filter after all fields are parsed
  • correct handling errors from bufio.Scan
  • documentation formatting fixes

github.com/moby/sys/mountinfo v0.3.1

  • mount: use MNT_* flags from golang.org/x/sys/unix on freebsd
  • various godoc and CI fixes
  • mountinfo: make GetMountinfoFromReader Linux-specific
  • Add support for OpenBSD in addition to FreeBSD
  • mountinfo: use idiomatic naming for fields

github.com/moby/sys/mountinfo v0.2.0

Bug fixes:

  • Fix path unescaping for paths with double quotes

Improvements:

  • Mounted: speed up by adding fast paths using openat2 (Linux-only) and stat
  • Mounted: relax path requirements (allow relative, non-cleaned paths, symlinks)
  • Unescape fstype and source fields
  • Documentation improvements

Testing/CI:

  • Unit tests: exclude darwin
  • CI: run tests under Fedora 32 to test openat2
  • TestGetMounts: fix for Ubuntu build system
  • Makefile: fix ignoring test failures
  • CI: add cross build

github.com/moby/sys/mount v0.1.1

https://github.com/moby/sys/releases/tag/mount%2Fv0.1.1

Improvements:

Testing/CI:

  • Unit tests: exclude darwin
  • Makefile: fix ignoring test failures
  • CI: add cross build

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 16, 2020
@thaJeztah
Copy link
Member Author

@kolyshkin @cpuguy83 PTAL

@thaJeztah
Copy link
Member Author

Ah, forgot to update x/sys;


[2020-09-16T10:25:54.222Z] vendor/github.com/moby/sys/mountinfo/mounted_linux.go:15:16: undefined: unix.Openat2
[2020-09-16T10:25:54.222Z] vendor/github.com/moby/sys/mountinfo/mounted_linux.go:15:50: undefined: unix.OpenHow
[2020-09-16T10:25:54.222Z] vendor/github.com/moby/sys/mountinfo/mounted_linux.go:24:13: undefined: unix.Openat2
[2020-09-16T10:25:54.222Z] vendor/github.com/moby/sys/mountinfo/mounted_linux.go:24:40: undefined: unix.OpenHow

@kolyshkin
Copy link
Contributor

Once moby will switch to golang 1.15 (#40353), I hope we can finally drop vndr and use go.mod. I worked on that (switching to go modules) more than a year ago and remember that replace directive and version specification features were not flexible enough -- I think/hope this is solved by now.

@thaJeztah thaJeztah force-pushed the bump_mountinfo branch 2 times, most recently from 187a841 to b6b850a Compare September 18, 2020 00:25
@thaJeztah
Copy link
Member Author

updated golang.org/x/sys; PTAL

@thaJeztah

This comment has been minimized.

@thaJeztah
Copy link
Member Author

I opened moby/sys#31 to fix the change in behavior

@kolyshkin
Copy link
Contributor

Looks like EnsureRemoveAll should be unix-specific, and for windows it should be an alias or wrapper of os.RemoveAll.

@kolyshkin
Copy link
Contributor

Looks like EnsureRemoveAll should be unix-specific, and for windows it should be an alias or wrapper of os.RemoveAll.

That's what I mean: #41478

WDYT @cpuguy83 @thaJeztah?

@cpuguy83
Copy link
Member

Agreed

@thaJeztah
Copy link
Member Author

Rebased; pls check if the remaining "stubs" for Windows look ok, or if we should refactor more to not have those in the windows codepath

@@ -253,7 +252,7 @@ func (daemon *Daemon) Cleanup(container *container.Container) {
logrus.Warnf("%s cleanup: failed to unmount secrets: %s", container.ID, err)
}

if err := mount.RecursiveUnmount(container.Root); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, now when I look at this line... isn't it supposed to be below container.UnmountVolumes?

cc @cpuguy83

Copy link
Contributor

Choose a reason for hiding this comment

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

This code was added by commit eaa5192, at that time container.UnmountVolumes was already there (below)

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpuguy83 could you have a look?

Copy link
Member

Choose a reason for hiding this comment

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

container.Root is /var/lib/docker/containers/<id>

Copy link
Contributor

Choose a reason for hiding this comment

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

What about volumes? I haven't checked but I suppose volumes are mounted under the container.Root, so it makes sense to have volumes unmounted first. Or am I missing something?

Surely it's a separate issue; just wondering.

pkg/system/rm_unix.go Outdated Show resolved Hide resolved
@thaJeztah thaJeztah force-pushed the bump_mountinfo branch 3 times, most recently from cf79fd5 to da9703c Compare September 23, 2020 08:28
@thaJeztah
Copy link
Member Author

@cpuguy83 @kolyshkin PTAL <3

@thaJeztah
Copy link
Member Author

@kolyshkin @cpuguy83 ptal

@thaJeztah
Copy link
Member Author

Test failure is a flaky test, let me kick it

func (v *localVolume) unmount() error {
if v.needsMount() {
if err := mount.Unmount(v.path); err != nil {
if mounted, mErr := mountinfo.Mounted(v.path); mounted || mErr != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I kill all those redundant checks already... Anyway, it's a separate issue.

@kolyshkin
Copy link
Contributor

So, maybe it's time to kill pkg/mount, rather than provide all those wrappers?

@thaJeztah
Copy link
Member Author

So, maybe it's time to kill pkg/mount, rather than provide all those wrappers?

Yeah, so my thinking was to keep it for this release (as it's the release in which we deprecate it), then remove for the next (allowing users one release cycle to migrate)

full diff: moby/sys@mountinfo/v0.1.3...mountinfo/v0.4.0

> Note that this dependency uses submodules, providing "github.com/moby/sys/mount"
> and "github.com/moby/sys/mountinfo". Our vendoring tool (vndr) currently doesn't
> support submodules, so we vendor the top-level moby/sys repository (which contains
> both) and pick the most recent tag, which could be either `mountinfo/vXXX` or
> `mount/vXXX`.

github.com/moby/sys/mountinfo v0.4.0
--------------------------------------------------------------------------------

Breaking changes:

- `PidMountInfo` is now deprecated and will be removed before v1.0; users should switch to `GetMountsFromReader`

Fixes and improvements:

- run filter after all fields are parsed
- correct handling errors from bufio.Scan
- documentation formatting fixes

github.com/moby/sys/mountinfo v0.3.1
--------------------------------------------------------------------------------

- mount: use MNT_* flags from golang.org/x/sys/unix on freebsd
- various godoc and CI fixes
- mountinfo: make GetMountinfoFromReader Linux-specific
- Add support for OpenBSD in addition to FreeBSD
- mountinfo: use idiomatic naming for fields

github.com/moby/sys/mountinfo v0.2.0
--------------------------------------------------------------------------------

Bug fixes:

- Fix path unescaping for paths with double quotes

Improvements:

- Mounted: speed up by adding fast paths using openat2 (Linux-only) and stat
- Mounted: relax path requirements (allow relative, non-cleaned paths, symlinks)
- Unescape fstype and source fields
- Documentation improvements

Testing/CI:

- Unit tests: exclude darwin
- CI: run tests under Fedora 32 to test openat2
- TestGetMounts: fix for Ubuntu build system
- Makefile: fix ignoring test failures
- CI: add cross build

github.com/moby/sys/mount v0.1.1
--------------------------------------------------------------------------------

https://github.com/moby/sys/releases/tag/mount%2Fv0.1.1

Improvements:

- RecursiveUnmount: add a fast path (moby#26)
- Unmount: improve doc
- fix CI linter warning on Windows

Testing/CI:

- Unit tests: exclude darwin
- Makefile: fix ignoring test failures
- CI: add cross build

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah changed the title vendor: moby/sys mountinfo/v0.3.1 vendor: moby/sys mountinfo/v0.4.0 Oct 29, 2020
@thaJeztah
Copy link
Member Author

updated to v0.4.0

@kolyshkin @cpuguy83 @AkihiroSuda PTAL

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda merged commit 0b93c6e into moby:master Nov 3, 2020
@thaJeztah thaJeztah deleted the bump_mountinfo branch November 3, 2020 10:15
@TBBle
Copy link
Contributor

TBBle commented Nov 5, 2020

It seems this PR broke the Windows unit tests, and in turn revealed that the Windows CI pipeline passes if the unit tests fail, as it somehow still exits with '0'. I'm looking into this now as I wanted to add a new Windows-specific test.

@thaJeztah
Copy link
Member Author

Arf.. we had a tracking issue for the false reports in #39576, but thought it was fixed; apparently not 😞

@TBBle
Copy link
Contributor

TBBle commented Nov 5, 2020

I suspect the fix for #39576 fixed the integration tests, but the unit tests are exiting 1 at one point, but still seeing an exit 0 later, which is what goes to Jenkins.

@thaJeztah
Copy link
Member Author

opened #41637 to move the parts that were undefined on windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants