Bump to latest libkv + dependencies by dhiltgen · Pull Request #16863 · moby/moby

dhiltgen · 2015-10-08T21:16:13Z

The latest libkv uses a different etcd library. Unfortunately
that library uses some funky import paths, so I've added a new cleanup
routine for our vendor scripts to be able to normalize the imports
to be consistent with how imports work in this tree.

Signed-off-by: Daniel Hiltgen [email protected]

icecrime · 2015-10-08T23:11:43Z

To summarize the discussions here:

The etcd client library we currently use is deprecated and has known issues (I'm told potential crashes due to lack of thread-safety).
This PR proposes to upgrade to the newer, supported, etcd client library, which only drawback is to pull a 40k+ LoC dependency with it (https://github.com/ugorji/go).
Most likely, the future etcd API v3 will be protobuf based (a dependency we already have), won't require this anymore, and should be available in time for Docker 1.10.

That is to say that I feel the current choice is:

Merge this PR in order to use the more robust client package, and close our eyes on the amount of code it requires. Most likely, this will all be cleaned up for 1.10 with the newer package.
Don't merge this PR, knowing that the daemon might crash when --cluster-store=etcd:// is used, and wait for etcd API v3 to upgrade.

I vote option 1.

jessfraz · 2015-10-08T23:27:53Z

🙈

no. 1

icecrime · 2015-10-08T23:30:51Z

Ping @abronan @mavenugo.

abronan · 2015-10-08T23:31:03Z

Thanks for summing up the situation @icecrime

🙈 option 1

jessfraz · 2015-10-08T23:31:23Z

are we all closing our eyes hahahaha lol

nathanleclaire · 2015-10-08T23:31:47Z

🙈

icecrime · 2015-10-09T00:54:58Z

Discussed with @mavenugo and team: they will be running stress tests tomorrow on Docker daemon with multihost networking backed by etcd using the existing driver. If we do experience the kind of crashes that we worry might happen, we will consider taking this PR for 1.9. If we don't, it might not be worth the effort.

vdemeester · 2015-10-09T06:47:14Z

🙈 🙉

tiborvass · 2015-10-13T00:07:25Z

hack/.vendor-helpers.sh

@dhiltgen a comment explaining what this function is doing would be appreciated :)

tiborvass · 2015-10-13T00:38:42Z

hack/.vendor-helpers.sh

~~should be sed -i'' to make the script work both on osx and linux (note there are no spaces between -i and '')~~ nope there is no reliable way of doing cross platform with -i.

Windows will balk on the "find" 😉

mavenugo · 2015-10-13T10:56:52Z

@abronan @dhiltgen I tried to integrate this PR on top of #16910 to make sure things work as expected. but I see the dreaded error -
Error response from daemon: failed to update store for object type *libnetwork.network: 104: Not a directory (/docker/network/v1.0/network) [23]

I know @abronan tried to solve it prior to the watch related changes and eventually was closed (moby/libnetwork#582). Do we need that patch in libnetwork in order to make progress ?
I will continue to dig into this. But we need to make sure this works so that we get to test it e2e.

mrjana · 2015-10-13T14:59:38Z

@mavenugo I might know what the problem is. I'll add an e2e integration test in libnetwork to verify etcd to fix this once for all. There is a small change needed in libnetwork to make this work. With that change you might be able to verify the new etcd client in docker.

abronan · 2015-10-13T16:45:28Z

Issue described above fixed in libnetwork with @mavenugo. So we good.

mavenugo · 2015-10-13T16:49:29Z

@mrjana @abronan thanks for the help. Yes, we have identified a fix. there is an sequencing issue with the PRs though. Lets vendor in libnetwork via #16910 and rebase this PR on top of it.
Sounds good ?

dhiltgen · 2015-10-13T17:12:50Z

Just bumped to the latest libkv at @mavenugo's request

dhiltgen · 2015-10-13T19:33:47Z

I have a version of this change rebased on top of 16910 which I'll push up as soon as 16910 is merged.

The latest libkv uses a different etcd library. Unfortunately that library uses some funky import paths, so I've added a new cleanup routine for our vendor scripts to be able to normalize the imports to be consistent with how imports work in this tree. Signed-off-by: Daniel Hiltgen <[email protected]>

tiborvass · 2015-10-13T21:59:13Z

@dhiltgen is 7078e7d the one you pushed after 16910 was merged?

dhiltgen · 2015-10-13T22:06:54Z

correct - it's rebased on top of madhu's change, and includes the latest libnetwork

tiborvass · 2015-10-13T22:08:20Z

hack/vendor.sh

@dhiltgen why do we need to bump libnetwork again?

@tiborvass This includes a fix for etcd (see this comment)

abronan · 2015-10-13T22:46:41Z

Still LGTM

tiborvass · 2015-10-13T22:46:45Z

LGTM for hack/

abronan · 2015-10-13T22:47:56Z

ping @mavenugo @icecrime

mavenugo · 2015-10-13T22:50:16Z

tested minimal etcd integration with libnetwork with the patch here. LGTM.
once we have proper etcd integration tests, we will cover many cases.

this PR is good to go.

Bump to latest libkv + dependencies

GordonTheTurtle added the status/0-triage label Oct 8, 2015

dhiltgen mentioned this pull request Oct 8, 2015

Add TLS support for discovery backend #16644

Merged

icecrime added status/2-code-review and removed status/0-triage labels Oct 8, 2015

tiborvass reviewed Oct 13, 2015
View reviewed changes

dhiltgen force-pushed the bump_libkv branch from b31bbe8 to a4132a0 Compare October 13, 2015 00:12

tiborvass reviewed Oct 13, 2015
View reviewed changes

tomdee mentioned this pull request Oct 13, 2015

Vendoring libnetwork for the pluggable IPAM driver support #16910

Merged

dhiltgen force-pushed the bump_libkv branch from a4132a0 to 7f14062 Compare October 13, 2015 17:10

tiborvass added this to the 1.9.0 milestone Oct 13, 2015

icecrime assigned tiborvass Oct 13, 2015

dhiltgen force-pushed the bump_libkv branch from 7f14062 to 7078e7d Compare October 13, 2015 21:41

tiborvass reviewed Oct 13, 2015
View reviewed changes

tiborvass added a commit that referenced this pull request Oct 13, 2015

Merge pull request #16863 from dhiltgen/bump_libkv

9c94dce

Bump to latest libkv + dependencies

tiborvass merged commit 9c94dce into moby:master Oct 13, 2015

tiborvass removed their assignment Oct 14, 2015

calavera deleted the bump_libkv branch February 9, 2016 05:43

kolyshkin mentioned this pull request Sep 8, 2018

get rid of ugorji/go fork #37804

Closed

Conversation

dhiltgen commented Oct 8, 2015

Uh oh!

icecrime commented Oct 8, 2015

Uh oh!

jessfraz commented Oct 8, 2015

Uh oh!

icecrime commented Oct 8, 2015

Uh oh!

abronan commented Oct 8, 2015

Uh oh!

jessfraz commented Oct 8, 2015

Uh oh!

nathanleclaire commented Oct 8, 2015

Uh oh!

icecrime commented Oct 9, 2015

Uh oh!

vdemeester commented Oct 9, 2015

Uh oh!

tiborvass Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

dhiltgen Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

tiborvass Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

tianon Oct 13, 2015 via email

Choose a reason for hiding this comment

Uh oh!

mavenugo commented Oct 13, 2015

Uh oh!

mrjana commented Oct 13, 2015

Uh oh!

abronan commented Oct 13, 2015

Uh oh!

mavenugo commented Oct 13, 2015

Uh oh!

dhiltgen commented Oct 13, 2015

Uh oh!

dhiltgen commented Oct 13, 2015

Uh oh!

tiborvass commented Oct 13, 2015

Uh oh!

dhiltgen commented Oct 13, 2015

Uh oh!

tiborvass Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

abronan Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

abronan commented Oct 13, 2015

Uh oh!

tiborvass commented Oct 13, 2015

Uh oh!

abronan commented Oct 13, 2015

Uh oh!

mavenugo commented Oct 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants