Skip to content

Move to supported boltdb#2634

Merged
crosbymichael merged 3 commits intocontainerd:masterfrom
lowenna:boltdb
Sep 14, 2018
Merged

Move to supported boltdb#2634
crosbymichael merged 3 commits intocontainerd:masterfrom
lowenna:boltdb

Conversation

@lowenna
Copy link
Copy Markdown

@lowenna lowenna commented Sep 11, 2018

Signed-off-by: John Howard jhoward@microsoft.com

This all relates to this underlying issue: moby/libnetwork#1950. While I have etcd-io/bbolt#122 pending being merged, it makes sense to move all projects which are dependencies of moby/moby from boltdb/bolt to go.etcd.io/bbolt so that there is a single database in use by all components.

These include containerd/containerd (this PR); docker/swarmkit; moby/buildkit; moby/moby; containerd/cri; docker/libkv (docker/libkv#202) and docker/libnetwork. (At least this is all I have found so far).

github.com/boltdb/bolt is no longer being maintained. From https://github.com/boltdb/bolt:

Unfortunately I no longer have the time or energy to continue this work. Bolt is in a stable state and has years of successful production use. As such, I feel that leaving it in its current state is the most prudent course of action.

If you are interested in using a more featureful version of Bolt, I suggest that you look at the CoreOS fork called bbolt.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 11, 2018

Codecov Report

Merging #2634 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2634      +/-   ##
==========================================
+ Coverage   43.76%   43.77%   +0.01%     
==========================================
  Files          94       94              
  Lines       10305    10307       +2     
==========================================
+ Hits         4510     4512       +2     
  Misses       5075     5075              
  Partials      720      720
Flag Coverage Δ
#linux 47.49% <100%> (+0.01%) ⬆️
#windows 40.43% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
snapshots/storage/bolt.go 59.71% <ø> (ø) ⬆️
metadata/namespaces.go 0% <ø> (ø) ⬆️
metadata/db.go 64.98% <ø> (ø) ⬆️
metadata/snapshot.go 45.8% <ø> (ø) ⬆️
metadata/containers.go 47.97% <ø> (ø) ⬆️
services/server/server.go 1.67% <ø> (ø) ⬆️
metadata/content.go 40.5% <ø> (ø) ⬆️
metadata/bolt.go 75% <ø> (ø) ⬆️
metadata/leases.go 44.27% <ø> (ø) ⬆️
metadata/gc.go 61.35% <ø> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66b984e...8e44270. Read the comment docs.

@dmcgowan
Copy link
Copy Markdown
Member

I am 👍 to updating this. Who can confirm that a file created with boltdb/bolt is compatible with this new version, we never supported the reverse downgrade case. We should also add an empty migration in the metadata database to bump the version.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 11, 2018

Who can confirm that a file created with boltdb/bolt is compatible with this new version, we never supported the reverse downgrade case.

Hmmmm. Good question, but I don't know. Perhaps @gyuho @xiang90 over at https://github.com/etcd-io/bbolt might know the answer.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 11, 2018

We should also add an empty migration in the metadata database to bump the version.

@dmcgowan I'm not sure what you mean here. Can you give me a clue and I can follow-up. Thanks,

@dmcgowan
Copy link
Copy Markdown
Member

@jhowardmsft add a version 3 here https://github.com/containerd/containerd/blob/master/metadata/migrations.go#L47 with a no-op migration and test case. This will allow us to tell if the file has been written to yet from the new library.

@dmcgowan dmcgowan added this to the 1.2 milestone Sep 12, 2018
@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 12, 2018

@dmcgowan That's added now.

@stevvooe
Copy link
Copy Markdown
Member

What features do we envision gaining from this change? Boltdb is extremely stable. I’d hate to have a bug because someone messed with something in bbolt.

I don’t see any reason to match deps with lib network. There should be no requirement for bolt in the client. containerd should only be compiled with the dependencies called out in the vendor file.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 12, 2018

@stevvooe Specifically etcd-io/bbolt#122 as I linked to in the PR description.

@xiang90
Copy link
Copy Markdown

xiang90 commented Sep 12, 2018

@jhowardmsft

We added a few new options, for example "no freelist persistence". However, if you do not change the db open options, there should not be a compatible problem if it is not a bug :P.

John Howard added 3 commits September 12, 2018 15:23
Signed-off-by: John Howard <jhoward@microsoft.com>
Signed-off-by: John Howard <jhoward@microsoft.com>

This is the maintained version of boltdb, which includes the Windows-specific fix
detailed in etcd-io/bbolt#122
Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 12, 2018

Thanks @xiang90.

I've bumped this PR to use v1.3.1-etcd.8 rather than .7. This includes the Windows-specific fix for database lock and unlock to be thread-safe (etcd-io/bbolt#122) which has been merged.

lowenna pushed a commit to microsoft/docker that referenced this pull request Sep 13, 2018
Signed-off-by: John Howard <jhoward@microsoft.com>

This also adds go.etcd.io/bbolt as boltdb/bolt is no longer
maintained, and we need etcd-io/bbolt#122 which
was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8
in order to fix moby/libnetwork#1950.

Note that I can't entirely remove boltdb/bolt as it is still used by
other components. Still need to work my way through them.... These include
containerd/containerd (containerd/containerd#2634),
docker/swarmkit; moby/buildkit. And probably more....
@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 13, 2018

@dmcgowan @crosbymichael Can we merge this? The dependency chain to resolve all the way through is getting complex - moby/buildkit for example vendors this repo, so I should really get this in first, before updating buildkit, before updating moby/moby and then finally resolving moby/moby#37843 (comment). We really should have all container related repos without the Windows thread safety bug in bolt. Thanks.

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit c95bb88 into containerd:master Sep 14, 2018
@lowenna
Copy link
Copy Markdown
Author

lowenna commented Sep 14, 2018

Thanks guys, one step closer to getting everything switched over 👍

@lowenna lowenna deleted the boltdb branch September 14, 2018 02:56
thaJeztah pushed a commit to thaJeztah/docker that referenced this pull request Sep 14, 2018
Signed-off-by: John Howard <jhoward@microsoft.com>

This also adds go.etcd.io/bbolt as boltdb/bolt is no longer
maintained, and we need etcd-io/bbolt#122 which
was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8
in order to fix moby/libnetwork#1950.

Note that I can't entirely remove boltdb/bolt as it is still used by
other components. Still need to work my way through them.... These include
containerd/containerd (containerd/containerd#2634),
docker/swarmkit; moby/buildkit. And probably more....

(cherry picked from commit 1a6e260)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Sep 14, 2018
Signed-off-by: John Howard <jhoward@microsoft.com>

This also adds go.etcd.io/bbolt as boltdb/bolt is no longer
maintained, and we need etcd-io/bbolt#122 which
was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8
in order to fix moby/libnetwork#1950.

Note that I can't entirely remove boltdb/bolt as it is still used by
other components. Still need to work my way through them.... These include
containerd/containerd (containerd/containerd#2634),
docker/swarmkit; moby/buildkit. And probably more....
Upstream-commit: 1a6e2609ead86144b75067bfe5154dad5e42d5cf
Component: engine
dims pushed a commit to dims/sys that referenced this pull request Oct 29, 2020
Signed-off-by: John Howard <jhoward@microsoft.com>

This also adds go.etcd.io/bbolt as boltdb/bolt is no longer
maintained, and we need etcd-io/bbolt#122 which
was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8
in order to fix moby/libnetwork#1950.

Note that I can't entirely remove boltdb/bolt as it is still used by
other components. Still need to work my way through them.... These include
containerd/containerd (containerd/containerd#2634),
docker/swarmkit; moby/buildkit. And probably more....
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants