Zookeeper: Make Create+Put on a new znode atomic #148

amirma · 2017-02-24T01:31:18Z

Writes to a new Zookeepr znode should take advantage of Zookeeper's
atomic create + write primitive. If not, it is possible that a read
that was triggered by a watch will return an empty string. The previous
workaround for this does not always work (e.g., when an empty string
is returned due to a race) and can potentially cause call-stack overflow.
This change-set fixes this race and removes the workaround.
It also adds a call to Zookeepeer's Sync() on a Get operation,
only when an empty string (or SOH) is returned to guard against an older
version of libkv doing create+write in a non atomic fashion.

This change-set addresses github.com/docker-archive/classicswarm/issues/1915

Signed-off-by: Amir Malekpour [email protected]

Writes to a new Zookeepr znode should take advantage of Zookeeper's atomic create + write primitive. If not, it is possible that a read that was triggered by a watch will return an empty string. The previous workaround for this does not always work (e.g., when an empty string is returnedi due to a race) and can potentially cause call-stack overflow. This change-set fixes this race and removes the workaround. It also adds a call to Zookeepeer's Sync() on a Get operation, only when an empty string (or SOH) is returned to guard against an older version of libkv doing create+write in a non atomic fashion. This change-set addresses github.com/docker-archive/classicswarm/issues/1915 Signed-off-by: Amir Malekpour <[email protected]>

dongluochen

If possible, we should add test to validate the changes.

dongluochen · 2017-03-03T02:21:58Z

store/zookeeper/zookeeper.go

-	// SOH control character instead of the actual value
-	if string(resp) == SOH {
-		return s.Get(store.Normalize(key))
+		if (string(resp) == SOH || string(resp) == "") && i < syncRetryLimit {


I feel i < syncRetryLimit here is unnecessary.

@dongluochen The i < syncRetryLimit is to prevent a Sync in the very last iteration of the for loop. Without this, we would unnecessarily call sync and then exit the loop. (So effectively we call sync not more than syncRetryLimit number of times).

dongluochen · 2017-03-03T02:33:11Z

I'm not familiar with this repo. CI is failing for last few PRs.

0.08s$ script/travis_consul.sh 0.6.3
--2017-02-24 01:32:49--  https://releases.hashicorp.com/consul/0.6.3/consul_0.6.3_linux_amd64.zip
Resolving releases.hashicorp.com (releases.hashicorp.com)... 151.101.33.183
Connecting to releases.hashicorp.com (releases.hashicorp.com)|151.101.33.183|:443... connected.
Unable to establish SSL connection.

dongluochen · 2017-03-04T01:09:55Z

store/zookeeper/zookeeper.go

-	// SOH control character instead of the actual value
-	if string(resp) == SOH {
-		return s.Get(store.Normalize(key))
+		if (string(resp) == SOH || string(resp) == "") && i < syncRetryLimit {


dongluochen · 2017-03-04T01:11:29Z

store/zookeeper/zookeeper.go

-	defaultTimeout = 10 * time.Second
+	defaultTimeout = 10 * time.Second	
+
+	syncRetryLimit = 5


Any reason to choose 5? I don't know but it seems high. How much delay this may introduce?

@dongluochen No specific reason, except in my test environment 3 retries would always be needed to get the correct value, and I rounded this up to 5.
Note that the sync currently happens on SOH or empty string getting returned, and not on all reads. Also this is to work around older versions of libkv which have this bug. The versions that have this patch will do create+write atomically and should not return SOH or empty so the sync should never be needed.

dongluochen · 2017-03-04T01:14:36Z

Ping @docker/core-libkv-maintainers.

CI is also failing.

mcapuccini · 2017-03-15T12:55:39Z

@dongluochen my PR fixes it #154

nishanttotla

@amirma are you still working on this?

amirma · 2017-06-21T20:42:47Z

@nishanttotla We've merged this in our production repository and I can confirm it has fixed the issue. I think the patch is ready to merge as is.

nishanttotla · 2017-06-21T21:04:29Z

@amirma thanks for the prompt reply! Seems like your build is failing. Could you take a look at that? It would be nice to merge this.

amirma · 2017-06-21T22:32:26Z

@nishanttotla I just pushed another build and it failed due to a new (I believe infrastructure-related) reason. Any insights?

nishanttotla · 2017-06-21T23:52:00Z

It seems like some package paths/locations may have changed:

27.62s$ go get -t -v ./...
github.com/stretchr/testify (download)
github.com/boltdb/bolt (download)
github.com/hashicorp/consul (download)
github.com/coreos/etcd (download)
github.com/coreos/go-semver (download)
github.com/ugorji/go (download)
Fetching https://golang.org/x/net/context?go-get=1
Parsing meta tags from https://golang.org/x/net/context?go-get=1 (status code 200)
get "golang.org/x/net/context": found meta tag main.metaImport{Prefix:"golang.org/x/net", VCS:"git", RepoRoot:"https://go.googlesource.com/net"} at https://golang.org/x/net/context?go-get=1
get "golang.org/x/net/context": verifying non-authoritative meta tag
Fetching https://golang.org/x/net?go-get=1
Parsing meta tags from https://golang.org/x/net?go-get=1 (status code 200)
golang.org/x/net (download)
github.com/samuel/go-zookeeper (download)
github.com/docker/libkv/store
github.com/boltdb/bolt
github.com/hashicorp/consul/vendor/github.com/hashicorp/go-cleanhttp
github.com/hashicorp/consul/vendor/github.com/hashicorp/serf/coordinate
github.com/coreos/etcd/pkg/pathutil
github.com/coreos/etcd/pkg/types
github.com/coreos/go-semver/semver
github.com/ugorji/go/codec
golang.org/x/net/context
github.com/docker/libkv
github.com/stretchr/testify/vendor/github.com/davecgh/go-spew/spew
github.com/stretchr/testify/vendor/github.com/pmezard/go-difflib/difflib
github.com/stretchr/testify/vendor/github.com/stretchr/objx
github.com/samuel/go-zookeeper/zk
github.com/coreos/etcd/version
github.com/hashicorp/consul/api
github.com/stretchr/testify/assert
github.com/docker/libkv/store/boltdb
github.com/docker/libkv/store/zookeeper
github.com/stretchr/testify/mock
github.com/docker/libkv/testutils
github.com/docker/libkv/store/mock
github.com/docker/libkv/store/consul
github.com/coreos/etcd/client
github.com/docker/libkv/store/etcd
0.08s$ script/travis_consul.sh 0.6.3
--2017-02-24 01:32:49--  https://releases.hashicorp.com/consul/0.6.3/consul_0.6.3_linux_amd64.zip
Resolving releases.hashicorp.com (releases.hashicorp.com)... 151.101.33.183
Connecting to releases.hashicorp.com (releases.hashicorp.com)|151.101.33.183|:443... connected.
Unable to establish SSL connection.
unzip:  cannot find or open consul_0.6.3_linux_amd64.zip, consul_0.6.3_linux_amd64.zip.zip or consul_0.6.3_linux_amd64.zip.ZIP.
script/travis_consul.sh: line 18: ./consul: No such file or directory

Ping @docker/core-libkv-maintainers, the CI might need fixing.

abronan · 2017-08-29T16:27:26Z

The fix is included to my fork: abronan/libkv along with other patches, thanks @amirma.

nishanttotla · 2017-08-29T20:37:37Z

@abronan should we merge this PR, or might it need more changes? I think this is useful and fixes a long standing issue.

abronan · 2017-08-29T23:15:02Z

@nishanttotla I think it is safe to merge it yes, just merge the other PRs that are fixing the build first (the commit history on my fork could be of help).

convergentio mentioned this pull request Mar 3, 2017

Move to the docker/swarm/1915-4 branch of libkv which fixes issue #1915 docker-archive/classicswarm#2642

Closed

dongluochen reviewed Mar 3, 2017

View reviewed changes

dongluochen reviewed Mar 4, 2017

View reviewed changes

nishanttotla reviewed Jun 21, 2017

View reviewed changes

nishanttotla mentioned this pull request Jun 21, 2017

zookeeper fails to add nodes back in integration test docker-archive/classicswarm#1915

Closed

nishanttotla mentioned this pull request May 29, 2018

Zookeeper: Make Create+Put on a new znode atomic docker-archive/classicswarm#2862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zookeeper: Make Create+Put on a new znode atomic #148

Zookeeper: Make Create+Put on a new znode atomic #148

amirma commented Feb 24, 2017 •

edited

Loading

dongluochen left a comment

dongluochen Mar 3, 2017

amirma Mar 3, 2017 •

edited

Loading

dongluochen Mar 4, 2017

dongluochen commented Mar 3, 2017

dongluochen Mar 4, 2017

dongluochen Mar 4, 2017

amirma Mar 15, 2017

dongluochen commented Mar 4, 2017

mcapuccini commented Mar 15, 2017

nishanttotla left a comment

amirma commented Jun 21, 2017 •

edited

Loading

nishanttotla commented Jun 21, 2017

amirma commented Jun 21, 2017

nishanttotla commented Jun 21, 2017

abronan commented Aug 29, 2017

nishanttotla commented Aug 29, 2017

abronan commented Aug 29, 2017

Zookeeper: Make Create+Put on a new znode atomic #148

Are you sure you want to change the base?

Zookeeper: Make Create+Put on a new znode atomic #148

Conversation

amirma commented Feb 24, 2017 • edited Loading

dongluochen left a comment

Choose a reason for hiding this comment

dongluochen Mar 3, 2017

Choose a reason for hiding this comment

amirma Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

dongluochen Mar 4, 2017

Choose a reason for hiding this comment

dongluochen commented Mar 3, 2017

dongluochen Mar 4, 2017

Choose a reason for hiding this comment

dongluochen Mar 4, 2017

Choose a reason for hiding this comment

amirma Mar 15, 2017

Choose a reason for hiding this comment

dongluochen commented Mar 4, 2017

mcapuccini commented Mar 15, 2017

nishanttotla left a comment

Choose a reason for hiding this comment

amirma commented Jun 21, 2017 • edited Loading

nishanttotla commented Jun 21, 2017

amirma commented Jun 21, 2017

nishanttotla commented Jun 21, 2017

abronan commented Aug 29, 2017

nishanttotla commented Aug 29, 2017

abronan commented Aug 29, 2017

amirma commented Feb 24, 2017 •

edited

Loading

amirma Mar 3, 2017 •

edited

Loading

amirma commented Jun 21, 2017 •

edited

Loading