Skip to content

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

Merged
AkihiroSuda merged 3 commits intomoby:masterfrom
thaJeztah:bump_mountinfo
Nov 3, 2020
Merged

vendor: moby/sys mountinfo/v0.4.0#41458
AkihiroSuda merged 3 commits intomoby:masterfrom
thaJeztah:bump_mountinfo

Conversation

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown
Member Author

@kolyshkin @cpuguy83 PTAL

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

updated golang.org/x/sys; PTAL

@thaJeztah

This comment has been minimized.

@thaJeztah
Copy link
Copy Markdown
Member Author

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

@kolyshkin
Copy link
Copy Markdown
Contributor

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

@kolyshkin
Copy link
Copy Markdown
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
Copy Markdown
Member

Agreed

@thaJeztah
Copy link
Copy Markdown
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

Comment thread daemon/start.go Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread pkg/system/rm_unix.go Outdated
Comment thread testutil/daemon/daemon.go Outdated
Comment thread volume/local/local.go Outdated
@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
Copy Markdown
Member Author

@cpuguy83 @kolyshkin PTAL <3

@thaJeztah thaJeztah changed the title vendor: bump moby/sys mountinfo/v0.3.1 vendor: moby/sys mountinfo/v0.3.1 Oct 6, 2020
Comment thread pkg/mount/deprecated.go Outdated
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.

Wasn't sure if we wanted to keep this one backward compatible for now; kept it as a separate commit, but can squash it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since mountinfo filters were introduced recently, I don't think they have lots of users so we can just remove those from this wrapper package, and if GetMounts is called with a filter, do panic("use github.com/moby/sys/mountinfo") or something like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or maybe it's time to kill the whole package

@thaJeztah
Copy link
Copy Markdown
Member Author

@kolyshkin @cpuguy83 ptal

@thaJeztah
Copy link
Copy Markdown
Member Author

Test failure is a flaky test, let me kick it

Comment thread volume/local/local_unix.go Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown
Member Author

updated to v0.4.0

@kolyshkin @cpuguy83 @AkihiroSuda PTAL

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@TBBle
Copy link
Copy Markdown
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
Copy Markdown
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