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.

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

Dockerfile*: bump devmapper library version #34362

Merged
merged 1 commit into from
Aug 14, 2017
Merged

Dockerfile*: bump devmapper library version #34362

merged 1 commit into from
Aug 14, 2017

Conversation

kolyshkin
Copy link
Contributor

Let's use latest lvm2 sources to compile the libdevmapper library.

Initial reason for compiling devmapper lib from sources was a need to
have the static version of the library at hand, in order to build
the static dockerd, but note that the same headers/solib are used
for dynamic build (dynbinary) as well.

The reason for this patch is to enable the deferral removal feature.
The supplied devmapper library (and headers) are too old, lacking the
needed functions, so the daemon is built with 'libdm_no_deferred_remove'
build tag (see the check in hack/make.sh). Because of this, even if the
kernel dm driver is perfectly able to support the feature, it can not
be used. For more details and background story, see #34298

Surely, one can't just change the version number. While at it:

  • improve the comments;
  • remove obsoleted URLs;
  • remove s390 and ppc configure updates that are no longer needed;
  • use pkg-config instead of hardcoding the flags (newer lib added
    some more dependencies).

PS the version chosen is the one that is supposed to be bundled in RHEL7u4

@kolyshkin
Dockerfile*: bump devmapper library version
c4fde49 
Let's use latest lvm2 sources to compile the libdevmapper library.

Initial reason for compiling devmapper lib from sources was a need to
have the static version of the library at hand, in order to build
the static dockerd, but note that the same headers/solib are used
for dynamic build (dynbinary) as well.

The reason for this patch is to enable the deferral removal feature.
The supplied devmapper library (and headers) are too old, lacking the
needed functions, so the daemon is built with 'libdm_no_deferred_remove'
build tag (see the check in hack/make.sh). Because of this, even if the
kernel dm driver is perfectly able to support the feature, it can not
be used. For more details and background story, see [1].

Surely, one can't just change the version number. While at it:
 - improve the comments;
 - remove obsoleted URLs;
 - remove s390 and ppc configure updates that are no longer needed;
 - use pkg-config instead of hardcoding the flags (newer lib added
   some more dependencies);

 [1] moby#34298

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@justincormack
Copy link
Contributor

The Z failure seems unrelated.

@vrothberg
Copy link

Thanks for this patch! I was about to prepare a similar update, but mostly motivated by security. The logging of libdm seems to be more verbose as well, which is good as those logs are parsed to detect errors.

dnephin
dnephin approved these changes Aug 2, 2017
View reviewed changes
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin changed the title [WIP] Dockerfile*: bump devmapper library version Dockerfile*: bump devmapper library version Aug 3, 2017
@kolyshkin
Copy link
Contributor Author

OK so I removed the [WIP], guess it's ready to be merged

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 5, 2017

Anybody test this with Docker?
d4mac doesn't have dm support, if someone else could fire it up and run just make sure basic functionality still works fine that'd be great.

@kolyshkin
Copy link
Contributor Author

if someone else could fire it up and run just make sure basic functionality still works fine that'd be great.

Are there any test suites or something for graph drivers? The ones I used back in the day (when working on ploop graphdriver) are:

One good stress test creating and removing many images in parallel is
github.com/crosbymichael/docker-stress:

go get github.com/crosbymichael/docker-stress
docker-stress --containers 50 -c 200 -k 3s

Another stress test used is github.com/spotify/docker-stress.

@justincormack justincormack mentioned this pull request Aug 7, 2017
Closed
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Aug 11, 2017 •
edited
Loading

I checked that dockerd with this patch works manually, as well as with docker-stress, using devmapper on a real device.

kir@kd:~/go/src/github.com/docker/docker$ docker-stress -c 10 -k 3s
INFO[0749] ran 1000 containers in 749.47 seconds (1.33 container per sec. or 0.75 sec. per container) 
INFO[0749] failures 0

Also, deferred removal now works by default:

docker info
...
Storage Driver: devicemapper
 Pool Name: docker-thinpool
 Pool Blocksize: 524.3kB
 Base Device Size: 10.74GB
 Backing Filesystem: xfs
 Data file: 
 Metadata file: 
 Data Space Used: 14.37GB
 Data Space Total: 32.41GB
 Data Space Available: 18.04GB
 Metadata Space Used: 8.888MB
 Metadata Space Total: 339.7MB
 Metadata Space Available: 330.9MB
 Thin Pool Minimum Free Space: 3.241GB
 Udev Sync Supported: true
 Deferred Removal Enabled: true
 Deferred Deletion Enabled: true
 Deferred Deleted Device Count: 0
 Library Version: 1.02.136 (2016-11-05)
...

Just for the reference, here is the same test using overlayfs gd:

kir@kd:~/go/src/github.com/docker/docker$ docker-stress -c 10 -k 3s
INFO[0370] ran 1000 containers in 370.47 seconds (2.70 container per sec. or 0.37 sec. per container) 
INFO[0370] failures 0

So, I guess @cpuguy83 you can consider it lightly tested, no apparent bugs.

@kolyshkin
Copy link
Contributor Author

I just realized the above test does not cover building images, so I did some more testing -- this time by running docker build on top of it, with some docker-stress jobs in parallel. The beast survives.

@thaJeztah
Copy link
Member

@cpuguy83 think it's ready to merge?

cpuguy83
cpuguy83 approved these changes Aug 14, 2017
View reviewed changes
Copy link
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 31d8d22 into moby:master Aug 14, 2017
@thaJeztah thaJeztah mentioned this pull request Jul 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@justincormack justincormack justincormack approved these changes

@dnephin dnephin dnephin approved these changes

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

Assignees
No one assigned
Labels
area/storage/devicemapper status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kolyshkin @justincormack @vrothberg @cpuguy83 @thaJeztah @dnephin @GordonTheTurtle