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.

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

puts that would succeed on consul & zookeeper fail on etcd #20

Open
spikecurtis opened this Issue Jun 24, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@spikecurtis
Contributor

spikecurtis commented Jun 24, 2015

Since etcd enforces a distinction between "directory" keys and "file" keys, some sequences of puts that would succeed on the other backends will fail on etcd.

Put("/path/to/", "Hello")
Put("/path/to/new/node", "World!")

will succeed on Consul, but the second call will fail on etcd, since /path/to is a file.

Likewise

Put("/path/to/new/node", "Hello")
Put("/path", "World!")

will fail on etcd since /path is a directory.

I've discussed the next-gen API for etcd with some of their maintainers, and I think they're planning to remove the directory/file distinction for v3. So, this issue may sort itself out on that new API.

I'm not sure there is a satisfactory resolution that doesn't involve a trade off.

One thing that could be done is to automatically change directories to files and vice versa, but this would result in data loss, and so is probably not acceptable.

Another possibility would be for libkv to append a file node suffix to every key when using etcd. E.g.

/path/to/  -> /path/to/__data__
/path/to/new/node -> /path/to/new/node/__data__

This leads to etcd backend behaving the same way as Consul & Zookeeper, however, it makes it very difficult to allow interoperation with other clients of the data store that are not libkv-based, since the keys they see are different.

We could also just do nothing, and warn developers that writing interoperable code requires that they plan their key use carefully so they always write to a leaf node in the tree.

@abronan

This comment has been minimized.

Show comment
Hide comment
@abronan

abronan Jun 29, 2015

Contributor

Thanks for opening the issue @spikecurtis

I'm actually torn between just changing the documentation and waiting for the support in etcd v3 (which will come at some point in the future, without certitude), or using hidden keys to store directory informations.

If I'm not mistaken, using hidden keys is the only way to store informations for a directory without the key being listed or events being triggered for that key. The interoperability with other client libraries is not really an issue in that case: they must point directly to /path/to/__data__ and know about the key structure anyway.

I'm not against making the change in that case because this is a bug. Even though this is a design issue in etcd that must be eventually fixed.

Contributor

abronan commented Jun 29, 2015

Thanks for opening the issue @spikecurtis

I'm actually torn between just changing the documentation and waiting for the support in etcd v3 (which will come at some point in the future, without certitude), or using hidden keys to store directory informations.

If I'm not mistaken, using hidden keys is the only way to store informations for a directory without the key being listed or events being triggered for that key. The interoperability with other client libraries is not really an issue in that case: they must point directly to /path/to/__data__ and know about the key structure anyway.

I'm not against making the change in that case because this is a bug. Even though this is a design issue in etcd that must be eventually fixed.

@abronan abronan added the kind/bug label Jun 29, 2015

@spikecurtis

This comment has been minimized.

Show comment
Hide comment
@spikecurtis

spikecurtis Jun 29, 2015

Contributor

@abronan it's more than other client libraries seeing keys they wouldn't recognize.

Suppose you have an existing application that writes to etcd, and you want to use libkv to interoperate with that application. If we append __data__ to every key the user of libkv supplies, then they will be unable to read or write to the "normal" keys the existing application uses.

We can't just use this trick only for "directory" keys, since we won't know a priori whether the path is a leaf or not. So every Put and Get for the etcd backend would have the suffix.

My personal feeling is to change the documentation. At this stage, anyone serious about multiple backends better be testing them with their application. Other options involve a lot of black magic and complexity for very little benefit.

Contributor

spikecurtis commented Jun 29, 2015

@abronan it's more than other client libraries seeing keys they wouldn't recognize.

Suppose you have an existing application that writes to etcd, and you want to use libkv to interoperate with that application. If we append __data__ to every key the user of libkv supplies, then they will be unable to read or write to the "normal" keys the existing application uses.

We can't just use this trick only for "directory" keys, since we won't know a priori whether the path is a leaf or not. So every Put and Get for the etcd backend would have the suffix.

My personal feeling is to change the documentation. At this stage, anyone serious about multiple backends better be testing them with their application. Other options involve a lot of black magic and complexity for very little benefit.

@abronan

This comment has been minimized.

Show comment
Hide comment
@abronan

abronan Jun 29, 2015

Contributor

To my understanding, the prefix would only be used for a single unique key __data__ under each directory (and not every key).

So we would have a structure like this:

path/to/
path/to/__data__    // Holds the data for the directory, hidden to regular GET operations like List/Watch, etc.
path/to/key1
path/to/key2
path/to/key3

path/
path/__data__       // We can have a key here as well because "path/" is a directory

The process of Put for etcd and to actually hide the directory/key distinction would be something like:

Put("path/to/directory/", []byte(""), nil)
// "path/to/directory/" is a regular key

Put("path/to/directory/node1/", []byte(""), nil)
// Changes "path/to/directory/" from a key to a directory, moving the data to the child key `__data__`

Put("path/", []byte(""), nil)
// We create a new child key `__data__` under "path/" to hold the data

Now, if you try to Get("path/to/directory/"), because this is a directory, it will append __data__ to get back the value from the hidden key.

Unless I'm missing something obvious, this would be possible but this involves a lot of trickery and back and forth transformations between key and directory.

Other options involve a lot of black magic and complexity for very little benefit.

I actually agree with this. Not sure if this is a good option as this hides a lot of things that could backfire on us if people are upgrading to new versions of etcd or using other client libraries. It is definitely possible but this will change a lot of code under each call to make the distinction and handle special cases.

Is there any actual Issue or open PR in the etcd repository to track the changes in API v3? We could probably concentrate our efforts here and help making this change happen in etcd.

Contributor

abronan commented Jun 29, 2015

To my understanding, the prefix would only be used for a single unique key __data__ under each directory (and not every key).

So we would have a structure like this:

path/to/
path/to/__data__    // Holds the data for the directory, hidden to regular GET operations like List/Watch, etc.
path/to/key1
path/to/key2
path/to/key3

path/
path/__data__       // We can have a key here as well because "path/" is a directory

The process of Put for etcd and to actually hide the directory/key distinction would be something like:

Put("path/to/directory/", []byte(""), nil)
// "path/to/directory/" is a regular key

Put("path/to/directory/node1/", []byte(""), nil)
// Changes "path/to/directory/" from a key to a directory, moving the data to the child key `__data__`

Put("path/", []byte(""), nil)
// We create a new child key `__data__` under "path/" to hold the data

Now, if you try to Get("path/to/directory/"), because this is a directory, it will append __data__ to get back the value from the hidden key.

Unless I'm missing something obvious, this would be possible but this involves a lot of trickery and back and forth transformations between key and directory.

Other options involve a lot of black magic and complexity for very little benefit.

I actually agree with this. Not sure if this is a good option as this hides a lot of things that could backfire on us if people are upgrading to new versions of etcd or using other client libraries. It is definitely possible but this will change a lot of code under each call to make the distinction and handle special cases.

Is there any actual Issue or open PR in the etcd repository to track the changes in API v3? We could probably concentrate our efforts here and help making this change happen in etcd.

@abronan

This comment has been minimized.

Show comment
Hide comment
@abronan

abronan Jul 7, 2015

Contributor

@spikecurtis After a discussion with @kelseyhightower (thanks for all the infos!), etcd API v3 will definitely drop this distinction about directories and keys. So we might just wait and document it properly until this drops and we make the appropriate changes :)

Contributor

abronan commented Jul 7, 2015

@spikecurtis After a discussion with @kelseyhightower (thanks for all the infos!), etcd API v3 will definitely drop this distinction about directories and keys. So we might just wait and document it properly until this drops and we make the appropriate changes :)

@abronan abronan added the store/etcd label Aug 18, 2015

@abronan abronan self-assigned this Aug 18, 2015

abronan added a commit to abronan/libnetwork that referenced this issue Oct 1, 2015

Ensure Directories are watchable with etcd
etcd v2.0+ with API v2 cannot watch on a regular key, we must ensure
that the initial steps involve creating a 'key' under a directory at
'path'. This is a workaround to recursively create the path and make
sure that 'path' is considered as a watchable directory.

These steps are necessary unless 'libkv' includes and supports the
new etcd API v3 which removes the directory/key distinction.

Fixes: docker#392
See: docker/libkv#20

Signed-off-by: Alexandre Beslic <abronan@docker.com>

abronan added a commit to abronan/libnetwork that referenced this issue Oct 1, 2015

Ensure Directories are watchable with etcd
etcd v2.0+ with API v2 cannot watch on a regular key, we must ensure
that the initial steps involve creating a 'key' under a directory at
'path'. This is a workaround to recursively create the path and make
sure that 'path' is considered as a watchable directory.

These steps are necessary unless 'libkv' includes and supports the
new etcd API v3 which removes the directory/key distinction to allow
for a better abstraction.

Fixes: docker#392
See: docker/libkv#20

Signed-off-by: Alexandre Beslic <abronan@docker.com>

abronan added a commit to abronan/libnetwork that referenced this issue Oct 4, 2015

Ensure Keys are watchable with etcd
etcd v2.0+ with API v2 cannot watch on a regular key, we must enforce
the 'IsDir' parameter while creating the key for it to be considered
as a directory.

This step is necessary unless 'libkv' includes and supports the new
etcd API v3 which removes the directory/key distinction to allow for
a better abstraction.

Fixes: docker#392
See: docker/libkv#20

Signed-off-by: Alexandre Beslic <abronan@docker.com>
@JeffChien

This comment has been minimized.

Show comment
Hide comment
@JeffChien

JeffChien Nov 11, 2015

Base on my observation, etcd and zookeeper act like a tree tree, consul on the other hand like a hash table.

Say we Put("foot/to/new/node") to an empty backend, if the command succeed, etcd and zookeeper will have the same amount of keys(3 in this case), while consul only create 1 key.

and consul treat foo, foo/ as two different keys.

theses should be considered.

JeffChien commented Nov 11, 2015

Base on my observation, etcd and zookeeper act like a tree tree, consul on the other hand like a hash table.

Say we Put("foot/to/new/node") to an empty backend, if the command succeed, etcd and zookeeper will have the same amount of keys(3 in this case), while consul only create 1 key.

and consul treat foo, foo/ as two different keys.

theses should be considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment